add histogram #1145
add histogram #1145
Conversation
💚 Build Succeeded
Expand to view the summary
Build stats
Test stats 🧪
🤖 GitHub commentsTo re-run your PR in the CI, just comment with:
|
the Add() method now takes a metricType argument, but I'm wondering if we should move to AddXXX similar to how there's AddHistogram.
seems a bit neater as an API
|
Tests are failing because of my use of |
There was a problem hiding this comment.
Looks good overall, just a few small things.
Tests are failing because of my use of math.Round, which was introduced in go1.10. Do we still want/need to support versions this old? go1.9 was released in august 2017.
I don't want to, but yes we need to until we release a v2 of the agent.
There's an internal package with a round implementation for older versions of Go: https://github.com/elastic/apm-agent-go/tree/master/internal/apmmath
shim for <go1.10, when math.Round was introduced
module/apmprometheus/gatherer.go
Outdated
| // we need to modify the current final bucket, | ||
| // and add an additional bucket with these | ||
| // observations. | ||
| if infBucketCount := totalCount - bucketCount; infBucketCount > 0 { |
There was a problem hiding this comment.
Is it worth adding a valuesLen > 0 check to this? I don't see why anyone would do that, but we probably shouldn't panic if they do.
There was a problem hiding this comment.
I don't think that can happen, since you can't create a histogram with 0 buckets, but it doesn't hurt to check
https://github.com/prometheus/client_golang/blob/master/prometheus/histogram.go#L221-L223
WIP for adding histogram support for prometheus.
The change to
Metricis very much "make it work". I'd prefer to either add some sort of metric type identifier within theMetrictype and some way to enforce validity within the type (BucketsandCountsshould both be set and the correct length,Valueshouldn't be set at the same time asBucketsandCounts), or some sort of metric type interface.Example sample: