Fix SimpleExemplarReservoir for cumulative#5230
Fix SimpleExemplarReservoir for cumulative#5230alanwest merged 13 commits intoopen-telemetry:mainfrom
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.
unrelated but kind of related nit: maybe this variable should be named isDelta for readability?
There was a problem hiding this comment.
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.
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.
The current APIs need to be revised before exposing them to public (its marked internal), and there are many TODOs to be solved.
Fixes
SimplerExemplarReservoirfor 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.mdfiles updated for non-trivial changes