opentelemetry-go icon indicating copy to clipboard operation
opentelemetry-go copied to clipboard

Published 20 hours ago • verified publisher icon open-telemetry

OTel Exponential Histogram implementation

Open jmacd opened this issue 3 years ago • 7 comments

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: https://github.com/open-telemetry/opentelemetry-collector-contrib/pull/6666.

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

jmacd avatar Jul 15 '22 07:07 jmacd

~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 avatar Jul 18 '22 15:07 jmacd

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 avatar Jul 18 '22 20:07 jmacd

This will fix #3027.

jmacd avatar Jul 18 '22 21:07 jmacd

~Needs to incorporate a tiny fix: https://github.com/lightstep/otel-launcher-go/pull/238~

jmacd avatar Aug 04 '22 00:08 jmacd

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.

jmacd avatar Aug 16 '22 06:08 jmacd

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%) :arrow_up:

codecov[bot] avatar Aug 25 '22 21:08 codecov[bot]

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 avatar Aug 25 '22 21:08 jmacd

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

jmacd avatar Oct 13 '22 15:10 jmacd