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

Add Graphite metrics provider #915

Merged
merged 4 commits into from
May 28, 2021

Conversation

mdb
Copy link
Contributor

@mdb mdb commented May 24, 2021

This adds a Graphite metric provider, closes #911.

Co-authored-by: Mike Ball mike.ball@warnermedia.com
Co-authored-by: Nathan Mische Nathan.Mische@warnermedia.com
Signed-off-by: Mike Ball mike.ball@warnermedia.com
Signed-off-by: Nathan Mische Nathan.Mische@warnermedia.com

This adds a Graphite metric provider to address
issue fluxcd#911.

Co-authored-by: Mike Ball <mike.ball@warnermedia.com>
Co-authored-by: Nathan Mische <Nathan.Mische@warnermedia.com>
Signed-off-by: Mike Ball <mike.ball@warnermedia.com>
Signed-off-by: Nathan Mische <Nathan.Mische@warnermedia.com>
@mdb mdb requested a review from stefanprodan as a code owner May 24, 2021 13:23
Copy link
Member

@stefanprodan stefanprodan left a comment

Choose a reason for hiding this comment

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

Please create a section for Graphite in the metrics docs https://github.com/fluxcd/flagger/blob/main/docs/gitbook/usage/metrics.md

@codecov-commenter
Copy link

codecov-commenter commented May 24, 2021

Codecov Report

Merging #915 (cd6f363) into main (39a3898) will increase coverage by 0.10%.
The diff coverage is 62.50%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #915      +/-   ##
==========================================
+ Coverage   56.78%   56.89%   +0.10%     
==========================================
  Files          69       70       +1     
  Lines        5727     5825      +98     
==========================================
+ Hits         3252     3314      +62     
- Misses       1990     2020      +30     
- Partials      485      491       +6     
Impacted Files Coverage Δ
pkg/metrics/providers/factory.go 0.00% <0.00%> (ø)
pkg/metrics/providers/graphite.go 63.82% <63.82%> (ø)
pkg/canary/config_tracker.go 83.40% <0.00%> (+0.15%) ⬆️

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 39a3898...cd6f363. Read the comment docs.

@mdb mdb force-pushed the graphite-metrics-provider branch from fa74b96 to 560fe14 Compare May 24, 2021 21:42
This adds documentation of the Graphite metrics
provider in support of addressing issue fluxcd#911.

Co-authored-by: Mike Ball <mike.ball@warnermedia.com>
Co-authored-by: Nathan Mische <Nathan.Mische@warnermedia.com>
Signed-off-by: Mike Ball <mike.ball@warnermedia.com>
Signed-off-by: Nathan Mische <Nathan.Mische@warnermedia.com>
@mdb mdb force-pushed the graphite-metrics-provider branch from 560fe14 to 6a66113 Compare May 24, 2021 21:45
@mdb mdb requested a review from stefanprodan May 24, 2021 21:45
target=summarize(
asPercent(
sumSeries(
stats.timers.httpServerRequests.exception.*.method.*.outcome.{CLIENT_ERROR,INFORMATIONAL,REDIRECTION,SUCCESS}.status.*.uri.*.count
Copy link
Member

Choose a reason for hiding this comment

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

Was this tested? I haven't used Graphite in ages but this query seem wrong to me, also it doesn't filter the canary pods.

Copy link
Contributor

Choose a reason for hiding this comment

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

@stefanprodan: We have updated the query to include the {{target}} parameter. One question we do have, is the query intended to consider metrics for both the primary and canary pods, or just the canary pods?

As for the query's correctness, we went ahead and set up a demo Spring Boot app that ships metrics to graphite to test this out. You can find the example here: https://github.com/nmische/spring-boot-graphite. Based on some testing with that application we believe this query correctly calculates the request success rate using the default metrics generated by Spring Boot (with an additional tag app configured to identify the application).

Copy link
Member

@stefanprodan stefanprodan May 26, 2021

Choose a reason for hiding this comment

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

The builtin checks are for the canary pods only, my assumption is that users don't want a canary analysis to fail if the older version (primary) is running into errors, imagine you have some bug in production, if you look at both versions you'll never manage to deliver a fix.

Assuming that your app uses the Kubernetes Deployment name when it reports to Graphite, then the current query is ok, as the primary app name will be {{target}}-primary thus its metrics are not included in the query.

@mdb mdb force-pushed the graphite-metrics-provider branch from 3788139 to c99ac18 Compare May 25, 2021 15:53
Co-authored-by: Mike Ball <mike.ball@warnermedia.com>
Co-authored-by: Nathan Mische <Nathan.Mische@warnermedia.com>
Signed-off-by: Mike Ball <mike.ball@warnermedia.com>
Signed-off-by: Nathan Mische <Nathan.Mische@warnermedia.com>
@mdb mdb force-pushed the graphite-metrics-provider branch from c99ac18 to 8e3ee34 Compare May 25, 2021 15:56
@mdb mdb requested a review from stefanprodan May 25, 2021 20:42
Copy link
Member

@stefanprodan stefanprodan left a comment

Choose a reason for hiding this comment

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

LGTM

Thanks Mike & Nathan 🏅

@stefanprodan
Copy link
Member

stefanprodan commented May 26, 2021

One last thing missing: in here https://github.com/fluxcd/flagger/blob/main/artifacts/flagger/crd.yaml#L1104 there should be graphite otherwise Kubernetes will reject the canary. After you add it there please run make crd and push all changes.

…te crd

Co-authored-by: Mike Ball <mike.ball@warnermedia.com>
Co-authored-by: Nathan Mische <Nathan.Mische@warnermedia.com>
Signed-off-by: Mike Ball <mike.ball@warnermedia.com>
Signed-off-by: Nathan Mische <Nathan.Mische@warnermedia.com>
@stefanprodan stefanprodan changed the title add a Graphite metrics provider Add Graphite metrics provider May 26, 2021
@stefanprodan stefanprodan added the kind/feature Feature request label May 28, 2021
@stefanprodan stefanprodan merged commit 4869a9f into fluxcd:main May 28, 2021
@mdb mdb deleted the graphite-metrics-provider branch May 28, 2021 15:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/feature Feature request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Feature request: Support Graphite metrics provider
4 participants