-
Notifications
You must be signed in to change notification settings - Fork 10
chore(benchmarks): run macrobenchmark on latest changes #226
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
Conversation
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 @dubloom, just a quick thing that went under my radar on #220 and that we could potentially include in this PR.
The check-slo-breaches
image should be set to registry.ddbuild.io/images/benchmarking-platform-tools-ubuntu:latest
so that the threshold comparison tooling is up to date.
I'm setting up Slack notifications for cases other than breaches/warnings so we know early when a gating job is failing for other reasons (like not running benchmarks on latest changes).
Augusto is right, you need to update the image
to
in order to always be up to date with latest fixes & improvements for SLOs tracking and gates. |
fe5ceef
to
3775344
Compare
.gitlab/benchmarks.yml
Outdated
@@ -8,14 +9,41 @@ include: | |||
file: 'images/templates/gitlab/notify-slo-breaches.template.yml' | |||
|
|||
variables: | |||
MACROBENCHMARKS_CI_IMAGE: 486234852809.dkr.ecr.us-east-1.amazonaws.com/ci/benchmarking-platform:cpp-nginx | |||
MACROBENCHMARKS_CI_IMAGE: registry.ddbuild.io/images/benchmarking-platform-tools-ubuntu:latest |
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 was correct, the image here should remain MACROBENCHMARKS_CI_IMAGE: 486234852809.dkr.ecr.us-east-1.amazonaws.com/ci/benchmarking-platform:cpp-nginx
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.
We need to update image only in check-slo-breaches
CI job to image: registry.ddbuild.io/images/benchmarking-platform-tools-ubuntu:latest
instead of using $MACROBENCHMARKS_CI_IMAGE
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 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.
fixed in 0b672c6
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.
@dmehala There are 2 different Docker images.
MACROBENCHMARKS_CI_IMAGE is the one that contains Nginx and C++ benchmarks. It is used by all benchmark variants jobs. You own it, update it as you fit / when you change Nginx setup.
registry.ddbuild.io/images/benchmarking-platform-tools-ubuntu:latest is the one that contains SLO gates tooling & other BP tooling, should be used by check-slo-breaches
job. APM DCS Performance owns it, updates it continuously.
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.
Please revert the change to MACROBENCHMARKS_CI_IMAGE: 486234852809.dkr.ecr.us-east-1.amazonaws.com/ci/benchmarking-platform:cpp-nginx
and update check-slo-breaches
's job image.
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.
Looks good from my side!
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.
LGTM
Before this PR, the macrobenchmarks were run only on the latest release which is highly reducing their utility.
This PR runs the macrobenchmarks on the latest changes.
Important: macrobenchmarks will fail until this PR is merged because changes were made also in the benchmarking-platform repo.