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: add support for Graphite metrics provider #1406

Merged
merged 13 commits into from
Sep 20, 2021

Conversation

mdb
Copy link
Contributor

@mdb mdb commented Aug 10, 2021

Address issue #1403 by adding a Graphite metrics provider.

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.

@mdb mdb force-pushed the 1403-graphite-metrics-provider branch from 8815bce to 10cbafc Compare August 10, 2021 12:53
Address issue argoproj#1403 by adding a Graphite
metrics provider.

Signed-off-by: Mike Ball <[email protected]>
@mdb mdb force-pushed the 1403-graphite-metrics-provider branch from 10cbafc to 8f4eb59 Compare August 10, 2021 12:59
@mdb mdb marked this pull request as ready for review August 10, 2021 13:17
@mdb mdb force-pushed the 1403-graphite-metrics-provider branch from 007a999 to d1c71ac Compare August 10, 2021 16:41
pkg/apis/rollouts/v1alpha1/analysis_types.go Outdated Show resolved Hide resolved
metricproviders/graphite/graphite.go Outdated Show resolved Hide resolved
metricproviders/graphite/graphite.go Outdated Show resolved Hide resolved
metricproviders/graphite/api.go Outdated Show resolved Hide resolved
@codecov
Copy link

codecov bot commented Aug 24, 2021

Codecov Report

Merging #1406 (3558d5d) into master (180b6fb) will increase coverage by 0.06%.
The diff coverage is 87.91%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1406      +/-   ##
==========================================
+ Coverage   81.67%   81.73%   +0.06%     
==========================================
  Files         110      112       +2     
  Lines       14798    14947     +149     
==========================================
+ Hits        12086    12217     +131     
- Misses       2078     2091      +13     
- Partials      634      639       +5     
Impacted Files Coverage Δ
metricproviders/graphite/api.go 78.04% <78.04%> (ø)
metricproviders/graphite/graphite.go 100.00% <100.00%> (ø)

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 180b6fb...3558d5d. Read the comment docs.

mdb added 5 commits August 25, 2021 09:19
Per code review feedback...

> It's a bad idea to use the golang HTTP default client because of possibility of hung connections, since it does not timeout

As outlined here:
https://medium.com/@nate510/don-t-use-go-s-default-http-client-4804cb19f779

The 10s timeout matches the default of the webmetric provider's HTTP client.

Signed-off-by: Mike Ball <[email protected]>
mdb added a commit to mdb/argo-rollouts that referenced this pull request Aug 27, 2021
Previously, `graphite.API#Query` returned the value of
the last item in the `datapoints` array returned by
the Graphite API. However, this somewhat diverged from
the Prometheus metricprovider implementation and fails
to consider a use case for obtaining the first or an
arbitrary element from the datapoints array, as
discussed here:

argoproj#1406 (comment)

Signed-off-by: Mike Ball <[email protected]>
@mdb mdb requested a review from jessesuen August 27, 2021 21:34
Previously, `graphite.API#Query` returned the value of
the last item in the `datapoints` array returned by
the Graphite API. However, this somewhat diverged from
the Prometheus metricprovider implementation and fails
to consider a use case for obtaining the first or an
arbitrary element from the datapoints array, as
discussed here:

argoproj#1406 (comment)

Signed-off-by: Mike Ball <[email protected]>
@mdb mdb force-pushed the 1403-graphite-metrics-provider branch from b4ca2de to 91e32aa Compare August 27, 2021 21:36
@mdb mdb force-pushed the 1403-graphite-metrics-provider branch from 049c1aa to cb56197 Compare August 30, 2021 13:47
@mdb
Copy link
Contributor Author

mdb commented Sep 3, 2021

@jessesuen Thanks for your patience. I'm curious to hear your feedback on my latest commits. Have I addressed your concerns?

@harikrongali harikrongali added this to the v1.1 milestone Sep 13, 2021
@harikrongali
Copy link
Contributor

@mdb can you please rebase to latest master

@harikrongali harikrongali self-requested a review September 15, 2021 17:21
Copy link
Member

@jessesuen jessesuen left a comment

Choose a reason for hiding this comment

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

LGTM!

@sonarcloud
Copy link

sonarcloud bot commented Sep 20, 2021

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 1 Code Smell

No Coverage information No Coverage information
5.1% 5.1% Duplication

@jessesuen jessesuen merged commit 7c54090 into argoproj:master Sep 20, 2021
@mdb mdb deleted the 1403-graphite-metrics-provider branch September 20, 2021 15:14
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