-
Notifications
You must be signed in to change notification settings - Fork 493
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
metrics: make metrics easier to use with prometheus #4020
Conversation
Codecov Report
@@ Coverage Diff @@
## master #4020 +/- ##
==========================================
+ Coverage 49.97% 54.81% +4.83%
==========================================
Files 393 373 -20
Lines 68211 47699 -20512
==========================================
- Hits 34089 26146 -7943
+ Misses 30417 19333 -11084
+ Partials 3705 2220 -1485
Continue to review full report at Codecov.
|
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.
Looks good.
buf.WriteRune('\n') | ||
name = tc.Name + "_" + tag | ||
} | ||
buf.WriteString("# HELP ") |
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.
What is the value of adding HELP 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.
the HELP line provides a description of the metric for some UIs & backends use ... for example Grafana 6.6+ shows it alongside the metric name when you are browsing metrics https://grafana.com/blog/2020/06/15/how-we-made-working-with-prometheus-easier-with-metric-metadata-in-grafanas-explore-view/
Ok, but now you point out that |
Yes, I didn't want to go that far into it because most of the other metrics tests start up a MetricService and wait for it to report metrics, which requires Register and the reporter accessing registered metrics through the defaultRegister global. But I suppose if you know your metric test is is not running in parallel with any others, these other tests could assign over the global, or refactor some metrics methods to make it easier to work with non-globals. |
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.
Looked a little closer and I see that counter_test.go tests are kinda doing integration, but tagcounter_test.go could be isolated easily
- tc := NewTagCounter("tc", "wat")
+ tc := TagCounter{Name: "tc", Description: "wat"}
but, at least the counter_test.go don't call t.Parallel() so they plan on being run serially and isolating the use of the global that way.
Why deleted segments. Not used but might be useful on some point... |
Segment is unused, kinda like some profiling/trace patterns I've seen elsewhere, but we don't have any infrastructure for collecting or processing this data. I think when the time comes that we need something like this we should build what we actually need then. "Oh look, this exists, let's use it" is misleading in this case. |
I talked with @brianolson and @winder about it and we figured we could bring something conceptually similar back some day in the future if we need it. I think if we were to reimplement it, we could have a sort of timing histogram system that didn't rely on metrics.Counter, perhaps. |
Summary
Some changes to make it easier to find and use metrics with Prometheus
Test Plan
New test added to assert Prometheus output from WriteMetric for TagCounter and Counter.