-
Notifications
You must be signed in to change notification settings - Fork 130
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
Job scheduler benchmarks #840
Conversation
7e6b243
to
4ccd2f8
Compare
25d2c09
to
637658f
Compare
5e9509a
to
b250234
Compare
b250234
to
b7ef8e3
Compare
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.
Shiny ✨
This functionality has been released in v0.27.0 of the language server. For further feature requests or bug reports with this functionality, please create a new GitHub issue following the template. Thank you! |
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.
Hey, hope it's alright if I swing by to leave some comments on the gobenchdata
usage - super excited to see it in use! 😁
not the graphing part - primarily because it would require us giving the Action push access to the repository (which AFAIK cannot be limited to a particular branch even).
This can be done separately from the action by generating the visualization app separately: https://github.com/bobheadxi/gobenchdata#visualisation and setting up a separate repository dedicated to hosting benchmark data (which should be low-risk enough for this use case hopefully!)
tl;dr in a new repository:
gobenchdata web generate .
ls
# css gobenchdata-web.yml index.html js overrides.css
Then, in your GitHub action here, you can basically re-implement the script used in the published action:
# fetch published data
echo
echo "📚 Checking out ${INPUT_PUBLISH_REPO}@${INPUT_PUBLISH_BRANCH}..."
cd /tmp/build
git clone https://${GITHUB_ACTOR}:${GITHUB_TOKEN}@github.com/${INPUT_PUBLISH_REPO}.git .
git checkout ${INPUT_PUBLISH_BRANCH}
echo
# merge results with published
echo '☝️ Updating results...'
if [[ -f "${INPUT_BENCHMARKS_OUT}" ]]; then
echo '📈 Existing report found - merging...'
gobenchdata merge "${RUN_OUTPUT}" "${INPUT_BENCHMARKS_OUT}" \
--prune "${INPUT_PRUNE_COUNT}" \
--json "${INPUT_BENCHMARKS_OUT}" \
--flat
else
cp "${RUN_OUTPUT}" "${INPUT_BENCHMARKS_OUT}"
fi
# publish results
echo
echo '📷 Committing and pushing new benchmark data...'
git add .
git commit -m "${INPUT_GIT_COMMIT_MESSAGE}"
git push -f origin ${INPUT_PUBLISH_BRANCH}
Would be happy to help you get the most out of the tool!
${DATA_PATH} \ | ||
${DATA_PATH} \ | ||
--checks.config ${CHECKS_CONFIG_PATH} \ | ||
2> $RESULTS_PATH # eval returns result JSON to stderr for some reason |
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.
Ah this command is intended to be used with the --json [path]
flag, and probably should not print JSON unless that flag is provided - I'm fixing this with bobheadxi/gobenchdata#53 ! (though that would be a breaking change here)
FAILURES=`cat ${RESULTS_PATH} | jq '.Checks[] | select(.Status == "fail")'` | ||
echo $FAILURES |
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.
There is a report command for CI, gobenchdata checks report $RESULTS_PATH
, which will print a markdown table of your benchmark results and exit with a non-zero status
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.
Ah, that's nice - I'll take a look into it! Thanks for the note.
I recall having considered this option too, and it would certainly be better than pushing to the same repo, assuming we also leverage deploy keys and probably somehow connect the two repos so that it rebuilds on every run. Having it self-contained in one repo safely is however still more appealing. To be clear the main problem here is GitHub's own inflexible permission model which doesn't allow tokens to be scoped to any particular repositories or branches, I don't see how gobenchdata can do any better here. 🤔 😄 We can revisit this later as we have plans to publish docs to a website at some point too, so we'll have to build some kind of solution/infrastructure for that anyway. This PR intended to just put something relatively quick & simple together. |
I'm going to lock this pull request because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues. |
Fixes #834
New Benchmarks
This adds 12 new benchmarks into the
handler
package, to measure the performance of the hot path - i.e. walking/indexing for various different setups.I created a few local modules with varying levels of complexity and different providers + picked some popular modules from the Registry using a similar pattern. All of them are described in the docs along with estimated average CPU time and memory consumption.
Evaluating Benchmarks
Unfortunately there is no easy way of processing the benchmark outputs as per golang/go#40588
I found this GitHub Action on the Marketplace: https://github.com/marketplace/actions/continuous-benchmarking-for-go but ended up only using the underlying gobenchdata to parse & evaluate the benchmark data, not the graphing part - primarily because it would require us giving the Action push access to the repository (which AFAIK cannot be limited to a particular branch even).
The charts would be lovely to have but not at these costs IMO.
Slack Notifications
The official Slack GitHub Action provides a few different integrations and I just picked the easiest one since creating a new App is far more involved. This results in a very brief message on failure with a link to the logs, such as
We could potentially explore better integration which would also allow us to post details about thresholds breached etc. straight into Slack, but I thought this is good enough to start with.