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

OTel Exponential Histogram implementation #3022

Closed
wants to merge 20 commits into from

Conversation

jmacd
Copy link
Contributor

@jmacd jmacd commented Jul 15, 2022

The implementation originally shown in #2393 used to finalize the OpenTelemetry specification and was split into parts to begin reviewing in pieces. The mapping functions were merged in #2502, and this is the corresponding data structure.

This data structure has been reviewed by Lightstep engineers and is running in production at Lightstep (i.e., for internal use). This code and the associated metrictransform logic (forked from upstream) is currently released at https://github.com/lightstep/otel-launcher-go/tree/v1.8.0/lightstep/sdk/metric/aggregator/histogram.

This code will be used to revive the OTel Collector statsd receiver: open-telemetry/opentelemetry-collector-contrib#6666.

Fixes #3027.
Part of #2966.
Related to #2501 and #2502.

@jmacd
Copy link
Contributor Author

jmacd commented Jul 18, 2022

This needs a bit more factoring work. I will move the lock out of the structure and add a Swap method to replace Move (requested by @paivagustavo).

@jmacd jmacd marked this pull request as draft July 18, 2022 15:17
@jmacd jmacd marked this pull request as ready for review July 18, 2022 17:59
@jmacd
Copy link
Contributor Author

jmacd commented Jul 18, 2022

Because this uses Go-1.18 features, it makes sense to merge this to the new_sdk/main branch. OTOH, we have sdk/metric/aggregator/exponential/mapping already in place, and this builds on that work. If I change this branch to merge into new_sdk/main, I will have to copy the existing sdk/metric/aggregator/exponential/mapping code into that branch as well.

@jmacd
Copy link
Contributor Author

jmacd commented Jul 18, 2022

This will fix #3027.

@jmacd
Copy link
Contributor Author

jmacd commented Aug 4, 2022

Needs to incorporate a tiny fix: lightstep/otel-launcher-go#238

Copy link

@aallawala aallawala left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👋 My team and I are interested in this implementation and are actively following along. I'm looking at pulling this change in (via the launcher-go SDK) this week to test in our environment and will raise anything we see. Thank you!

@jmacd
Copy link
Contributor Author

jmacd commented Aug 16, 2022

I believe this is blocked until #3081 merges, since it depends on Go-1.18+ and cannot build with 1.17. Otherwise, ready to review.

@codecov
Copy link

codecov bot commented Aug 25, 2022

Codecov Report

Merging #3022 (9296a28) into main (55b49c4) will increase coverage by 0.7%.
The diff coverage is 96.9%.

Additional details and impacted files

Impacted file tree graph

@@           Coverage Diff           @@
##            main   #3022     +/-   ##
=======================================
+ Coverage   76.2%   77.0%   +0.7%     
=======================================
  Files        179     182      +3     
  Lines      11953   12376    +423     
=======================================
+ Hits        9117    9531    +414     
- Misses      2596    2601      +5     
- Partials     240     244      +4     
Impacted Files Coverage Δ
...ic/aggregator/exponential/structure/exponential.go 96.5% <96.5%> (ø)
.../metric/aggregator/exponential/structure/config.go 100.0% <100.0%> (ø)
...dk/metric/aggregator/exponential/structure/test.go 100.0% <100.0%> (ø)
sdk/trace/batch_span_processor.go 81.9% <0.0%> (+1.7%) ⬆️

@jmacd
Copy link
Contributor Author

jmacd commented Aug 25, 2022

Commit e85aad4 was a bit painful because there were many hand-crafted tests that were off-by-one in different ways after the inclusivity change #2982. I've resolved some of them by changing the inputs (2x) and some of them by subtracting one from the expected values.

@jmacd
Copy link
Contributor Author

jmacd commented Oct 13, 2022

There are now slight upstream changes (e.g., an improved comment). https://github.com/lightstep/go-expohisto

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.

Expose exponential histogram data structure for public use
4 participants