search

open-telemetry / opentelemetry-python

OpenTelemetry Python API and SDK
https://opentelemetry.io
Apache License 2.0
1.66k stars 568 forks source link

Fix `ExponentialBucketHistogramAggregation` #3978

Open ocelotl opened 1 week ago

ocelotl commented 1 week ago

Fixes #3977, this PR is analogous to #3429

ocelotl commented 4 days ago

@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 commented 3 days ago

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 commented 2 days ago

I have moved all variable renaming here.

ocelotl commented 1 day ago

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.