Skip to content

feat: add basic exponential histogram #1736

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 14 commits into from
May 12, 2025

Conversation

xuan-cao-swi
Copy link
Contributor

@xuan-cao-swi xuan-cao-swi commented Sep 27, 2024

Description

This PR introduces basic functionality for exponential histograms, similar to Python and JavaScript. Most of the code is adapted/copied from Python. The only missing feature compared to other languages is merging.

Exponential histogram merging aims to make bucket sizes consistent. With the current otel-ruby metrics collection mechanism, raw data is sent unmodified to the collector (e.g., from @data_point).

I think it's best to implement the merge feature on the collector side to reduce duplicate work and computation on the client side. Downscaling is implemented on the language side to adjust bucket sizes, avoiding out-of-range indices for very large or small numbers.

Test

Most test cases are copied from Python/JavaScript to ensure the correctness of mapping and downscaling.

I didn't add all test case from Python as I think some of the test cases are redundant (e.g. 0 increment, etc.) but please correct me if I am wrong.

Copy link
Contributor

👋 This pull request has been marked as stale because it has been open with no activity. You can: comment on the issue or remove the stale label to hold stale off for a while, add the keep label to hold stale off permanently, or do nothing. If you do nothing this pull request will be closed eventually by the stale bot

Copy link
Contributor

@kaylareopelle kaylareopelle left a comment

Choose a reason for hiding this comment

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

Thank you, @xuan-cao-swi!

xuan-cao-swi and others added 5 commits January 10, 2025 11:04
…ial_bucket_histogram.rb

Co-authored-by: Kayla Reopelle <87386821+kaylareopelle@users.noreply.github.com>
…ial_bucket_histogram.rb

Co-authored-by: Kayla Reopelle <87386821+kaylareopelle@users.noreply.github.com>
…tial_bucket_histogram_test.rb

Co-authored-by: Kayla Reopelle <87386821+kaylareopelle@users.noreply.github.com>
@akahn
Copy link

akahn commented May 7, 2025

@xuan-cao-swi @kaylareopelle we would love to start using this in our app—is there any way I can support getting this change merged and released?

@kaylareopelle
Copy link
Contributor

Thanks for the ping on this, @akahn! I'll re-review this afternoon to see what's left before we can merge.

@kaylareopelle
Copy link
Contributor

kaylareopelle commented May 8, 2025

Exponential histogram merging aims to make bucket sizes consistent. With the current otel-ruby metrics collection mechanism, raw data is sent unmodified to the collector (e.g., from @data_point).

I think it's best to implement the merge feature on the collector side to reduce duplicate work and computation on the client side. Downscaling is implemented on the language side to adjust bucket sizes, avoiding out-of-range indices for very large or small numbers.

@xuan-cao-swi - Are other languages merging on the collector side? Or do their implementations merge within the SDK?

Update: I think this is a good experiment to see what users are comfortable with. If other languages don't require this, I think we should update to match their strategies. I don't think we need to tackle this before the first version is released.

Copy link
Contributor

@kaylareopelle kaylareopelle 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 everyone's patience with this PR. Provided the Rubocop problem is resolved, I'm comfortable merging this as a first step toward exponential histograms.

We need to make updates to the OTLP exporter to support the exponential histogram data points (at the moment, trying to export them will raise an error since the exporter matches on instrument, which resolves to :histogram).

We may also want to consider adding examples with different instruments/views to our existing examples repository.

…tial_histogram/buckets_test.rb

Co-authored-by: Kayla Reopelle <87386821+kaylareopelle@users.noreply.github.com>
@xuan-cao-swi
Copy link
Contributor Author

xuan-cao-swi commented May 9, 2025

Are other languages merging on the collector side? Or do their implementations merge within the SDK?

Other languages implement the merge strategy; but yes, I would agree let user try it out first, and if they prefer have merge logic, I can certainly add it.

We need to make updates to the OTLP exporter to support the exponential histogram data points (at the moment, trying to export them will raise an error since the exporter matches on instrument, which resolves to :histogram).

I can work on this

@kaylareopelle kaylareopelle merged commit 4069031 into open-telemetry:main May 12, 2025
65 checks passed
@github-project-automation github-project-automation bot moved this from To do to Done in Ruby - Metrics May 12, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

Implement ExponentialBucketHistogram aggregation
3 participants