Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #4582 +/- ##
==========================================
+ Coverage 86.52% 86.52% +0.01%
==========================================
Files 525 525
Lines 20475 20509 +34
==========================================
+ Hits 17713 17744 +31
- Misses 2762 2765 +3
🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Went through this given the overlap with the TraceState overwrite-semantics bug I was just in (#4586's area). Checked three things that looked like they could be real problems, and none of them are:
-
other_subkeysasnostd::string_viewinstead ofstd::string. These are views into theot_valuebuffer passed toParse(), which is itself a view into the original trace state's entry storage via the newGetOtValue(). Traced the lifetime throughCompositeSampler::ShouldSample/ProbabilitySampler::ShouldSample:parent_trace_statestays alive for the whole call (aconst auto &bound toparent_context.trace_state(), andparent_contextoutlives the function), andTraceState::Set()/Delete()never mutatethis, only build a new object, so the backing storage the views point into is never touched whileSerialize()reads them. TheOPENTELEMETRY_ATTRIBUTE_LIFETIME_BOUNDannotation plus the comments onParse()/other_subkeysdocument the constraint for future callers. -
The skip-reserialization optimization in
GetTraceStateForOtValue(). It compares the before/after threshold state and returnstrace_stateuntouched if unchanged, without re-checkingrv/other subkeys. My first thought was whetherintent.trace_state_provider(inCompositeSampler::ShouldSample) could swapout_trace_statefor an object not derived fromparent_trace_state, which would make the skip compare against the wrong object's "ot" content. CheckedTraceStateProvider's doc comment: "The resulting TraceState must not modify the 'ot' sub-key: that is owned by the CompositeSampler." That contract is what makes the skip safe. -
The
AppendThresholdHexsplit intoThresholdHexStartShift()/AppendThreshold(), specifically whether pulling thethreshold == 0case out of the bit-55-sentinel trick changes the emitted digits. Worked through both paths by hand for the zero and nonzero cases; they produce identical output. The sentinel OR was only there to makeCountTrailingZeroBitswell-defined for the all-zero input, and the new code branches on that case directly instead.
Nothing incorrect found. The new tests (PreservesMultipleOtherSubkeys, PreservesUnrelatedTraceStateKey, NoOtKeyPreservesUnrelatedTraceStateKey) cover the edges I checked above.
d886de2 to
a1b8f61
Compare
| // BM_SpanCreationWithSamplingResultAttributes/128_stddev 30.5 ns 30.3 ns 5 | ||
| // BM_SpanCreationWithSamplingResultAttributes/128_cv 0.52 % 0.52 % 5 | ||
| // BM_TraceIdRatioBasedSamplerShouldSample_stddev 0.003 ns 0.003 ns 5 | ||
| // BM_TraceIdRatioBasedSamplerShouldSample_cv 0.10 % 0.11 % 5 |
There was a problem hiding this comment.
The optimizations reduce the ShouldSample latency with composite and probability samplers and with greater impact in nested span cases where current code repeats trace state operations unnecessarily for every span in a trace.
| Benchmark | main (ns) | this PR (ns) | Delta % |
|---|---|---|---|
| ProbabilitySamplerShouldSampleDrops | 225 | 126 | −44.0% |
| ProbabilitySamplerShouldSampleSamples | 241 | 22.2 | −90.8% |
| CompositeProbabilitySamplerShouldSampleDrops | 190 | 127 | −33.2% |
| CompositeProbabilitySamplerShouldSampleSamples | 198 | 24.0 | −87.9% |
| CompositeProbabilitySamplerShouldSampleOtSubkeyCount/1 | 216 | 155 | −28.2% |
| CompositeProbabilitySamplerShouldSampleOtSubkeyCount/23 | 732 | 441 | −39.8% |
| CompositeProbabilitySamplerShouldSampleOtSubkeyCount/46 | 1251 | 692 | −44.7% |
| CompositeProbabilitySamplerShouldSampleTraceStateMemberCount/1 | 213 | 152 | −28.6% |
| CompositeProbabilitySamplerShouldSampleTraceStateMemberCount/16 | 607 | 541 | −10.9% |
| CompositeProbabilitySamplerShouldSampleTraceStateMemberCount/31 | 957 | 917 | −4.2% |
| CompositeProbabilitySamplerShouldSampleNestedDepth/1 | 100 | 99.6 | −0.4% |
| CompositeProbabilitySamplerShouldSampleNestedDepth/32 | 16350 | 702 | −95.7% |
| CompositeProbabilitySamplerShouldSampleNestedDepth/256 | 214737 | 4813 | −97.8% |
| CompositeRuleBasedSamplerShouldSampleMatchesKind | 213 | 137 | −35.7% |
| CompositeRuleBasedSamplerShouldSampleMatchesParent | 216 | 139 | −35.6% |
| CompositeRuleBasedSamplerShouldSampleNoMatch | 193 | 129 | −33.2% |
| CompositeRuleBasedSamplerShouldSampleMatchesValue | 226 | 152 | −32.7% |
| CompositeRuleBasedSamplerShouldSampleMatchesPattern | 229 | 159 | −30.6% |
| CompositeParentThresholdSamplerShouldSampleParentHasThreshold | 228 | 152 | −33.3% |
| CompositeParentThresholdSamplerShouldSampleNoParent | 51.7 | 12.0 | −76.8% |
| CompositeParentThresholdSamplerShouldSampleParentSampledNoThreshold | 55.8 | 13.4 | −76.0% |
| CompositeParentThresholdSamplerShouldSampleNestedDepth/1 | 97.6 | 84.4 | −13.5% |
| CompositeParentThresholdSamplerShouldSampleNestedDepth/32 | 16412 | 953 | −94.2% |
| CompositeParentThresholdSamplerShouldSampleNestedDepth/256 | 211833 | 7111 | −96.6% |
| CompositeAlwaysOnSamplerShouldSample | 202 | 133 | −34.2% |
| CompositeAlwaysOnSamplerShouldSampleNestedDepth/1 | 98.5 | 83.8 | −14.9% |
| CompositeAlwaysOnSamplerShouldSampleNestedDepth/32 | 16131 | 624 | −96.1% |
| CompositeAlwaysOnSamplerShouldSampleNestedDepth/256 | 214488 | 4400 | −97.9% |
| CompositeAlwaysOffSamplerShouldSample | 188 | 127 | −32.4% |
| CompositeAlwaysOffSamplerShouldSampleNestedDepth/1 | 53.4 | 16.4 | −69.3% |
| CompositeAlwaysOffSamplerShouldSampleNestedDepth/32 | 1682 | 450 | −73.2% |
| CompositeAlwaysOffSamplerShouldSampleNestedDepth/256 | 13404 | 3573 | −73.4% |
Fixes # (issue)
Optimizes the composite and probability samplers to reduce dynamic memory allocation and operations on the trace state object.
Changes
For significant contributions please make sure you have completed the following items:
CHANGELOG.mdupdated for non-trivial changes