Skip to content
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

[processor/deltatocumulativeprocessor]: fix incorrect bucket counts when downscaling exp histograms #33831

Merged

Conversation

edma2
Copy link
Contributor

@edma2 edma2 commented Jul 1, 2024

Description:

The downscaling algorithm collapses bucket by pairing neighbors into new buckets. The simple case occurs when there is an even number of buckets and offset is zero or even.

1 1 1 1
 2   2

When there is an odd number of buckets, the last bucket is accounted for by incrementing the size variable to ensure an extra loop iteration.

1 1 1
 2   1

When offset is odd, the first bucket is accounted for by special handling in the loop.

ø 1 1 1
 1   2

However, when there is an even number of buckets, AND an odd offset, this happens:

ø 1 1
 1   0

The collapsed buckets are incorrect because the buckets sum up to 1 (1+0) instead of 2 (1+1).

Handle this edge case by incrementing the size variable when offset is odd

ø 1 1
 1   1

Link to tracking Issue:

Testing:

Added some test cases to showcase the bug.

Documentation:

@edma2 edma2 requested a review from jpkrohling as a code owner July 1, 2024 18:14
@edma2 edma2 requested a review from a team July 1, 2024 18:14
Copy link

linux-foundation-easycla bot commented Jul 1, 2024

CLA Signed

The committers listed above are authorized under a signed CLA.

@edma2 edma2 force-pushed the fix-deltatocumulative-nh-downscaling branch from 4c199d7 to 045ce5c Compare July 1, 2024 18:23
Copy link
Contributor

@jesusvazquez jesusvazquez left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Member

@sh0rez sh0rez left a comment

Choose a reason for hiding this comment

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

@edma2
Copy link
Contributor Author

edma2 commented Jul 10, 2024

@sh0rez thanks, added a changelog entry

@edma2
Copy link
Contributor Author

edma2 commented Jul 17, 2024

@sh0rez @jpkrohling thanks for your reviews and approvals, can we merge it now?

@jpkrohling jpkrohling merged commit e7c5b97 into open-telemetry:main Jul 18, 2024
154 checks passed
@github-actions github-actions bot added this to the next release milestone Jul 18, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants