✨ feat: implement initial benchmark tests #1071

OchiengEd · 2024-07-18T21:11:39Z

Description

This PR is for implementing benchmarking tests which will be used in CI to evaluate change in performance / compute resource utilization as new features are implemented.

Reviewer Checklist

API Go Documentation
Tests: Unit Tests (and E2E Tests, if appropriate)
Comprehensive Commit Messages
Links to related GitHub Issue(s)

Relates to #920

netlify · 2024-07-18T21:11:58Z

✅ Deploy Preview for olmv1 ready!

Name	Link
🔨 Latest commit	`1075eb3`
🔍 Latest deploy log	https://app.netlify.com/sites/olmv1/deploys/66b52ff7e13e21000831fa97
😎 Deploy Preview	https://deploy-preview-1071--olmv1.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

codecov · 2024-07-18T21:18:35Z

Codecov Report

Attention: Patch coverage is 0% with 2 lines in your changes missing coverage. Please review.

Project coverage is 75.23%. Comparing base (6864b0c) to head (1075eb3).
Report is 234 commits behind head on main.

Files with missing lines	Patch %	Lines
internal/rukpak/source/image_registry.go	0.00%	2 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1071      +/-   ##
==========================================
- Coverage   75.28%   75.23%   -0.06%     
==========================================
  Files          35       35              
  Lines        1914     1914              
==========================================
- Hits         1441     1440       -1     
- Misses        330      331       +1     
  Partials      143      143

Flag	Coverage Δ
e2e	`57.41% <0.00%> (-0.06%)`	⬇️
unit	`50.78% <0.00%> (ø)`

Flags with carried forward coverage won't be shown. Click here to find out more.

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

internal/contentmanager/contentmanager_test.go

internal/authentication/tokengetter_test.go

joelanford

Broadly, I would suggest that we focus benchmarks on areas of code that we think may be particularly prone to CPU time and allocation.

Do we have any indication that the contentmanager, tokengetter, or schemebuilder are particularly problematic? If not, I'm concerned that these benchmarks could become a source of noise and toil.

In particular, I think the four best places to add benchmarks are:

The catalog cache and/or client.
The Resolver interface implementation.
The image unpacker.
The registry+v1 to helm chart conversion.

In my observation, these seem to be the largest contributors to the duration of a Reconcile call, so even performance regressions in these areas likely have a bigger impact that other areas of the codebase.

OchiengEd · 2024-07-24T17:25:19Z

In my observation, these seem to be the largest contributors to the duration of a Reconcile

Totally agree. I was intent on going through the code base and identifying where we have more CPU / memory intensive tasks. However, to start working on GH actions, I wanted to have something in place.

OchiengEd · 2024-07-26T23:17:53Z

.github/workflows/benchmark.yaml

+      with:
+        name: benchmark-results
+        path: benchmark.txt
+        overwrite: true


Still needs a step to compare the change in performance as a result of the PR .

In addition, what do we think would be a good threshold for a computing performance degradation

.github/workflows/benchmark.yaml

internal/rukpak/source/image_registry.go

OchiengEd · 2024-08-05T18:08:46Z

internal/rukpak/source/image_registry_test.go

+	bundleSource := &source.BundleSource{
+		Type: source.SourceTypeImage,
+		Image: &source.ImageSource{
+			Ref: "quay.io/eochieng/litmus-edge-operator-bundle@sha256:104b294fa1f4c2e45aa0eac32437a51de32dce0eff923eced44a0dddcb7f363f",


This bundle image will also need to be changed to something that we expect to stay around longer.

OchiengEd · 2024-08-06T16:57:52Z

While reviewing, consider:

What should be threshold for an increase in computational resource utilization?
And should we error when above threshold is exceeded?
Should we add a comment with the benchmark results?
Informational logs in the function being benchmarked will break the benchmark format. As a workaround, should we remove the comments altogether or suppress them when benchmarks are running?

Signed-off-by: Edmund Ochieng <[email protected]>

This reverts commit f8ae501.

Signed-off-by: Edmund Ochieng <[email protected]>

Parse the results of the GH actions and return output that has 20% decrease in performance. Signed-off-by: Edmund Ochieng <[email protected]>

Signed-off-by: Edmund Ochieng <[email protected]>

Suppress logs from Unpack function which is causing errors to be written to the benchmark results. Signed-off-by: Edmund Ochieng <[email protected]>

Signed-off-by: Edmund Ochieng <[email protected]>

OchiengEd · 2024-08-13T21:43:33Z

During a meeting on the benchmarking of operator-controller functions held on 08/13/2024, the following action items were identified:

Committing a golden benchmark to the repository and comparing benchmarks when new PRs are created to the golden / reference benchmark
Set the threshold as low as possible, preferrably zero, and alert when there is any measurable change
Switch the analysis of the benchmark data to use Golang tools / modules from Pandas which is written in Python
Fail / Alert when a benchmark is removed from the golden benchmark or when a benchmark which does not exist in the Golden benchmark is added to a PR

openshift-merge-robot · 2024-08-24T09:21:32Z

PR needs rebase.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

OchiengEd · 2024-11-27T15:32:30Z

Will be revisited at a later time.

OchiengEd requested a review from a team as a code owner July 18, 2024 21:11

OchiengEd marked this pull request as draft July 18, 2024 21:11

openshift-ci bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jul 18, 2024

joelanford reviewed Jul 19, 2024

View reviewed changes

internal/contentmanager/contentmanager_test.go Outdated Show resolved Hide resolved

joelanford reviewed Jul 19, 2024

View reviewed changes

internal/contentmanager/contentmanager_test.go Outdated Show resolved Hide resolved

joelanford reviewed Jul 19, 2024

View reviewed changes

internal/authentication/tokengetter_test.go Outdated Show resolved Hide resolved

joelanford requested changes Jul 19, 2024

View reviewed changes

OchiengEd force-pushed the benchmark_tests branch from 3b6cb69 to 50c5f61 Compare July 24, 2024 15:20

OchiengEd force-pushed the benchmark_tests branch from 50c5f61 to eedef0e Compare July 25, 2024 20:32

OchiengEd commented Jul 26, 2024

View reviewed changes

OchiengEd force-pushed the benchmark_tests branch from f632473 to 311fe2b Compare July 31, 2024 18:40

OchiengEd requested a review from joelanford July 31, 2024 18:40

OchiengEd marked this pull request as ready for review July 31, 2024 18:41

openshift-ci bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jul 31, 2024

OchiengEd commented Aug 1, 2024

View reviewed changes

.github/workflows/benchmark.yaml Outdated Show resolved Hide resolved

tmshort reviewed Aug 1, 2024

View reviewed changes

internal/rukpak/source/image_registry.go Show resolved Hide resolved

OchiengEd commented Aug 5, 2024

View reviewed changes

OchiengEd added 9 commits August 8, 2024 15:51

feat: implement initial benchmark tests

e6e1672

Signed-off-by: Edmund Ochieng <[email protected]>

feat: add GH actions for running benchmarks

f8272a0

Signed-off-by: Edmund Ochieng <[email protected]>

tests: Test the unpack feature

8ae49f5

Signed-off-by: Edmund Ochieng <[email protected]>

test: add benchmark for RegistryV1ToHelmChart() function

47901c1

Signed-off-by: Edmund Ochieng <[email protected]>

Revert initial benchmark tests

52bea5b

This reverts commit f8ae501.

test: benchmark for the Resolve() function

28609e6

Signed-off-by: Edmund Ochieng <[email protected]>

test: benchmark FilesystemCache() function

32d43da

Signed-off-by: Edmund Ochieng <[email protected]>

ci: parse the results of benchstats

73ee125

Parse the results of the GH actions and return output that has 20% decrease in performance. Signed-off-by: Edmund Ochieng <[email protected]>

set the logger in controller runtime

364ba3e

Signed-off-by: Edmund Ochieng <[email protected]>

OchiengEd added 4 commits August 8, 2024 15:51

test: Suppress the logs when Unpack function

fe5b5ed

Suppress logs from Unpack function which is causing errors to be written to the benchmark results. Signed-off-by: Edmund Ochieng <[email protected]>

fix yaml lint error

6fa0a59

fix gci lint errors

a13ba5c

Signed-off-by: Edmund Ochieng <[email protected]>

evaluate the benchstat results for changes

1075eb3

Signed-off-by: Edmund Ochieng <[email protected]>

OchiengEd force-pushed the benchmark_tests branch from 42fafc7 to 1075eb3 Compare August 8, 2024 20:52

openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 24, 2024

OchiengEd closed this Nov 27, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

✨ feat: implement initial benchmark tests #1071

✨ feat: implement initial benchmark tests #1071

OchiengEd commented Jul 18, 2024

netlify bot commented Jul 18, 2024 •

edited

Loading

codecov bot commented Jul 18, 2024 •

edited

Loading

joelanford left a comment

OchiengEd commented Jul 24, 2024 •

edited

Loading

OchiengEd Jul 26, 2024

OchiengEd Aug 5, 2024

OchiengEd commented Aug 6, 2024

OchiengEd commented Aug 13, 2024 •

edited

Loading

openshift-merge-robot commented Aug 24, 2024

OchiengEd commented Nov 27, 2024

✨ feat: implement initial benchmark tests #1071

✨ feat: implement initial benchmark tests #1071

Conversation

OchiengEd commented Jul 18, 2024

Description

Reviewer Checklist

netlify bot commented Jul 18, 2024 • edited Loading

✅ Deploy Preview for olmv1 ready!

codecov bot commented Jul 18, 2024 • edited Loading

Codecov Report

joelanford left a comment

Choose a reason for hiding this comment

OchiengEd commented Jul 24, 2024 • edited Loading

OchiengEd Jul 26, 2024

Choose a reason for hiding this comment

OchiengEd Aug 5, 2024

Choose a reason for hiding this comment

OchiengEd commented Aug 6, 2024

OchiengEd commented Aug 13, 2024 • edited Loading

openshift-merge-robot commented Aug 24, 2024

OchiengEd commented Nov 27, 2024

netlify bot commented Jul 18, 2024 •

edited

Loading

codecov bot commented Jul 18, 2024 •

edited

Loading

OchiengEd commented Jul 24, 2024 •

edited

Loading

OchiengEd commented Aug 13, 2024 •

edited

Loading