-
Notifications
You must be signed in to change notification settings - Fork 785
New issue
Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? # to your account
Fix SimpleExemplarReservoir for cumulative #5230
Fix SimpleExemplarReservoir for cumulative #5230
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #5230 +/- ##
==========================================
- Coverage 83.38% 83.12% -0.27%
==========================================
Files 297 271 -26
Lines 12531 11970 -561
==========================================
- Hits 10449 9950 -499
+ Misses 2082 2020 -62
Flags with carried forward coverage won't be shown. Click here to find out more.
|
@@ -57,9 +57,13 @@ public override Exemplar[] Collect(ReadOnlyTagCollection actualTags, bool reset) | |||
if (reset) |
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.
unrelated but kind of related nit: maybe this variable should be named isDelta
for readability?
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.
yes... The Reservoir itself is not necessarily aware of temporality concept. All it needs to know is if it should reset its internal state or not.. The caller knows temporality and passes reset to this...
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.
Not a blocker for this PR, but just looking at this API shape, is the correct thing to do to expose bool reset
?
SDKs are free to decide whether "collect" should also reset internal storage for delta temporal aggregation collection, or use a more optimal implementation.
Curious what other languages have done to solve this.
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.
The current APIs need to be revised before exposing them to public (its marked internal), and there are many TODOs to be solved.
Fixes
SimplerExemplarReservoir
for cumulative temporality to reset internal state during collect. Before this change, newer measurements had no fair chance to make it to the reservoir. This PR resets the "number_of_measurements_seen" with every collect, so new measurements has fair chance to make it to reservoir, subject to the reservoir sampling algorithm.Spec ref: https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/metrics/sdk.md#simplefixedsizeexemplarreservoir
Merge requirement checklist
CHANGELOG.md
files updated for non-trivial changes