Skip to content

fix: convert Prometheus histograms to non-cumulative otel histogram#6685

Merged
MrAlias merged 5 commits into
open-telemetry:mainfrom
mehran-prs:fix-prometheus-histogram
Jan 31, 2025
Merged

fix: convert Prometheus histograms to non-cumulative otel histogram#6685
MrAlias merged 5 commits into
open-telemetry:mainfrom
mehran-prs:fix-prometheus-histogram

Conversation

@mehran-prs
Copy link
Copy Markdown
Contributor

When we record a value using Prometheus histograms, it captures that value for that bucket and also previous buckets, but open telemetry histogram captures in a non-cumulative way, I mean it just captures for the matched bucket and doesn't add that value to the previous buckets.
This issue causes getting wrong histogram values when we use Prometheus bridge to send our data to open telemetry collector.
Please take a look at issue #4484 as an example of this issue.

Its fix was quick, so I made a PR, but let me know if I'm wrong or I should use the bridge in another way please.

@mehran-prs mehran-prs requested review from a team and dashpole as code owners January 26, 2025 21:02
@linux-foundation-easycla
Copy link
Copy Markdown

linux-foundation-easycla Bot commented Jan 26, 2025

CLA Signed

The committers listed above are authorized under a signed CLA.

Copy link
Copy Markdown
Contributor

@dashpole dashpole left a comment

Choose a reason for hiding this comment

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

Thanks! good find.

Copy link
Copy Markdown
Member

@dmathieu dmathieu left a comment

Choose a reason for hiding this comment

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

Could you add a changelog entry?

@codecov
Copy link
Copy Markdown

codecov Bot commented Jan 28, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 73.1%. Comparing base (01cfd20) to head (1688969).
Report is 1 commits behind head on main.

Additional details and impacted files

Impacted file tree graph

@@          Coverage Diff          @@
##            main   #6685   +/-   ##
=====================================
  Coverage   73.1%   73.1%           
=====================================
  Files        191     191           
  Lines      15866   15867    +1     
=====================================
+ Hits       11603   11607    +4     
+ Misses      3927    3925    -2     
+ Partials     336     335    -1     
Files with missing lines Coverage Δ
bridges/prometheus/producer.go 89.6% <100.0%> (+<0.1%) ⬆️

... and 1 file with indirect coverage changes

@MrAlias
Copy link
Copy Markdown
Contributor

MrAlias commented Jan 28, 2025

@mehran-prs thank you for the contribution. Please add a changelog entry so these changes can be accepted.

@MrAlias MrAlias added the response needed Waiting on user input before progress can be made label Jan 28, 2025
@mehran-prs
Copy link
Copy Markdown
Contributor Author

@dmathieu @MrAlias Sure. Added.

@mehran-prs mehran-prs force-pushed the fix-prometheus-histogram branch from 9cbc22a to b7923f6 Compare January 29, 2025 07:37
Comment thread CHANGELOG.md Outdated
Comment thread bridges/prometheus/producer_test.go
@dmathieu dmathieu removed the response needed Waiting on user input before progress can be made label Jan 29, 2025
@MrAlias MrAlias merged commit 8fb6fa9 into open-telemetry:main Jan 31, 2025
@MrAlias MrAlias added this to the v1.35.0 milestone Feb 6, 2025
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.

4 participants