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

Use the github.com/lightstep/go-expohisto exponential histogram mapping functions #5938

Closed
wants to merge 24 commits into from

Conversation

jmacd
Copy link
Contributor

@jmacd jmacd commented Aug 19, 2022

Description:
This updates the logging exporter to use the an external library of mapping functions which corrects the inclusivity for OTel specification v1.13.0 (i.e., with upper-inclusive bucket boundaries). The external library was originally pending for inclusion in the OTel-Go SDK and may be adopted in that repository eventually.

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.

@jmacd jmacd added the Skip Changelog PRs that do not require a CHANGELOG.md entry label Aug 19, 2022
@jmacd jmacd requested review from a team and bogdandrutu August 19, 2022 21:55
@codecov
Copy link

codecov bot commented Aug 19, 2022

Codecov Report

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

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.

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

@jmacd
Copy link
Contributor Author

jmacd commented Sep 1, 2022

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

@github-actions
Copy link
Contributor

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

@jmacd jmacd marked this pull request as ready for review October 5, 2022 23:08
@jmacd jmacd changed the title Use the otel-go expo-histogram mapping functions Use the github.com/lightstep-goexpohisto exponential histogram mapping functions Oct 5, 2022
@jmacd jmacd changed the title Use the github.com/lightstep-goexpohisto exponential histogram mapping functions Use the github.com/lightstep/go-expohisto exponential histogram mapping functions Oct 5, 2022
@jmacd
Copy link
Contributor Author

jmacd commented Oct 13, 2022

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 open-telemetry/opentelemetry-collector-contrib#12951.

@jpkrohling
Copy link
Member

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

@github-actions
Copy link
Contributor

github-actions bot commented Nov 2, 2022

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

@github-actions github-actions bot added the Stale label Dec 6, 2022
@bogdandrutu
Copy link
Member

/cc @codeboten please review.

@github-actions github-actions bot removed the Stale label Dec 10, 2022
internal/testdata/metric.go Outdated Show resolved Hide resolved
internal/testdata/metric.go Outdated Show resolved Hide resolved
@jmacd
Copy link
Contributor Author

jmacd commented Dec 15, 2022

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
Copy link
Contributor Author

jmacd commented Dec 15, 2022

@jack-berg PTAL

Copy link
Member

@jack-berg jack-berg left a comment

Choose a reason for hiding this comment

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

Just spot checking for correctness. Looks ok to me.

@github-actions
Copy link
Contributor

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

@github-actions
Copy link
Contributor

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

@github-actions github-actions bot added the Stale label Jan 19, 2023
@jmacd
Copy link
Contributor Author

jmacd commented Feb 1, 2023

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

@github-actions github-actions bot removed the Stale label Feb 2, 2023
@github-actions
Copy link
Contributor

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

@github-actions github-actions bot added the Stale label Feb 16, 2023
@github-actions
Copy link
Contributor

github-actions bot commented Mar 2, 2023

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Skip Changelog PRs that do not require a CHANGELOG.md entry Stale
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants