Add new parser for DD_TAGS and prioritizing DD_SERVICE

internal-api/src/main/java/datadog/trace/api/Config.java

@@ -775,7 +775,14 @@ private Config(final ConfigProvider configProvider, final InstrumenterConfig ins
 
     {
       final Map<String, String> tags = new HashMap<>(configProvider.getMergedMap(GLOBAL_TAGS));
-      tags.putAll(configProvider.getMergedMap(TRACE_TAGS, TAGS));
+      if (experimentalFeaturesEnabled.contains("DD_TAGS")) {
+        tags.putAll(configProvider.getMergedTagsMap(TRACE_TAGS, TAGS));
+      } else {
+        tags.putAll(configProvider.getMergedMap(TRACE_TAGS, TAGS));
+      }
+      if (serviceNameSetByUser) { // prioritize service name set by DD_SERVICE over DD_TAGS config
+        tags.remove("service");
+      }
       this.tags = getMapWithPropertiesDefinedByEnvironment(tags, ENV, VERSION);
     }
 

internal-api/src/main/java/datadog/trace/bootstrap/config/provider/ConfigConverter.java

@@ -84,6 +84,19 @@ static Map<String, String> parseMap(
     return map;
   }
 
+  @Nonnull
+  static Map<String, String> parseTraceTagsMap(
+      final String str, final char keyValueSeparator, final List<Character> argSeparators) {
+    // If we ever want to have default values besides an empty map, this will need to change.
+    String trimmed = Strings.trim(str);
+    if (trimmed.isEmpty()) {
+      return Collections.emptyMap();
+    }
+    Map<String, String> map = new HashMap<>();
+    loadTraceTagsMap(map, trimmed, keyValueSeparator, argSeparators);
+    return map;
+  }
+
   /**
    * This parses a mixed map that can have both key value pairs, and also keys only, that will get
    * values on the form "defaultPrefix.key". For keys without a value, the corresponding value will
@@ -201,6 +214,63 @@ private static void loadMap(
     }
   }
 
+  private static void loadTraceTagsMap(
+      Map<String, String> map,
+      String str,
+      char keyValueSeparator,
+      final List<Character> argSeparators) {
+    int start = 0;
+    int splitter = str.indexOf(keyValueSeparator, start);
+    char argSeparator = '\0';
+    int argSeparatorInd = -1;
+
+    // Given a list of separators ordered by priority, find the first (highest priority) separator
+    // that appears in the string and store its value and first occurrence in the string
+    for (Character sep : argSeparators) {
+      argSeparatorInd = str.indexOf(sep);
+      if (argSeparatorInd != -1) {
+        argSeparator = sep;
+        break;
+      }
+    }
+    while (start < str.length()) {
+      int nextSplitter =
+          argSeparatorInd == -1
+              ? -1
+              : str.indexOf(
+                  keyValueSeparator,
+                  argSeparatorInd + 1); // next splitter after the next argSeparator
+      int nextArgSeparator =
+          argSeparatorInd == -1 ? -1 : str.indexOf(argSeparator, argSeparatorInd + 1);
+      int end = argSeparatorInd == -1 ? str.length() : argSeparatorInd;
+
+      if (start >= end) { // the character is only the delimiter
+        start = end + 1;
+        splitter = nextSplitter;
+        argSeparatorInd = nextArgSeparator;
+        continue;
+      }
+
+      String key, value;
+      if (splitter >= end
+          || splitter
+              == -1) { // only key, no value; either due end of string or substring not having
+        // splitter
+        key = str.substring(start, end).trim();
+        value = "";
+      } else {
+        key = str.substring(start, splitter).trim();
+        value = str.substring(splitter + 1, end).trim();
+      }
+      if (!key.isEmpty()) {
+        map.put(key, value);
+      }
+      splitter = nextSplitter;
+      argSeparatorInd = nextArgSeparator;
+      start = end + 1;
+    }
+  }
+
   private static void loadMapWithOptionalMapping(
       Map<String, String> map,
       String str,

internal-api/src/main/java/datadog/trace/bootstrap/config/provider/ConfigProvider.java

@@ -9,6 +9,7 @@
 import java.io.FileNotFoundException;
 import java.io.FileReader;
 import java.io.IOException;
+import java.util.Arrays;
 import java.util.BitSet;
 import java.util.HashMap;
 import java.util.HashSet;
@@ -263,6 +264,28 @@ public Map<String, String> getMergedMap(String key, String... aliases) {
     return merged;
   }
 
+  public Map<String, String> getMergedTagsMap(String key, String... aliases) {
+    Map<String, String> merged = new HashMap<>();
+    ConfigOrigin origin = ConfigOrigin.DEFAULT;
+    // System properties take precedence over env
+    // prior art:
+    // https://docs.spring.io/spring-boot/docs/1.5.6.RELEASE/reference/html/boot-features-external-config.html
+    // We reverse iterate to allow overrides
+    for (int i = sources.length - 1; 0 <= i; i--) {
+      String value = sources[i].get(key, aliases);
+      Map<String, String> parsedMap =
+          ConfigConverter.parseTraceTagsMap(value, ':', Arrays.asList(',', ' '));
+      if (!parsedMap.isEmpty()) {
+        origin = sources[i].origin();
+      }
+      merged.putAll(parsedMap);
+    }
+    if (collectConfig) {
+      ConfigCollector.get().put(key, merged, origin);
+    }
+    return merged;
+  }
+
   public Map<String, String> getOrderedMap(String key) {
     LinkedHashMap<String, String> merged = new LinkedHashMap<>();
     ConfigOrigin origin = ConfigOrigin.DEFAULT;

internal-api/src/test/groovy/datadog/trace/api/ConfigTest.groovy

@@ -1795,6 +1795,19 @@ class ConfigTest extends DDSpecification {
       'service.version': 'my-svc-vers']
   }
 
+  def "service name prioritizes values from DD_SERVICE over tags"() {
+    setup:
+    System.setProperty(PREFIX + TAGS, "service:service-name-from-tags")
+    System.setProperty(PREFIX + SERVICE, "service-name-from-dd-service")
+
+    when:
+    def config = new Config()
+
+    then:
+    config.serviceName == "service-name-from-dd-service"
+    !config.mergedSpanTags.containsKey("service")
+  }
+
   def "DD_SERVICE precedence over 'dd.service.name' java property is set; 'dd.service' overwrites DD_SERVICE"() {
     setup:
     environmentVariables.set(DD_SERVICE_NAME_ENV, "dd-service-name-env-var")
@@ -1808,7 +1821,7 @@ class ConfigTest extends DDSpecification {
 
     then:
     config.serviceName == "dd-service-java-prop"
-    config.mergedSpanTags == [service: 'service-tag-in-dd-trace-global-tags-java-property', 'service.version': 'my-svc-vers']
+    config.mergedSpanTags == ['service.version': 'my-svc-vers']
     config.mergedJmxTags == [(RUNTIME_ID_TAG) : config.getRuntimeId(), (SERVICE_TAG): config.serviceName,
       'service.version': 'my-svc-vers']
   }
@@ -1824,7 +1837,7 @@ class ConfigTest extends DDSpecification {
 
     then:
     config.serviceName == "dd-service-env-var"
-    config.mergedSpanTags == [service: 'service-tag-in-dd-trace-global-tags-java-property', 'service.version': 'my-svc-vers']
+    config.mergedSpanTags == ['service.version': 'my-svc-vers']
     config.mergedJmxTags == [(RUNTIME_ID_TAG) : config.getRuntimeId(), (SERVICE_TAG): config.serviceName,
       'service.version': 'my-svc-vers']
   }
@@ -1840,12 +1853,12 @@ class ConfigTest extends DDSpecification {
 
     then:
     config.serviceName == "dd-service-java-prop"
-    config.mergedSpanTags == [service: 'service-tag-in-dd-trace-global-tags-java-property', 'service.version': 'my-svc-vers']
+    config.mergedSpanTags == ['service.version': 'my-svc-vers']
     config.mergedJmxTags == [(RUNTIME_ID_TAG) : config.getRuntimeId(), (SERVICE_TAG): config.serviceName,
       'service.version': 'my-svc-vers']
   }
 
-  def "set servicenaem by DD_SERVICE"() {
+  def "set servicename by DD_SERVICE"() {
     setup:
     environmentVariables.set("DD_SERVICE", "dd-service-env-var")
     System.setProperty(PREFIX + GLOBAL_TAGS, "service:service-tag-in-dd-trace-global-tags-java-property,service.version:my-svc-vers")
@@ -1856,7 +1869,7 @@ class ConfigTest extends DDSpecification {
 
     then:
     config.serviceName == "dd-service-env-var"
-    config.mergedSpanTags == [service: 'service-tag-in-dd-trace-global-tags-java-property', 'service.version': 'my-svc-vers']
+    config.mergedSpanTags == ['service.version': 'my-svc-vers']
     config.mergedJmxTags == [(RUNTIME_ID_TAG) : config.getRuntimeId(), (SERVICE_TAG): config.serviceName,
       'service.version': 'my-svc-vers']
   }
@@ -1876,6 +1889,35 @@ class ConfigTest extends DDSpecification {
     [serviceProperty, serviceName] << [[SERVICE, SERVICE_NAME], [DEFAULT_SERVICE_NAME, "my-service"]].combinations()
   }
 
+  def "verify behavior of features under DD_TRACE_EXPERIMENTAL_FEATURES_ENABLED"() {
+    setup:
+    environmentVariables.set("DD_TRACE_EXPERIMENTAL_FEATURES_ENABLED", "DD_TAGS")
+    environmentVariables.set("DD_TAGS", "env:test,aKey:aVal bKey:bVal cKey:")
+
+    when:
+    def config = new Config()
+
+    then:
+    config.experimentalFeaturesEnabled == ["DD_TAGS"].toSet()
+
+    //verify expected behavior enabled under feature flag
+    config.globalTags == [env: "test", aKey: "aVal bKey:bVal cKey:"]
+  }
+
+  def "verify behavior of 'breaking change' configs when not under DD_TRACE_EXPERIMENTAL_FEATURES_ENABLED"() {
+    setup:
+    environmentVariables.set("DD_TAGS", "env:test,aKey:aVal bKey:bVal cKey:")
+
+    when:
+    def config = new Config()
+
+    then:
+    config.experimentalFeaturesEnabled == [].toSet()
+
+    //verify expected behavior when not enabled under feature flag
+    config.globalTags == [env:"test", aKey:"aVal", bKey:"bVal"]
+  }
+
   def "detect if agent is configured using default values"() {
     setup:
     if (host != null) {

internal-api/src/test/groovy/datadog/trace/bootstrap/config/provider/ConfigConverterTest.groovy

@@ -99,6 +99,40 @@ class ConfigConverterTest extends DDSpecification {
     // spotless:on
   }
 
+  def "parsing map #mapString with List of arg separators for with key value separator #separator"() {
+    //testing parsing for DD_TAGS
+    setup:
+    def separatorList = [',' as char, ' ' as char]
+
+    when:
+    def result = ConfigConverter.parseTraceTagsMap(mapString, separator as char, separatorList as List<Character>)
+
+    then:
+    result == expected
+
+    where:
+    // spotless:off
+    mapString                                       | separator | expected
+    "key1:value1,key2:value2"                       | ':'       | [key1: "value1", key2: "value2"]
+    "key1:value1 key2:value2"                       | ':'       | [key1: "value1", key2: "value2"]
+    "env:test aKey:aVal bKey:bVal cKey:"            | ':'       | [env: "test", aKey: "aVal", bKey: "bVal", cKey:""]
+    "env:test,aKey:aVal,bKey:bVal,cKey:"            | ':'       | [env: "test", aKey: "aVal", bKey: "bVal", cKey:""]
+    "env:test,aKey:aVal bKey:bVal cKey:"            | ':'       | [env: "test", aKey: "aVal bKey:bVal cKey:"]
+    "env:test     bKey :bVal dKey: dVal cKey:"      | ':'       | [env: "test", bKey: "", dKey: "", dVal: "", cKey: ""]
+    'env :test, aKey : aVal bKey:bVal cKey:'        | ':'       | [env: "test", aKey : "aVal bKey:bVal cKey:"]
+    "env:keyWithA:Semicolon bKey:bVal cKey"         | ':'       | [env: "keyWithA:Semicolon", bKey: "bVal", cKey: ""]
+    "env:keyWith:  , ,   Lots:Of:Semicolons "       | ':'       | [env: "keyWith:", Lots: "Of:Semicolons"]
+    "a:b,c,d"                                       | ':'       | [a: "b", c: "", d: ""]
+    "a,1"                                           | ':'       | [a: "", "1": ""]
+    "a:b:c:d"                                       | ':'       | [a: "b:c:d"]
+    //edge cases
+    "noDelimiters"                                  | ':'       | [noDelimiters: ""]
+    "            "                                  | ':'       | [:]
+    ",,,,,,,,,,,,"                                  | ':'       | [:]
+    ", , , , , , "                                  | ':'       | [:]
+    // spotless:on
+  }
+
   def "test parseMapWithOptionalMappings"() {
     when:
     def result = ConfigConverter.parseMapWithOptionalMappings(mapString, "test", "", lowercaseKeys)