Tracing: normalize tracestate encoding at propagator ingress#5867
Closed
p-datadog wants to merge 6 commits into
Closed
Tracing: normalize tracestate encoding at propagator ingress#5867p-datadog wants to merge 6 commits into
p-datadog wants to merge 6 commits into
Conversation
HTTP frameworks (Rack, etc.) hand header values to applications tagged as ASCII-8BIT. msgpack-ruby uses string encoding as a type signal — UTF-8 serializes as `str`, ASCII-8BIT as `bin`. The trace-agent's wire schema expects `SpanLinks[N].Tracestate` to be `str`, so receiving `bin` is rejected/mishandled. Fix at the two ingress boundaries where tracestate strings cross into the domain from foreign producers: - W3C TraceContext propagator (`extract_tracestate`) — extraction of the `tracestate` HTTP header. - OpenTelemetry SpanProcessor — `link.span_context.tracestate&.to_s` from the OTel SDK. Both call a new `Distributed::Helpers.normalize_tracestate_encoding`, which `force_encoding`s the bytes to UTF-8 (the W3C tracestate grammar is restricted to printable ASCII, a subset of UTF-8 — so the bytes are correct and only the encoding tag is wrong). Strings whose bytes do not form valid UTF-8 are dropped rather than propagated as corrupted data; such bytes are spec violations.
p-datadog
added
the
AI Generated
Contributor
Typing analysisNote: Ignored files are excluded from the next sections. Untyped methodsThis PR introduces 1 partially typed method, and clears 1 partially typed method. It increases the percentage of typed methods from 65.14% to 65.16% (+0.02%). Partially typed methods (+1-1)❌ Introduced:If you believe a method or an attribute is rightfully untyped or partially typed, you can add |
🎉 All green!🧪 All tests passed 🎯 Code Coverage (details) 🔗 Commit SHA: 6a7a5fe | Docs | Datadog PR Page | Give us feedback! |
BenchmarksBenchmark execution time: 2026-06-08 20:13:12 Comparing candidate commit 6a7a5fe in PR branch Found 0 performance improvements and 0 performance regressions! Performance is the same for 45 metrics, 1 unstable metrics.
|
Member
Author
|
@codex review |
Builds a SpanLink from a digest extracted via an ASCII-8BIT-tagged tracestate header, packs and unpacks via msgpack, and asserts the tracestate field round-trips as a `str` (unpacked as UTF-8) — the wire shape the trace-agent's schema requires. Pins the end-to-end claim of the previous commit rather than relying only on intermediate digest.trace_state.encoding assertions.
|
Codex Review: Didn't find any major issues. Breezy! ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
There was a problem hiding this comment.
Pull request overview
This PR normalizes the encoding tag of inbound W3C tracestate values to UTF-8 at propagator ingress (W3C TraceContext extraction and the OpenTelemetry SpanProcessor link path), preventing msgpack from serializing tracestate as bin (ASCII-8BIT) and avoiding agent-side schema rejection for span link tracestate.
Changes:
- Added
Distributed::Helpers.normalize_tracestate_encodingto retag ASCII-8BIT bytes as UTF-8 and drop invalid UTF-8 byte sequences. - Routed W3C
TraceContext#extract_tracestateand OTelSpanProcessorspan-linktrace_statethrough the new helper. - Added specs covering ASCII-8BIT inputs, frozen inputs, valid UTF-8 byte sequences, and invalid byte sequences; updated RBS signature.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| lib/datadog/tracing/distributed/helpers.rb | Adds the normalize_tracestate_encoding helper used to retag/drop inbound tracestate strings. |
| lib/datadog/tracing/distributed/trace_context.rb | Normalizes tracestate encoding during W3C propagator extraction. |
| lib/datadog/opentelemetry/sdk/span_processor.rb | Normalizes tracestate encoding when creating span links from OTel SpanContext. |
| spec/datadog/tracing/distributed/helpers_spec.rb | Adds unit coverage for tracestate encoding normalization behavior. |
| spec/datadog/tracing/distributed/trace_context_spec.rb | Adds extraction-path coverage for ASCII-8BIT and invalid-byte tracestate headers. |
| sig/datadog/tracing/distributed/helpers.rbs | Declares the new helper method signature for type checking. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Original comment overstated the policy ("non-ASCII bytes are spec
violations and are dropped") — the helper only drops byte sequences that
fail `valid_encoding?`. A non-ASCII byte that forms a valid UTF-8
sequence (e.g. a multi-byte character) is retagged and kept. Also dropped
an unverified consumer list ("JSON, log encoders") — only msgpack-ruby
actually keys behavior off `String#encoding` in dd-trace-rb.
Existing annotation claimed a single 4-field array; the method actually returns one of three shapes: nil (no header / empty), String (header present but no `dd=` entry), or a 6-element array when `dd=` is present. The 4-field array's listed order also had origin/ts_parent_id swapped relative to the actual code at line 304. Doc-only; the sole caller (`TraceContext#extract`) handles the polymorphism via Ruby destructuring (nil RHS collapses to all-nil vars; a single String RHS goes into the first var, rest nil), which is why the inaccurate doc never caused a bug.
![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: 88a02ef707
ℹ️ 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".
The previous PR commit called Helpers.normalize_tracestate_encoding at the top of extract_tracestate, before split_tracestate's 512-byte truncation. A single invalid UTF-8 byte anywhere in the input — even in the tail that split_tracestate would have discarded — dropped the whole field. Move the normalize call into split_tracestate, after the byteslice/chop block. valid_encoding? now only sees the bytes the function would have kept, so an oversized header with a bad byte in the discarded tail preserves its parseable prefix. Add coverage for the oversized-header-with-0xFF-in-tail case. Existing test for invalid bytes in the kept portion (under the 512-byte limit) still drops the field as before.
Member
|
@p-datadog I've left some comments in previous PR by the original author 🤔 |
Per Strech's review on #5844 (which targets the same helper as #5867): the prior name implied a normalization the method does not perform. The method re-tags the input via String#force_encoding(UTF-8) and returns nil when the bytes are not a valid UTF-8 sequence. The new name reads off the underlying operation without overpromising success. Also collapses the body: the previous `if value.encoding == UTF_8` early-return branch merges into a single conditional re-encode followed by one `valid_encoding?` check at the end. Behavior unchanged: already-UTF-8 input is returned by object identity (no dup), input is never mutated, frozen input is handled via the dup in the non-UTF-8 branch — all covered by helpers_spec. Updates the helper, the RBS signature, both call sites (TraceContext#extract_tracestate, OTel SpanProcessor span-link path), and the helpers spec describe/subject.
MilanGarnier
added a commit
that referenced
this pull request
Jun 19, 2026
* Tracing: normalize tracestate encoding at propagator ingress
HTTP frameworks (Rack, etc.) hand header values to applications tagged as
ASCII-8BIT. msgpack-ruby uses string encoding as a type signal — UTF-8
serializes as `str`, ASCII-8BIT as `bin`. The trace-agent's wire schema
expects `SpanLinks[N].Tracestate` to be `str`, so receiving `bin` is
rejected/mishandled.
Fix at the two ingress boundaries where tracestate strings cross into the
domain from foreign producers:
- W3C TraceContext propagator (`extract_tracestate`) — extraction of the
`tracestate` HTTP header.
- OpenTelemetry SpanProcessor — `link.span_context.tracestate&.to_s` from
the OTel SDK.
Both call a new `Distributed::Helpers.normalize_tracestate_encoding`,
which `force_encoding`s the bytes to UTF-8 (the W3C tracestate grammar is
restricted to printable ASCII, a subset of UTF-8 — so the bytes are
correct and only the encoding tag is wrong). Strings whose bytes do not
form valid UTF-8 are dropped rather than propagated as corrupted data;
such bytes are spec violations.
* Tracing: add msgpack end-to-end spec for tracestate encoding fix
Builds a SpanLink from a digest extracted via an ASCII-8BIT-tagged
tracestate header, packs and unpacks via msgpack, and asserts the
tracestate field round-trips as a `str` (unpacked as UTF-8) — the wire
shape the trace-agent's schema requires. Pins the end-to-end claim of
the previous commit rather than relying only on intermediate
digest.trace_state.encoding assertions.
* Tracing: tighten normalize_tracestate_encoding doc-comment
Original comment overstated the policy ("non-ASCII bytes are spec
violations and are dropped") — the helper only drops byte sequences that
fail `valid_encoding?`. A non-ASCII byte that forms a valid UTF-8
sequence (e.g. a multi-byte character) is retagged and kept. Also dropped
an unverified consumer list ("JSON, log encoders") — only msgpack-ruby
actually keys behavior off `String#encoding` in dd-trace-rb.
* Tracing: fix extract_tracestate @return annotation
Existing annotation claimed a single 4-field array; the method actually
returns one of three shapes: nil (no header / empty), String (header
present but no `dd=` entry), or a 6-element array when `dd=` is present.
The 4-field array's listed order also had origin/ts_parent_id swapped
relative to the actual code at line 304.
Doc-only; the sole caller (`TraceContext#extract`) handles the
polymorphism via Ruby destructuring (nil RHS collapses to all-nil
vars; a single String RHS goes into the first var, rest nil), which
is why the inaccurate doc never caused a bug.
* Tracing: validate tracestate encoding after size-limit truncation
The previous PR commit called Helpers.normalize_tracestate_encoding at
the top of extract_tracestate, before split_tracestate's 512-byte
truncation. A single invalid UTF-8 byte anywhere in the input — even in
the tail that split_tracestate would have discarded — dropped the whole
field.
Move the normalize call into split_tracestate, after the byteslice/chop
block. valid_encoding? now only sees the bytes the function would have
kept, so an oversized header with a bad byte in the discarded tail
preserves its parseable prefix.
Add coverage for the oversized-header-with-0xFF-in-tail case. Existing
test for invalid bytes in the kept portion (under the 512-byte limit)
still drops the field as before.
* Add DD_TRACE_PROPAGATION_BEHAVIOR_EXTRACT to supported configurations list (implementation B)
* Add config option parsing in settings.
* Implementation of DD_TRACE_PROPAGATION_BEHAVIOR
* Update spec with new field propagate behavior
* Add documentation
* Update constructors in contrib
* remove stray comment
* fix: add missing span links arguments in contructors, links -> span_links
* do not pass span links in to_digest
* Update trace digest spec and signature (pass steep check)
* Update propagation spec in contrib
* Refactor trace restart behavior in propagation logic
* Extract style name for the propagator and add it to the span link attributes
* Behavior matrix mirroring dd-trace-go's `TestPropagationBehaviorExtract`.
* Bugfix, reencode to str for span link
tracestate from W3C headers arrives via Rack as
ASCII-8BIT-encoded strings. SpanLink#initialize stored them
with .dup (preserving ASCII-8BIT), and msgpack serializes
ASCII-8BIT strings as the bin type — but the Datadog agent
expects str at SpanLinks/N/Tracestate
* Also pass bagage related distributed tags when created trace digest
* propagation behavior extract tests for trace ditributed tags that are linked to the bagage
* rake standard:fix
* Add types in function signatures
* clarify comment
* nil instead of empty list
* Fix OTEL path when DD_TRACE_BEHAVIOR=restart
* Generic user id
* Tracing: normalize tracestate encoding at propagator ingress
HTTP frameworks (Rack, etc.) hand header values to applications tagged as
ASCII-8BIT. msgpack-ruby uses string encoding as a type signal — UTF-8
serializes as `str`, ASCII-8BIT as `bin`. The trace-agent's wire schema
expects `SpanLinks[N].Tracestate` to be `str`, so receiving `bin` is
rejected/mishandled.
Fix at the two ingress boundaries where tracestate strings cross into the
domain from foreign producers:
- W3C TraceContext propagator (`extract_tracestate`) — extraction of the
`tracestate` HTTP header.
- OpenTelemetry SpanProcessor — `link.span_context.tracestate&.to_s` from
the OTel SDK.
Both call a new `Distributed::Helpers.normalize_tracestate_encoding`,
which `force_encoding`s the bytes to UTF-8 (the W3C tracestate grammar is
restricted to printable ASCII, a subset of UTF-8 — so the bytes are
correct and only the encoding tag is wrong). Strings whose bytes do not
form valid UTF-8 are dropped rather than propagated as corrupted data;
such bytes are spec violations.
* Tracing: add msgpack end-to-end spec for tracestate encoding fix
Builds a SpanLink from a digest extracted via an ASCII-8BIT-tagged
tracestate header, packs and unpacks via msgpack, and asserts the
tracestate field round-trips as a `str` (unpacked as UTF-8) — the wire
shape the trace-agent's schema requires. Pins the end-to-end claim of
the previous commit rather than relying only on intermediate
digest.trace_state.encoding assertions.
* Tracing: tighten normalize_tracestate_encoding doc-comment
Original comment overstated the policy ("non-ASCII bytes are spec
violations and are dropped") — the helper only drops byte sequences that
fail `valid_encoding?`. A non-ASCII byte that forms a valid UTF-8
sequence (e.g. a multi-byte character) is retagged and kept. Also dropped
an unverified consumer list ("JSON, log encoders") — only msgpack-ruby
actually keys behavior off `String#encoding` in dd-trace-rb.
* Tracing: fix extract_tracestate @return annotation
Existing annotation claimed a single 4-field array; the method actually
returns one of three shapes: nil (no header / empty), String (header
present but no `dd=` entry), or a 6-element array when `dd=` is present.
The 4-field array's listed order also had origin/ts_parent_id swapped
relative to the actual code at line 304.
Doc-only; the sole caller (`TraceContext#extract`) handles the
polymorphism via Ruby destructuring (nil RHS collapses to all-nil
vars; a single String RHS goes into the first var, rest nil), which
is why the inaccurate doc never caused a bug.
* Tracing: validate tracestate encoding after size-limit truncation
The previous PR commit called Helpers.normalize_tracestate_encoding at
the top of extract_tracestate, before split_tracestate's 512-byte
truncation. A single invalid UTF-8 byte anywhere in the input — even in
the tail that split_tracestate would have discarded — dropped the whole
field.
Move the normalize call into split_tracestate, after the byteslice/chop
block. valid_encoding? now only sees the bytes the function would have
kept, so an oversized header with a bad byte in the discarded tail
preserves its parseable prefix.
Add coverage for the oversized-header-with-0xFF-in-tail case. Existing
test for invalid bytes in the kept portion (under the 512-byte limit)
still drops the field as before.
* Remove Core::Utils.utf8_encode from span links
* Apply suggestion from @Strech
Co-authored-by: Sergey Fedorov <[email protected]>
* Update lib/datadog/opentelemetry/sdk/propagator.rb
Co-authored-by: Sergey Fedorov <[email protected]>
* Update sig/datadog/tracing/distributed/propagation.rbs
Co-authored-by: Sergey Fedorov <[email protected]>
* Update sig/datadog/tracing/trace_digest.rbs
Co-authored-by: Sergey Fedorov <[email protected]>
* Apply @Strech suggestion (nesting)
* Tracing: rename normalize_tracestate_encoding to force_utf8_encoding
Per Strech's review on #5844 (which targets the same helper as #5867):
the prior name implied a normalization the method does not perform.
The method re-tags the input via String#force_encoding(UTF-8) and
returns nil when the bytes are not a valid UTF-8 sequence. The new
name reads off the underlying operation without overpromising success.
Also collapses the body: the previous `if value.encoding == UTF_8`
early-return branch merges into a single conditional re-encode followed
by one `valid_encoding?` check at the end. Behavior unchanged:
already-UTF-8 input is returned by object identity (no dup), input is
never mutated, frozen input is handled via the dup in the non-UTF-8
branch — all covered by helpers_spec.
Updates the helper, the RBS signature, both call sites
(TraceContext#extract_tracestate, OTel SpanProcessor span-link path),
and the helpers spec describe/subject.
* Fix Otel path when BEHAVIOR_EXTRACT is restart
* Use existing UTF-8 encoding
* Shorten implementation comment
* Improve comments and RBS
---------
Co-authored-by: ddsign <[email protected]>
Co-authored-by: Sergey Fedorov <[email protected]>
Co-authored-by: Marco Costa <[email protected]>
Member
Author
|
Merged in #5844. |
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
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.
What does this PR do?
Routes the two ingress sites where W3C
tracestatestrings enter the tracer's domain — theTraceContextpropagator'ssplit_tracestate(called fromextract_tracestate) and the OpenTelemetrySpanProcessorlink bridge — through a newDistributed::Helpers.force_utf8_encoding. The helper retags ASCII-8BIT-encoded strings as UTF-8 (the bytes are unchanged since W3C tracestate is restricted to printable ASCII, a subset of UTF-8), and drops strings whose bytes do not form valid UTF-8 rather than propagating corrupted data.The propagator-side call sits inside
split_tracestate, after the pre-existing size-limit truncation, so an invalid byte that lands in the truncated tail is discarded by the size cap rather than dropping the whole field — the parseable prefix is preserved.Also rewrites the
@returndoc comment onextract_tracestate, which previously described a 4-tuple but listed five values in the wrong order. The new comment documents the three actual return shapes (nil,String, orArray(String, Integer, String, String, Hash, String)).Motivation:
This is preventive, not a fix for a confirmed customer-impacting bug — see the "Change log entry" section for why.
HTTP servers hand header values to Ruby applications tagged as ASCII-8BIT. The W3C propagator preserves that tag (Ruby's
Array#joinkeeps the source encoding when separator and elements are compatible), so the binary tag flows ontoTraceDigest#trace_state. msgpack-ruby uses string encoding as a type tag — UTF-8 serializes asstr, ASCII-8BIT asbin— and the agent's wire schema specifiesstrforSpanLinks[N].Tracestate.The fix could land in
SpanLink#initializeinstead, but the model layer isn't the right place to make wire-format decisions, and an earlier draft that did so usedCore::Utils.utf8_encode— which silently swallows non-UTF-8 input into an empty string, replacing one kind of corruption with another. Doing the work at the propagator boundary surfaces malformed input as a dropped field with a single explicit code path.Change log entry
None.
In shipping code today, the only path that msgpacks an extracted tracestate is
SpanLink#to_hash, and the only place SpanLinks are constructed from a foreign tracestate is the OpenTelemetry bridge — which reads fromlink.span_context.tracestate&.to_s, an OTel SDK object whose encoding behavior I have not independently verified to be ASCII-8BIT. For the invalid-UTF-8-bytes path this PR adds an explicit drop-to-nil; pre-PR, those bytes would have reached the agent asbin-tagged opaque data, and whatever the agent did with that, the observable customer outcome (no usable tracestate) is the same. So I can't substantiate a customer-visible behavior change either way, and the changelog isNone.rather than a speculative claim.Additional Notes:
Audited the other distributed propagators (
B3,Datadog,Baggage) — none touchtrace_state. The intra-process re-use inlib/datadog/tracing/distributed/propagation.rbinherits its tracestate from the W3C digest and is fixed transitively.How to test the change?
bundle exec rspec spec/datadog/tracing/distributed/helpers_spec.rb— unit coverage forDistributed::Helpers.force_utf8_encoding: nil, empty ASCII-8BIT, ASCII-only ASCII-8BIT (retagged), already-valid UTF-8 (identity), ASCII-8BIT whose bytes form valid UTF-8 (retagged), ASCII-8BIT with invalid UTF-8 byte sequences (dropped), frozen input.bundle exec rspec spec/datadog/tracing/distributed/trace_context_spec.rb— propagator coverage: ASCII-8BIT header produces UTF-8digest.trace_state; invalid UTF-8 drops the field while keeping the traceparent; oversized ASCII-8BIT tracestate with invalid bytes only in the truncated tail preserves the parseable prefix; end-to-end msgpack round-trip fromSpanLink#to_hashreturns the tracestate asstr(UTF-8 on unpack), notbin.bundle exec rake test:opentelemetry— existing OTel link tests pass with the helper inserted.