Skip to content
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

workflow: add cockroach-microbench-ci step in github actions #127228

Merged
merged 1 commit into from
Aug 9, 2024

Conversation

sambhav-jain-16
Copy link
Contributor

@sambhav-jain-16 sambhav-jain-16 commented Jul 16, 2024

Fixes: #106661
Release note: None
Epic: https://cockroachlabs.atlassian.net/browse/CRDB-29662

This change intends to add a new CI step which will run BenchmarkKV microbenchmark for the current commit and compares the result with that of the base commit's results which is stored in a cloud bucket.

This is needed to avoid minor perf regressions so that any regression is caught in the PR itself. The current threshold is set to 5%.

There are highly 3 steps

  1. Run benchmarks
  2. If this is PR branch, download the latest commit of the target branch's bench results from the cloud storage and compare the results. If there's a regression fail here itself
  3. If it is a push step (PR merge to master/release), upload the result to the cloud storage with the current commit sha.

Currently, this change won't block the PR. However it will still run the microbenchmarks and upload the results during the push step.

@cockroach-teamcity
Copy link
Member

This change is Reviewable

@sambhav-jain-16 sambhav-jain-16 force-pushed the CRDB-29662 branch 6 times, most recently from b2085aa to 57284c8 Compare July 17, 2024 09:37
@sambhav-jain-16 sambhav-jain-16 force-pushed the CRDB-29662 branch 4 times, most recently from 9ec7527 to cafc986 Compare July 22, 2024 05:18
@sambhav-jain-16 sambhav-jain-16 force-pushed the CRDB-29662 branch 2 times, most recently from d0e2c23 to 6e77eab Compare July 29, 2024 15:11
@sambhav-jain-16 sambhav-jain-16 marked this pull request as ready for review July 30, 2024 06:20
@sambhav-jain-16 sambhav-jain-16 requested a review from a team as a code owner July 30, 2024 06:20
Copy link
Collaborator

@herkolategan herkolategan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice work. A few comments: I realised you might have copied some of my earlier work regarding using dev to build roachprod-microbench. I've been meaning to update it to use the CI way run_bazel rather. It should be an easy update here.

.github/workflows/github-actions-essential-ci.yml Outdated Show resolved Hide resolved
build/github/cockroach-microbench-ci.sh Outdated Show resolved Hide resolved
build/github/cockroach-microbench-ci.sh Outdated Show resolved Hide resolved
build/github/cockroach-microbench-ci.sh Outdated Show resolved Hide resolved
build/github/cockroach-microbench-ci.sh Outdated Show resolved Hide resolved
@sambhav-jain-16 sambhav-jain-16 force-pushed the CRDB-29662 branch 10 times, most recently from ca2246c to bd75265 Compare July 30, 2024 16:51
@sambhav-jain-16 sambhav-jain-16 force-pushed the CRDB-29662 branch 5 times, most recently from e3be5ed to cb0bdd1 Compare July 31, 2024 09:59
@rickystewart
Copy link
Collaborator

By the way, the build is failing, please have a look at that.

Fixes: cockroachdb#106661
Release note: None
Epic: https://cockroachlabs.atlassian.net/browse/CRDB-29662

This change intends to add a new CI step which will run `BenchmarkKV`
microbenchmark for the current commit and compares the result with that
of the base commit's results which is stored in a cloud bucket.

This is needed to avoid minor perf regressions so that any regression is
caught in the PR itself. The current threshold is set to 5%.

There are highly 3 steps 1. Run benchmarks 2. If this is PR branch,
download the latest commit of the target branch's bench results from the
cloud storage and compare the results. If there's a regression fail here
itself 3. If it is a push step (PR merge to master/release), upload the
result to the cloud storage with the current commit sha.

Currently, this change won't block the PR. However it will still run the
microbenchmarks and upload the results during the push step.
@sambhav-jain-16
Copy link
Contributor Author

By the way, the build is failing, please have a look at that.

@rickystewart Fixed it. PTAL

@rickystewart
Copy link
Collaborator

Are you aware that the script in CI does not seem to be working correctly?

+ _bazel/bin/pkg/cmd/roachprod-microbench/roachprod-microbench_/roachprod-microbench clean ./artifacts/microbench/microbench.log ./artifacts/microbench/current/pkg_sql-report.log
+ false
+ gcloud storage cp gs://cockroach-microbench-ci/master/15efb7e2270b05f5c18183d30ae95a0eea8670ad.log ./artifacts/microbench/base/pkg_sql-report.log
  

ERROR: (gcloud.storage.cp) The following URLs matched no objects or files:
-gs://cockroach-microbench-ci/master/15efb7e2270b05f5c18183d30ae95a0eea8670ad.log
+ echo 'Couldn'\''t download base bench file, exiting.'
+ exit 0
Couldn't download base bench file, exiting.

Copy link
Collaborator

@rickystewart rickystewart left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Spoke with Herko and it seems the current behavior is expected so this is fine.

@sambhav-jain-16
Copy link
Contributor Author

Spoke with Herko and it seems the current behavior is expected so this is fine.

Yes, this is expected since there's no bench file for the current latest master commit. Thanks for the review !

@sambhav-jain-16
Copy link
Contributor Author

TFTR!

bors r=rickystewart,herkolategan

@craig craig bot merged commit 2558f3f into cockroachdb:master Aug 9, 2024
23 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

perf: avoid small performance regressions
4 participants