-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
Exponential histogram aggregator & export support #2393
Conversation
…o jmacd/expohistoaggro
…o jmacd/expohistoaggro
…o jmacd/expohistoaggro
…o jmacd/expohistoaggro
…o jmacd/expohistoaggro
…o jmacd/expohistoaggro
I only had a cursory review on this one. And I am not fluent in Go. Overall looks good. A few minor comments:
|
…o jmacd/expohistoaggro
Codecov Report
@@ Coverage Diff @@
## main #2393 +/- ##
=======================================
+ Coverage 76.3% 76.8% +0.4%
=======================================
Files 174 181 +7
Lines 12027 12810 +783
=======================================
+ Hits 9184 9842 +658
- Misses 2598 2712 +114
- Partials 245 256 +11
|
@MrAlias and potential reviewers. I have a proposal to split this PR into two parts. The first part will introduce only the mapping functions. In the present PR I have these in an internal package, but these are the functions needed in the OTel collector to support printing exponential histogram data points. If you agree, I propose to close this PR and resubmit it as two: (1) public mapping functions, (2) the new aggregator. |
(Closing this anyway; I found a way to simplify the logarithm implementation somewhat.) |
@MrAlias Re: open-telemetry/opentelemetry-specification#2252 The "somewhat simplified" change I have in mind is hinted at in the datamodel example there -- in the present state of this PR I have prebuilt a table of histogram settings for scales -10 through 20; the calculation is trivial and leads to no allocations at runtime, but I was using a separate program to generate the table as code. The reason this was helpful is that it (a) the largest finite bucket boundary is somewhat difficult to compute, (b) if you're pregenerating code, the value can be calculated via I will change the code to calculate and cache these constants, not build them in. |
I will re-open this PR after #2502, potentially. |
Yes, after open-telemetry/opentelemetry-specification#2252, which will merge in the next few days. |
@jmacd how are things looking here? open-telemetry/opentelemetry-specification#2252 looks like it got merged and I'm excited to be able to use exponential histograms |
I am working on bringing this code forward. I will check with the maintainers about whether they're willing to review and approve this as a "back port" to the current SDK while the new SDK is in development. For the moment, I am building and testing this in a branch of a new metrics SDK code base being hosted at Lightstep. |
This implementation of the OTLP v0.11 exponential histogram provides an aggregator for calculating ExponentialHistogramDataPoint values from individual measurements. There are two configurable parameters:
This implementation includes a README documenting the implementation strategy and several points about the treatment subnormal values. This is meant to be used as a reference implementation, for producing exponential histograms according to the OTel data model.
Although this type is not specified in the currently-frozen Metrics SDK specification, it has been specified in the data model. Merging this code now will allow this implementation to be used both for experimental purposes (e.g., vendors supporting OTLP v0.11) and inside the OpenTelemetry collector (e.g., open-telemetry/opentelemetry-collector-contrib#6666).