[SDK] feat: exemplar filters#4267
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #4267 +/- ##
==========================================
+ Coverage 81.25% 81.29% +0.05%
==========================================
Files 446 447 +1
Lines 18872 18891 +19
==========================================
+ Hits 15332 15356 +24
+ Misses 3540 3535 -5
🚀 New features to boost your workflow:
|
| GetSimpleFixedReservoirDefaultSize(agg_type, agg_config), | ||
| SimpleFixedSizeExemplarReservoir::GetSimpleFixedSizeCellSelector(), | ||
| GetMapAndResetCellMethod(instrument_descriptor))); | ||
| return ExemplarReservoir::GetSimpleFilteredExemplarReservoir( |
There was a problem hiding this comment.
For AlwaysOff, can we return GetNoExemplarReservoir() directly? Otherwise every metric recording still goes through the exemplar filter path just to drop the sample, so AlwaysOff is not really the same as disabling exemplar sampling
There was a problem hiding this comment.
For the performance, you are right. But according to spec, looks like current implementation is correct.
Here is what i understand:
ExemplarFilter decide whether measurement is eligible for the examplar. ExamplarReservoir decide wheter examplar to be stored or not.
If i missunderstand the spec, please let me know where I got it wrong. (spec is bit confused to me.)
| { | ||
| if (should_sample_(context)) | ||
| { | ||
| reservoir_->OfferMeasurement(value, attributes, context, timestamp); |
There was a problem hiding this comment.
I think AlwaysOn still does not fully work here. This forwards the measurement, but the real reservoir drops it later if there is no valid span context. Since trace/span ids are optional for exemplars, AlwaysOn + Context{} should still produce an exemplar without trace ids. Can we add a test that collects from the real reservoir and checks the returned exemplar is non-null?
There was a problem hiding this comment.
Same point for the above #4267 (comment)
…to feat-exemplar-filters
…to feat-exemplar-filters
…to feat-exemplar-filters
Fixes #4178, #2526
Changes
Breaking Change is accompanied, But Because this is preview version, I think acceptable.
CHANGELOG.mdupdated for non-trivial changes