Map OpenTelemetry tag names via the registry (pass-through default) - #12230
Map OpenTelemetry tag names via the registry (pass-through default)#12230dougqh wants to merge 1 commit into
Conversation
|
@codex review |
dougqh
added
comp: core
![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: b12ee865ab
ℹ️ 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".
| String otelName = tagEntry.openTelemetryName(); | ||
| String key = otelName != null ? otelName : tagEntry.tag(); |
There was a problem hiding this comment.
With the default trace.experimental.dense.tags.enabled=false, the only production call to KnownTags.init() remains guarded by DENSE_TAGS_ENABLED in CoreTracer, so KnownTagCodec is inactive here. Consequently tagEntry.openTelemetryName() returns null for every tag and normal OTLP exports continue emitting http.method, http.status_code, and the other Datadog names instead of the mappings introduced by this change; only users enabling the experimental dense-tag store see the new behavior. Registry name resolution needs to be initialized independently of whether dense storage is enabled.
Useful? React with 👍 / 👎.
| - { tag: servlet.path, type: string, required: optional } | ||
| - { tag: servlet.context, type: string, required: optional } | ||
| - { dd-name: http.url, type: string, required: required, otel-name: url.full } # NOTE: single http.url identity (shared w/ http.client) => one otel-name. url.full is the client-correct rename; server's spec mapping (url.path + url.scheme + url.query) is a one-to-many split reserved for the derivation layer (needs span.kind). TODO(otel): server split. | ||
| - { dd-name: http.route, type: string, required: conditional, otel-name: http.route } # same name in both namespaces |
There was a problem hiding this comment.
Declaring http.route as its own OpenTelemetry name makes the generated resolver return "http.route", but KnownTagsTest.tagsWithoutOtelNameReturnNull still asserts that openTelemetryNameOf(KnownTags.HTTP_ROUTE_ID) is null. The internal-api test suite therefore fails deterministically once this generated registry is tested; update that assertion and include the same-name mapping among the applicable-tag cases.
Useful? React with 👍 / 👎.
| require(!requireOtelName || m.containsKey("otel-name")) { | ||
| "tag '$ddName' must declare otel-name (a name, the same name, or 'none')" |
There was a problem hiding this comment.
For a stored tag written as otel-name: with no value, SnakeYAML supplies a present key whose value is null, so this check passes even though the documented tri-state permits only a nonempty name or the literal none; the subsequent safe cast silently converts that invalid value into a Datadog-only tag. Non-string values are accepted the same way, allowing a typo in this registry to bypass the intended build-time guardrail and generate an incorrect applicability bit. Require a valid string value and explicitly recognize none, rather than checking only containsKey.
Useful? React with 👍 / 👎.
|
🎯 Code Coverage (details) 🔗 Commit SHA: 2808033 | Docs | View more details | Give us feedback! |
🟢 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. |
b12ee86 to
d8ce773
Compare
dougqh
changed the title
Give every known tag an OpenTelemetry name derived from the registry, so a serializer can emit DD spans under OTel semantic-convention names without a separate mapping table. - otel-name is OPTIONAL and tri-state: absent means the OTel name is implicitly the dd-name (pass-through / RFC "retain"); a value renames; the literal `none` reserves a Datadog-only tag (suppressed from OTel). `none` has zero instances today and currently behaves as pass-through. - dd-name is the canonical Datadog name AND the tag identity. keyOf is many->one: any name (canonical or OTel) resolves to the one id; nameOf still returns the canonical name. - openTelemetryNameOf(id) returns the RENAME or null; OtlpTraceProto falls back to the dd-name on null, so pass-through needs no per-tag data on the wire path. - Rename the YAML keys tag: -> dd-name: and open-telemetry-name -> otel-name. - Guard tag identity: fail the build when one canonical name is declared with conflicting otel-names across span types/mixins (a span-kind- dependent mapping is a derivation, not a rename). - No OTEL_APPLICABLE id bit: with pass-through as the default every known tag today is emitted under OpenTelemetry, so the flag would be constant (dropped as a false generalization). It returns as a follow-on once a Datadog-only (`none`) tag exists. - Per the DDOT HTTP semconv RFC: http.useragent -> user_agent.original; http.url stays url.full (client-correct) with server's split deferred to the derivation layer. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
d8ce773 to
2808033
Compare
19222e1 to
33f32c4
Compare
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2808033700
ℹ️ 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".
| - { tag: http.status_code, type: int, required: conditional, open-telemetry-name: http.response.status_code } | ||
| - { tag: network.protocol.version, type: string, required: recommended } | ||
| - { dd-name: http.method, type: string, required: required, otel-name: http.request.method } | ||
| - { dd-name: http.status_code, type: int, required: conditional, otel-name: http.response.status_code } |
There was a problem hiding this comment.
When an HTTP status is set through AgentSpan.setHttpStatusCode or the TagInterceptor, it is stored in Metadata.httpStatusCode, not metadata.getTags(). OtlpTraceProto.MetaWriter.accept emits that value through the writeSpanTag(buf, HTTP_STATUS, ...) overload, bypassing the new EntryReader.openTelemetryName() projection, so ordinary spans still export http.status_code rather than the mapping declared here. Fresh evidence beyond the earlier resolver-initialization comment is that the resolver is now initialized unconditionally, but this first-class metadata path never consults it.
Useful? React with 👍 / 👎.
| String otelName = tagEntry.openTelemetryName(); | ||
| String key = otelName != null ? otelName : tagEntry.tag(); |
There was a problem hiding this comment.
With the registry activated by the static CoreTracer in OtlpTraceProtoTest, this lookup changes the http.method test tags at lines 363 and 378 into http.request.method. However, verifySpan at lines 996-1000 still requires every original extraTags key to be present, so the string-tag and mixed-tag parameterized cases fail deterministically instead of validating the new output namespace.
Useful? React with 👍 / 👎.
| String otelName = tagEntry.openTelemetryName(); | ||
| String key = otelName != null ? otelName : tagEntry.tag(); |
There was a problem hiding this comment.
With the default KnownTagCodec.DENSE_STORE=false, TagMap.set keeps a Datadog name and its OpenTelemetry alias as separate bucket entries. If a span contains both http.method and http.request.method—for example when Datadog and OpenTelemetry instrumentation both contribute attributes—this projection maps both entries to http.request.method, and MetaWriter emits two attributes with the same key and potentially conflicting, iteration-order-dependent values. These keys remained distinct before this change; the exporter needs to canonicalize or deduplicate projected names independently of the experimental dense store.
Useful? React with 👍 / 👎.
| default String openTelemetryName() { | ||
| return KnownTagCodec.openTelemetryNameOf(tagId()); |
There was a problem hiding this comment.
perf: When dense tags are enabled, MetaWriter.accept reaches this method through TagMap.forEach for every serialized tag, but that dense iteration path initializes EntryReadingHelper with only nameOf(knownIds[i]). Calling tagId() here therefore hashes and probes keyOf(name) again for every emitted dense tag, even though knownIds[i] is already available and the existing three-argument EntryReadingHelper.set overload can retain it. This is mechanism-determined repeated work on the per-span serialization path; pass the known ID through and verify the improvement with the serialization benchmark.
AGENTS.md reference: AGENTS.md:L79-L81
Useful? React with 👍 / 👎.
1 participant
What
Give every known tag an OpenTelemetry name derived from the tag registry, so a serializer can emit Datadog spans under OTel semantic-convention names without a separate mapping table. Draft direction-demo for the OTel team, based on #12047 so their follow-on PR shows only the OTel delta.
Namespace model
otel-nameis optional and tri-state:dd-namenone¹
nonehas zero instances today and currently behaves as pass-through; true suppression is a follow-on.dd-nameis the canonical Datadog name and the tag identity.keyOfis many→one — any name (canonical or OTel) resolves to the one id;nameOfstill returns the canonical name.openTelemetryNameOf(id)returns the rename or null;OtlpTraceProtofalls back todd-nameon null, so pass-through needs no per-tag data on the wire path.Notable decisions
OTEL_APPLICABLEid bit — with pass-through as the default, every known tag today is emitted under OpenTelemetry, so the flag would be constant (dropped as a false generalization). It returns once anone(Datadog-only) tag exists.otel-names across span types/mixins. A span-kind-dependent mapping (serverhttp.url) is a derivation, not a rename — deferred to the derivation layer.tag:→dd-name:andopen-telemetry-name→otel-name.http.useragent→user_agent.original;http.urlstaysurl.full(client-correct), server split deferred.Test
generate+spotless+verifyclean;internal-api+dd-trace-corecompile clean;KnownTagsTestandTagMapDenseFuzzForkedTestpass.🤖 Generated with Claude Code