Do not remove the version tag if manually set (#10703)

do not remove the version tag on the BaseServiceAdder

Make version working nicely with config consistency and manual set spans

Fix tests and corner cases

Co-authored-by: devflow.devflow-routing-intake <devflow.devflow-routing-intake@kubernetes.us1.ddbuild.io>

Author: amarziali

dd-trace-core/src/main/java/datadog/trace/core/CoreTracer.java

@@ -68,6 +68,7 @@
 import datadog.trace.bootstrap.instrumentation.api.SpanAttributes;
 import datadog.trace.bootstrap.instrumentation.api.SpanLink;
 import datadog.trace.bootstrap.instrumentation.api.TagContext;
+import datadog.trace.bootstrap.instrumentation.api.Tags;
 import datadog.trace.civisibility.interceptor.CiVisibilityApmProtocolInterceptor;
 import datadog.trace.civisibility.interceptor.CiVisibilityTelemetryInterceptor;
 import datadog.trace.civisibility.interceptor.CiVisibilityTraceInterceptor;
@@ -2074,6 +2075,9 @@ protected static final DDSpanContext buildSpanContext(
       context.setAllTags(coreTags, coreTagsNeedsIntercept);
       context.setAllTags(rootSpanTags, rootSpanTagsNeedsIntercept);
       context.setAllTags(contextualTags);
+      // remove version here since will be done later on the postProcessor.
+      // it will allow knowing if it will be set manually or not
+      context.removeTag(Tags.VERSION);
       return context;
     }
   }
@@ -2238,9 +2242,6 @@ static TagMap withTracerTags(
       if (!config.getEnv().isEmpty()) {
         result.put("env", config.getEnv());
       }
-      if (!config.getVersion().isEmpty()) {
-        result.put("version", config.getVersion());
-      }
       if (config.isDataJobsEnabled()) {
         result.put(DJM_ENABLED, 1);
       }

dd-trace-core/src/main/java/datadog/trace/core/tagprocessor/BaseServiceAdder.java

@@ -0,0 +1,40 @@
+package datadog.trace.core.tagprocessor;
+
+import static datadog.trace.bootstrap.instrumentation.api.Tags.VERSION;
+
+import datadog.trace.api.DDTags;
+import datadog.trace.api.TagMap;
+import datadog.trace.bootstrap.instrumentation.api.AgentSpanLink;
+import datadog.trace.bootstrap.instrumentation.api.UTF8BytesString;
+import datadog.trace.core.DDSpanContext;
+import java.util.List;
+import javax.annotation.Nullable;
+
+public final class InternalTagsAdder extends TagsPostProcessor {
+  private final UTF8BytesString ddService;
+  private final UTF8BytesString version;
+
+  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;
+  }
+
+  @Override
+  public void processTags(
+      TagMap unsafeTags, DDSpanContext spanContext, List<AgentSpanLink> spanLinks) {
+    if (spanContext == null || ddService == null) {
+      return;
+    }
+
+    if (!ddService.toString().equalsIgnoreCase(spanContext.getServiceName())) {
+      // service name !=  DD_SERVICE
+      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);
+      }
+    }
+  }
+}

dd-trace-core/src/main/java/datadog/trace/core/tagprocessor/InternalTagsAdder.java

@@ -5,7 +5,7 @@
 import java.util.List;
 
 public final class TagsPostProcessorFactory {
-  private static boolean addBaseService = true;
+  private static boolean addInternalTags = true;
   private static boolean addRemoteHostname = true;
 
   private static class Lazy {
@@ -15,8 +15,9 @@ private static class Lazy {
     private static TagsPostProcessor createEagerChain() {
       final List<TagsPostProcessor> processors = new ArrayList<>(3);
       processors.add(new PeerServiceCalculator());
-      if (addBaseService) {
-        processors.add(new BaseServiceAdder(Config.get().getServiceName()));
+      if (addInternalTags) {
+        processors.add(
+            new InternalTagsAdder(Config.get().getServiceName(), Config.get().getVersion()));
       }
       // Add HTTP endpoint post processor for resource renaming
       // This must run BEFORE metrics aggregation so the correct resource name is used in metrics
@@ -64,10 +65,10 @@ public static TagsPostProcessor lazyProcessor() {
   /**
    * Mostly used for test purposes.
    *
-   * @param enabled if false, {@link BaseServiceAdder} is not put in the chain.
+   * @param enabled if false, {@link InternalTagsAdder} is not put in the chain.
    */
-  public static void withAddBaseService(boolean enabled) {
-    addBaseService = enabled;
+  public static void withAddInternalTags(boolean enabled) {
+    addInternalTags = enabled;
     Lazy.eagerProcessor = Lazy.createEagerChain();
   }
 
@@ -83,7 +84,7 @@ public static void withAddRemoteHostname(boolean enabled) {
 
   /** Used for testing purposes. It reset the singleton and restore default options */
   public static void reset() {
-    withAddBaseService(true);
+    withAddInternalTags(true);
     withAddRemoteHostname(true);
   }
 }

dd-trace-core/src/main/java/datadog/trace/core/tagprocessor/TagsPostProcessorFactory.java

@@ -476,7 +476,7 @@ class CoreTracerTest extends DDCoreSpecification {
     setup:
     injectSysConfig(SERVICE_NAME, "dd_service_name")
     injectSysConfig(VERSION, "1.0.0")
-    TagsPostProcessorFactory.withAddBaseService(true)
+    TagsPostProcessorFactory.withAddInternalTags(true)
     def tracer = tracerBuilder().writer(new ListWriter()).build()
 
     when:
@@ -491,7 +491,7 @@ class CoreTracerTest extends DDCoreSpecification {
     span2.finish()
     then:
     span2.getServiceName() == "dd_service_name"
-    span2.getTags()["version"] == "1.0.0"
+    span2.getTags()["version"]?.toString() == "1.0.0"
 
     cleanup:
     tracer?.close()

dd-trace-core/src/test/groovy/datadog/trace/core/CoreTracerTest.groovy

@@ -0,0 +1,55 @@
+package datadog.trace.core.tagprocessor
+
+import static datadog.trace.bootstrap.instrumentation.api.Tags.VERSION
+
+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)
+
+    when:
+    def enrichedTags = calculator.processTags([:], spanContext, [])
+
+    then:
+    1 * spanContext.getServiceName() >> serviceName
+
+    and:
+    assert enrichedTags == 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)
+
+    when:
+    def enrichedTags = calculator.processTags(tags, spanContext, [])
+
+    then:
+    1 * spanContext.getServiceName() >> serviceName
+
+    and:
+    assert enrichedTags?.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"
+  }
+}

dd-trace-core/src/test/groovy/datadog/trace/core/tagprocessor/BaseServiceAdderTest.groovy

@@ -45,7 +45,7 @@ abstract class DDCoreSpecification extends DDSpecification {
 
   @Override
   void setupSpec() {
-    TagsPostProcessorFactory.withAddBaseService(false)
+    TagsPostProcessorFactory.withAddInternalTags(false)
     TagsPostProcessorFactory.withAddRemoteHostname(false)
   }
 

dd-trace-core/src/test/groovy/datadog/trace/core/tagprocessor/InternalTagsAdderTest.groovy

@@ -164,6 +164,7 @@ public class Tags {
   public static final String DD_ENV = "dd.env";
 
   public static final String ENV = "env";
+  public static final String VERSION = "version";
 
   /** ASM force tracer to keep the trace */
   public static final String ASM_KEEP = "asm.keep";