-
Notifications
You must be signed in to change notification settings - Fork 610
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
remove unnecessary buckets on exponential histograms #3771
base: main
Are you sure you want to change the base?
remove unnecessary buckets on exponential histograms #3771
Conversation
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.
AFAIU for reference the js sdk implements the trimming on the bucket and calls it when merging or diffing exponential histograms:
https://github.com/open-telemetry/opentelemetry-js/blob/main/packages/sdk-metrics/src/aggregator/exponential-histogram/Buckets.ts#L116
@euroelessar any chance you can take a look at this please? |
unfortunately this PR has to be rebased due to #3798 being merged |
@loomis-relativity any chance you can rebase your changes please? |
@xrmx Merge conflict resolved. |
@euroelessar hello, do you have time to take a look at it? |
@@ -272,18 +278,18 @@ def encode_metrics(data: MetricsData) -> ExportMetricsServiceRequest: | |||
elif isinstance(metric.data, ExponentialHistogramType): | |||
for data_point in metric.data.data_points: | |||
|
|||
if data_point.positive.bucket_counts: | |||
if positive_buckets := truncate_trailing_zeros(data_point.positive.bucket_counts): |
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.
maybe move truncation logic into def bucket_counts
implementation to avoid duplication?
Apologies, the notifications got lost in my inbox. @euroelessar I've refactored the code to remove some duplication. Don't think that it can be moved into the |
Description
Updates the serialization of exponential histogram buckets to remove unnecessary zero buckets. If there are no non-zero buckets, then none are serialized. This matches better the payloads from the JavaScript SDK and avoids ingestion problems with a downstream vendor when an empty negative bucket appears.
Fixes #3767
Type of change
Please delete options that are not relevant.
How Has This Been Tested?
Added unit tests to ensure that the truncation of the zero buckets works as expected. Updated the unit test for metric encoding to exercise this functionality.
Does This PR Require a Contrib Repo Change?
Answer the following question based on these examples of changes that would require a Contrib Repo Change:
The OTel specification has changed which prompted this PR to update the method interfaces of
opentelemetry-api/
oropentelemetry-sdk/
The method interfaces of
test/util
have changedScripts in
scripts/
that were copied over to the Contrib repo have changedConfiguration files that were copied over to the Contrib repo have changed (when consistency between repositories is applicable) such as in
pyproject.toml
isort.cfg
.flake8
When a new
.github/CODEOWNER
is addedMajor changes to project information, such as in:
README.md
CONTRIBUTING.md
Yes. - Link to PR:
No.
Checklist:
[ ] Documentation has been updated