Skip to content
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

Closed
wants to merge 64 commits into from

Conversation

jmacd
Copy link
Contributor

@jmacd jmacd commented Nov 15, 2021

This implementation of the OTLP v0.11 exponential histogram provides an aggregator for calculating ExponentialHistogramDataPoint values from individual measurements. There are two configurable parameters:

  1. the maximum size: unless range limits are set, the implementation auto-scales to fit the observed values into the available buckets
  2. the range limits (optional): when configured, the scale is fixed to allow the full range of values at the best resolution possible given the maximum size.

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).

@yzhuge
Copy link

yzhuge commented Dec 21, 2021

I only had a cursory review on this one. And I am not fluent in Go. Overall looks good. A few minor comments:

  • You may want to document that underflow/overflow errors will be returned if the "update" value is beyond the configured min/max limit.

  • exponential.go L86: // the less than equal bucket count for the pre-determined boundaries.
    Is "less than" still true? or should this be "greater than"

  • exponential.go
    L309: // Count implements aggregation.Sum.
    L310: func (a *Aggregator) Count() (uint64, error) {
    Comment/code mismatch?

@codecov
Copy link

codecov bot commented Jan 7, 2022

Codecov Report

Merging #2393 (77d01c9) into main (1e4a609) will increase coverage by 0.4%.
The diff coverage is 85.8%.

Impacted file tree graph

@@           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     
Impacted Files Coverage Δ
...nternal/mapping/logarithm/cmd/prebuild/prebuild.go 0.0% <0.0%> (ø)
sdk/metric/export/aggregation/aggregation.go 100.0% <ø> (ø)
...otlp/otlpmetric/internal/metrictransform/metric.go 75.5% <64.1%> (-2.6%) ⬇️
sdk/metric/aggregator/exponential/exponential.go 96.8% <96.8%> (ø)
.../exponential/internal/mapping/exponent/exponent.go 100.0% <100.0%> (ø)
...r/exponential/internal/mapping/exponent/float64.go 100.0% <100.0%> (ø)
...xponential/internal/mapping/logarithm/logarithm.go 100.0% <100.0%> (ø)
sdk/metric/selector/simple/simple.go 100.0% <100.0%> (ø)
sdk/trace/batch_span_processor.go 83.4% <0.0%> (-2.1%) ⬇️
sdk/export/metric/aggregation/temporality.go 0.0% <0.0%> (ø)
... and 1 more

@jmacd
Copy link
Contributor Author

jmacd commented Jan 7, 2022

@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.

@jmacd
Copy link
Contributor Author

jmacd commented Jan 7, 2022

(Closing this anyway; I found a way to simplify the logarithm implementation somewhat.)

@jmacd
Copy link
Contributor Author

jmacd commented Jan 7, 2022

@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 math/big.

I will change the code to calculate and cache these constants, not build them in.
For the largest finite bucket, instead of storing a value calculated via math/big I'll use the alternate formula given in open-telemetry/opentelemetry-specification#2252.

@jmacd
Copy link
Contributor Author

jmacd commented Jan 7, 2022

I will re-open this PR after #2502, potentially.

@sk-
Copy link

sk- commented Apr 14, 2022

@jmacd Now that #2502 was merged do you plan on revisiting this PR?

@jmacd
Copy link
Contributor Author

jmacd commented Apr 15, 2022

Yes, after open-telemetry/opentelemetry-specification#2252, which will merge in the next few days.

@github-actions github-actions bot mentioned this pull request May 7, 2022
@BrandonArp
Copy link

@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

@jmacd
Copy link
Contributor Author

jmacd commented May 17, 2022

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 was referenced May 18, 2022
jmacd added a commit to jmacd/opentelemetry-go that referenced this pull request Jun 27, 2022
@github-actions github-actions bot mentioned this pull request Aug 3, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants