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..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,12 +10,29 @@ import javax.annotation.Nullable; public final class InternalTagsAdder extends TagsPostProcessor { + // 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; - private final UTF8BytesString version; + + // 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.version = version != null && !version.isEmpty() ? UTF8BytesString.create(version) : null; + this.baseServiceEntry = + this.ddService != null && this.ddService.length() > 0 + ? TagMap.Entry.create(DDTags.BASE_SERVICE, this.ddService) + : null; + this.versionEntry = + version != null && !version.isEmpty() + ? TagMap.Entry.create(VERSION, UTF8BytesString.create(version)) + : null; } @Override @@ -26,13 +43,18 @@ public void processTags( } if (!ddService.toString().equalsIgnoreCase(spanContext.getServiceName())) { - // service name != DD_SERVICE - unsafeTags.set(DDTags.BASE_SERVICE, ddService); + // 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 - if (version != null && !unsafeTags.containsKey(VERSION)) { - unsafeTags.set(VERSION, version); + if (versionEntry != null && !unsafeTags.containsKey(VERSION)) { + unsafeTags.set(versionEntry); } } } 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; + } +}