Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

[processor/metricstransform]: Add scaling exponential histogram support #34039

Conversation

wildum
Copy link
Contributor

@wildum wildum commented Jul 11, 2024

Description: This PR adds support for the exponential histograms for the experimental_scale_value in the metricstransformprocessor.

The scaling works by scaling the middle value of the first bucket (the one that corresponds to the offset) and finding the offset corresponding to this new value (the method used is described here: https://opentelemetry.io/docs/specs/otel/metrics/data-model/#all-scales-use-the-logarithm-function).

The buckets are actually unchanged because they are "shifted" by the new offset. I initially remapped all the values but I ended up always having the same buckets so I left the buckets untouched to make the code simpler and save on performance.

Link to tracking Issue: #29803

Testing: unit test + e2e local test

@wildum wildum requested a review from dmitryax as a code owner July 11, 2024 09:52
@wildum wildum requested a review from a team July 11, 2024 09:52
@github-actions github-actions bot added the processor/metricstransform Metrics Transform processor label Jul 11, 2024
Copy link
Member

@jpkrohling jpkrohling left a comment

Choose a reason for hiding this comment

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

The code looks sane, but I need help from someone who understands about histograms.

@carrieedwards, are you available to review this one?

Copy link
Contributor

@carrieedwards carrieedwards left a comment

Choose a reason for hiding this comment

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

This looks good to me. I would just consider adding a comment about how there is an inherent loss of accuracy with transform exponential histogram buckets.

@wildum
Copy link
Contributor Author

wildum commented Jul 22, 2024

I would just consider adding a comment about how there is an inherent loss of accuracy with transform exponential histogram buckets.

Done :)

@wildum
Copy link
Contributor Author

wildum commented Jul 26, 2024

@jpkrohling @dmitryax the PR is ready for another review / to be merged

Copy link
Contributor

This PR was marked stale due to lack of activity. It will be closed in 14 days.

@github-actions github-actions bot added the Stale label Aug 10, 2024
@jpkrohling jpkrohling merged commit 7068fd9 into open-telemetry:main Aug 13, 2024
152 of 154 checks passed
@github-actions github-actions bot added this to the next release milestone Aug 13, 2024
f7o pushed a commit to f7o/opentelemetry-collector-contrib that referenced this pull request Sep 12, 2024
…rt (open-telemetry#34039)

**Description:** This PR adds support for the exponential histograms for
the `experimental_scale_value` in the metricstransformprocessor.

The scaling works by scaling the middle value of the first bucket (the
one that corresponds to the offset) and finding the offset corresponding
to this new value (the method used is described here:
https://opentelemetry.io/docs/specs/otel/metrics/data-model/#all-scales-use-the-logarithm-function).

The buckets are actually unchanged because they are "shifted" by the new
offset. I initially remapped all the values but I ended up always having
the same buckets so I left the buckets untouched to make the code
simpler and save on performance.

**Link to tracking Issue:**
open-telemetry#29803

**Testing:** unit test + e2e local test
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
processor/metricstransform Metrics Transform processor Stale
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants