Skip to content

[SDK] Optimize composite and probability samplers - #4582

Open
dbarker wants to merge 2 commits into
open-telemetry:mainfrom
dbarker:perf_composite_and_probability_samplers
Open

dbarker wants to merge 2 commits into
open-telemetry:mainfrom
dbarker:perf_composite_and_probability_samplers

Conversation

@dbarker

@dbarker dbarker commented Sep 16, 2026

Copy link
Copy Markdown
Member

Fixes # (issue)

Optimizes the composite and probability samplers to reduce dynamic memory allocation and operations on the trace state object.

Changes

  • Add new method to read the "ot" value from trace state without making string copies
  • Update composite and probability sampler to track if the "ot" threshold has changed and only update the trace state kv properties when a change is needed.
  • Add shortcuts for the threshold == 0 (AlwaysOn) case
  • Reserve the string size for the new ot value before appending values into it on serialization.

For significant contributions please make sure you have completed the following items:

  • CHANGELOG.md updated for non-trivial changes
  • Unit tests have been added
  • Changes in public API reviewed

@codecov

codecov Bot commented Sep 16, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 94.26230% with 7 lines in your changes missing coverage. Please review.
✅ Project coverage is 86.52%. Comparing base (b4257f0) to head (ee75924).

Files with missing lines Patch % Lines
sdk/src/trace/samplers/ot_trace_state.cc 93.34% 6 Missing ⚠️
sdk/src/trace/samplers/composite_sampler.cc 92.86% 1 Missing ⚠️
Additional details and impacted files

Impacted file tree graph

@@            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     
Files with missing lines Coverage Δ
.../src/trace/samplers/composable_parent_threshold.cc 100.00% <100.00%> (ø)
sdk/src/trace/samplers/probability.cc 100.00% <100.00%> (+2.64%) ⬆️
sdk/src/trace/samplers/composite_sampler.cc 97.62% <92.86%> (-2.38%) ⬇️
sdk/src/trace/samplers/ot_trace_state.cc 95.28% <93.34%> (-4.72%) ⬇️

... and 3 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@om7057 om7057 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

  1. other_subkeys as nostd::string_view instead of std::string. These are views into the ot_value buffer passed to Parse(), which is itself a view into the original trace state's entry storage via the new GetOtValue(). Traced the lifetime through CompositeSampler::ShouldSample/ProbabilitySampler::ShouldSample: parent_trace_state stays alive for the whole call (a const auto & bound to parent_context.trace_state(), and parent_context outlives the function), and TraceState::Set()/Delete() never mutate this, only build a new object, so the backing storage the views point into is never touched while Serialize() reads them. The OPENTELEMETRY_ATTRIBUTE_LIFETIME_BOUND annotation plus the comments on Parse()/other_subkeys document the constraint for future callers.

  2. The skip-reserialization optimization in GetTraceStateForOtValue(). It compares the before/after threshold state and returns trace_state untouched if unchanged, without re-checking rv/other subkeys. My first thought was whether intent.trace_state_provider (in CompositeSampler::ShouldSample) could swap out_trace_state for an object not derived from parent_trace_state, which would make the skip compare against the wrong object's "ot" content. Checked TraceStateProvider'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.

  3. The AppendThresholdHex split into ThresholdHexStartShift()/AppendThreshold(), specifically whether pulling the threshold == 0 case 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 make CountTrailingZeroBits well-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.

@dbarker
dbarker force-pushed the perf_composite_and_probability_samplers branch from d886de2 to a1b8f61 Compare September 21, 2026 17:37
// 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

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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%

@dbarker
dbarker marked this pull request as ready for review September 21, 2026 17:45
@dbarker
dbarker requested a review from a team as a code owner September 21, 2026 17:45
@dbarker
dbarker requested a review from om7057 September 21, 2026 17:45

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants