-
Notifications
You must be signed in to change notification settings - Fork 263
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
Conversation
👋 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 |
There was a problem hiding this 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!
metrics_sdk/lib/opentelemetry/sdk/metrics/aggregation/exponential_bucket_histogram.rb
Outdated
Show resolved
Hide resolved
metrics_sdk/lib/opentelemetry/sdk/metrics/aggregation/exponential_bucket_histogram.rb
Outdated
Show resolved
Hide resolved
metrics_sdk/lib/opentelemetry/sdk/metrics/aggregation/exponential_bucket_histogram.rb
Outdated
Show resolved
Hide resolved
metrics_sdk/lib/opentelemetry/sdk/metrics/aggregation/exponential_bucket_histogram.rb
Outdated
Show resolved
Hide resolved
metrics_sdk/lib/opentelemetry/sdk/metrics/aggregation/exponential_bucket_histogram.rb
Show resolved
Hide resolved
metrics_sdk/lib/opentelemetry/sdk/metrics/aggregation/exponential_bucket_histogram.rb
Outdated
Show resolved
Hide resolved
metrics_sdk/lib/opentelemetry/sdk/metrics/aggregation/exponential_histogram/buckets.rb
Show resolved
Hide resolved
metrics_sdk/test/opentelemetry/sdk/metrics/aggregation/exponential_bucket_histogram_test.rb
Outdated
Show resolved
Hide resolved
metrics_sdk/test/opentelemetry/sdk/metrics/aggregation/exponential_bucket_histogram_test.rb
Outdated
Show resolved
Hide resolved
metrics_sdk/test/opentelemetry/sdk/metrics/aggregation/exponential_bucket_histogram_test.rb
Outdated
Show resolved
Hide resolved
metrics_sdk/test/opentelemetry/sdk/metrics/aggregation/histogram_mapping_test.rb
Show resolved
Hide resolved
…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>
…uby into issues-1722
@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? |
Thanks for the ping on this, @akahn! I'll re-review this afternoon to see what's left before we can merge. |
metrics_sdk/test/opentelemetry/sdk/metrics/aggregation/exponential_histogram/buckets_test.rb
Outdated
Show resolved
Hide resolved
@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. |
There was a problem hiding this 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>
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.
I can work on this |
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.