-
Notifications
You must be signed in to change notification settings - Fork 11
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 benchmark test for scans #1067
Conversation
25ebfe7
to
e8c3dd0
Compare
be5441a
to
81a36fd
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.
This is great to see the benchmark tests coming to cnspec.
@@ -0,0 +1,63 @@ | |||
name: Benchmark main |
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 we simply call this benchmark>
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 one runs only on the main
branch, that's why I called it Benchmark main
@@ -84,6 +84,49 @@ jobs: | |||
name: Event File | |||
path: ${{ github.event_path }} | |||
|
|||
go-bench: |
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.
Why do we implement this twice?
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.
in this file we run the benchmark for a PR. It compares the results with the results of the latest commit on main
. The implementation for the main
branch is different because it runs the benchmark and then uploads the benchmark results in the cache, such that PRs start comparing results with that commit.
It's a slightly different flow but I couldn't combine the 2 into a single job
{ | ||
Connections: []*inventory.Config{ | ||
{ | ||
Type: "k8s", |
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.
Normally we do not have any provider installed by default. Do we not need to ensure the k8s provider is installed. If so we also would need to clean this up afterwards to not mess with other tests.
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.
Yeah this will fail after we merged #1060
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 don't think it will fail. When we run a scan, the required provider is auto-installed if it's not present. That should be fine. Since the goal of this benchmark is not to test provider installation or uninstall, I don't think it's necessary to cleanup providers. Here we only care about the CPU and memory usage during a scan
Signed-off-by: Ivan Milchev <ivan@mondoo.com>
Signed-off-by: Ivan Milchev <ivan@mondoo.com>
Signed-off-by: Ivan Milchev <ivan@mondoo.com>
Signed-off-by: Ivan Milchev <ivan@mondoo.com>
81a36fd
to
32333c8
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.
Great work @imilchev Would love to see this for cnquery as well.
add benchmark testing that compares every commit in a PR with latest
main
. If the diff is above 150%, it errors out and it comments on the violating commit. Looks like this 6e83272#commentswe also get a job summary like this https://github.com/mondoohq/cnspec/actions/runs/7643019168#summary-20824279571