Repository navigation
Conversation
The OTLP trace writers dropped the Metadata baggage map, which carries the propagation tags (_dd.p.dm, _dd.p.ksr, _dd.p.tid...) and baggage items injected as tags. Write it like the Datadog mappers do, with propagation tags on the local root span only, so the sampling decision maker reaches the Agent. Co-Authored-By: Claude Opus 5.5 <[email protected]>
anna-git
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. |
The OTLP traceId already carries all 128 bits, so the tag is redundant. Co-Authored-By: Claude Opus 5.5 <[email protected]>
A public Tags constant must be registered in the tag registry; the key is only needed by the OTLP writers. Co-Authored-By: Claude Opus 5.5 <[email protected]>
There was a problem hiding this comment.
🟡 Changes recommended
Both encoders can emit duplicate attribute keys when baggage collides with directly emitted span fields or metadata.
1 open issue
What changed in this PR
This PR adds propagation tags and baggage-as-tags to Java’s OTLP trace export, allowing downstream Agent processing to retain sampling decision metadata.
Changes:
- Serialize baggage and propagation tags in protobuf and JSON, with collision checks against span tags.
- Export propagation tags only on local roots and omit redundant
_dd.p.tid. - Add tests for sampling tags, inherited baggage, and root-only propagation tags.
| File | Description |
|---|---|
| dd-trace-core/src/test/java/datadog/trace/core/otlp/trace/OtlpTraceProtoTest.java | Adds protobuf export assertions and attribute parsing helpers. |
| dd-trace-core/src/test/java/datadog/trace/core/otlp/trace/OtlpTraceJsonCollectorTest.java | Adds JSON export assertions and span lookup helper. |
| dd-trace-core/src/main/java/datadog/trace/core/otlp/trace/OtlpTraceProto.java | Serializes baggage and root propagation tags into protobuf attributes. |
| dd-trace-core/src/main/java/datadog/trace/core/otlp/trace/OtlpTraceJson.java | Applies equivalent attribute serialization to JSON export. |
Review effort: Balanced
| // which also keeps OTLP attribute keys unique. _dd.p.tid is skipped: the OTLP traceId already | ||
| // carries the upper 64 bits of the trace id. | ||
| for (Map.Entry<String, String> entry : metadata.getBaggage().entrySet()) { | ||
| if (TRACE_ID_HIGH_TAG.equals(entry.getKey()) || tags.containsKey(entry.getKey())) { |
There was a problem hiding this comment.
Good catch. By default OTLP mode doesn't inject baggage as tags (writer.baggage.inject defaults to false when the OTLP exporter is on), so only _dd.p.* reaches this loop. With it enabled, a raw (non-W3C) baggage key could still collide with attributes the writer emits directly. Fixed in a0096d2: baggage now also skips those keys in both encoders, with regression tests asserting each key appears once with the span's value. 6d518d3 then moves those keys and the skip set into one shared OtlpTraceAttributes class so the two writers can't drift apart.
Co-Authored-By: Claude Opus 5.5 <[email protected]>
Co-Authored-By: Claude Opus 5.5 <[email protected]>
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. |
![datadog-datadog-prod-us1[bot]](https://avatars.githubusercontent.com/u/365230?s=60&v=4){kind=small}
There was a problem hiding this comment.
Baggage collision filtering misses direction-dependent OTLP tag renames. With baggage injection and OTel semantics enabled, both encoders can emit conflicting server.port attributes instead of preserving the span's value.
🤖 Bits Code Review · Commit 6d518d3
| // baggage items injected as tags, as in the Datadog mappers. Span tags and the attributes | ||
| // written directly above win on a key clash, which keeps OTLP attribute keys unique. | ||
| for (Map.Entry<String, String> entry : metadata.getBaggage().entrySet()) { | ||
| if (SKIPPED_BAGGAGE_KEYS.contains(entry.getKey()) || tags.containsKey(entry.getKey())) { |
There was a problem hiding this comment.
With baggage injection and OTel semantics enabled, a client span carrying peer.port=443 and raw baggage server.port="80" exports two server.port attributes. TagMap.containsKey("server.port") checks the distinct server-port tag, while writeSpanTag renames peer.port to server.port for client spans. The span's value should win, but consumers instead receive conflicting duplicate keys. Both protobuf and JSON writers need deduplication against actual exported names, including direction-dependent renames, with regression coverage.
React: 👍 useful · 👎 wrong · 😕 unclear · 👀 right, but not acting on it
What each reaction means
- 👍 Useful: correct, and you're fixing it or will.
- 👎 Wrong: the finding is incorrect, for example the bug can't happen or the code was misread.
- 😕 Unclear: you can't tell what it means or why it matters.
- 👀 Right, not acting: correct, but intended, out of scope for this PR, a known risk, or too minor.
🤖 Bits Code Review · Open Bits AI session
There was a problem hiding this comment.
Proposed patch from Codex, posted at Stuart's request.
Move the existing direction calculation above the baggage loop and resolve direction-dependent baggage names before checking for a matching span tag. containsKey() has no direction-aware overload; directionalKeyOf() plus getEntry(long) covers these aliases. Gate that extra lookup on OTel semantics, matching writeSpanTag().
diff --git a/dd-trace-core/src/main/java/datadog/trace/core/otlp/trace/OtlpTraceProto.java b/dd-trace-core/src/main/java/datadog/trace/core/otlp/trace/OtlpTraceProto.java
--- a/dd-trace-core/src/main/java/datadog/trace/core/otlp/trace/OtlpTraceProto.java
+++ b/dd-trace-core/src/main/java/datadog/trace/core/otlp/trace/OtlpTraceProto.java
@@
TagMap tags = metadata.getTags();
+ // The direction is computed once per span; boxing -1..2 hits the Integer cache.
+ Integer direction = KnownTagCodec.directionOf(tags);
+ boolean otelSemantics = Config.get().isTraceOtelSemanticsEnabled();
@@
for (Map.Entry<String, String> entry : metadata.getBaggage().entrySet()) {
- if (SKIPPED_BAGGAGE_KEYS.contains(entry.getKey()) || tags.containsKey(entry.getKey())) {
+ long directionalTagId =
+ otelSemantics ? KnownTagCodec.directionalKeyOf(entry.getKey(), direction) : 0;
+ if (SKIPPED_BAGGAGE_KEYS.contains(entry.getKey())
+ || tags.containsKey(entry.getKey())
+ || (directionalTagId != 0 && tags.getEntry(directionalTagId) != null)) {
continue;
}
@@
- // The direction is computed once per span; boxing -1..2 hits the Integer cache.
- Integer direction = KnownTagCodec.directionOf(tags);
tags.forEach(buf, direction, OtlpTraceProto::writeSpanTag);
diff --git a/dd-trace-core/src/main/java/datadog/trace/core/otlp/trace/OtlpTraceJson.java b/dd-trace-core/src/main/java/datadog/trace/core/otlp/trace/OtlpTraceJson.java
--- a/dd-trace-core/src/main/java/datadog/trace/core/otlp/trace/OtlpTraceJson.java
+++ b/dd-trace-core/src/main/java/datadog/trace/core/otlp/trace/OtlpTraceJson.java
@@
TagMap tags = metadata.getTags();
+ // The direction is computed once per span; boxing -1..2 hits the Integer cache.
+ Integer direction = KnownTagCodec.directionOf(tags);
+ boolean otelSemantics = Config.get().isTraceOtelSemanticsEnabled();
@@
for (Map.Entry<String, String> entry : metadata.getBaggage().entrySet()) {
- if (SKIPPED_BAGGAGE_KEYS.contains(entry.getKey()) || tags.containsKey(entry.getKey())) {
+ long directionalTagId =
+ otelSemantics ? KnownTagCodec.directionalKeyOf(entry.getKey(), direction) : 0;
+ if (SKIPPED_BAGGAGE_KEYS.contains(entry.getKey())
+ || tags.containsKey(entry.getKey())
+ || (directionalTagId != 0 && tags.getEntry(directionalTagId) != null)) {
continue;
}
@@
- // The direction is computed once per span; boxing -1..2 hits the Integer cache.
- Integer direction = KnownTagCodec.directionOf(tags);
tags.forEach(writer, direction, OtlpTraceJson::writeSpanTag);This retains the current collision guards and adds the missing direction-dependent check. Suggested regression coverage in both encodings: with baggage injection and OTel semantics enabled, a client span with peer.port=443 and peer.hostname=example.com, plus conflicting baggage server.port=80 and server.address=other.example, should export each key exactly once with the span's value. Assert against the raw attribute list so a map does not hide duplicates. Also cover inbound aliases and OTel semantics disabled.
This is a proposed patch based on source inspection; I have not applied or run it.
There was a problem hiding this comment.
Applied @mcculls's patch in 6dfd8eedde ("Don't let baggage duplicate direction-dependent OTLP attribute names"): under OTel semantics, each baggage key is now resolved to its direction-specific tag (KnownTagCodec.directionalKeyOf) and skipped if the span has that tag, so e.g. peer.hostname/peer.port on a client span win over baggage server.address/server.port. Added regression tests (outbound and inbound aliases, plus OTel semantics disabled) asserting each key appears once in the raw attribute list with the span's value. Note this only applies with baggage-as-tags enabled, which is off by default in OTLP mode.
| TagMap tags = metadata.getTags(); | ||
|
|
||
| // Baggage carries the propagation tags (e.g. the _dd.p.dm sampling decision maker) and any | ||
| // baggage items injected as tags, as in the Datadog mappers. Span tags and the attributes | ||
| // written directly above win on a key clash, which keeps OTLP attribute keys unique. | ||
| for (Map.Entry<String, String> entry : metadata.getBaggage().entrySet()) { | ||
| if (SKIPPED_BAGGAGE_KEYS.contains(entry.getKey()) || tags.containsKey(entry.getKey())) { | ||
| continue; | ||
| } | ||
| writeAttribute(writer, STRING_ATTRIBUTE, entry.getKey(), entry.getValue()); | ||
| } | ||
|
|
||
| // The direction is computed once per span; boxing -1..2 hits the Integer cache. | ||
| Integer direction = KnownTagCodec.directionOf(tags); | ||
| tags.forEach(writer, direction, OtlpTraceJson::writeSpanTag); |
There was a problem hiding this comment.
Proposed patch from Codex, posted at Stuart's request. This is the JSON counterpart of the protobuf-thread suggestion.
Compute direction before the baggage loop and check direction-dependent aliases when OTel semantics are enabled. For example, client span tags peer.port and peer.hostname export as server.port and server.address; baggage under those exported names must not create duplicate attributes.
| TagMap tags = metadata.getTags(); | |
| // Baggage carries the propagation tags (e.g. the _dd.p.dm sampling decision maker) and any | |
| // baggage items injected as tags, as in the Datadog mappers. Span tags and the attributes | |
| // written directly above win on a key clash, which keeps OTLP attribute keys unique. | |
| for (Map.Entry<String, String> entry : metadata.getBaggage().entrySet()) { | |
| if (SKIPPED_BAGGAGE_KEYS.contains(entry.getKey()) || tags.containsKey(entry.getKey())) { | |
| continue; | |
| } | |
| writeAttribute(writer, STRING_ATTRIBUTE, entry.getKey(), entry.getValue()); | |
| } | |
| // The direction is computed once per span; boxing -1..2 hits the Integer cache. | |
| Integer direction = KnownTagCodec.directionOf(tags); | |
| tags.forEach(writer, direction, OtlpTraceJson::writeSpanTag); | |
| TagMap tags = metadata.getTags(); | |
| // The direction is computed once per span; boxing -1..2 hits the Integer cache. | |
| Integer direction = KnownTagCodec.directionOf(tags); | |
| boolean otelSemantics = Config.get().isTraceOtelSemanticsEnabled(); | |
| // Baggage carries the propagation tags (e.g. the _dd.p.dm sampling decision maker) and any | |
| // baggage items injected as tags, as in the Datadog mappers. Span tags and the attributes | |
| // written directly above win on a key clash, which keeps OTLP attribute keys unique. | |
| for (Map.Entry<String, String> entry : metadata.getBaggage().entrySet()) { | |
| long directionalTagId = | |
| otelSemantics ? KnownTagCodec.directionalKeyOf(entry.getKey(), direction) : 0; | |
| if (SKIPPED_BAGGAGE_KEYS.contains(entry.getKey()) | |
| || tags.containsKey(entry.getKey()) | |
| || (directionalTagId != 0 && tags.getEntry(directionalTagId) != null)) { | |
| continue; | |
| } | |
| writeAttribute(writer, STRING_ATTRIBUTE, entry.getKey(), entry.getValue()); | |
| } | |
| tags.forEach(writer, direction, OtlpTraceJson::writeSpanTag); |
Suggested regression coverage: enable baggage injection and OTel semantics, add conflicting baggage named server.port and server.address to a client span, and assert each key occurs once with the span's value in the raw JSON attribute list. Also cover inbound aliases and OTel semantics disabled.
This is a proposed patch based on source inspection; I have not applied or run it.
There was a problem hiding this comment.
Applied in 6dfd8eedde, same change as in the protobuf encoder: direction-dependent aliases are checked before writing baggage under OTel semantics. JSON tests cover client peer.hostname vs baggage server.address, server http.hostname / network.client.ip, and OTel semantics disabled (where both keys are kept).
| // Like the Datadog mappers, only serialize propagation tags (_dd.p.dm, _dd.p.ksr...) once per | ||
| // trace segment: on the local root span, which owns them. | ||
| span.processTagsAndBaggage( | ||
| metaWriter, span.getLocalRootSpan() == span ? ROOT_PROPAGATION_TAGS : 0); |
There was a problem hiding this comment.
Proposed patch from Codex, posted at Stuart's request.
Use the collector's existing includeSamplingTags boundary flag to select the span that carries root propagation tags. A partial flush can contain only child spans; the local-root identity check then omits _dd.p.dm and _dd.p.ksr entirely. ROOT_PROPAGATION_TAGS already reads the local root's propagation tags when invoked on a child.
| // Like the Datadog mappers, only serialize propagation tags (_dd.p.dm, _dd.p.ksr...) once per | |
| // trace segment: on the local root span, which owns them. | |
| span.processTagsAndBaggage( | |
| metaWriter, span.getLocalRootSpan() == span ? ROOT_PROPAGATION_TAGS : 0); | |
| // Export root propagation tags on the span selected by the collector for sampling tags, | |
| // including when the exported trace contains no local root span. | |
| span.processTagsAndBaggage( | |
| metaWriter, metaWriter.includeSamplingTags ? ROOT_PROPAGATION_TAGS : 0); |
The enclosing writer can access its nested MetaWriter's private flag, so this needs no accessor or collector changes. The flag is read before accept() resets it. Consecutive chunks of the same trace in one payload can share a single copy of these tags.
Apply the same change in both encodings. Update the current root-only regression tests to assert one exported copy per trace in the payload: protobuf prepends spans and selects the first exported span, while JSON selects the last exported span. Add coverage for a child-only partial flush and multiple trace IDs in one payload, retaining the _dd.p.tid exclusion and baggage assertions.
This is a proposed patch based on source inspection; I have not applied or run it.
There was a problem hiding this comment.
Applied in ee1fa5bb57 ("Export propagation tags on partial flushes without the local root"): propagation tags now go on the span the collector selects for sampling tags, so partial flushes without the local root keep _dd.p.dm/_dd.p.ksr. Added tests for a child-only flush and multiple traces per payload (both encoders).
| // Like the Datadog mappers, only serialize propagation tags (_dd.p.dm, _dd.p.ksr...) once per | ||
| // trace segment: on the local root span, which owns them. | ||
| span.processTagsAndBaggage( | ||
| metaWriter, span.getLocalRootSpan() == span ? ROOT_PROPAGATION_TAGS : 0); |
There was a problem hiding this comment.
Proposed patch from Codex, posted at Stuart's request.
Use the collector's existing includeSamplingTags boundary flag to select the span that carries root propagation tags. A partial flush can contain only child spans; the local-root identity check then omits _dd.p.dm and _dd.p.ksr entirely. ROOT_PROPAGATION_TAGS already reads the local root's propagation tags when invoked on a child.
| // Like the Datadog mappers, only serialize propagation tags (_dd.p.dm, _dd.p.ksr...) once per | |
| // trace segment: on the local root span, which owns them. | |
| span.processTagsAndBaggage( | |
| metaWriter, span.getLocalRootSpan() == span ? ROOT_PROPAGATION_TAGS : 0); | |
| // Export root propagation tags on the span selected by the collector for sampling tags, | |
| // including when the exported trace contains no local root span. | |
| span.processTagsAndBaggage( | |
| metaWriter, metaWriter.includeSamplingTags ? ROOT_PROPAGATION_TAGS : 0); |
The enclosing writer can access its nested MetaWriter's private flag, so this needs no accessor or collector changes. The flag is read before accept() resets it. Consecutive chunks of the same trace in one payload can share a single copy of these tags.
Apply the same change in both encodings. Update the current root-only regression tests to assert one exported copy per trace in the payload: protobuf prepends spans and selects the first exported span, while JSON selects the last exported span. Add coverage for a child-only partial flush and multiple trace IDs in one payload, retaining the _dd.p.tid exclusion and baggage assertions.
This is a proposed patch based on source inspection; I have not applied or run it.
There was a problem hiding this comment.
Applied in ee1fa5bb57, same change as in the protobuf encoder. In JSON the tags now sit on the last exported span of each trace (where sampling tags already go), including child-only partial flushes. Tests cover a single trace, a child-only flush and multiple traces in one payload.
mcculls
left a comment
mcculls
left a comment
There was a problem hiding this comment.
Added some proposed patches (drafted with codex) - once those are fixed then I'll re-review
Co-Authored-By: Claude Opus 5.5 <[email protected]>
Co-Authored-By: Claude Opus 5.5 <[email protected]>
What Does This Do
Exports the span propagation tags (
_dd.p.dm,_dd.p.ksr, ...) and baggage-as-tags items as string attributes in OTLP trace export (protobuf and JSON), mirroring the Datadog mappers:OtlpTraceProto/OtlpTraceJsonMetaWriters now writeMetadata.getBaggage(), which was computed but never serialized. Span tags, and the attributes the writers emit directly (resource.name,operation.name,thread.name,_sampling_priority_v1, ...), win on a key clash, so OTLP attribute keys stay unique.ROOT_PROPAGATION_TAGS, read from the local root's context) are written once per trace in the payload, on the span the collector already selects for sampling tags (first exported span of a trace in protobuf, last in JSON). This covers partial flushes whose chunk has no local root span; other spans don't carry_dd.p.*. The Agent takes_dd.p.dmfrom the first span in a chunk that has it, so the carrier doesn't need to be the root._dd.p.tidis intentionally not exported: the OTLPtraceIdalready carries all 128 bits, and the Agent's OTLP receiver derives the upper 64 bits from it, so the tag would be redundant (Java generates 128-bit trace ids by default, so every root span would otherwise carry it). Same as the .NET change.Motivation
With
DD_TRACE_OTEL_EXPORTER=otlp, remote and adaptive sampling rules are applied (priority,_dd.rule_psrare exported), but the sampling decision maker_dd.p.dmnever was. The Agent then falls back to a default decision maker, so the backend doesn't count the traffic as rule/adaptive-sampled and adaptive sampling can't converge for the service.This complements DataDog/datadog-agent#58471, which preserves
_dd.p.dmfrom OTLP span attributes (ort.dmfrom a tracestatedd=member). The .NET equivalent is DataDog/dd-trace-dotnet#9498.Additional Notes
_dd.p.dm:-4; with this change the exported trace is expected to carry the rule's decision maker (-11remote user rule /-12remote adaptive rule), which the Agent keeps with [trace-agent] Preserve tracer sampling decision maker in OTLP receiver datadog-agent#58471. The end-to-end run with this exact build is still to do.OtlpTraceProtoTestandOtlpTraceJsonCollectorTest:propagationTagsAreWrittenOncePerTraceOnTheSpanSelectedForSamplingTags,partialFlushWithoutTheLocalRootStillCarriesPropagationTagsandpropagationTagsAreWrittenOnceForEachTraceInThePayloadcheck exactly one copy of_dd.p.dm/_dd.p.ksrper trace, on the selected span, no_dd.p.tid, and baggage on every span;baggageDoesNotDuplicateDirectlyWrittenAttributesin both checks that baggage items named like directly emitted attributes don't duplicate them.:dd-trace-core:spotlessCheckcouldn't run locally because the Eclipse p2 site used by the Groovy formatter was down; changed files were checked with google-java-format 1.36.1.dd=tracestate member, so the OTLPtrace_statedoesn't carryt.dm.Contributor Checklist
type:and (comp:orinst:) labels in addition to any other useful labelsclose,fix, or any linking keywords when referencing an issueUse
solvesinstead, and assign the PR milestone to the issueJira ticket: N/A
🤖 Generated with Claude Code