Fix trailing element separator rejecting valid dd tracestate - #12229
Conversation
mcculls
added
type: bug fix
This comment has been minimized.
This comment has been minimized.
🟢 Java Benchmark SLOs — All performance SLOs passed
PR vs. master results
Commit: Load and DaCapo benchmarks can be triggered manually in the GitLab pipeline. Results will appear in the Benchmarking Platform UI after completion. |
![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: 18d90ea4f7
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
This PR makes W3C propagation parsing more tolerant by accepting a trailing element separator in the dd= member (e.g., dd=...;) and adds a regression test to ensure the behavior is preserved.
Changes:
- Add a test case covering
dd=state values with a trailing;. - Relax parsing/validation logic to treat a trailing separator as a valid terminator.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| dd-trace-core/src/test/java/datadog/trace/core/propagation/W3CHttpExtractorTest.java | Adds coverage for dd= tracestate values that end with a trailing element separator. |
| dd-trace-core/src/main/java/datadog/trace/core/propagation/ptags/W3CPTagsCodec.java | Updates validation to stop at the separator even when it is the final character, enabling acceptance of trailing separators. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
![datadog-datadog-us1-prod[bot]](https://avatars.githubusercontent.com/u/365230?s=60&v=4){kind=small}
There was a problem hiding this comment.
A trailing ; is now accepted only when it is the literal final character. If valid optional whitespace follows it, the parser mistakes that whitespace for another tag key and still drops the decoded Datadog sampling, origin, and propagation tags.
🤖 Datadog Autotest · Commit 18d90ea · What is Autotest? · @DataDog review to ask questions · Any feedback? Reach out in #autotest
|
🤖 Claude (via automated review) Nice fix for the trailing-separator rejection in Repro: So an upstream producer emitting a trailing separator on the |
afb3f3b to
4853d64
Compare
isAllowedKeyChar didn't exclude the element separator, so a key scan starting right after an empty element (e.g. "dd=s:2;;o:some") would swallow the next ';' into the key instead of failing, silently dropping the origin tag rather than rejecting the malformed member. Datadog's sibling codec already excludes its element separator from key chars; align W3C's the same way. Flagged by Copilot review on PR #12229.
|
@codex review |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated no new comments.
Suppressed comments (3)
dd-trace-core/src/main/java/datadog/trace/core/propagation/ptags/W3CPTagsCodec.java:357
- This change makes
validateCharsUntilSeparatorOrEndaccept a trailingseparatorfor all separators passed to it. If this helper is also used with key/value separators (e.g.,:), it can broaden accepted inputs (e.g.,key:with an empty value) beyond the intended relaxation (trailing element separator only). Consider adding anallowTrailingSeparatorparameter (or a second helper) so only element/tag-list separators allow trailing separators, while key/value separators keep the stricter behavior.
pos++;
if (pos < end) {
c = s.charAt(pos);
if (c == separator) {
break; // trailing separator allowed; caller resumes parsing from here
}
}
dd-trace-core/src/main/java/datadog/trace/core/propagation/ptags/DatadogPTagsCodec.java:182
- Same concern as in
W3CPTagsCodec: this helper now permits trailing separators for any separator it is invoked with. If the method is reused for validating segments split by the key/value separator, this can unintentionally allow empty values. Consider scoping the relaxation to list separators only (e.g., via a boolean flag or dedicated method) to avoid widening the accepted grammar more than required.
pos++;
if (pos < end) {
c = s.charAt(pos);
if (c == separator) {
break; // trailing separator allowed; caller resumes parsing from here
}
}
dd-trace-core/src/main/java/datadog/trace/core/propagation/ptags/W3CPTagsCodec.java:359
- The updated validation/parsing behavior (including the trailing-separator relaxation) appears duplicated across
W3CPTagsCodecandDatadogPTagsCodec. To reduce the risk of the two codecs drifting over time, consider extracting the common validation routine(s) into a shared utility (or a small internal base/helper) with codec-specific configuration for separators/allowed-char predicates.
pos++;
if (pos < end) {
c = s.charAt(pos);
if (c == separator) {
break; // trailing separator allowed; caller resumes parsing from here
}
}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d11ce6a2e9
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6fa097a94d
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated no new comments.
Suppressed comments (2)
dd-trace-core/src/main/java/datadog/trace/core/propagation/ptags/W3CPTagsCodec.java:120
- The new “skip malformed element” behavior means an element with a known tag key but an empty value (e.g.
t.dm:/t.tid:) will be treated as a malformed element and skipped, so earlier valid elements in thedd=value will still be kept. This contradicts the PR description’s exception that known tags with semantically invalid values should drop the wholeddmember, and also undermines the intent that a trailing:remains invalid.
Consider explicitly detecting the empty-value case before calling validateCharsUntilSeparatorOrEnd and, for known semantic tags (t.dm, t.tid, t.ts), reverting to dropping the entire dd member (while still skipping empty values for unknown tags). Also add a test like dd=s:0;t.dm:934086a686-4;t.tid: to lock this behavior down.
int tagValuePos = tagKeyEndsAt + 1;
int tagValueEndsAt =
dd-trace-core/src/main/java/datadog/trace/core/propagation/ptags/W3CPTagsCodec.java:663
cleanUpAndAppendUnknownfinds the end of eachdd=element usingoriginal.indexOf(ELEMENT_SEPARATOR, elementStart)without bounding it toddMemberValueEnd. If theddmember has a single unknown element without any;(e.g.dd=x:y) and a later non-dd tracestate member contains;in its value,indexOfcan jump past theddmember and cause this method to skip or append an incorrect slice that spans across the comma into the next member.
Cap elementEnd to ddMemberValueEnd when indexOf returns a position beyond it.
int elementEnd = original.indexOf(ELEMENT_SEPARATOR, elementStart);
if (elementEnd < 0) {
elementEnd = w3CPTags.ddMemberValueEnd;
}
A trailing ';' at the end of the W3C tracestate 'dd' member value was incorrectly rejected as invalid, dropping the entire dd member and any decoded tags, priority, or origin instead of ignoring the separator. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Skipping to the next comma when a trailing separator is followed by OWS (e.g. "dd=s:2;o:some; ,x=y") left the parser resuming on a space, which isAllowedKeyChar rejects and drops the whole dd member. We add skipLeadingOWC to consume that OWS, and restyle stripTrailingOWC to the same while-loop shape.
isAllowedKeyChar didn't exclude the element separator, so a key scan starting right after an empty element (e.g. "dd=s:2;;o:some") would swallow the next ';' into the key instead of failing, silently dropping the origin tag rather than rejecting the malformed member. Datadog's sibling codec already excludes its element separator from key chars; align W3C's the same way. Flagged by Copilot review on PR #12229.
… codec's isAllowedKeyChar. Note TAG_KEY_SEPARATOR is already caught by the separator check in validateCharsUntilSeparatorOrEnd, so this doesn't change behavior, but keeps the predicate decoupled from how it's used by the caller. (the similar KEY_VALUE_SEPARATOR check in the W3C codec is also a no-op given how it's used, but is also worth keeping in case the calling code changes.)
…content cleanUpAndAppendUnknown re-scans the raw tracestate to re-append unknown dd submembers, assuming every element is at least 2 chars long so it can peek a second character to detect known s/o tags. Accepting a trailing separator followed only by OWS (e.g. "dd=x:y; ") breaks that assumption. We skip leading OWS before each element the same way the value scanner already does, so a trailing whitespace-only remainder is recognized as padding and the loop exits cleanly.
(unless we're sure it's a bad tid/dm/ts value in which case it's safer to drop the whole section) Ensure last parent tag (p:) is not duplicated when updating
6b1cc37 to
a226c19
Compare
a226c19 to
2618d35
Compare
|
@codex review |
|
Codex Review: Didn't find any major issues. Keep them coming! Reviewed commit: ℹ️ About Codex in GitHubCodex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback". |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated no new comments.
Suppressed comments (1)
dd-trace-core/src/main/java/datadog/trace/core/propagation/ptags/W3CPTagsCodec.java:134
- When
tagValueEndsAt < 0, the code now skips the malformed element unconditionally. Fort.*elements this means an empty value liket.tid:/t.dm:(i.e.,:followed immediately by;or end-of-dd-member) will be silently dropped while the rest of theddmember is still accepted and propagated. This contradicts the stated intent that the trailing-:(key with no value) case remains rejected, and can mask genuinely corruptt.*content when valid siblings are present (e.g.,dd=s:2;t.tid:;o:rumwould round-trip without rejecting theddmember).
if (tagValueEndsAt < 0) {
int nextTagPos = skipMalformedElement(value, tagValuePos, ddMemberValueEnd);
maxUnknownSize += (nextTagPos - tagPos); // still relay malformed elements
tagPos = nextTagPos;
continue;
|
/merge |
|
View all feedbacks in Devflow UI.
The expected merge time in
The merge request has been interrupted because the build 3316069053876440707 took longer than expected. The current limit for the base branch 'master' is 120 minutes. |
|
/merge |
|
View all feedbacks in Devflow UI.
The expected merge time in
|
Summary
;at the end of the W3C tracestateddmember value was rejected as invalid, dropping the entireddmember (and its decoded tags/priority/origin) instead of just ignoring the harmless trailing separator.;at the end, as the preceding content is still parseable.W3CPTagsCodec.validateCharsUntilSeparatorOrEndto always terminate on the element separator, regardless of position, matching the intent of the surrounding parser. The still-invalid case of a trailing:(key with no value) remains rejected via an existing downstream check.DatadogPTagsCodec(the_dd.p.*header codec), so_dd.p.dm=-4,is accepted the same way.;(element separator) fromisAllowedKeyCharin the W3C codec, and=/,(TAG_KEY_SEPARATOR/TAGS_SEPARATOR) in the Datadog codec, so a stray separator can no longer be silently swallowed into the next key.;separators, anywhere in theddvalue (leading, interior, or trailing), optionally padded with OWS — are ignored rather than rejected, e.g.dd=s:2;;o:someanddd=;s:2;o:someare both accepted and behave the same asdd=s:2;o:some. This mirrors how the outer W3C tracestate grammar already tolerates OWS around commas between list-members.ddvalue (invalid key/value characters, interior OWS abutting real content, missing:) are now skipped over during parsing instead of discarding the entireddmember — they never become decoded tags. The one exception: a known tag whose value fails semantic validation (t.dm/t.tid/t.djformat checks) still drops the wholeddmember, since at that point we know the content is genuinely corrupt rather than just malformed framing.ddmember for propagation, malformed/unrecognized elements are relayed byte-for-byte alongside the elements we do understand, rather than being dropped from the outgoing header. We don't extract data from them (they're excluded from the decoded tag map), but we also don't assume they're meaningless to every consumer — a different/future tracer version may understand a shape we don't. E.g.dd=s:0;t.dm:934086a686-4; t.x:ydecodes onlys:0;t.dm:934086a686-4into tags, but the outgoing header still contains the trailingt.x:yunchanged. This behavior is now deterministic regardless of what else is present in theddvalue (previously it depended by accident on whether another unrelated unrecognized-but-valid element happened to be present).cleanUpAndAppendUnknownre-emitting a knownp:element a second time as if it was unrecognized passthrough content, producing a duplicated tag in the outgoing header whenever an unrelated unknown element was also present (e.g.dd=p:b6241412414a;x:yround-tripping todd=p:b6241412414a;x:y;p:b6241412414a).Test plan
W3CHttpExtractorTest,W3CPropagationTagsTest, andDatadogPropagationTagsTestcases covering: trailing element separator, trailing separator + OWS (space/tab), trailing separator + OWS before the next list-member comma, empty/leading/interior element separators (ignored), interior OWS directly abutting content (dropped from decoded tags, not a full rejection), a malformed empty-value element (dropped from decoded tags, siblings retained), bare/colon-less and space-containing malformed elements before/after/between known elements (relayed verbatim in the outgoing header, excluded from decoded tags, consistently regardless of neighboring content), ando:/s:/p:elements not being duplicated when re-serialized alongside an unknown element../gradlew :dd-trace-core:test --tests "datadog.trace.core.propagation.*"passes.🤖 Generated with Claude Code