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

Support exponential histograms in the prometheus bridge #5093

Merged

Conversation

dashpole
Copy link
Contributor

@dashpole dashpole commented Feb 14, 2024

Changes

Support converting exponential histograms in the prometheus bridge. To use exponential histograms, users can enable the NativeHistogram... options when creating their prometheus histogram.

When prometheus native histograms are enabled, BOTH fixed-bucket and native histograms are available in the protobuf in a "combined" histogram format. Since we can only support one-or-the-other, we always use exponential histograms if they are available.

Alternatives

As an alternative, we could consider adding an option, like WithExponentialHistograms() to the bridge that a user would additionally need to enable to get exponential histograms. If the prometheus client decided in the future to enable native histograms by default, users of the bridge would suddenly start getting exponential histograms instead of fixed-bucket histograms. If their backend doesn't support native histograms, this would be breaking for them. If there was not a way to disable that behavior in the Prometheus client, they would be stuck.

I have not implemented it here because we can always add it later if needed.

Implementation references

This follows the inverse of this specification: https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/compatibility/prometheus_and_openmetrics.md#exponential-histograms.

It is also the inverse of the conversion here: https://github.com/open-telemetry/opentelemetry-collector-contrib/blob/main/pkg/translator/prometheusremotewrite/histograms.go

Thankfully, OTel doesn't have a min or max scale, so we don't have to handle scale up/scale down.

Future work

Exemplars on native histograms are not yet supported in the prometheus client. They were added to the protocol here: prometheus/client_model#80, but hasn't been released yet, and hasn't been implemented in the prometheus go client yet.

@bwplotka do you think we can/should rely on NativeHistogram.* options in HistogramOpts to determine whether users get an exponential vs fixed-bucket histogram? Or should we introduce our own option on the bridge to be safe.

@dashpole dashpole added the enhancement New feature or request label Feb 14, 2024
@dashpole dashpole force-pushed the prometheus_bridge_exponential_histogram branch 2 times, most recently from c5b1e85 to b04ab2e Compare February 14, 2024 17:38
Copy link

codecov bot commented Feb 14, 2024

Codecov Report

Attention: 13 lines in your changes are missing coverage. Please review.

Comparison is base (dabfd13) 61.0% compared to head (c721ec7) 61.3%.
Report is 1 commits behind head on main.

Additional details and impacted files

Impacted file tree graph

@@           Coverage Diff           @@
##            main   #5093     +/-   ##
=======================================
+ Coverage   61.0%   61.3%   +0.2%     
=======================================
  Files        185     185             
  Lines      13760   13882    +122     
=======================================
+ Hits        8405    8511    +106     
- Misses      5160    5171     +11     
- Partials     195     200      +5     
Files Coverage Δ
bridges/prometheus/producer.go 89.0% <89.6%> (+0.1%) ⬆️

... and 1 file with indirect coverage changes

@dashpole dashpole force-pushed the prometheus_bridge_exponential_histogram branch from 54dcedd to 1a01ea9 Compare February 14, 2024 17:54
@dashpole dashpole marked this pull request as ready for review February 14, 2024 18:39
@dashpole dashpole requested a review from a team as a code owner February 14, 2024 18:39
bridges/prometheus/producer.go Outdated Show resolved Hide resolved
@dashpole dashpole force-pushed the prometheus_bridge_exponential_histogram branch from beacd35 to 40cdd42 Compare February 15, 2024 16:02
@dashpole dashpole force-pushed the prometheus_bridge_exponential_histogram branch from 40cdd42 to c721ec7 Compare February 15, 2024 16:03
@MrAlias MrAlias merged commit 65f3667 into open-telemetry:main Feb 15, 2024
27 checks passed
@dashpole dashpole deleted the prometheus_bridge_exponential_histogram branch February 15, 2024 19:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants