refactor(propagation): Parse OpenTelemetry tracestate member - #12405
refactor(propagation): Parse OpenTelemetry tracestate member#12405MilanGarnier wants to merge 15 commits into
Conversation
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. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 397a1ff69a
ℹ️ 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.
More details
The change keeps the raw ot member and its inherited position when it rebuilds tracestate. The static review finds no reportable regression in the changed parsing and serialization paths.
🤖 Datadog Autotest · Commit 397a1ff · What is Autotest? · @DataDog review to ask questions · Any feedback? Reach out in #autotest
mcculls
left a comment
There was a problem hiding this comment.
The double-parsing issue will need to be fixed before this can be merged.
Claude suggests giving PTags a way to accept an already-built OtelTraceState directly (e.g. setOtelTraceState), and have applyTraceContextToFirstContext pass the parsed object from traceContext.getPropagationTags() instead of round-tripping through the string.
The double-counting issue is easily solved by removing the second addition.
db321c0 to
1c02400
Compare
61e502b to
2bc71c1
Compare
b6c0c9a to
074cb0d
Compare
dad360e to
c7a64c3
Compare
There was a problem hiding this comment.
🟡 Changes recommended
The core round-trip behavior lacks coverage, and parsing introduces an avoidable hot-path allocation.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
Separates the OpenTelemetry ot tracestate member for future sampling support while preserving its opaque value and position.
Changes:
- Extracts and stores
otmember metadata. - Re-emits
otin its inherited position. - Copies parsed state across propagation contexts.
File summaries
| File | Description |
|---|---|
OtelTraceStateParsingTest.java |
Tests the new state container. |
W3CPTagsCodec.java |
Extracts and rebuilds ot members. |
PTagsFactory.java |
Stores and transfers parsed state. |
OtelTraceState.java |
Defines the opaque state representation. |
PropagationTags.java |
Adds state-copying API. |
HttpCodec.java |
Uses state-aware copying during extraction. |
Review details
- Files reviewed: 6/6 changed files
- Comments generated: 2
- Review effort level: Balanced
💡 Configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| void retainsValueAndMemberMetadata() { | ||
| OtelTraceState state = | ||
| OtelTraceState.parse(VALUE, INHERITED_POSITION, ORIGINAL_MEMBER_CONTRIBUTION_SIZE); |
|
Note the new muzzle failure happens on master too - it will be unblocked once #12478 is merged |
already found in extraction
mcculls
left a comment
There was a problem hiding this comment.
I think this is ready to be merged now - thanks for the continual improvements to this PR, and the extra tests.
Note I'm going to add a task to revisit this parser in the future to think about ways it could be refactored to simplify the overall structure/logic, but that's separate to this feature
What Does This Do
Refactors W3C tracestate handling so the OpenTelemetry
otmember isrepresented separately while its value remains opaque.
otmember.rvorthor connect the member to sampling.Motivation
Isolate the tracestate parsing foundation from the consistent-sampling
behavior added by the follow-up PR.
Additional Notes
This is the first PR in a two-PR stack. The sampling behavior follows in
#12397.
Validation:
:dd-trace-core:compileJava:dd-trace-core:spotlessCheckgit diff --checkContributor Checklist
type:andcomp:labels.Jira ticket: APMAPI-2171