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.

Sign up for GitHub

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

Jump to bottom

Fix ExponentialBucketHistogramAggregation #3978

Open
wants to merge 33 commits into
base: main
Choose a base branch
from
Open

Fix ExponentialBucketHistogramAggregation #3978

wants to merge 33 commits into from

Conversation

ocelotl
Copy link
Contributor

@ocelotl ocelotl commented Jun 17, 2024 •
edited
Loading

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 as a code owner 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
ocelotl commented Jun 26, 2024
View reviewed changes
opentelemetry-sdk/src/opentelemetry/sdk/metrics/_internal/aggregation.py
@@ -126,17 +126,17 @@ def __init__(
)
self._instrument_is_monotonic = instrument_is_monotonic

self._current_value = None
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I renamed _current_value to _value and _previous_cumulative_value to _previous_value for better consistency.

ocelotl
ocelotl commented Jun 26, 2024
View reviewed changes
opentelemetry-sdk/src/opentelemetry/sdk/metrics/_internal/aggregation.py
@@ -403,42 +401,43 @@ def __init__(
):
super().__init__(attributes)

self._instrument_aggregation_temporality = (
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Rearranged these lines to improve consistency with _ExponentialBucketHistogramAggregation, this way it is much easier to compare them and find the analogous parts between them.

@ocelotl ocelotl added the bug Something isn't working label Jun 26, 2024
@xrmx xrmx mentioned this pull request Jun 26, 2024
Open
@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.

xrmx
xrmx reviewed Jun 28, 2024
View reviewed changes
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
xrmx
xrmx approved these changes Jul 9, 2024
View reviewed changes
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 😓

aabmass
aabmass reviewed Jul 11, 2024
View reviewed changes
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

opentelemetry-sdk/tests/metrics/integration_test/test_exponential_bucket_histogram.py Show resolved Hide resolved
opentelemetry-sdk/tests/metrics/integration_test/test_exponential_bucket_histogram.py
for metrics_data in results:
self.assertIsNone(metrics_data)

results = []
Copy link
Member

Choose a reason for hiding this comment

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

Everything above here seems like a separate test case like test_synchronous_delta_temporality_no_observations. Could you separate out?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I also thought about doing that, but for this particular case it is important for us todo all this testing consecutively with the same histogram object. The order matters so I am keeping this all in a single test case.

Nevertheless, what you mention is important so I added comments that "separate the subtests" in this test case.

Copy link
Member

Choose a reason for hiding this comment

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

but for this particular case it is important for us todo all this testing consecutively with the same histogram object.

Can you update the test case name to capture what is being tested?

opentelemetry-sdk/tests/metrics/integration_test/test_exponential_bucket_histogram.py Show resolved Hide resolved
@ocelotl ocelotl force-pushed the issue_3977 branch 2 times, most recently from bce6df1 to 4530d55 Compare July 11, 2024 19:07
@ocelotl ocelotl mentioned this pull request Jul 11, 2024
Open
@ocelotl
Remove old code
b7a527d
@ocelotl
Fix some test cases
622924d
@ocelotl
Relocate method
4a35473
@ocelotl
Debugging test_aggregate_collect
92f4dcd
@ocelotl
Fix empty previous bucket handling
facce85
@ocelotl
Fix max and min setting
8324839
@ocelotl
Fix explicit bucket aggregation to make it consistent
601f519
@ocelotl
Rearrange __init__s
55a3ddb
@ocelotl
Set scale in aggregate
dc21650
@ocelotl
Use right values in exponential point
00d8a8a
@ocelotl
Set scale right
9ab5c5f
@ocelotl
Start scale as None
c04d55e
@ocelotl
Make test_collect_results_cumulative pass
851ffcb 
I am not sure these changes are right, I just wanted to find what would
be the value that would make this test case pass.
@ocelotl
Actually use random values
6efbced
@ocelotl
Add integration test for exponential histogram
21b2171
@ocelotl
Handle all cases for current and previous buckets and scale
1087687
@ocelotl
Rename test module
fc394e9
@ocelotl
Use random values
9e98c5e
@ocelotl
Fix bucket setting
ed1a0ad
@ocelotl
WIP integration test
4f3ed43
@ocelotl
WIP
8120692
@ocelotl
Finish integration tests
c9458ad
@ocelotl
Rename variables
be646d1
@ocelotl
Explain analogy with ExplicitBucket
5b7f3a8
@ocelotl
Add changelog and fix lint
9dc8a79
@ocelotl
Fix equality tests
03e5d30
@ocelotl
Fix location of returns
8532b80
@ocelotl
Fix another issue and add test case
84c81d0
@ocelotl
Added comments to integration test case
24e51ca
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@aabmass aabmass aabmass left review comments

@xrmx xrmx xrmx approved these changes

Assignees

@ocelotl ocelotl

Labels
bug Something isn't working
Projects
None yet
Milestone
No milestone
3 participants
@ocelotl @xrmx @aabmass