-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
Use the github.com/lightstep/go-expohisto exponential histogram mapping functions #5938
Conversation
Codecov ReportBase: 89.37% // Head: 89.41% // Increases project coverage by
Additional details and impacted files@@ Coverage Diff @@
## main #5938 +/- ##
==========================================
+ Coverage 89.37% 89.41% +0.04%
==========================================
Files 286 286
Lines 14017 14073 +56
==========================================
+ Hits 12528 12584 +56
Misses 1219 1219
Partials 270 270
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report at Codecov. |
…tor into jmacd/histo_bounds_fix
…ry-collector into jmacd/histo_bounds_fix
This is blocked, we need an otel-go release. I'll set this as a draft. |
This PR was marked stale due to lack of activity. It will be closed in 14 days. |
…tor into jmacd/histo_bounds_fix
…tor into jmacd/histo_bounds_fix
Note that the corresponding change of exponential histogram boundary inclusivity has been released in specification v0.13, so at this point the collector is printing boundaries incorrectly. I have published Lightstep's production-tested exponential histogram data structure at https://github.com/lightstep/go-expohisto for use here and in open-telemetry/opentelemetry-collector-contrib#12951. |
What are the chances of having the code from https://github.com/lightstep/go-expohisto being part of otel-go? |
This PR was marked stale due to lack of activity. It will be closed in 14 days. |
/cc @codeboten please review. |
…tor into jmacd/histo_bounds_fix
…a valid boundary is subnormal
With respect to the question about taking a dependency on the go-expohisto reference implementation: Note that the otel-collector-contrib statsd receiver has taken a go-expohisto dependency already, and that the library exposes an interface that would not strictly be needed in the context of an otel-go SDK. The statsd receiver wants to count sampled metric events, which means supporting a histogram Increment() function for counts greater than 1, which means we're asking otel-go to maintain something on behalf of the collector, essentially, if we move it out of the lightstep repo. Note that this dependency, which is now limited to the logging exporter, is smaller than that dependency. The mapping functions are smaller than the data structure itself. As for the change here, and the fact that this code is complicated. The fact is, Prometheus forced OTel to change their histogram boundaries for reasons that they hold dear. We presented a ton of evidence that their request leads to more complicated implementations, but we in the end decided to do what they wanted. As a result, printing boundaries of exponential histograms correctly became substantially harder. If we want to print these absolutely correctly according to the specification, we need this code somewhere. Note also that nothing about being an SDK requires printing boundaries correctly, so how to print boundaries is not covered in the SDK specification, so the code here is somewhat opinionated. I would encourage us to accept this PR and let otel-go adopt the go-expohisto dependency at its own pace. |
@jack-berg PTAL |
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.
Just spot checking for correctness. Looks ok to me.
This PR was marked stale due to lack of activity. It will be closed in 14 days. |
This PR was marked stale due to lack of activity. It will be closed in 14 days. |
Currently, the logging exporter prints exponential histograms incorrectly according to the specification. I would like it fixed. |
This PR was marked stale due to lack of activity. It will be closed in 14 days. |
Closed as inactive. Feel free to reopen if this PR is still being worked on. |
Description:
This updates the logging exporter to use the an external library of mapping functions which corrects the inclusivity for OTel specification v1.13.0 (i.e., with upper-inclusive bucket boundaries). The external library was originally pending for inclusion in the OTel-Go SDK and may be adopted in that repository eventually.
This addresses boundary conditions more correctly; with this PR the logging exporter is able to correctly print the upper boundary of the last valid bucket. Extreme values will print as "OVERFLOW" or "UNDERFLOW" in cases where this logic is unable to produce a correct string representation.
This leaves a TODO allowing more work to format subnormal values in certain corner cases.
Link to tracking Issue:
Part of #4197
Testing: New tests introduced.