Skip to content

Commit 46a5f54

Browse files
mhliddmhlidd
authored
Add new parser for DD_TAGS and prioritizing DD_SERVICE (#8296)
* testing * cleanup * first iteration on parser * initial working parsing for DD_TAGS * cleanup part 1 * cleanup pt 2 * adding traceTagsParser as separate parser due to different rule * dd_service override * reverting irrelevant changes * cleanup * reverting irrelevant changes * cleanup again * adding edge cases tests and creating specific tags mergedtags function * updating comment for clarity and removing irrelevant args * cleanup * updating DD_TAGS to belong under breaking change flag
1 parent 46e4075 commit 46a5f54

File tree

5 files changed

+182
-6
lines changed

5 files changed

+182
-6
lines changed

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

Lines changed: 8 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -775,7 +775,14 @@ private Config(final ConfigProvider configProvider, final InstrumenterConfig ins
775775

776776
{
777777
final Map<String, String> tags = new HashMap<>(configProvider.getMergedMap(GLOBAL_TAGS));
778-
tags.putAll(configProvider.getMergedMap(TRACE_TAGS, TAGS));
778+
if (experimentalFeaturesEnabled.contains("DD_TAGS")) {
779+
tags.putAll(configProvider.getMergedTagsMap(TRACE_TAGS, TAGS));
780+
} else {
781+
tags.putAll(configProvider.getMergedMap(TRACE_TAGS, TAGS));
782+
}
783+
if (serviceNameSetByUser) { // prioritize service name set by DD_SERVICE over DD_TAGS config
784+
tags.remove("service");
785+
}
779786
this.tags = getMapWithPropertiesDefinedByEnvironment(tags, ENV, VERSION);
780787
}
781788

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

Lines changed: 70 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -84,6 +84,19 @@ static Map<String, String> parseMap(
8484
return map;
8585
}
8686

87+
@Nonnull
88+
static Map<String, String> parseTraceTagsMap(
89+
final String str, final char keyValueSeparator, final List<Character> argSeparators) {
90+
// If we ever want to have default values besides an empty map, this will need to change.
91+
String trimmed = Strings.trim(str);
92+
if (trimmed.isEmpty()) {
93+
return Collections.emptyMap();
94+
}
95+
Map<String, String> map = new HashMap<>();
96+
loadTraceTagsMap(map, trimmed, keyValueSeparator, argSeparators);
97+
return map;
98+
}
99+
87100
/**
88101
* This parses a mixed map that can have both key value pairs, and also keys only, that will get
89102
* values on the form "defaultPrefix.key". For keys without a value, the corresponding value will
@@ -201,6 +214,63 @@ private static void loadMap(
201214
}
202215
}
203216

217+
private static void loadTraceTagsMap(
218+
Map<String, String> map,
219+
String str,
220+
char keyValueSeparator,
221+
final List<Character> argSeparators) {
222+
int start = 0;
223+
int splitter = str.indexOf(keyValueSeparator, start);
224+
char argSeparator = '\0';
225+
int argSeparatorInd = -1;
226+
227+
// Given a list of separators ordered by priority, find the first (highest priority) separator
228+
// that appears in the string and store its value and first occurrence in the string
229+
for (Character sep : argSeparators) {
230+
argSeparatorInd = str.indexOf(sep);
231+
if (argSeparatorInd != -1) {
232+
argSeparator = sep;
233+
break;
234+
}
235+
}
236+
while (start < str.length()) {
237+
int nextSplitter =
238+
argSeparatorInd == -1
239+
? -1
240+
: str.indexOf(
241+
keyValueSeparator,
242+
argSeparatorInd + 1); // next splitter after the next argSeparator
243+
int nextArgSeparator =
244+
argSeparatorInd == -1 ? -1 : str.indexOf(argSeparator, argSeparatorInd + 1);
245+
int end = argSeparatorInd == -1 ? str.length() : argSeparatorInd;
246+
247+
if (start >= end) { // the character is only the delimiter
248+
start = end + 1;
249+
splitter = nextSplitter;
250+
argSeparatorInd = nextArgSeparator;
251+
continue;
252+
}
253+
254+
String key, value;
255+
if (splitter >= end
256+
|| splitter
257+
== -1) { // only key, no value; either due end of string or substring not having
258+
// splitter
259+
key = str.substring(start, end).trim();
260+
value = "";
261+
} else {
262+
key = str.substring(start, splitter).trim();
263+
value = str.substring(splitter + 1, end).trim();
264+
}
265+
if (!key.isEmpty()) {
266+
map.put(key, value);
267+
}
268+
splitter = nextSplitter;
269+
argSeparatorInd = nextArgSeparator;
270+
start = end + 1;
271+
}
272+
}
273+
204274
private static void loadMapWithOptionalMapping(
205275
Map<String, String> map,
206276
String str,

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

Lines changed: 23 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -9,6 +9,7 @@
99
import java.io.FileNotFoundException;
1010
import java.io.FileReader;
1111
import java.io.IOException;
12+
import java.util.Arrays;
1213
import java.util.BitSet;
1314
import java.util.HashMap;
1415
import java.util.HashSet;
@@ -263,6 +264,28 @@ public Map<String, String> getMergedMap(String key, String... aliases) {
263264
return merged;
264265
}
265266

267+
public Map<String, String> getMergedTagsMap(String key, String... aliases) {
268+
Map<String, String> merged = new HashMap<>();
269+
ConfigOrigin origin = ConfigOrigin.DEFAULT;
270+
// System properties take precedence over env
271+
// prior art:
272+
// https://docs.spring.io/spring-boot/docs/1.5.6.RELEASE/reference/html/boot-features-external-config.html
273+
// We reverse iterate to allow overrides
274+
for (int i = sources.length - 1; 0 <= i; i--) {
275+
String value = sources[i].get(key, aliases);
276+
Map<String, String> parsedMap =
277+
ConfigConverter.parseTraceTagsMap(value, ':', Arrays.asList(',', ' '));
278+
if (!parsedMap.isEmpty()) {
279+
origin = sources[i].origin();
280+
}
281+
merged.putAll(parsedMap);
282+
}
283+
if (collectConfig) {
284+
ConfigCollector.get().put(key, merged, origin);
285+
}
286+
return merged;
287+
}
288+
266289
public Map<String, String> getOrderedMap(String key) {
267290
LinkedHashMap<String, String> merged = new LinkedHashMap<>();
268291
ConfigOrigin origin = ConfigOrigin.DEFAULT;

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

Lines changed: 47 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -1795,6 +1795,19 @@ class ConfigTest extends DDSpecification {
17951795
'service.version': 'my-svc-vers']
17961796
}
17971797

1798+
def "service name prioritizes values from DD_SERVICE over tags"() {
1799+
setup:
1800+
System.setProperty(PREFIX + TAGS, "service:service-name-from-tags")
1801+
System.setProperty(PREFIX + SERVICE, "service-name-from-dd-service")
1802+
1803+
when:
1804+
def config = new Config()
1805+
1806+
then:
1807+
config.serviceName == "service-name-from-dd-service"
1808+
!config.mergedSpanTags.containsKey("service")
1809+
}
1810+
17981811
def "DD_SERVICE precedence over 'dd.service.name' java property is set; 'dd.service' overwrites DD_SERVICE"() {
17991812
setup:
18001813
environmentVariables.set(DD_SERVICE_NAME_ENV, "dd-service-name-env-var")
@@ -1808,7 +1821,7 @@ class ConfigTest extends DDSpecification {
18081821

18091822
then:
18101823
config.serviceName == "dd-service-java-prop"
1811-
config.mergedSpanTags == [service: 'service-tag-in-dd-trace-global-tags-java-property', 'service.version': 'my-svc-vers']
1824+
config.mergedSpanTags == ['service.version': 'my-svc-vers']
18121825
config.mergedJmxTags == [(RUNTIME_ID_TAG) : config.getRuntimeId(), (SERVICE_TAG): config.serviceName,
18131826
'service.version': 'my-svc-vers']
18141827
}
@@ -1824,7 +1837,7 @@ class ConfigTest extends DDSpecification {
18241837

18251838
then:
18261839
config.serviceName == "dd-service-env-var"
1827-
config.mergedSpanTags == [service: 'service-tag-in-dd-trace-global-tags-java-property', 'service.version': 'my-svc-vers']
1840+
config.mergedSpanTags == ['service.version': 'my-svc-vers']
18281841
config.mergedJmxTags == [(RUNTIME_ID_TAG) : config.getRuntimeId(), (SERVICE_TAG): config.serviceName,
18291842
'service.version': 'my-svc-vers']
18301843
}
@@ -1840,12 +1853,12 @@ class ConfigTest extends DDSpecification {
18401853

18411854
then:
18421855
config.serviceName == "dd-service-java-prop"
1843-
config.mergedSpanTags == [service: 'service-tag-in-dd-trace-global-tags-java-property', 'service.version': 'my-svc-vers']
1856+
config.mergedSpanTags == ['service.version': 'my-svc-vers']
18441857
config.mergedJmxTags == [(RUNTIME_ID_TAG) : config.getRuntimeId(), (SERVICE_TAG): config.serviceName,
18451858
'service.version': 'my-svc-vers']
18461859
}
18471860

1848-
def "set servicenaem by DD_SERVICE"() {
1861+
def "set servicename by DD_SERVICE"() {
18491862
setup:
18501863
environmentVariables.set("DD_SERVICE", "dd-service-env-var")
18511864
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 {
18561869

18571870
then:
18581871
config.serviceName == "dd-service-env-var"
1859-
config.mergedSpanTags == [service: 'service-tag-in-dd-trace-global-tags-java-property', 'service.version': 'my-svc-vers']
1872+
config.mergedSpanTags == ['service.version': 'my-svc-vers']
18601873
config.mergedJmxTags == [(RUNTIME_ID_TAG) : config.getRuntimeId(), (SERVICE_TAG): config.serviceName,
18611874
'service.version': 'my-svc-vers']
18621875
}
@@ -1876,6 +1889,35 @@ class ConfigTest extends DDSpecification {
18761889
[serviceProperty, serviceName] << [[SERVICE, SERVICE_NAME], [DEFAULT_SERVICE_NAME, "my-service"]].combinations()
18771890
}
18781891

1892+
def "verify behavior of features under DD_TRACE_EXPERIMENTAL_FEATURES_ENABLED"() {
1893+
setup:
1894+
environmentVariables.set("DD_TRACE_EXPERIMENTAL_FEATURES_ENABLED", "DD_TAGS")
1895+
environmentVariables.set("DD_TAGS", "env:test,aKey:aVal bKey:bVal cKey:")
1896+
1897+
when:
1898+
def config = new Config()
1899+
1900+
then:
1901+
config.experimentalFeaturesEnabled == ["DD_TAGS"].toSet()
1902+
1903+
//verify expected behavior enabled under feature flag
1904+
config.globalTags == [env: "test", aKey: "aVal bKey:bVal cKey:"]
1905+
}
1906+
1907+
def "verify behavior of 'breaking change' configs when not under DD_TRACE_EXPERIMENTAL_FEATURES_ENABLED"() {
1908+
setup:
1909+
environmentVariables.set("DD_TAGS", "env:test,aKey:aVal bKey:bVal cKey:")
1910+
1911+
when:
1912+
def config = new Config()
1913+
1914+
then:
1915+
config.experimentalFeaturesEnabled == [].toSet()
1916+
1917+
//verify expected behavior when not enabled under feature flag
1918+
config.globalTags == [env:"test", aKey:"aVal", bKey:"bVal"]
1919+
}
1920+
18791921
def "detect if agent is configured using default values"() {
18801922
setup:
18811923
if (host != null) {

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

Lines changed: 34 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -99,6 +99,40 @@ class ConfigConverterTest extends DDSpecification {
9999
// spotless:on
100100
}
101101

102+
def "parsing map #mapString with List of arg separators for with key value separator #separator"() {
103+
//testing parsing for DD_TAGS
104+
setup:
105+
def separatorList = [',' as char, ' ' as char]
106+
107+
when:
108+
def result = ConfigConverter.parseTraceTagsMap(mapString, separator as char, separatorList as List<Character>)
109+
110+
then:
111+
result == expected
112+
113+
where:
114+
// spotless:off
115+
mapString | separator | expected
116+
"key1:value1,key2:value2" | ':' | [key1: "value1", key2: "value2"]
117+
"key1:value1 key2:value2" | ':' | [key1: "value1", key2: "value2"]
118+
"env:test aKey:aVal bKey:bVal cKey:" | ':' | [env: "test", aKey: "aVal", bKey: "bVal", cKey:""]
119+
"env:test,aKey:aVal,bKey:bVal,cKey:" | ':' | [env: "test", aKey: "aVal", bKey: "bVal", cKey:""]
120+
"env:test,aKey:aVal bKey:bVal cKey:" | ':' | [env: "test", aKey: "aVal bKey:bVal cKey:"]
121+
"env:test bKey :bVal dKey: dVal cKey:" | ':' | [env: "test", bKey: "", dKey: "", dVal: "", cKey: ""]
122+
'env :test, aKey : aVal bKey:bVal cKey:' | ':' | [env: "test", aKey : "aVal bKey:bVal cKey:"]
123+
"env:keyWithA:Semicolon bKey:bVal cKey" | ':' | [env: "keyWithA:Semicolon", bKey: "bVal", cKey: ""]
124+
"env:keyWith: , , Lots:Of:Semicolons " | ':' | [env: "keyWith:", Lots: "Of:Semicolons"]
125+
"a:b,c,d" | ':' | [a: "b", c: "", d: ""]
126+
"a,1" | ':' | [a: "", "1": ""]
127+
"a:b:c:d" | ':' | [a: "b:c:d"]
128+
//edge cases
129+
"noDelimiters" | ':' | [noDelimiters: ""]
130+
" " | ':' | [:]
131+
",,,,,,,,,,,," | ':' | [:]
132+
", , , , , , " | ':' | [:]
133+
// spotless:on
134+
}
135+
102136
def "test parseMapWithOptionalMappings"() {
103137
when:
104138
def result = ConfigConverter.parseMapWithOptionalMappings(mapString, "test", "", lowercaseKeys)

0 commit comments

Comments
 (0)