-
Notifications
You must be signed in to change notification settings - Fork 1.1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
docs(.github/release): describe performance benchmarking steps #2442
Conversation
Document the steps required to test an upcoming go-libp2p release using the https://github.com/libp2p/test-plans/blob/master/perf/ libp2p performance benchmark tooling. Document the follow-up steps required once a release is cut.
- [ ] Performance Benchmarking | ||
- [ ] Follow the instructions [on adding a new version to the libp2p performance benchmark tooling](https://github.com/libp2p/test-plans/tree/master/perf#adding-a-new-implementation-or-a-new-version). Instead of the final go-libp2p release commit, which obviously doesn't yet exist, use the most recent available commit. See https://github.com/libp2p/test-plans/pull/242 as an example. | ||
- [ ] Run the benchmarks [via the GitHub _perf_ Action](https://github.com/libp2p/test-plans/tree/master/perf#running-via-github-action), wait for the action to push a commit to your https://github.com/libp2p/test-plans/ pull request and compare the performance of the upcoming release with the previous release [using the dashboard](https://github.com/libp2p/test-plans/tree/master/perf#running-via-github-action). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This feels kind of heavy. Any chance we can add this to the uCI release workflow?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Off the top of my head I can think of the following automation:
- On creation of a new release in libp2p/go-libp2p
- Checkout https://github.com/libp2p/test-plans/
- Copy the most recent go-libp2p version folder in https://github.com/libp2p/test-plans/tree/master/perf/impl/go-libp2p
- Run
go get -u ...
to update to the to-be-tagged-as-release go-libp2p commit. - Commit the changes, push them as a new branch to libp2p/test-plans and create a pull request.
- Trigger the
perf
GitHub Action on https://github.com/libp2p/test-plans/ with the above created branch. - Post a comment on the pull request pointing users to
https://observablehq.com/@libp2p-workspace/performance-dashboard?branch=<LIBP2P-TEST-PANS-BRANCH-NAME>
@galargh what do you think of the above? Any suggestions? Also would you be interested and have capacity to help?
@marten-seemann in case of a breaking change in go-libp2p you would still want to manually update the go-libp2p perf protocol implementation in libp2p/test-plans. Though this is not a blocker for any automation here. E.g. one could have the libp2p/test-plans pull request creation be automated and then only require the final touches to the pull request to be manual.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Friendly ping @galargh. Any thoughts on the above?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I quickly put together how something like this could look like - libp2p/test-plans#256.
- [ ] **Stage 1 - Release** | ||
- [ ] Publish the release through the GitHub UI, adding the release notes. Some users rely on this to receive notifications of new releases. | ||
- [ ] Announce the release on the [discuss.libp2p.io](https://discuss.libp2p.io). | ||
- [ ] Performance Benchmarking | ||
- [ ] Replace the most recent (pre-release) go-libp2p commit with the final go-libp2p release commit on your previously created https://github.com/libp2p/test-plans/ pull request. | ||
- [ ] Retrigger [the GitHub _perf_ Action](https://github.com/libp2p/test-plans/tree/master/perf#running-via-github-action) and wait for it to push the results as a new commit. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can this be triggered automatically?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we can trigger this when we add a comment in the pull request per https://docs.github.com/en/actions/using-workflows/events-that-trigger-workflows#discussion_comment
For example commenting /perf
in a PR would trigger the workflow. My original thought was to use a label but we might need to trigger the workflow multiple times in one PR. I'm down to investigate this to free up @mxinden (unless you'd like to take it up Max)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My original thought was to use a label but we might need to trigger the workflow multiple times in one PR.
👍 preference for comment vs label for that reason.
We might be able to ask IPDX for some help on some of the GHA suggestions here, but I'd first like to see what actually makes sense for us. |
General thought: before automation, lets do the manual steps to work out the kinks. Automation can come after it's proven out. |
@BigLep In general I agree, but this is adding 5 additional steps to every release. We've been working very hard to make our releases easy and smooth, so that we can release more frequently. I'm not saying we need to automate everything at this point, but what's being proposed here feels too heavy. I'm also worried that once this PR is merged, it will ultimately become the go-libp2p maintainer's responsibility to create this automation. |
Closing here. With libp2p/test-plans#256 releases are automatically added. I don't think we need to document merging the automatically created pull request on Thank you @galargh for making this happen! |
Document the steps required to test an upcoming go-libp2p release using the https://github.com/libp2p/test-plans/blob/master/perf/ libp2p performance benchmark tooling.
Document the follow-up steps required once a release is cut.