-
Notifications
You must be signed in to change notification settings - Fork 187
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
Adds Prometheus export (requests and variant eval) #221
Conversation
Codecov Report
@@ Coverage Diff @@
## master #221 +/- ##
==========================================
- Coverage 85.43% 83.87% -1.56%
==========================================
Files 24 24
Lines 1387 1433 +46
==========================================
+ Hits 1185 1202 +17
- Misses 146 174 +28
- Partials 56 57 +1
Continue to review full report at Codecov.
|
return | ||
} | ||
config.Global.Prometheus.EvalCounter.WithLabelValues( | ||
util.SafeStringWithDefault(r.EvalContext.EntityType, "null"), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not very familiar with the convention of prometheus lable values, does it need to have a name in the value? For example,
fmt.Sprintf("flag_id:%s", util.SafeStringWithDefault(r.FlagID, "null"))
Otherwise, how can we know if it's variantID or flagID, or something else?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@zhouzhuojie that's a bit confusing, yeah - the labels are coded in when the Counter is created here: https://github.com/checkr/flagr/pull/221/files#diff-091ace02b49a46f55f2586cfec89a160R96
|
||
func TestSetupPrometheusWithLatencies(t *testing.T) { | ||
prometheus.DefaultRegisterer = prometheus.NewRegistry() | ||
Config.PrometheusEnabled = true |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you reset the configuration after the test?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sure, no problem
Config.PrometheusEnabled = false | ||
setupPrometheus() | ||
assert.Nil(t, Global.Prometheus.EvalCounter) | ||
Config.PrometheusEnabled = true |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done, just pushed
PrometheusEnabled bool `env:"FLAGR_PROMETHEUS_ENABLED" envDefault:"false"` | ||
// PrometheusPath - set the path on which prometheus metrics are available to scrape | ||
PrometheusPath string `env:"FLAGR_PROMETHEUS_PATH" envDefault:"/metrics"` | ||
// PrometheusIncludeLatencyHistogram - set whether Prometheus should also export a histogram of request latencies (this increases cardinality significantly) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1, good to know
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🎉 lgtm
@zhouzhuojie thanks! |
Description
This PR adds a Prometheus exporter based on the prometheus client-go package. It exports metrics regarding the requests (a count of requests as well as optionally a histogram of request latencies), and a count of variant allocations per flag (similar to datadog statsd).
Motivation and Context
Closes #196
We are trialing flagr at Ecosia, and we use Prometheus for monitoring all our systems running Kubernetes.
How Has This Been Tested?
Added unit tests for config, including creating the prometheus metrics (which will panic if they cannot be created). I also built the docker image and am running it in our dev cluster.
Types of changes
Checklist: