-
Notifications
You must be signed in to change notification settings - Fork 117
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
Reporting Histogram Sum Metric #208
Comments
@brawndou any suggestions for this? In addition, I'm also trying to see if there is support for summary metrics. For eg. zookeeper provides metrics in Prometheus format such as
And
Do you know what reporter function can I use to record these summary metrics? |
I believe this is a limitation with how Tally ingests histogram metrics in the Scope. Line 367 in ecb4ac9
When a histogram value is recorded in the scope, the scope will store/cache the value as the upper bound of the bucket it belongs in. When the scope tells the Prometheus reporter to report these histogram metrics, it flushes out the bucket upper bound value instead of the actual observed value: Line 207 in ecb4ac9
As a result, the sum prometheus generates won't be accurate, but rather an approximation. Here's a gist that shows this behavior: To fix this, there could be a workaround by using a counter to keep track of a |
Hey folks,
I'm trying to get some clarification on Histogram reporting for prometheus metrics. A sample histogram metric in Prometheus format looks like
From our exercise, we are seeing that
<histogram_metric>_sum
metric is being ingested but not published/reported using the Histogram struct. As a result, we are not able to calculate averages.If I'm not mistaken, this suggests scope for a patch to support
_sum
metric (http_request_duration_seconds_sum
from the above example). And an addition of a field such assum float64
to the above struct.To avoid introducing breaking changes, can we add another method
func (h *histogram) RecordSum(sum float64)
which will allow us to update the Histogram struct.Please comment on the reasonability of this issue. And if there's something that we are missing. If this seems reasonable, my team will be happy to contribute with a patch upstream.
Thanks
The text was updated successfully, but these errors were encountered: