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

feat(analysis): Added additional metadata to the status of AnalysisRun to be used by Prometheus Provider #1628

Merged
merged 2 commits into from
Feb 2, 2022

Conversation

agrawroh
Copy link
Member

@agrawroh agrawroh commented Nov 8, 2021

Background

Currently, AnalsysRun doesn't display the prometheus query with resolved arguments which makes it a little bit harder to do any troubleshooting on the AnalysisTemplate.

See: #1591

Changes

In this PR, we are adding an additional Metadata in the metric results so that providers can use it to store any additional metadata they'd like to display in the AnalysisRuns. In case of prometheus, we store the final resolved query by substituting the template arguments.

Note: This is only addressing the Prometheus Provider as of now.

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
Copy link

codecov bot commented Nov 8, 2021

Codecov Report

Merging #1628 (416c181) into master (53643c5) will decrease coverage by 0.32%.
The diff coverage is 100.00%.

❗ Current head 416c181 differs from pull request most recent head 213fa47. Consider uploading reports for the commit 213fa47 to get more accurate results
Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1628      +/-   ##
==========================================
- Coverage   82.30%   81.98%   -0.33%     
==========================================
  Files         116      115       -1     
  Lines       16370    15918     -452     
==========================================
- Hits        13474    13050     -424     
+ Misses       2217     2198      -19     
+ Partials      679      670       -9     
Impacted Files Coverage Δ
analysis/analysis.go 84.77% <100.00%> (-1.32%) ⬇️
metricproviders/cloudwatch/cloudwatch.go 72.97% <100.00%> (+0.49%) ⬆️
metricproviders/datadog/datadog.go 77.93% <100.00%> (+0.30%) ⬆️
metricproviders/graphite/graphite.go 100.00% <100.00%> (ø)
metricproviders/job/job.go 86.89% <100.00%> (+0.18%) ⬆️
metricproviders/kayenta/kayenta.go 84.47% <100.00%> (+0.19%) ⬆️
metricproviders/newrelic/newrelic.go 91.96% <100.00%> (+0.14%) ⬆️
metricproviders/prometheus/prometheus.go 100.00% <100.00%> (ø)
metricproviders/wavefront/wavefront.go 94.40% <100.00%> (+0.09%) ⬆️
metricproviders/webmetric/webmetric.go 67.76% <100.00%> (+0.54%) ⬆️
... and 53 more

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 53643c5...213fa47. Read the comment docs.

@agrawroh agrawroh force-pushed the add-resolved-queries-info branch 2 times, most recently from 9dfd946 to 80dc1ee Compare November 8, 2021 04:13
@huikang
Copy link
Member

huikang commented Nov 10, 2021

Hi, @agrawroh , it would be nice if we can add some explanation about the change to the doc: https://github.com/argoproj/argo-rollouts/blob/master/docs/analysis/prometheus.md

Thanks.

@agrawroh
Copy link
Member Author

@jessesuen Could you help merge this PR?

@agrawroh agrawroh requested a review from jessesuen November 11, 2021 01:31
@agrawroh agrawroh force-pushed the add-resolved-queries-info branch from 417bcac to 3659a12 Compare November 11, 2021 17:38
@sonarqubecloud
Copy link

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
6.7% 6.7% Duplication

@agrawroh agrawroh force-pushed the add-resolved-queries-info branch from 997cceb to eb65c03 Compare February 1, 2022 20:42
Signed-off-by: Rohit Agrawal <[email protected]>
@agrawroh agrawroh force-pushed the add-resolved-queries-info branch from eb65c03 to 213fa47 Compare February 1, 2022 23:16
@sonarqubecloud
Copy link

sonarqubecloud bot commented Feb 1, 2022

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
3.8% 3.8% Duplication

@harikrongali
Copy link
Contributor

@agrawroh seems PR addresses only Prometheus provider. can you add a description to the ticket that current PR address Prometheus provider only?

@agrawroh
Copy link
Member Author

agrawroh commented Feb 2, 2022

@agrawroh seems PR addresses only Prometheus provider. can you add a description to the ticket that current PR address Prometheus provider only?

@huikang Done! Thanks

@agrawroh agrawroh changed the title feat(analysis): Added additional metadata to the status of AnalysisRun feat(analysis): Added additional metadata to the status of AnalysisRun tobe used by Prometheus Provider Feb 2, 2022
@harikrongali
Copy link
Contributor

lgtm. apart from description update

@agrawroh agrawroh changed the title feat(analysis): Added additional metadata to the status of AnalysisRun tobe used by Prometheus Provider feat(analysis): Added additional metadata to the status of AnalysisRun to be used by Prometheus Provider Feb 2, 2022
@huikang huikang merged commit f388937 into argoproj:master Feb 2, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants