Use the otel-go expo-histogram mapping functions

[jmacd]

trafficstars

Description: This updates the logging exporter to use the otel-go mapping functions and changes the inclusivity. This depends (logically) on https://github.com/open-telemetry/opentelemetry-go/pull/2982 releasing (i.e., otel-go 1.10.x).

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.

[codecov[bot]]

Codecov Report

Base: 89.37% // Head: 89.41% // Increases project coverage by +0.04% :tada:

Coverage data is based on head (4e6693c) compared to base (272ebe6). Patch coverage: 100.00% of modified lines in pull request are covered.

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              

Impacted Files	Coverage Δ
...er/loggingexporter/internal/otlptext/databuffer.go	100.00% <100.00%> (ø)
...orter/loggingexporter/internal/otlptext/metrics.go	100.00% <100.00%> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

:umbrella: View full report at Codecov.
:loudspeaker: Do you have feedback about the report comment? Let us know in this issue.

[jmacd]

This is blocked, we need an otel-go release. I'll set this as a draft.

[github-actions[bot]]

This PR was marked stale due to lack of activity. It will be closed in 14 days.

[jmacd]

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

[jpkrohling]

What are the chances of having the code from https://github.com/lightstep/go-expohisto being part of otel-go?

[github-actions[bot]]

This PR was marked stale due to lack of activity. It will be closed in 14 days.

[github-actions[bot]]

This PR was marked stale due to lack of activity. It will be closed in 14 days.

[github-actions[bot]]

This PR was marked stale due to lack of activity. It will be closed in 14 days.

[bogdandrutu]

/cc @codeboten please review.

[jmacd]

With respect to the question about taking a dependency on the go-expohisto reference implementation: It's well documented that I briefly succeeded at placing this code into the otel-go metrics SDK. That code was withdrawn in the alpha release, and I feel it's not my fight at this point. I welcome the otel-go repository to use the go-expohisto implementation by reference or by copy.

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.

[jmacd]

@jack-berg PTAL

[github-actions[bot]]

This PR was marked stale due to lack of activity. It will be closed in 14 days.

[github-actions[bot]]

This PR was marked stale due to lack of activity. It will be closed in 14 days.

[jmacd]

Currently, the logging exporter prints exponential histograms incorrectly according to the specification. I would like it fixed.

[github-actions[bot]]

This PR was marked stale due to lack of activity. It will be closed in 14 days.

[github-actions[bot]]

Closed as inactive. Feel free to reopen if this PR is still being worked on.