-
Notifications
You must be signed in to change notification settings - Fork 777
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 exponential histogram bucket snapshot #4313
Fix exponential histogram bucket snapshot #4313
Conversation
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## main #4313 +/- ##
==========================================
+ Coverage 84.50% 84.57% +0.07%
==========================================
Files 298 298
Lines 11888 11899 +11
==========================================
+ Hits 10046 10064 +18
+ Misses 1842 1835 -7
|
@@ -81,7 +86,7 @@ internal Enumerator(long[] buckets) | |||
/// collection.</returns> | |||
public bool MoveNext() |
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.
I think the unit tests should also cover a scale-down scenario when the CircularBufferBuckets offset is updated.
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.
It's not very obvious, but the existing tests actually do cover this scenario.
Even this simple test that only records two values requires a scale down. When the first value is recorded (10) the initial scale of 20 is fine, but when the second value is recorded (5) a scale down is necessary to fit this range of values within the default 160 buckets.
opentelemetry-dotnet/test/OpenTelemetry.Tests/Metrics/MetricSnapshotTests.cs
Lines 238 to 241 in d924663
// SECOND EXPORT | |
expectedHistogram.Record(5); | |
histogram.Record(5); | |
meterProvider.ForceFlush(); |
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.
Got it. Thanks!
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.
I'm going to merge this PR, but I will be coming back to the tests. Clearly there's improvements to make as the bug this PR fixes is not really covered by any tests yet.
As @utpilla points out (#4303 (comment)), I have lost all ability to reason over
CircularBufferBuckets
. With some enhancements to my 🧠, I believe this PR makes the world right again.