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

Fix ExponentialBucketHistogramAggregation #3978

Merged
merged 36 commits into from
Jul 18, 2024

Conversation

ocelotl
Copy link
Contributor

@ocelotl ocelotl commented Jun 17, 2024

Fixes #3977
Fixes #3974
Fixes #4037

This PR is analogous to #3429 and #4009

@ocelotl ocelotl self-assigned this Jun 17, 2024
@ocelotl ocelotl force-pushed the issue_3977 branch 4 times, most recently from 4d83990 to e8a9bc5 Compare June 24, 2024 17:18
@ocelotl ocelotl marked this pull request as ready for review June 25, 2024 23:48
@ocelotl ocelotl requested a review from a team June 25, 2024 23:48
@ocelotl ocelotl changed the title WIP Fix ExponentialBucketHistogramAggregation Jun 25, 2024
@ocelotl
Copy link
Contributor Author

ocelotl commented Jun 26, 2024

@euroelessar @rbtz-openai please take a look. This mainly fixes issues with start_time_unix_nano, but is is a necessary refactoring analogous to #3429.

@ocelotl
Copy link
Contributor Author

ocelotl commented Jun 26, 2024

Note to reviewers: I recommend to check out this PR branch and compare it to main in separate windows to make it easier to understand what is going on here. I also strongly recommend to check out this PR branch and compare the implementation of aggregate and collect in the _ExplicitBucketHistogramAggregation and _ExponentialBucketHistogramAggregation classes. The former is simpler and you'll find out both are pretty much the same thing, the only main difference is the extra processing the latter does on its buckets to resize them while the former does not do that.

@ocelotl ocelotl force-pushed the issue_3977 branch 4 times, most recently from 4fdb18f to df23d88 Compare June 27, 2024 17:53
@ocelotl
Copy link
Contributor Author

ocelotl commented Jun 27, 2024

I have moved all variable renaming here.

Copy link
Contributor

@xrmx xrmx left a comment

Choose a reason for hiding this comment

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

Took a look at this but I fail to recognize the actual code change that fixes something 😓

@ocelotl
Copy link
Contributor Author

ocelotl commented Jun 28, 2024

Took a look at this but I fail to recognize the actual code change that fixes something 😓

I added a README.rst that explains the actual and expected results more clearly.

So, this PR fixes the issue with the wrong start_time_unix_nano values when the collection temporality is CUMULATIVE for the exponential bucket histogram aggregation. But remember that this PR is analogous to #3429 which fixed #3407 and #3089, so, we probably have the same issues reported in #3407 and #3089 but for the exponential histogram and the refactoring makes both the explicit and exponential bucket histogram aggregation implementations consistent.

@ocelotl ocelotl force-pushed the issue_3977 branch 2 times, most recently from 8a5b1a4 to 7332465 Compare July 3, 2024 16:34
Copy link
Contributor

@xrmx xrmx left a comment

Choose a reason for hiding this comment

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

LGTM but please wait for an ack from someone more clueful on histograms 😓

Copy link
Member

@aabmass aabmass left a comment

Choose a reason for hiding this comment

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

Just some comments related to the tests that I had forgotten to publish

@ocelotl
Copy link
Contributor Author

ocelotl commented Jul 18, 2024

Agreed to merge this in the SIG, since this is urgent and needed for the release.

@ocelotl ocelotl merged commit e78675e into open-telemetry:main Jul 18, 2024
284 checks passed
@ocelotl ocelotl deleted the issue_3977 branch July 18, 2024 19:10
@lzchen
Copy link
Contributor

lzchen commented Jul 19, 2024

@aabmass

Feel free to make additional comments/approve the changes separately.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
5 participants