feat(analysis): Add Measurements Retention Limit Option for Metrics

[agrawroh]

Description

It could be very useful to retain the maximum data points while running an analysis in the dry-run mode to understand what caused a particular metric to error out or why the final result came out as inconclusive.

Currently, the controller only retains the ten latest measurements and it's not possible to configure/change this value as its hard-coded in the code.

Changes

This PR adds a new option called measurementRetention which can be used to retain other than the latest ten results for the metrics running in any mode (dry/non-dry). Setting this option to 0 would disable it and, the controller will revert back to the existing behavior of retaining the latest ten measurements.

Checklist

• Either (a) I've created an enhancement proposal and discussed it with the community, (b) this is a bug fix, or (c) this is a chore.
• The title of the PR is (a) conventional, (b) states what changed, and (c) suffixes the related issues number. E.g. "fix(controller): Updates such and such. Fixes #1234".
• I've signed my commits with DCO
• I have written unit and/or e2e tests for my change. PRs without these are unlikely to be merged.
• My builds are green. Try syncing with master if they are not.
• My organization is added to USERS.md.

---

Signed-off-by: Rohit Agrawal [email protected]

[codecov]

Codecov Report

Merging #1729 (ee91d9c) into master (a3477cf) will increase coverage by 0.01%.
The diff coverage is 91.42%.

@@            Coverage Diff             @@
##           master    #1729      +/-   ##
==========================================
+ Coverage   82.02%   82.04%   +0.01%     
==========================================
  Files         116      116              
  Lines       16096    16126      +30     
==========================================
+ Hits        13203    13230      +27     
- Misses       2218     2220       +2     
- Partials      675      676       +1     

Impacted Files	Coverage Δ
utils/analysis/helpers.go	78.08% <81.25%> (+0.14%)	⬆️
analysis/analysis.go	86.09% <100.00%> (+0.35%)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a3477cf...ee91d9c. Read the comment docs.

[sonarcloud]

Kudos, SonarCloud Quality Gate passed!   

0 Bugs
0 Vulnerabilities
0 Security Hotspots
5 Code Smells

No Coverage information
7.2% Duplication

[jessesuen]

This looks mostly good but I do not think the spec.measurementRetention is honored when specified from an AnalysisTemplate.

I think a change to utils/analysis/helpers.go::NewAnalysisRunFromTemplates needs to be added similar to the dryRun feature.

[agrawroh]

This looks mostly good but I do not think the spec.measurementRetention is honored when specified from an AnalysisTemplate.

I think a change to utils/analysis/helpers.go::NewAnalysisRunFromTemplates needs to be added similar to the dryRun feature.

@jessesuen Yes, I think we need to add it to NewAnalysisRunFromTemplates for the analysis being created from Rollouts/Experiments and I was thinking of doing it in a follow-up PR. AnalysisTemplate itself should work fine with the change we have here.

If you think that we should just include all the changes for Rollouts/Experiments in the same PR then I can add everything to this PR.

[jessesuen]

If you think that we should just include all the changes for Rollouts/Experiments in the same PR then I can add everything to this PR.

I am okay with a follow-up PR that takes advantage of the feature.