Repository navigation
Conversation
Entries set on the span builder are canonicalized to their Datadog name (#12354), so a split-by-tags entry configured by its OpenTelemetry name (e.g. db.system) no longer matched. Add the Datadog name of each configured tag at load. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
dougqh
added
comp: core
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-prod-us1-3[bot]](https://avatars.githubusercontent.com/u/365230?s=60&v=4){kind=small}
There was a problem hiding this comment.
More details
Canonical aliases allow builder and default tags to match OpenTelemetry-configured split tags while preserving the original spellings used by span setters. The completed static review identified no actionable regression.
🤖 Bits Code Review · Commit ab3d841
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. |
bric3
left a comment
bric3
left a comment
There was a problem hiding this comment.
I think the test should cover more cases, I asked codex to suggest a test that cover more ground.
Otherwise LGTM
| @TableTest({ | ||
| "scenario | configured | tag ", | ||
| "configured by OTel name | db.system | db.system", | ||
| "configured by Datadog name | db.type | db.type ", | ||
| "Datadog name, set with OTel name | db.type | db.system" | ||
| }) | ||
| void splitByTagsMatchesTheCanonicalNameOfABuilderTag(String configured, String tag) { | ||
| CoreTracer tracer = createSplittingTracer(configured); | ||
|
|
||
| AgentSpan builderSpan = tracer.buildSpan("datadog", "some span").withTag(tag, "split").start(); | ||
| AgentSpan setterSpan = tracer.buildSpan("datadog", "some span").start(); | ||
| setterSpan.setTag(tag, "split"); | ||
|
|
||
| assertEquals("split", builderSpan.getServiceName()); | ||
| if (configured.equals(tag)) { | ||
| assertEquals("split", setterSpan.getServiceName()); | ||
| } | ||
| } |
There was a problem hiding this comment.
suggestion: Expand the inputs to cover the reverse alias pairing, custom and unconfigured tags, an empty configuration, both names already configured, and multiple renamed tags. This exercises the remaining branches in withCanonicalNames, including preserving custom tags when the set is copied.
Give the setter an explicit expected result: with db.type configured and db.system set, the builder splits but the setter keeps my-service. The conditional currently skips that assertion. The same rows can check default tags, which also arrive canonicalized.
| @TableTest({ | |
| "scenario | configured | tag ", | |
| "configured by OTel name | db.system | db.system", | |
| "configured by Datadog name | db.type | db.type ", | |
| "Datadog name, set with OTel name | db.type | db.system" | |
| }) | |
| void splitByTagsMatchesTheCanonicalNameOfABuilderTag(String configured, String tag) { | |
| CoreTracer tracer = createSplittingTracer(configured); | |
| AgentSpan builderSpan = tracer.buildSpan("datadog", "some span").withTag(tag, "split").start(); | |
| AgentSpan setterSpan = tracer.buildSpan("datadog", "some span").start(); | |
| setterSpan.setTag(tag, "split"); | |
| assertEquals("split", builderSpan.getServiceName()); | |
| if (configured.equals(tag)) { | |
| assertEquals("split", setterSpan.getServiceName()); | |
| } | |
| } | |
| @TableTest({ | |
| "scenario | configured | tag | expectedBuilder | expectedSetter", | |
| "configured by OTel name | db.system | db.system | split | split ", | |
| "configured by Datadog name | db.type | db.type | split | split ", | |
| "Datadog config, OTel tag | db.type | db.system | split | my-service ", | |
| "OTel config, Datadog tag | db.system | db.type | split | split ", | |
| "custom tag | custom.tag | custom.tag | split | split ", | |
| "unconfigured tag | db.system | other.tag | my-service | my-service ", | |
| "empty configuration | '' | db.system | my-service | my-service ", | |
| "both names, OTel tag | db.system,db.type | db.system | split | split ", | |
| "both names, Datadog tag | db.system,db.type | db.type | split | split ", | |
| "custom tag survives expansion | db.system,custom.tag | custom.tag | split | split ", | |
| "first of two renamed tags | db.system,db.operation.name | db.type | split | split ", | |
| "second of two renamed tags | db.system,db.operation.name | db.operation | split | split " | |
| }) | |
| void splitByTagsMatchesCanonicalAndOriginalNames( | |
| String configured, String tag, String expectedBuilder, String expectedSetter) { | |
| injectSysConfig(SPLIT_BY_TAGS, configured); | |
| CoreTracer tracer = | |
| tracerBuilder() | |
| .serviceName("my-service") | |
| .writer(new LoggingWriter()) | |
| .sampler(new AllSampler()) | |
| .build(); | |
| AgentSpan builderSpan = tracer.buildSpan("datadog", "some span").withTag(tag, "split").start(); | |
| AgentSpan setterSpan = tracer.buildSpan("datadog", "some span").start(); | |
| setterSpan.setTag(tag, "split"); | |
| assertEquals(expectedBuilder, builderSpan.getServiceName()); | |
| assertEquals(expectedSetter, setterSpan.getServiceName()); | |
| CoreTracer defaultTagTracer = | |
| tracerBuilder() | |
| .serviceName("my-service") | |
| .writer(new LoggingWriter()) | |
| .sampler(new AllSampler()) | |
| .defaultSpanTags(singletonMap(tag, "split")) | |
| .build(); | |
| AgentSpan defaultTagSpan = defaultTagTracer.buildSpan("datadog", "some span").start(); | |
| assertEquals(expectedBuilder, defaultTagSpan.getServiceName()); | |
| } |
There was a problem hiding this comment.
Thanks, applied in 73a0d4b. I kept the existing createSplittingTracer helper (now taking a set of tags plus default span tags) instead of injectSysConfig. All rows pass as written, including the setter keeping my-service for an OTel-name tag against Datadog-name config.
Written by Claude, reviewed by @dougqh
Cover reverse alias pairing, custom and unconfigured tags, empty configuration, both names configured, multiple renamed tags, and default span tags, with an explicit expected result for the setter. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
This branch has not been deployed
2 participants
What Does This Do
Fixes
DD_TRACE_SPLIT_BY_TAGSfor a tag that is configured by its OpenTelemetry name, for exampledb.system. When such a tag was set on the span builder, the service name was no longer split.Motivation
Since #12354,
TagMap.Entryconstruction canonicalizes OpenTelemetry names to Datadog names. Adb.systemtag set on the builder (or through default tags) is stored asdb.type.TagInterceptorcompared the canonical name with the configureddb.system, so the two didn't match.Span setters pass the raw name, so they kept working. The case mostly affects users of the OpenTelemetry API, because the bridge sets attributes on the builder.
Additional Notes
This is a short-term fix. At construction, the interceptor adds the Datadog name of each configured tag. The longer-term plan is to move
TagInterceptorto id-based dispatch, which removes this whole category of name mismatch. That change will come in a separate PR.The new test fails without the fix (the row configured by OTel name and set on the builder).
🤖 Generated with Claude Code