feat(otel): Optimize processing of OpenTelemetry consistent probability sampling - #9184
Open
zacharycmontoya wants to merge 3 commits into
Open
feat(otel): Optimize processing of OpenTelemetry consistent probability sampling#9184zacharycmontoya wants to merge 3 commits into
zacharycmontoya wants to merge 3 commits into
Conversation
…ant information in a OtelTraceState object and only serialize it at the time of context injection.
…t.OtelTraceState store a _copy_. This ensures that when a TraceContext is started from a remote SpanContext, its OtelTraceState updates do not affect other TraceContext objects started from the same SpanContext
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e84dbe2e19
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| spanContext.PropagatedTags = traceTags; | ||
| spanContext.AdditionalW3CTraceState = traceState.AdditionalValues; | ||
| spanContext.OtelTraceState = OtelTraceStateHelpers.Normalize(traceState.OtTraceState); | ||
| spanContext.OtelTraceState = OtelTraceState.Parse(traceState.OtTraceState); |
There was a problem hiding this comment.
ot is absent
For every successful W3C extraction whose tracestate has no ot= member, OtelTraceState.Parse(null) still constructs an OtelTraceState before checking for empty input; continuing the extracted context then creates another copy in TraceContext. This adds avoidable heap allocations to the context-propagation hot path, including the common default case, so leave the property null unless an ot member was actually extracted.
AGENTS.md reference: AGENTS.md:L219-L225
Useful? React with 👍 / 👎.
| { | ||
| var inheritedRv = _containsLocallyGeneratedOtelRandomValue ? null : OtelTraceStateHelpers.ExtractRv(_otelTraceState); | ||
| OtelTraceState = OtelTraceStateHelpers.SetRvTh(_otelTraceState, inheritedRv, th: null); | ||
| _otelTraceState ??= new(headerString: null); |
There was a problem hiding this comment.
Whenever a trace with no prior ot state receives a Manual or Asm sampling decision, this creates and mutates an OtelTraceState whose serialized content is still empty; the injector ultimately emits no ot= member. This adds an otherwise unused allocation to manual sampling and AppSec sampling-decision paths, even when W3C injection is disabled, so only rewrite the state here when one already exists.
AGENTS.md reference: AGENTS.md:L219-L225
Useful? React with 👍 / 👎.
BenchmarksBenchmark execution time: 2026-09-04 21:25:29 Comparing candidate commit e84dbe2 in PR branch Found 0 performance improvements and 22 performance regressions! Performance is the same for 50 metrics, 0 unstable metrics, 93 known flaky benchmarks, 33 flaky benchmarks without significant changes.
|
andrewlock
added
type:performance
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary of changes
Builds directly on top of the original feature PR (#8983) and "revert-the-revert" PR (#9178) to reduce the overhead of the feature. This should now have minimal changes to the default tracing code paths, as demonstrated in our microbenchmarks.
There is also one other bug fix: When the incoming
ottracestate list-member is not modified from its incoming value, it now remains in the same position in thetracestateheader rather than moving to the front -- the order of unchanged list-members must remain unchanged, as required by the W3C Trace Context specification.The results are improved (nearly pre-feature level) execution time and similar (post-feature level) allocations.
Reason for change
Performance is critical because this feature is essentially on the hot path of every trace scenario. If the SDK is configured to propagate W3C Trace Context headers, then we must calculate these consistent probability values when we make a sampling decision and later insert them into the
tracestatepropagation header. Further, whenever we emit OTLP spans, we must ensure these OpenTelemetry values are emitted in the OTLP span'stracestatefield.This PR aims to minimize the overhead of the feature so that cost of creating the header string is only incurred when it is needed, which is currently for injecting trace context injection into a downstream HTTP request and later when serializing OTLP spans. This means that the default scenario where W3C propagation is enabled (but no trace context injection is performed) and spans are exported in Datdog MessagePack, there should be minimal overhead costs.
Local benchmarks
I had Claude run the
Benchmarks.Trace.RedisBenchmark.SendReceiveandBenchmarks.Trace.SerilogBenchmark.EnrichedLogmicrobenchmarks to measure the effects on the default case that doesn't use the results of the tracestate handling. This measures the execution-time and allocation cost of the OpenTelemetry consistent-probability-sampling feature (#8983) and the results are the AI's own words.Variants compared
aa978b874b34861f39c7[Propagators] Add OpenTelemetry consistent probability sampling (#8983), as mergedOtelTraceState—rv/thkept asulong?and theot=string materialized only at injection timeResults
Benchmarks.Trace.RedisBenchmark.SendReceiveBenchmarks.Trace.SerilogBenchmark.EnrichedLogC removes 79% (Redis) and 96% (Serilog) of the execution-time overhead introduced by B.
For
EnrichedLogthe residual overhead is within run-to-run noise — C's range overlaps A's. ForSendReceivethe residual+17.8 nsis real; the two ranges do not overlap.Allocation overhead is only reduced by 9% (
+88 B→+80 B).Implementation details
The main change: Create a
OtelTraceStateclass to store all of the needed logic, including the cached inputtracestateheader, the newly calculatedulong? rv, and the newly calculatedulong? thso that we do not serialize a new OpenTelemetrytracestatelist-member in the sampling hot path.The rest of the changes follow from this modification, including:
SpanContext.OtelTraceStateandTraceContext.OtelTraceStateto have typeOtelTraceStateOtelTraceStateHelpers.Normalizeimplementation into thestatic OtelTraceState.Parse(string? raw)method, which creates a newOtelTraceStateobject and records the well-known values to be serialized later. Also, the loop is optimized to work in one pass of the original header.OtelTraceState.IsModifiedandOtelTraceState.CachedHeaderStringpropertiesOne enhancement in this PR
Test coverage
OtelTraceStateobject into a string for assertions (since the object is no longer a string primitive)CreateTraceStateHeader_EmitsOtInOriginalPosition_WhenOtelTraceStateIsUnchangedandCreateTraceStateHeader_DoesNotEmitEmptySubKey_WhenOnlyUnknownOtItemsRemainto assert tracestate ordering propertiesOther details
Possible improvements suggested by Claude:
OtelTraceStatea struct so when we store it by value on aTraceContextwe increase the baseline size ofTraceContextbut we reduce heap allocations (seems reasonable as this field is needed on every trace context now)rvandthasulong?fields directly on TraceContext, but I'm not a fan of this since we do need several additional fields to keep track of