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

Add degradation tests into CI #1331

Open
wants to merge 35 commits into
base: stage
Choose a base branch
from

Conversation

AKorpusenko
Copy link
Contributor

@AKorpusenko AKorpusenko commented Feb 28, 2024

Added a new script for CI which:

  • Runs go test -bench per each of files listed in the config.yml

  • Per each of the Benchmark test it's optional to specify allocations count and operations count delta

  • Script requires yq and benchstat tools for work

  • Each test case is executed 10 times. This is required minimum to get at least somewhat accurate values.

Makefile

Added two new commands:

  • degradation-test -- runs the tests with benchmarks and compares the results with the existing in the repo
  • update-benchmarks -- a manual Github Actions workflow which can we triggered to update the benchmarks in the repo. Should be used for approved optimizations and performance downgrades

@AKorpusenko AKorpusenko added enhancement New feature or request and removed DO NOT MERGE labels Feb 29, 2024
nkryuchkov
nkryuchkov previously approved these changes Mar 5, 2024
Copy link
Contributor

@nkryuchkov nkryuchkov left a comment

Choose a reason for hiding this comment

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

Great job! Just minor comments

logger := zaptest.NewLogger(b)
db, err := kv.NewInMemory(logger, basedb.Options{})
if err != nil {
b.Fatal(err)
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we use the testify library instead?

message/validation/rsa_benchmark_test.go Outdated Show resolved Hide resolved
scripts/degradation-tester/main.go Outdated Show resolved Hide resolved
scripts/degradation-tester/main.go Show resolved Hide resolved
scripts/degradation-tester/main.go Outdated Show resolved Hide resolved
opThresholds[testCase.Name] = testCase.OpDelta
}
if testCase.AllocDelta > 0 {
opThresholds[testCase.Name] = testCase.OpDelta
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
opThresholds[testCase.Name] = testCase.OpDelta
allocThresholds[testCase.Name] = testCase.OpDelta

MatusKysel
MatusKysel previously approved these changes Mar 6, 2024
@MatusKysel MatusKysel requested a review from y0sher March 6, 2024 12:21
MatusKysel
MatusKysel previously approved these changes Mar 11, 2024
nkryuchkov
nkryuchkov previously approved these changes Mar 11, 2024
MatusKysel
MatusKysel previously approved these changes Mar 28, 2024
Copy link

codecov bot commented Apr 4, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 49.1%. Comparing base (4966dce) to head (14b6064).
Report is 4 commits behind head on stage.

Additional details and impacted files

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Contributor

Choose a reason for hiding this comment

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

This test/benchmark needs to be updated after #1274 and #1358

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated

@AKorpusenko AKorpusenko requested a review from y0sher April 9, 2024 11:11
MatusKysel
MatusKysel previously approved these changes Apr 9, 2024
@y0sher
Copy link
Contributor

y0sher commented Apr 10, 2024

@AKorpusenko I don't understand how you updated the test but the benchmark results didn't change? especially how this passes on CI since on linux we now use openssl which should be 10x faster than stdlib rsa (which you run locally on your mac).

@AKorpusenko
Copy link
Contributor Author

@y0sher, we have to compare the benchmarks only between different GHA workflow runs, right? I've added a manual-update-benchmarks GHA workflow for this, but it only can be used after this code will be in master. Before this, I used my own fork of ssv to get linux runs benchmarks.
In the last commit, I've just made you can see significant performance increase. So it works like we expect.

Another thing I've noticed, is that we have queue_test.go which uses b.Log and this logs appear in the benchmarks, what is wrong from my perspective and should be fixed in another PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
DO NOT MERGE enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants