Skip to content

[prometheusremotewritereceiver] add exponential histograms datapoints #39864

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

Merged
merged 27 commits into from
Jun 27, 2025

Conversation

perebaj
Copy link
Contributor

@perebaj perebaj commented May 6, 2025

Description

This PR partially implements the native histograms from prometheus v2 to the Otel exponential histograms. For now, it's missing some implementations that I take care on a next PR. Like:

  • Deal with scale
  • Deal with positive and negative spans
  • Maybe exemplars (I'm think that we can deal with exemplars in a next PR)
  • Deal with positive and negative buckets

Link to tracking issue

Related to #37277

@perebaj
Copy link
Contributor Author

perebaj commented May 6, 2025

Would appreciate the review from @carrieedwards @krajorama @ywwg and @ArthurSens

@perebaj
Copy link
Contributor Author

perebaj commented May 6, 2025

Guys in the section prometheus metric points -> OLTP we just have a documentation talking about histograms. Should there be something talking about native histograms -> exponential histograms?

Copy link
Member

@krajorama krajorama left a comment

Choose a reason for hiding this comment

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

Nice start, however I've added some comments on high level.

@perebaj perebaj changed the title [prometheusremotewritereceiver] add histograms datapoints [prometheusremotewritereceiver] add exponential histograms datapoints May 8, 2025
@perebaj perebaj marked this pull request as ready for review May 22, 2025 10:57
@perebaj perebaj requested a review from a team as a code owner May 22, 2025 10:57
Copy link
Member

@ArthurSens ArthurSens left a comment

Choose a reason for hiding this comment

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

I spent a very decent amount of time trying to review this PR and I feel like I don't have the necessary knowledge to do a decent review.

If @krajorama or @carrieedwards could do this for me so we can unblock your work, I'd be super grateful. In the meantime, I'll read up Carrie's document to learn more about this kind of histograms

@perebaj
Copy link
Contributor Author

perebaj commented May 22, 2025

I spent a very decent amount of time trying to review this PR and I feel like I don't have the necessary knowledge to do a decent review.

If @krajorama or @carrieedwards could do this for me so we can unblock your work, I'd be super grateful. In the meantime, I'll read up Carrie's document to learn more about this kind of histograms

Nice, Carry docs plus the krajo implementation here is the way!

Copy link
Member

@krajorama krajorama left a comment

Choose a reason for hiding this comment

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

I've left a couple of comments , should be close now.

@perebaj perebaj requested a review from krajorama June 25, 2025 13:56
Copy link
Member

@krajorama krajorama left a comment

Choose a reason for hiding this comment

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

The conversion LGTM.

I've tried testing with source->prometheus->otel collector->mimir , but had some issues around handling unsupported types (summary, classic histogram) which meant I couldn't test end to end, but the debug exporter showed that the conversion is ok.

Incidentally the debug exporter crashed for me consistently even without native histograms , so there's more to be fixed for sure.

Copy link
Member

@ArthurSens ArthurSens left a comment

Choose a reason for hiding this comment

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

Thanks for the patience while working on this one, it sure wasn't easy :)

@ArthurSens ArthurSens added the ready to merge Code review completed; ready to merge by maintainers label Jun 27, 2025
@mx-psi mx-psi merged commit ae204d9 into open-telemetry:main Jun 27, 2025
305 of 307 checks passed
@github-actions github-actions bot added this to the next release milestone Jun 27, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ready to merge Code review completed; ready to merge by maintainers receiver/prometheusremotewrite
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants