From 934fd5dc3bd974bf14f6725ca4033f04b90c2f9f Mon Sep 17 00:00:00 2001 From: Douglas Q Hawkins Date: Wed, 3 Jun 2026 19:40:16 -0400 Subject: [PATCH 1/2] Pre-construct TagMap.Entry objects in InternalTagsAdder InternalTagsAdder set base.service / version via TagMap.set(tag, value), allocating a fresh TagMap.Entry per span. Both values are fixed for the life of the tracer, and TagMap.Entry objects are safe to share across maps (the OptimizedTagMap collision design relies on it), so build the two Entry objects once in the constructor and reuse them via set(entry). A JFR profile of petclinic (2026-06-03) attributed ~52 allocation samples to InternalTagsAdder.processTags (one Entry per span); this drops them to zero. Re-applies the change from the stale PR #10965 (415 commits behind master, drifted signature) onto current master. Co-Authored-By: Claude Opus 4.8 (1M context) --- .../core/tagprocessor/InternalTagsAdder.java | 27 ++++++++++++------- 1 file changed, 18 insertions(+), 9 deletions(-) diff --git a/dd-trace-core/src/main/java/datadog/trace/core/tagprocessor/InternalTagsAdder.java b/dd-trace-core/src/main/java/datadog/trace/core/tagprocessor/InternalTagsAdder.java index 68b13d19faf..fa4429f480a 100644 --- a/dd-trace-core/src/main/java/datadog/trace/core/tagprocessor/InternalTagsAdder.java +++ b/dd-trace-core/src/main/java/datadog/trace/core/tagprocessor/InternalTagsAdder.java @@ -10,29 +10,38 @@ import javax.annotation.Nullable; public final class InternalTagsAdder extends TagsPostProcessor { - private final UTF8BytesString ddService; - private final UTF8BytesString version; + // Pre-built once at construction and reused on every span. The base.service / version tags are + // fixed for the life of the tracer, so reusing the same immutable TagMap.Entry (Entries are + // safe to share across maps) avoids allocating a fresh Entry per span in processTags. + private final TagMap.Entry baseServiceEntry; + private final TagMap.Entry versionEntry; public InternalTagsAdder(@Nullable final String ddService, @Nullable final String version) { - this.ddService = ddService != null ? UTF8BytesString.create(ddService) : null; - this.version = version != null && !version.isEmpty() ? UTF8BytesString.create(version) : null; + this.baseServiceEntry = + ddService != null + ? TagMap.Entry.create(DDTags.BASE_SERVICE, UTF8BytesString.create(ddService)) + : null; + this.versionEntry = + version != null && !version.isEmpty() + ? TagMap.Entry.create(VERSION, UTF8BytesString.create(version)) + : null; } @Override public void processTags( TagMap unsafeTags, DDSpanContext spanContext, AppendableSpanLinks spanLinks) { - if (spanContext == null || ddService == null) { + if (spanContext == null || baseServiceEntry == null) { return; } - if (!ddService.toString().equalsIgnoreCase(spanContext.getServiceName())) { + if (!baseServiceEntry.stringValue().equalsIgnoreCase(spanContext.getServiceName())) { // service name != DD_SERVICE - unsafeTags.set(DDTags.BASE_SERVICE, ddService); + unsafeTags.set(baseServiceEntry); } else { // as per config consistency, the version tag is added across tracers only if // the service name is DD_SERVICE and version tag is not manually set - if (version != null && !unsafeTags.containsKey(VERSION)) { - unsafeTags.set(VERSION, version); + if (versionEntry != null && !unsafeTags.containsKey(VERSION)) { + unsafeTags.set(versionEntry); } } } From f5f5cc90ef4fdb743a3cb66f954eb12f53d99756 Mon Sep 17 00:00:00 2001 From: Douglas Q Hawkins Date: Wed, 3 Jun 2026 21:19:41 -0400 Subject: [PATCH 2/2] Fix empty-DD_SERVICE version drop; migrate test to JUnit 5 Addresses the Codex review comment on #11555: pre-building the base.service Entry must not change behavior for an explicitly-empty DD_SERVICE. Entry.create rejects empty values, so baseServiceEntry is null in that case; the processTags branch now falls back to set(BASE_SERVICE, ddService) to preserve byte-identical behavior, and the version branch is still reached when the span service also matches the empty configured service. Migrate InternalTagsAdderTest from Groovy/Spock to JUnit 5 (parameterized with @MethodSource) and add regression coverage for the empty-DD_SERVICE case (9 migrated cases + 2 new = 11). Co-Authored-By: Claude Opus 4.8 (1M context) --- .../core/tagprocessor/InternalTagsAdder.java | 35 +++-- .../tagprocessor/InternalTagsAdderTest.groovy | 61 -------- .../tagprocessor/InternalTagsAdderTest.java | 140 ++++++++++++++++++ 3 files changed, 164 insertions(+), 72 deletions(-) delete mode 100644 dd-trace-core/src/test/groovy/datadog/trace/core/tagprocessor/InternalTagsAdderTest.groovy create mode 100644 dd-trace-core/src/test/java/datadog/trace/core/tagprocessor/InternalTagsAdderTest.java diff --git a/dd-trace-core/src/main/java/datadog/trace/core/tagprocessor/InternalTagsAdder.java b/dd-trace-core/src/main/java/datadog/trace/core/tagprocessor/InternalTagsAdder.java index fa4429f480a..0136e14f18d 100644 --- a/dd-trace-core/src/main/java/datadog/trace/core/tagprocessor/InternalTagsAdder.java +++ b/dd-trace-core/src/main/java/datadog/trace/core/tagprocessor/InternalTagsAdder.java @@ -10,16 +10,24 @@ import javax.annotation.Nullable; public final class InternalTagsAdder extends TagsPostProcessor { - // Pre-built once at construction and reused on every span. The base.service / version tags are - // fixed for the life of the tracer, so reusing the same immutable TagMap.Entry (Entries are - // safe to share across maps) avoids allocating a fresh Entry per span in processTags. - private final TagMap.Entry baseServiceEntry; - private final TagMap.Entry versionEntry; + // ddService drives the guard and the service-name comparison; kept even when empty so behavior + // matches the prior implementation exactly (an explicitly-empty DD_SERVICE is a valid, if + // degenerate, config -- see getStringExcludingSource, which passes "" through, not the default). + private final UTF8BytesString ddService; + + // base.service / version values are fixed for the life of the tracer, and TagMap.Entry objects + // are safe to share across maps (the OptimizedTagMap collision design relies on it), so the + // entries are built once and reused on the hot path instead of allocating one per span. + // baseServiceEntry is null when ddService is null OR empty (Entry.create rejects empty values); + // the empty case falls back to set(tag, value) below to preserve byte-identical behavior. + @Nullable private final TagMap.Entry baseServiceEntry; + @Nullable private final TagMap.Entry versionEntry; public InternalTagsAdder(@Nullable final String ddService, @Nullable final String version) { + this.ddService = ddService != null ? UTF8BytesString.create(ddService) : null; this.baseServiceEntry = - ddService != null - ? TagMap.Entry.create(DDTags.BASE_SERVICE, UTF8BytesString.create(ddService)) + this.ddService != null && this.ddService.length() > 0 + ? TagMap.Entry.create(DDTags.BASE_SERVICE, this.ddService) : null; this.versionEntry = version != null && !version.isEmpty() @@ -30,13 +38,18 @@ public InternalTagsAdder(@Nullable final String ddService, @Nullable final Strin @Override public void processTags( TagMap unsafeTags, DDSpanContext spanContext, AppendableSpanLinks spanLinks) { - if (spanContext == null || baseServiceEntry == null) { + if (spanContext == null || ddService == null) { return; } - if (!baseServiceEntry.stringValue().equalsIgnoreCase(spanContext.getServiceName())) { - // service name != DD_SERVICE - unsafeTags.set(baseServiceEntry); + if (!ddService.toString().equalsIgnoreCase(spanContext.getServiceName())) { + // service name != DD_SERVICE + if (baseServiceEntry != null) { + unsafeTags.set(baseServiceEntry); + } else { + // Empty DD_SERVICE: no prebuilt entry exists; preserve the original behavior. + unsafeTags.set(DDTags.BASE_SERVICE, ddService); + } } else { // as per config consistency, the version tag is added across tracers only if // the service name is DD_SERVICE and version tag is not manually set diff --git a/dd-trace-core/src/test/groovy/datadog/trace/core/tagprocessor/InternalTagsAdderTest.groovy b/dd-trace-core/src/test/groovy/datadog/trace/core/tagprocessor/InternalTagsAdderTest.groovy deleted file mode 100644 index 5429a9e8b24..00000000000 --- a/dd-trace-core/src/test/groovy/datadog/trace/core/tagprocessor/InternalTagsAdderTest.groovy +++ /dev/null @@ -1,61 +0,0 @@ -package datadog.trace.core.tagprocessor - -import static datadog.trace.bootstrap.instrumentation.api.Tags.VERSION - -import datadog.trace.api.TagMap -import datadog.trace.bootstrap.instrumentation.api.AppendableSpanLinks -import datadog.trace.bootstrap.instrumentation.api.UTF8BytesString -import datadog.trace.core.DDSpanContext -import datadog.trace.test.util.DDSpecification - -class InternalTagsAdderTest extends DDSpecification { - def "should add _dd.base_service when service differs to ddService"() { - setup: - def calculator = new InternalTagsAdder("test", null) - def spanContext = Mock(DDSpanContext) - def links = Mock(AppendableSpanLinks) - - when: - def unsafeTags = TagMap.fromMap([:]) - calculator.processTags(unsafeTags, spanContext, links) - - then: - 1 * spanContext.getServiceName() >> serviceName - - and: - assert unsafeTags == expectedTags - - where: - serviceName | expectedTags - "anotherOne" | ["_dd.base_service": UTF8BytesString.create("test")] - "test" | [:] - "TeSt" | [:] - } - - def "should add version when DD_SERVICE = #serviceName and version = #ddVersion"() { - setup: - def calculator = new InternalTagsAdder("same", ddVersion) - def spanContext = Mock(DDSpanContext) - def links = Mock(AppendableSpanLinks) - - when: - def unsafeTags = TagMap.fromMap(tags) - calculator.processTags(unsafeTags, spanContext, links) - - then: - 1 * spanContext.getServiceName() >> serviceName - - and: - assert unsafeTags?.get(VERSION)?.toString() == expected - - - where: - serviceName | ddVersion | tags | expected - "same" | null | [:] | null - "different" | "1.0" | [:] | null - "different" | "1.0" | ["version": "2.0"] | "2.0" - "same" | null | ["version": "2.0"] | "2.0" - "same" | "1.0" | ["version": "2.0"] | "2.0" - "same" | "1.0" | [:] | "1.0" - } -} diff --git a/dd-trace-core/src/test/java/datadog/trace/core/tagprocessor/InternalTagsAdderTest.java b/dd-trace-core/src/test/java/datadog/trace/core/tagprocessor/InternalTagsAdderTest.java new file mode 100644 index 00000000000..4644f2322ba --- /dev/null +++ b/dd-trace-core/src/test/java/datadog/trace/core/tagprocessor/InternalTagsAdderTest.java @@ -0,0 +1,140 @@ +package datadog.trace.core.tagprocessor; + +import static datadog.trace.bootstrap.instrumentation.api.Tags.VERSION; +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.params.provider.Arguments.arguments; +import static org.mockito.Mockito.lenient; +import static org.mockito.Mockito.when; + +import datadog.trace.api.DDTags; +import datadog.trace.api.TagMap; +import datadog.trace.bootstrap.instrumentation.api.AppendableSpanLinks; +import datadog.trace.core.DDSpanContext; +import java.util.Collections; +import java.util.HashMap; +import java.util.Map; +import java.util.stream.Stream; +import org.junit.jupiter.api.extension.ExtendWith; +import org.junit.jupiter.params.ParameterizedTest; +import org.junit.jupiter.params.provider.Arguments; +import org.junit.jupiter.params.provider.MethodSource; +import org.mockito.Mock; +import org.mockito.junit.jupiter.MockitoExtension; + +@ExtendWith(MockitoExtension.class) +class InternalTagsAdderTest { + + @Mock DDSpanContext spanContext; + @Mock AppendableSpanLinks links; + + static Stream baseServiceArguments() { + return Stream.of( + // (scenario, spanServiceName, expected base.service tag or null) + arguments("service differs -> base.service set", "anotherOne", "test"), + arguments("service matches exactly -> no base.service", "test", null), + arguments("service matches case-insensitively -> no base.service", "TeSt", null)); + } + + @ParameterizedTest(name = "{0}") + @MethodSource("baseServiceArguments") + void addsBaseServiceWhenServiceDiffersFromDdService( + String scenario, String spanServiceName, String expectedBaseService) { + InternalTagsAdder adder = new InternalTagsAdder("test", null); + when(spanContext.getServiceName()).thenReturn(spanServiceName); + + TagMap unsafeTags = TagMap.fromMap(Collections.emptyMap()); + adder.processTags(unsafeTags, spanContext, links); + + assertEquals(expectedBaseService, unsafeTags.getString(DDTags.BASE_SERVICE)); + } + + static Stream versionArguments() { + return Stream.of( + // (scenario, ddService, spanServiceName, ddVersion, existing tags, expected version tag) + arguments("matches, no version configured", "same", "same", null, noTags(), null), + arguments( + "differs, version not added on base.service branch", + "same", + "different", + "1.0", + noTags(), + null), + arguments( + "differs, span keeps its own version", + "same", + "different", + "1.0", + versionTag("2.0"), + "2.0"), + arguments( + "matches, existing version not overwritten", + "same", + "same", + null, + versionTag("2.0"), + "2.0"), + arguments( + "matches, configured version not overwriting existing", + "same", + "same", + "1.0", + versionTag("2.0"), + "2.0"), + arguments("matches, configured version added", "same", "same", "1.0", noTags(), "1.0")); + } + + @ParameterizedTest(name = "{0}") + @MethodSource("versionArguments") + void addsVersionWhenServiceMatchesDdService( + String scenario, + String ddService, + String spanServiceName, + String ddVersion, + Map existingTags, + String expectedVersion) { + InternalTagsAdder adder = new InternalTagsAdder(ddService, ddVersion); + when(spanContext.getServiceName()).thenReturn(spanServiceName); + + TagMap unsafeTags = TagMap.fromMap(existingTags); + adder.processTags(unsafeTags, spanContext, links); + + assertEquals(expectedVersion, unsafeTags.getString(VERSION)); + } + + /** + * Regression for the empty-DD_SERVICE edge case (see PR #11555 review): an explicitly-empty + * service name is a valid config (the config provider passes "" through, not the default). The + * version branch must still be reached when the span's service name also matches the empty + * configured service -- pre-building the base.service entry must not short-circuit it. + */ + @ParameterizedTest(name = "{0}") + @MethodSource("emptyServiceArguments") + void emptyDdServicePreservesVersionHandling( + String scenario, String spanServiceName, String expectedVersion) { + InternalTagsAdder adder = new InternalTagsAdder("", "1.0"); + lenient().when(spanContext.getServiceName()).thenReturn(spanServiceName); + + TagMap unsafeTags = TagMap.fromMap(Collections.emptyMap()); + adder.processTags(unsafeTags, spanContext, links); + + assertEquals(expectedVersion, unsafeTags.getString(VERSION)); + } + + static Stream emptyServiceArguments() { + return Stream.of( + // empty span service matches empty DD_SERVICE -> version branch reached + arguments("empty service matches -> version added", "", "1.0"), + // non-empty span service differs from empty DD_SERVICE -> base.service branch, no version + arguments("non-empty service differs -> no version", "nonempty", null)); + } + + private static Map noTags() { + return Collections.emptyMap(); + } + + private static Map versionTag(String version) { + Map tags = new HashMap<>(); + tags.put("version", version); + return tags; + } +}