From 04cc57740a72909098b54abad72ce0a4e3de511d Mon Sep 17 00:00:00 2001 From: Matthew Li Date: Thu, 16 Jan 2025 11:51:54 -0500 Subject: [PATCH 01/13] testing --- .../datadog/trace/api/ConfigDefaults.java | 2 +- .../common/writer/DDSpanJsonAdapter.java | 3 +++ .../main/java/datadog/trace/core/DDSpan.java | 1 + .../datadog/trace/core/DDSpanContext.java | 6 +++++- .../trace/core/DDSpanContextTest.groovy | 19 +++++++++++++++++++ 5 files changed, 29 insertions(+), 2 deletions(-) diff --git a/dd-trace-api/src/main/java/datadog/trace/api/ConfigDefaults.java b/dd-trace-api/src/main/java/datadog/trace/api/ConfigDefaults.java index 4eb414f3394..cd75d73c307 100644 --- a/dd-trace-api/src/main/java/datadog/trace/api/ConfigDefaults.java +++ b/dd-trace-api/src/main/java/datadog/trace/api/ConfigDefaults.java @@ -229,7 +229,7 @@ public final class ConfigDefaults { static final int DEFAULT_TELEMETRY_DEPENDENCY_RESOLUTION_QUEUE_SIZE = 100000; static final boolean DEFAULT_TRACE_128_BIT_TRACEID_GENERATION_ENABLED = true; - static final boolean DEFAULT_TRACE_128_BIT_TRACEID_LOGGING_ENABLED = false; + static final boolean DEFAULT_TRACE_128_BIT_TRACEID_LOGGING_ENABLED = true; static final boolean DEFAULT_SECURE_RANDOM = false; public static final int DEFAULT_TRACE_X_DATADOG_TAGS_MAX_LENGTH = 512; diff --git a/dd-trace-core/src/main/java/datadog/trace/common/writer/DDSpanJsonAdapter.java b/dd-trace-core/src/main/java/datadog/trace/common/writer/DDSpanJsonAdapter.java index 93c6995d4e8..f7f8f0ed224 100644 --- a/dd-trace-core/src/main/java/datadog/trace/common/writer/DDSpanJsonAdapter.java +++ b/dd-trace-core/src/main/java/datadog/trace/common/writer/DDSpanJsonAdapter.java @@ -75,6 +75,7 @@ public void toJson(final com.squareup.moshi.JsonWriter writer, final DDSpan span writer.name("meta"); writer.beginObject(); final Map tags = span.getTags(); + System.out.println("Writing tags!!!"); for (final Map.Entry entry : span.context().getBaggageItems().entrySet()) { if (!tags.containsKey(entry.getKey())) { writer.name(entry.getKey()); @@ -83,6 +84,8 @@ public void toJson(final com.squareup.moshi.JsonWriter writer, final DDSpan span } for (final Map.Entry entry : tags.entrySet()) { if (!(entry.getValue() instanceof Number)) { + System.out.println("key: " + entry.getKey()); + System.out.println("value: " + String.valueOf(entry.getValue())); writer.name(entry.getKey()); writer.value(String.valueOf(entry.getValue())); } diff --git a/dd-trace-core/src/main/java/datadog/trace/core/DDSpan.java b/dd-trace-core/src/main/java/datadog/trace/core/DDSpan.java index 6a8e6f713c1..933c787f9f2 100644 --- a/dd-trace-core/src/main/java/datadog/trace/core/DDSpan.java +++ b/dd-trace-core/src/main/java/datadog/trace/core/DDSpan.java @@ -716,6 +716,7 @@ public CharSequence getType() { @Override public void processTagsAndBaggage(final MetadataConsumer consumer) { + System.out.println("Context.toString(): " + context.toString()); context.processTagsAndBaggage(consumer, longRunningVersion, links); } diff --git a/dd-trace-core/src/main/java/datadog/trace/core/DDSpanContext.java b/dd-trace-core/src/main/java/datadog/trace/core/DDSpanContext.java index fcbf046122a..f086f177482 100644 --- a/dd-trace-core/src/main/java/datadog/trace/core/DDSpanContext.java +++ b/dd-trace-core/src/main/java/datadog/trace/core/DDSpanContext.java @@ -367,7 +367,9 @@ public DDSpanContext( propagationTags != null ? propagationTags : traceCollector.getTracer().getPropagationTagsFactory().empty(); - this.propagationTags.updateTraceIdHighOrderBits(this.traceId.toHighOrderLong()); + if (parentId == 0) { // higher bits of traceID are only used in root span + this.propagationTags.updateTraceIdHighOrderBits(this.traceId.toHighOrderLong()); + } this.injectBaggageAsTags = injectBaggageAsTags; if (origin != null) { setOrigin(origin); @@ -766,6 +768,8 @@ void setAllTags(final Map map) { } void unsafeSetTag(final String tag, final Object value) { + // System.out.println("Tag: " + tag); + // System.out.println("Value: " + value); unsafeTags.put(tag, value); } diff --git a/dd-trace-core/src/test/groovy/datadog/trace/core/DDSpanContextTest.groovy b/dd-trace-core/src/test/groovy/datadog/trace/core/DDSpanContextTest.groovy index c880c17d94c..e2b9bb95276 100644 --- a/dd-trace-core/src/test/groovy/datadog/trace/core/DDSpanContextTest.groovy +++ b/dd-trace-core/src/test/groovy/datadog/trace/core/DDSpanContextTest.groovy @@ -23,6 +23,7 @@ class DDSpanContextTest extends DDCoreSpecification { def profilingContextIntegration def setup() { + injectSysConfig("trace.128.bit.traceid.generation.enabled", "true") writer = new ListWriter() profilingContextIntegration = Mock(ProfilingContextIntegration) tracer = tracerBuilder().writer(writer) @@ -285,6 +286,24 @@ class DDSpanContextTest extends DDCoreSpecification { span.context.encodedResourceName == -2 } + def "higher order bits of 128bit traceID is only set as PTag in root span"() { + when: + def parent = tracer.buildSpan("fakeOperation") + .withServiceName("fakeService") + .withResourceName("fakeResource") + .start() + + def child = tracer.buildSpan("fakeOperation") + .withServiceName("fakeService") + .withResourceName("fakeResource") + .asChildOf(parent) + .start() + + then: + parent.context().getPropagationTags().getTraceIdHighOrderBits() != 0 + // child.context().getPropagationTags().getTraceIdHighOrderBits() == 0 + } + private static String dataTag(String tag) { "_dd.${tag}.json" } From 5cbca2cc5b22e484db5ebccb14bd1d6cc58c73ea Mon Sep 17 00:00:00 2001 From: Matthew Li Date: Thu, 16 Jan 2025 14:02:02 -0500 Subject: [PATCH 02/13] cleanup --- .../java/datadog/trace/common/writer/DDSpanJsonAdapter.java | 3 --- dd-trace-core/src/main/java/datadog/trace/core/DDSpan.java | 1 - 2 files changed, 4 deletions(-) diff --git a/dd-trace-core/src/main/java/datadog/trace/common/writer/DDSpanJsonAdapter.java b/dd-trace-core/src/main/java/datadog/trace/common/writer/DDSpanJsonAdapter.java index f7f8f0ed224..93c6995d4e8 100644 --- a/dd-trace-core/src/main/java/datadog/trace/common/writer/DDSpanJsonAdapter.java +++ b/dd-trace-core/src/main/java/datadog/trace/common/writer/DDSpanJsonAdapter.java @@ -75,7 +75,6 @@ public void toJson(final com.squareup.moshi.JsonWriter writer, final DDSpan span writer.name("meta"); writer.beginObject(); final Map tags = span.getTags(); - System.out.println("Writing tags!!!"); for (final Map.Entry entry : span.context().getBaggageItems().entrySet()) { if (!tags.containsKey(entry.getKey())) { writer.name(entry.getKey()); @@ -84,8 +83,6 @@ public void toJson(final com.squareup.moshi.JsonWriter writer, final DDSpan span } for (final Map.Entry entry : tags.entrySet()) { if (!(entry.getValue() instanceof Number)) { - System.out.println("key: " + entry.getKey()); - System.out.println("value: " + String.valueOf(entry.getValue())); writer.name(entry.getKey()); writer.value(String.valueOf(entry.getValue())); } diff --git a/dd-trace-core/src/main/java/datadog/trace/core/DDSpan.java b/dd-trace-core/src/main/java/datadog/trace/core/DDSpan.java index 933c787f9f2..6a8e6f713c1 100644 --- a/dd-trace-core/src/main/java/datadog/trace/core/DDSpan.java +++ b/dd-trace-core/src/main/java/datadog/trace/core/DDSpan.java @@ -716,7 +716,6 @@ public CharSequence getType() { @Override public void processTagsAndBaggage(final MetadataConsumer consumer) { - System.out.println("Context.toString(): " + context.toString()); context.processTagsAndBaggage(consumer, longRunningVersion, links); } From 96be6c85c08ad14873c3612892e7e550ba329d3e Mon Sep 17 00:00:00 2001 From: Matthew Li Date: Mon, 27 Jan 2025 12:17:43 -0500 Subject: [PATCH 03/13] first iteration on parser --- .../config/provider/AgentArgsInjector.java | 4 +- .../config/provider/ConfigConverter.java | 59 +++++++++++-------- .../provider/ConfigConverterTest.groovy | 28 +++++++++ 3 files changed, 65 insertions(+), 26 deletions(-) diff --git a/internal-api/src/main/java/datadog/trace/bootstrap/config/provider/AgentArgsInjector.java b/internal-api/src/main/java/datadog/trace/bootstrap/config/provider/AgentArgsInjector.java index 967f5a7dda4..566a321de19 100644 --- a/internal-api/src/main/java/datadog/trace/bootstrap/config/provider/AgentArgsInjector.java +++ b/internal-api/src/main/java/datadog/trace/bootstrap/config/provider/AgentArgsInjector.java @@ -1,6 +1,8 @@ package datadog.trace.bootstrap.config.provider; import datadog.trace.util.Strings; + +import java.util.Arrays; import java.util.Map; public class AgentArgsInjector { @@ -11,7 +13,7 @@ public class AgentArgsInjector { * @param agentArgs Agent arguments to be parsed and set */ public static void injectAgentArgsConfig(String agentArgs) { - Map args = ConfigConverter.parseMap(agentArgs, "javaagent arguments", '='); + Map args = ConfigConverter.parseMap(agentArgs, "javaagent arguments", '=', Arrays.asList(',', ' ')); injectAgentArgsConfig(args); } diff --git a/internal-api/src/main/java/datadog/trace/bootstrap/config/provider/ConfigConverter.java b/internal-api/src/main/java/datadog/trace/bootstrap/config/provider/ConfigConverter.java index 1bd9422087f..a2484836030 100644 --- a/internal-api/src/main/java/datadog/trace/bootstrap/config/provider/ConfigConverter.java +++ b/internal-api/src/main/java/datadog/trace/bootstrap/config/provider/ConfigConverter.java @@ -68,19 +68,19 @@ static List parseList(final String str, final String separator) { @Nonnull static Map parseMap(final String str, final String settingName) { - return parseMap(str, settingName, ':'); + return parseMap(str, settingName, ':', Arrays.asList(',', ' ')); //is this the best place stylistically to put the list? } @Nonnull static Map parseMap( - final String str, final String settingName, final char keyValueSeparator) { + final String str, final String settingName, final char keyValueSeparator, final List 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 map = new HashMap<>(); - loadMap(map, trimmed, settingName, keyValueSeparator); + loadMap(map, trimmed, settingName, keyValueSeparator, argSeparators); return map; } @@ -122,7 +122,7 @@ static Map parseOrderedMap(final String str, final String settin return Collections.emptyMap(); } Map map = new LinkedHashMap<>(); - loadMap(map, trimmed, settingName, ':'); + loadMap(map, trimmed, settingName, ':', Arrays.asList(',', ' ')); //is this the best place stylistically to put the list? return map; } @@ -133,50 +133,59 @@ public BadFormatException(String message) { } private static void loadMap( - Map map, String str, String settingName, char keyValueSeparator) { + Map map, String str, String settingName, char keyValueSeparator, final List argSeparators) { // we know that the str is trimmed and rely on that there is no leading/trailing whitespace try { int start = 0; int splitter = str.indexOf(keyValueSeparator, start); + if (splitter == -1){ + return; + } + char argSeparator = '\u0000'; + int argSeparatorInd = -1; + for (Character sep : argSeparators){ //find the first instance of the first possible separator + argSeparatorInd = str.indexOf(sep, splitter + 1); + if (argSeparatorInd != -1) { + argSeparator = sep; + break; + } + } + if (argSeparator == '\u0000'){ //no argSeparator found + return; + } while (splitter != -1) { int nextSplitter = str.indexOf(keyValueSeparator, splitter + 1); - int nextComma = str.indexOf(',', splitter + 1); - nextComma = nextComma == -1 ? str.length() : nextComma; - int nextSpace = str.indexOf(' ', splitter + 1); - nextSpace = nextSpace == -1 ? str.length() : nextSpace; + int nextArgSeparator = -1; + nextArgSeparator = str.indexOf(argSeparator, splitter + 1); + nextArgSeparator = nextArgSeparator == -1 ? str.length() : nextArgSeparator; // if we have a delimiter after this splitter, then try to move the splitter forward to // allow for tags with ':' in them - int end = nextComma < str.length() ? nextComma : nextSpace; - while (nextSplitter != -1 && nextSplitter < end) { + int end = nextArgSeparator; + while (nextSplitter != -1 && nextSplitter < end) { //skips all following splitters before the nextArgSeparator nextSplitter = str.indexOf(keyValueSeparator, nextSplitter + 1); } if (nextSplitter == -1) { // this is either the end of the string or the next position where the value should be // trimmed - end = nextComma; - if (nextComma < str.length() - 1) { - // there are non-space characters after the ',' - throw new BadFormatException("Non white space characters after trailing ','"); + if (end < str.length() - 1) { + // there are characters after the argSeparator + throw new BadFormatException(String.format("Non white space characters after trailing '%c'", nextArgSeparator)); } } else { - if (nextComma < str.length()) { - end = nextComma; - } else if (nextSpace < str.length()) { - end = nextSpace; - } else { + if (end >= str.length()) { // this should not happen throw new BadFormatException("Illegal position of split character ':'"); } } String key = str.substring(start, splitter).trim(); - if (key.indexOf(',') != -1) { - throw new BadFormatException("Illegal ',' character in key '" + key + "'"); + if (key.indexOf(argSeparator) != -1) { + throw new BadFormatException("Illegal '" + argSeparator + "' character in key '" + key + "'"); } String value = str.substring(splitter + 1, end).trim(); - if (value.indexOf(' ') != -1) { - throw new BadFormatException("Illegal ' ' character in value for key '" + key + "'"); + if (value.indexOf(argSeparator) != -1) { + throw new BadFormatException("Illegal '" + argSeparator + "' character in value for key '" + key + "'"); } - if (!key.isEmpty() && !value.isEmpty()) { + if (!key.isEmpty()) { map.put(key, value); } splitter = nextSplitter; diff --git a/internal-api/src/test/groovy/datadog/trace/bootstrap/config/provider/ConfigConverterTest.groovy b/internal-api/src/test/groovy/datadog/trace/bootstrap/config/provider/ConfigConverterTest.groovy index 36abcf887f8..7ee938e2c81 100644 --- a/internal-api/src/test/groovy/datadog/trace/bootstrap/config/provider/ConfigConverterTest.groovy +++ b/internal-api/src/test/groovy/datadog/trace/bootstrap/config/provider/ConfigConverterTest.groovy @@ -99,6 +99,34 @@ class ConfigConverterTest extends DDSpecification { // spotless:on } + def "parsing map #mapString with List of arg separators #argSeparators for with key value separator #separator"() { + setup: + def separatorList = [','.charAt(0), ' '.charAt(0)] + + when: + def result = ConfigConverter.parseMap(mapString, "test", separator as char, separatorList as List) + + then: + result == expected + + where: + // spotless:off + mapString | separator | argSeparators | 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: "", 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"] + // spotless:on + } + def "test parseMapWithOptionalMappings"() { when: def result = ConfigConverter.parseMapWithOptionalMappings(mapString, "test", "", lowercaseKeys) From 79e12ea3e90eaa4c983753ddb303e2b052835b34 Mon Sep 17 00:00:00 2001 From: Matthew Li Date: Mon, 27 Jan 2025 14:47:03 -0500 Subject: [PATCH 04/13] initial working parsing for DD_TAGS --- .../config/provider/ConfigConverter.java | 69 +++++++++---------- .../provider/ConfigConverterTest.groovy | 4 +- 2 files changed, 34 insertions(+), 39 deletions(-) diff --git a/internal-api/src/main/java/datadog/trace/bootstrap/config/provider/ConfigConverter.java b/internal-api/src/main/java/datadog/trace/bootstrap/config/provider/ConfigConverter.java index a2484836030..704f53a5565 100644 --- a/internal-api/src/main/java/datadog/trace/bootstrap/config/provider/ConfigConverter.java +++ b/internal-api/src/main/java/datadog/trace/bootstrap/config/provider/ConfigConverter.java @@ -138,9 +138,6 @@ private static void loadMap( try { int start = 0; int splitter = str.indexOf(keyValueSeparator, start); - if (splitter == -1){ - return; - } char argSeparator = '\u0000'; int argSeparatorInd = -1; for (Character sep : argSeparators){ //find the first instance of the first possible separator @@ -150,45 +147,43 @@ private static void loadMap( break; } } - if (argSeparator == '\u0000'){ //no argSeparator found - return; - } - while (splitter != -1) { - int nextSplitter = str.indexOf(keyValueSeparator, splitter + 1); - int nextArgSeparator = -1; - nextArgSeparator = str.indexOf(argSeparator, splitter + 1); - nextArgSeparator = nextArgSeparator == -1 ? str.length() : nextArgSeparator; - // if we have a delimiter after this splitter, then try to move the splitter forward to - // allow for tags with ':' in them - int end = nextArgSeparator; - while (nextSplitter != -1 && nextSplitter < end) { //skips all following splitters before the nextArgSeparator - nextSplitter = str.indexOf(keyValueSeparator, nextSplitter + 1); + while (splitter != -1 || start < str.length()) { + int nextSplitter = argSeparatorInd == -1 ? -1 : str.indexOf(keyValueSeparator, argSeparatorInd + 1); + 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; } - if (nextSplitter == -1) { - // this is either the end of the string or the next position where the value should be - // trimmed - if (end < str.length() - 1) { - // there are characters after the argSeparator - throw new BadFormatException(String.format("Non white space characters after trailing '%c'", nextArgSeparator)); + + if(splitter >= end || splitter == -1){ //only key, no value + String key = str.substring(start, end).trim(); + if(!key.isEmpty()){ + map.put(key, ""); } - } else { - if (end >= str.length()) { - // this should not happen - throw new BadFormatException("Illegal position of split character ':'"); + }else{ + String key = str.substring(start, splitter).trim(); + if (key.indexOf(argSeparator) != -1) { + throw new BadFormatException("Illegal '" + argSeparator + "' character in key '" + key + "'"); + } + String value; + if (splitter + 1 >= end){ // no splitter in this string: only key, no value + value = ""; + }else{ + value = str.substring(splitter + 1, end).trim(); + } + if (value.indexOf(argSeparator) != -1) { + throw new BadFormatException("Illegal '" + argSeparator + "' character in value for key '" + key + "'"); + } + if (!key.isEmpty()) { + map.put(key, value); } - } - String key = str.substring(start, splitter).trim(); - if (key.indexOf(argSeparator) != -1) { - throw new BadFormatException("Illegal '" + argSeparator + "' character in key '" + key + "'"); - } - String value = str.substring(splitter + 1, end).trim(); - if (value.indexOf(argSeparator) != -1) { - throw new BadFormatException("Illegal '" + argSeparator + "' character in value for key '" + key + "'"); - } - if (!key.isEmpty()) { - map.put(key, value); } splitter = nextSplitter; + argSeparatorInd = nextArgSeparator; start = end + 1; } } catch (Throwable t) { diff --git a/internal-api/src/test/groovy/datadog/trace/bootstrap/config/provider/ConfigConverterTest.groovy b/internal-api/src/test/groovy/datadog/trace/bootstrap/config/provider/ConfigConverterTest.groovy index 7ee938e2c81..3308c3e6dc8 100644 --- a/internal-api/src/test/groovy/datadog/trace/bootstrap/config/provider/ConfigConverterTest.groovy +++ b/internal-api/src/test/groovy/datadog/trace/bootstrap/config/provider/ConfigConverterTest.groovy @@ -117,12 +117,12 @@ class ConfigConverterTest extends DDSpecification { "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: "", cKey: ""] + "env:test bKey :bVal dKey: dVal cKey:" | ':' | [] | [env: "test", bKey: "", dKey: "", dVal: "", cKey: ""] //[bKey: "", cKey: "", dVal: "", dKey: "", env:"test"] '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,1" | ':' | [] | [a: "", "1": ""] "a:b:c:d" | ':' | [] | [a: "b:c:d"] // spotless:on } From 8c6cbaabd92293f4e3871d1fb34b72435f134020 Mon Sep 17 00:00:00 2001 From: Matthew Li Date: Mon, 27 Jan 2025 15:03:50 -0500 Subject: [PATCH 05/13] cleanup part 1 --- .../config/provider/ConfigConverter.java | 17 ++--------- .../provider/ConfigConverterTest.groovy | 28 +++++++++---------- 2 files changed, 17 insertions(+), 28 deletions(-) diff --git a/internal-api/src/main/java/datadog/trace/bootstrap/config/provider/ConfigConverter.java b/internal-api/src/main/java/datadog/trace/bootstrap/config/provider/ConfigConverter.java index 704f53a5565..5e5d306eebb 100644 --- a/internal-api/src/main/java/datadog/trace/bootstrap/config/provider/ConfigConverter.java +++ b/internal-api/src/main/java/datadog/trace/bootstrap/config/provider/ConfigConverter.java @@ -148,7 +148,7 @@ private static void loadMap( } } while (splitter != -1 || start < str.length()) { - int nextSplitter = argSeparatorInd == -1 ? -1 : str.indexOf(keyValueSeparator, argSeparatorInd + 1); + 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; @@ -159,25 +159,14 @@ private static void loadMap( continue; } - if(splitter >= end || splitter == -1){ //only key, no value + if(splitter >= end || splitter == -1){ //only key, no value; either due end of string or substring not having splitter String key = str.substring(start, end).trim(); if(!key.isEmpty()){ map.put(key, ""); } }else{ String key = str.substring(start, splitter).trim(); - if (key.indexOf(argSeparator) != -1) { - throw new BadFormatException("Illegal '" + argSeparator + "' character in key '" + key + "'"); - } - String value; - if (splitter + 1 >= end){ // no splitter in this string: only key, no value - value = ""; - }else{ - value = str.substring(splitter + 1, end).trim(); - } - if (value.indexOf(argSeparator) != -1) { - throw new BadFormatException("Illegal '" + argSeparator + "' character in value for key '" + key + "'"); - } + String value = str.substring(splitter + 1, end).trim(); if (!key.isEmpty()) { map.put(key, value); } diff --git a/internal-api/src/test/groovy/datadog/trace/bootstrap/config/provider/ConfigConverterTest.groovy b/internal-api/src/test/groovy/datadog/trace/bootstrap/config/provider/ConfigConverterTest.groovy index 3308c3e6dc8..a504e172a0c 100644 --- a/internal-api/src/test/groovy/datadog/trace/bootstrap/config/provider/ConfigConverterTest.groovy +++ b/internal-api/src/test/groovy/datadog/trace/bootstrap/config/provider/ConfigConverterTest.groovy @@ -99,7 +99,7 @@ class ConfigConverterTest extends DDSpecification { // spotless:on } - def "parsing map #mapString with List of arg separators #argSeparators for with key value separator #separator"() { + def "parsing map #mapString with List of arg separators #argSeparators for with key value separator #separator"() { //testing parsing for DD_TAGS setup: def separatorList = [','.charAt(0), ' '.charAt(0)] @@ -111,19 +111,19 @@ class ConfigConverterTest extends DDSpecification { where: // spotless:off - mapString | separator | argSeparators | 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: ""] //[bKey: "", cKey: "", dVal: "", dKey: "", env:"test"] - '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"] + 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"] // spotless:on } From f31e8e8728d9dc3a07add020a3f4e0cfe1758041 Mon Sep 17 00:00:00 2001 From: Matthew Li Date: Mon, 27 Jan 2025 15:41:56 -0500 Subject: [PATCH 06/13] cleanup pt 2 --- .../config/provider/AgentArgsInjector.java | 4 +- .../config/provider/ConfigConverter.java | 113 +++++++++--------- .../provider/ConfigConverterTest.groovy | 3 +- 3 files changed, 63 insertions(+), 57 deletions(-) diff --git a/internal-api/src/main/java/datadog/trace/bootstrap/config/provider/AgentArgsInjector.java b/internal-api/src/main/java/datadog/trace/bootstrap/config/provider/AgentArgsInjector.java index 566a321de19..a4e652de59e 100644 --- a/internal-api/src/main/java/datadog/trace/bootstrap/config/provider/AgentArgsInjector.java +++ b/internal-api/src/main/java/datadog/trace/bootstrap/config/provider/AgentArgsInjector.java @@ -1,7 +1,6 @@ package datadog.trace.bootstrap.config.provider; import datadog.trace.util.Strings; - import java.util.Arrays; import java.util.Map; @@ -13,7 +12,8 @@ public class AgentArgsInjector { * @param agentArgs Agent arguments to be parsed and set */ public static void injectAgentArgsConfig(String agentArgs) { - Map args = ConfigConverter.parseMap(agentArgs, "javaagent arguments", '=', Arrays.asList(',', ' ')); + Map args = + ConfigConverter.parseMap(agentArgs, "javaagent arguments", '=', Arrays.asList(',', ' ')); injectAgentArgsConfig(args); } diff --git a/internal-api/src/main/java/datadog/trace/bootstrap/config/provider/ConfigConverter.java b/internal-api/src/main/java/datadog/trace/bootstrap/config/provider/ConfigConverter.java index 5e5d306eebb..56e88066c4a 100644 --- a/internal-api/src/main/java/datadog/trace/bootstrap/config/provider/ConfigConverter.java +++ b/internal-api/src/main/java/datadog/trace/bootstrap/config/provider/ConfigConverter.java @@ -68,12 +68,19 @@ static List parseList(final String str, final String separator) { @Nonnull static Map parseMap(final String str, final String settingName) { - return parseMap(str, settingName, ':', Arrays.asList(',', ' ')); //is this the best place stylistically to put the list? + return parseMap( + str, + settingName, + ':', + Arrays.asList(',', ' ')); // is this the best place stylistically to put the list? } @Nonnull static Map parseMap( - final String str, final String settingName, final char keyValueSeparator, final List argSeparators) { + final String str, + final String settingName, + final char keyValueSeparator, + final List 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()) { @@ -122,7 +129,12 @@ static Map parseOrderedMap(final String str, final String settin return Collections.emptyMap(); } Map map = new LinkedHashMap<>(); - loadMap(map, trimmed, settingName, ':', Arrays.asList(',', ' ')); //is this the best place stylistically to put the list? + loadMap( + map, + trimmed, + settingName, + ':', + Arrays.asList(',', ' ')); // is this the best place stylistically to put the list? return map; } @@ -133,64 +145,57 @@ public BadFormatException(String message) { } private static void loadMap( - Map map, String str, String settingName, char keyValueSeparator, final List argSeparators) { - // we know that the str is trimmed and rely on that there is no leading/trailing whitespace - try { - int start = 0; - int splitter = str.indexOf(keyValueSeparator, start); - char argSeparator = '\u0000'; - int argSeparatorInd = -1; - for (Character sep : argSeparators){ //find the first instance of the first possible separator - argSeparatorInd = str.indexOf(sep, splitter + 1); - if (argSeparatorInd != -1) { - argSeparator = sep; - break; - } + Map map, + String str, + String settingName, + char keyValueSeparator, + final List argSeparators) { + int start = 0; + int splitter = str.indexOf(keyValueSeparator, start); + char argSeparator = '\0'; + int argSeparatorInd = -1; + for (Character sep : argSeparators) { // find the first instance of the first possible separator + argSeparatorInd = str.indexOf(sep); + if (argSeparatorInd != -1) { + argSeparator = sep; + break; } - while (splitter != -1 || 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; - } + } + 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(splitter >= end || splitter == -1){ //only key, no value; either due end of string or substring not having splitter - String key = str.substring(start, end).trim(); - if(!key.isEmpty()){ - map.put(key, ""); - } - }else{ - String key = str.substring(start, splitter).trim(); - String value = str.substring(splitter + 1, end).trim(); - if (!key.isEmpty()) { - map.put(key, value); - } - } + if (start >= end) { // the character is only the delimiter + start = end + 1; splitter = nextSplitter; argSeparatorInd = nextArgSeparator; - start = end + 1; + continue; } - } catch (Throwable t) { - if (t instanceof BadFormatException) { - log.warn( - "Invalid config for {}. {}. Must match " - + "'key1{}value1,key2{}value2' or " - + "'key1{}value1 key2{}value2'.", - settingName, - t.getMessage(), - keyValueSeparator, - keyValueSeparator, - keyValueSeparator, - keyValueSeparator); + + 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 { - log.warn("Unexpected exception during config parsing of {}.", settingName, t); + key = str.substring(start, splitter).trim(); + value = str.substring(splitter + 1, end).trim(); } - map.clear(); + if (!key.isEmpty()) { + map.put(key, value); + } + splitter = nextSplitter; + argSeparatorInd = nextArgSeparator; + start = end + 1; } } diff --git a/internal-api/src/test/groovy/datadog/trace/bootstrap/config/provider/ConfigConverterTest.groovy b/internal-api/src/test/groovy/datadog/trace/bootstrap/config/provider/ConfigConverterTest.groovy index a504e172a0c..da0cc29651c 100644 --- a/internal-api/src/test/groovy/datadog/trace/bootstrap/config/provider/ConfigConverterTest.groovy +++ b/internal-api/src/test/groovy/datadog/trace/bootstrap/config/provider/ConfigConverterTest.groovy @@ -99,7 +99,8 @@ class ConfigConverterTest extends DDSpecification { // spotless:on } - def "parsing map #mapString with List of arg separators #argSeparators for with key value separator #separator"() { //testing parsing for DD_TAGS + def "parsing map #mapString with List of arg separators for with key value separator #separator"() { + //testing parsing for DD_TAGS setup: def separatorList = [','.charAt(0), ' '.charAt(0)] From 61d1d4d9253e3112180c3eb2486c85fb0db6357c Mon Sep 17 00:00:00 2001 From: Matthew Li Date: Mon, 27 Jan 2025 16:04:26 -0500 Subject: [PATCH 07/13] adding traceTagsParser as separate parser due to different rule --- .../config/provider/AgentArgsInjector.java | 2 +- .../config/provider/ConfigConverter.java | 106 ++++++++++++++++-- .../provider/ConfigConverterTest.groovy | 2 +- 3 files changed, 100 insertions(+), 10 deletions(-) diff --git a/internal-api/src/main/java/datadog/trace/bootstrap/config/provider/AgentArgsInjector.java b/internal-api/src/main/java/datadog/trace/bootstrap/config/provider/AgentArgsInjector.java index a4e652de59e..5dc636a8b4b 100644 --- a/internal-api/src/main/java/datadog/trace/bootstrap/config/provider/AgentArgsInjector.java +++ b/internal-api/src/main/java/datadog/trace/bootstrap/config/provider/AgentArgsInjector.java @@ -13,7 +13,7 @@ public class AgentArgsInjector { */ public static void injectAgentArgsConfig(String agentArgs) { Map args = - ConfigConverter.parseMap(agentArgs, "javaagent arguments", '=', Arrays.asList(',', ' ')); + ConfigConverter.parseMap(agentArgs, "javaagent arguments", '='); injectAgentArgsConfig(args); } diff --git a/internal-api/src/main/java/datadog/trace/bootstrap/config/provider/ConfigConverter.java b/internal-api/src/main/java/datadog/trace/bootstrap/config/provider/ConfigConverter.java index 56e88066c4a..9c2bb4237a0 100644 --- a/internal-api/src/main/java/datadog/trace/bootstrap/config/provider/ConfigConverter.java +++ b/internal-api/src/main/java/datadog/trace/bootstrap/config/provider/ConfigConverter.java @@ -68,15 +68,35 @@ static List parseList(final String str, final String separator) { @Nonnull static Map parseMap(final String str, final String settingName) { - return parseMap( - str, - settingName, - ':', - Arrays.asList(',', ' ')); // is this the best place stylistically to put the list? + if (settingName.equals("trace.tags")){ + return parseTraceTagsMap(str, + settingName, + ':', Arrays.asList(',', ' ')); + } else{ + return parseMap( + str, + settingName, + ':'); + } } @Nonnull static Map parseMap( + final String str, + final String settingName, + final char keyValueSeparator) { + // 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 map = new HashMap<>(); + loadMap(map, trimmed, settingName, keyValueSeparator); + return map; + } + + @Nonnull + static Map parseTraceTagsMap( final String str, final String settingName, final char keyValueSeparator, @@ -87,7 +107,7 @@ static Map parseMap( return Collections.emptyMap(); } Map map = new HashMap<>(); - loadMap(map, trimmed, settingName, keyValueSeparator, argSeparators); + loadTraceTagsMap(map, trimmed, settingName, keyValueSeparator, argSeparators); return map; } @@ -133,8 +153,7 @@ static Map parseOrderedMap(final String str, final String settin map, trimmed, settingName, - ':', - Arrays.asList(',', ' ')); // is this the best place stylistically to put the list? + ':'); return map; } @@ -144,7 +163,78 @@ public BadFormatException(String message) { } } + private static void loadMap( + Map map, String str, String settingName, char keyValueSeparator) { + // we know that the str is trimmed and rely on that there is no leading/trailing whitespace + try { + int start = 0; + int splitter = str.indexOf(keyValueSeparator, start); + while (splitter != -1) { + int nextSplitter = str.indexOf(keyValueSeparator, splitter + 1); + int nextComma = str.indexOf(',', splitter + 1); + nextComma = nextComma == -1 ? str.length() : nextComma; + int nextSpace = str.indexOf(' ', splitter + 1); + nextSpace = nextSpace == -1 ? str.length() : nextSpace; + // if we have a delimiter after this splitter, then try to move the splitter forward to + // allow for tags with ':' in them + int end = nextComma < str.length() ? nextComma : nextSpace; + while (nextSplitter != -1 && nextSplitter < end) { + nextSplitter = str.indexOf(keyValueSeparator, nextSplitter + 1); + } + if (nextSplitter == -1) { + // this is either the end of the string or the next position where the value should be + // trimmed + end = nextComma; + if (nextComma < str.length() - 1) { + // there are non-space characters after the ',' + throw new BadFormatException("Non white space characters after trailing ','"); + } + } else { + if (nextComma < str.length()) { + end = nextComma; + } else if (nextSpace < str.length()) { + end = nextSpace; + } else { + // this should not happen + throw new BadFormatException("Illegal position of split character ':'"); + } + } + String key = str.substring(start, splitter).trim(); + if (key.indexOf(',') != -1) { + throw new BadFormatException("Illegal ',' character in key '" + key + "'"); + } + String value = str.substring(splitter + 1, end).trim(); + if (value.indexOf(' ') != -1) { + throw new BadFormatException("Illegal ' ' character in value for key '" + key + "'"); + } + if (!key.isEmpty() && !value.isEmpty()) { + map.put(key, value); + } + splitter = nextSplitter; + start = end + 1; + } + } catch (Throwable t) { + if (t instanceof BadFormatException) { + log.warn( + "Invalid config for {}. {}. Must match " + + "'key1{}value1,key2{}value2' or " + + "'key1{}value1 key2{}value2'.", + settingName, + t.getMessage(), + keyValueSeparator, + keyValueSeparator, + keyValueSeparator, + keyValueSeparator); + } else { + log.warn("Unexpected exception during config parsing of {}.", settingName, t); + } + map.clear(); + } + } + + + private static void loadTraceTagsMap( Map map, String str, String settingName, diff --git a/internal-api/src/test/groovy/datadog/trace/bootstrap/config/provider/ConfigConverterTest.groovy b/internal-api/src/test/groovy/datadog/trace/bootstrap/config/provider/ConfigConverterTest.groovy index da0cc29651c..47d32081914 100644 --- a/internal-api/src/test/groovy/datadog/trace/bootstrap/config/provider/ConfigConverterTest.groovy +++ b/internal-api/src/test/groovy/datadog/trace/bootstrap/config/provider/ConfigConverterTest.groovy @@ -105,7 +105,7 @@ class ConfigConverterTest extends DDSpecification { def separatorList = [','.charAt(0), ' '.charAt(0)] when: - def result = ConfigConverter.parseMap(mapString, "test", separator as char, separatorList as List) + def result = ConfigConverter.parseTraceTagsMap(mapString, "test", separator as char, separatorList as List) then: result == expected From dee14cefae9a095b556e49b629498066a25276cc Mon Sep 17 00:00:00 2001 From: Matthew Li Date: Tue, 28 Jan 2025 16:45:36 -0500 Subject: [PATCH 08/13] dd_service override --- .../main/java/datadog/trace/api/Config.java | 3 +++ .../config/provider/AgentArgsInjector.java | 4 +-- .../config/provider/ConfigConverter.java | 26 +++++-------------- .../datadog/trace/api/ConfigTest.groovy | 23 ++++++++++++---- 4 files changed, 29 insertions(+), 27 deletions(-) diff --git a/internal-api/src/main/java/datadog/trace/api/Config.java b/internal-api/src/main/java/datadog/trace/api/Config.java index 37da6f30c4a..6a54fb9808a 100644 --- a/internal-api/src/main/java/datadog/trace/api/Config.java +++ b/internal-api/src/main/java/datadog/trace/api/Config.java @@ -760,6 +760,9 @@ private Config(final ConfigProvider configProvider, final InstrumenterConfig ins { final Map tags = new HashMap<>(configProvider.getMergedMap(GLOBAL_TAGS)); 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); } diff --git a/internal-api/src/main/java/datadog/trace/bootstrap/config/provider/AgentArgsInjector.java b/internal-api/src/main/java/datadog/trace/bootstrap/config/provider/AgentArgsInjector.java index 5dc636a8b4b..967f5a7dda4 100644 --- a/internal-api/src/main/java/datadog/trace/bootstrap/config/provider/AgentArgsInjector.java +++ b/internal-api/src/main/java/datadog/trace/bootstrap/config/provider/AgentArgsInjector.java @@ -1,7 +1,6 @@ package datadog.trace.bootstrap.config.provider; import datadog.trace.util.Strings; -import java.util.Arrays; import java.util.Map; public class AgentArgsInjector { @@ -12,8 +11,7 @@ public class AgentArgsInjector { * @param agentArgs Agent arguments to be parsed and set */ public static void injectAgentArgsConfig(String agentArgs) { - Map args = - ConfigConverter.parseMap(agentArgs, "javaagent arguments", '='); + Map args = ConfigConverter.parseMap(agentArgs, "javaagent arguments", '='); injectAgentArgsConfig(args); } diff --git a/internal-api/src/main/java/datadog/trace/bootstrap/config/provider/ConfigConverter.java b/internal-api/src/main/java/datadog/trace/bootstrap/config/provider/ConfigConverter.java index 9c2bb4237a0..cd289a71a87 100644 --- a/internal-api/src/main/java/datadog/trace/bootstrap/config/provider/ConfigConverter.java +++ b/internal-api/src/main/java/datadog/trace/bootstrap/config/provider/ConfigConverter.java @@ -68,23 +68,17 @@ static List parseList(final String str, final String separator) { @Nonnull static Map parseMap(final String str, final String settingName) { - if (settingName.equals("trace.tags")){ - return parseTraceTagsMap(str, - settingName, - ':', Arrays.asList(',', ' ')); - } else{ - return parseMap( - str, - settingName, - ':'); + if (settingName.equals( + "trace.tags")) { // if we are parsing dd.tags, use the tags specific parser + return parseTraceTagsMap(str, settingName, ':', Arrays.asList(',', ' ')); + } else { + return parseMap(str, settingName, ':'); } } @Nonnull static Map parseMap( - final String str, - final String settingName, - final char keyValueSeparator) { + final String str, final String settingName, final char keyValueSeparator) { // 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()) { @@ -149,11 +143,7 @@ static Map parseOrderedMap(final String str, final String settin return Collections.emptyMap(); } Map map = new LinkedHashMap<>(); - loadMap( - map, - trimmed, - settingName, - ':'); + loadMap(map, trimmed, settingName, ':'); return map; } @@ -163,7 +153,6 @@ public BadFormatException(String message) { } } - private static void loadMap( Map map, String str, String settingName, char keyValueSeparator) { // we know that the str is trimmed and rely on that there is no leading/trailing whitespace @@ -233,7 +222,6 @@ private static void loadMap( } } - private static void loadTraceTagsMap( Map map, String str, diff --git a/internal-api/src/test/groovy/datadog/trace/api/ConfigTest.groovy b/internal-api/src/test/groovy/datadog/trace/api/ConfigTest.groovy index ae07c2d5cd6..3eb1c9e213b 100644 --- a/internal-api/src/test/groovy/datadog/trace/api/ConfigTest.groovy +++ b/internal-api/src/test/groovy/datadog/trace/api/ConfigTest.groovy @@ -1785,6 +1785,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") @@ -1798,7 +1811,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'] } @@ -1814,7 +1827,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'] } @@ -1830,12 +1843,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") @@ -1846,7 +1859,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'] } From bf49e2085028518101e59fc793c46ec82328b127 Mon Sep 17 00:00:00 2001 From: Matthew Li Date: Tue, 28 Jan 2025 17:12:01 -0500 Subject: [PATCH 09/13] reverting irrelevant changes --- .../src/main/java/datadog/trace/api/ConfigDefaults.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/dd-trace-api/src/main/java/datadog/trace/api/ConfigDefaults.java b/dd-trace-api/src/main/java/datadog/trace/api/ConfigDefaults.java index cd75d73c307..4eb414f3394 100644 --- a/dd-trace-api/src/main/java/datadog/trace/api/ConfigDefaults.java +++ b/dd-trace-api/src/main/java/datadog/trace/api/ConfigDefaults.java @@ -229,7 +229,7 @@ public final class ConfigDefaults { static final int DEFAULT_TELEMETRY_DEPENDENCY_RESOLUTION_QUEUE_SIZE = 100000; static final boolean DEFAULT_TRACE_128_BIT_TRACEID_GENERATION_ENABLED = true; - static final boolean DEFAULT_TRACE_128_BIT_TRACEID_LOGGING_ENABLED = true; + static final boolean DEFAULT_TRACE_128_BIT_TRACEID_LOGGING_ENABLED = false; static final boolean DEFAULT_SECURE_RANDOM = false; public static final int DEFAULT_TRACE_X_DATADOG_TAGS_MAX_LENGTH = 512; From 8bef8d051dbf13280644be1b1b8aded2c81b335b Mon Sep 17 00:00:00 2001 From: Matthew Li Date: Tue, 28 Jan 2025 17:14:13 -0500 Subject: [PATCH 10/13] cleanup --- .../src/main/java/datadog/trace/core/DDSpanContext.java | 2 -- 1 file changed, 2 deletions(-) diff --git a/dd-trace-core/src/main/java/datadog/trace/core/DDSpanContext.java b/dd-trace-core/src/main/java/datadog/trace/core/DDSpanContext.java index f086f177482..336938c7a12 100644 --- a/dd-trace-core/src/main/java/datadog/trace/core/DDSpanContext.java +++ b/dd-trace-core/src/main/java/datadog/trace/core/DDSpanContext.java @@ -768,8 +768,6 @@ void setAllTags(final Map map) { } void unsafeSetTag(final String tag, final Object value) { - // System.out.println("Tag: " + tag); - // System.out.println("Value: " + value); unsafeTags.put(tag, value); } From 5dbe9768d7082943e9db3e0283bf5ccd752b845a Mon Sep 17 00:00:00 2001 From: Matthew Li Date: Wed, 29 Jan 2025 16:24:59 -0500 Subject: [PATCH 11/13] reverting irrelevant changes --- .../java/datadog/trace/core/DDSpanContext.java | 4 +--- .../trace/core/DDSpanContextTest.groovy | 18 ------------------ 2 files changed, 1 insertion(+), 21 deletions(-) diff --git a/dd-trace-core/src/main/java/datadog/trace/core/DDSpanContext.java b/dd-trace-core/src/main/java/datadog/trace/core/DDSpanContext.java index 336938c7a12..fcbf046122a 100644 --- a/dd-trace-core/src/main/java/datadog/trace/core/DDSpanContext.java +++ b/dd-trace-core/src/main/java/datadog/trace/core/DDSpanContext.java @@ -367,9 +367,7 @@ public DDSpanContext( propagationTags != null ? propagationTags : traceCollector.getTracer().getPropagationTagsFactory().empty(); - if (parentId == 0) { // higher bits of traceID are only used in root span - this.propagationTags.updateTraceIdHighOrderBits(this.traceId.toHighOrderLong()); - } + this.propagationTags.updateTraceIdHighOrderBits(this.traceId.toHighOrderLong()); this.injectBaggageAsTags = injectBaggageAsTags; if (origin != null) { setOrigin(origin); diff --git a/dd-trace-core/src/test/groovy/datadog/trace/core/DDSpanContextTest.groovy b/dd-trace-core/src/test/groovy/datadog/trace/core/DDSpanContextTest.groovy index e2b9bb95276..c5b98058558 100644 --- a/dd-trace-core/src/test/groovy/datadog/trace/core/DDSpanContextTest.groovy +++ b/dd-trace-core/src/test/groovy/datadog/trace/core/DDSpanContextTest.groovy @@ -286,24 +286,6 @@ class DDSpanContextTest extends DDCoreSpecification { span.context.encodedResourceName == -2 } - def "higher order bits of 128bit traceID is only set as PTag in root span"() { - when: - def parent = tracer.buildSpan("fakeOperation") - .withServiceName("fakeService") - .withResourceName("fakeResource") - .start() - - def child = tracer.buildSpan("fakeOperation") - .withServiceName("fakeService") - .withResourceName("fakeResource") - .asChildOf(parent) - .start() - - then: - parent.context().getPropagationTags().getTraceIdHighOrderBits() != 0 - // child.context().getPropagationTags().getTraceIdHighOrderBits() == 0 - } - private static String dataTag(String tag) { "_dd.${tag}.json" } From 798b8d8c164092a56d608940fa6b997e08129744 Mon Sep 17 00:00:00 2001 From: Matthew Li Date: Thu, 30 Jan 2025 13:39:01 -0500 Subject: [PATCH 12/13] cleanup again --- .../src/test/groovy/datadog/trace/core/DDSpanContextTest.groovy | 1 - 1 file changed, 1 deletion(-) diff --git a/dd-trace-core/src/test/groovy/datadog/trace/core/DDSpanContextTest.groovy b/dd-trace-core/src/test/groovy/datadog/trace/core/DDSpanContextTest.groovy index c5b98058558..c880c17d94c 100644 --- a/dd-trace-core/src/test/groovy/datadog/trace/core/DDSpanContextTest.groovy +++ b/dd-trace-core/src/test/groovy/datadog/trace/core/DDSpanContextTest.groovy @@ -23,7 +23,6 @@ class DDSpanContextTest extends DDCoreSpecification { def profilingContextIntegration def setup() { - injectSysConfig("trace.128.bit.traceid.generation.enabled", "true") writer = new ListWriter() profilingContextIntegration = Mock(ProfilingContextIntegration) tracer = tracerBuilder().writer(writer) From 32e9861d17cb3951014fdd511c0d41d06b50ca48 Mon Sep 17 00:00:00 2001 From: Matthew Li Date: Tue, 4 Feb 2025 13:33:39 -0500 Subject: [PATCH 13/13] adding edge cases tests and creating specific tags mergedtags function --- .../main/java/datadog/trace/api/Config.java | 2 +- .../config/provider/ConfigConverter.java | 7 +----- .../config/provider/ConfigProvider.java | 23 +++++++++++++++++++ .../provider/ConfigConverterTest.groovy | 7 +++++- 4 files changed, 31 insertions(+), 8 deletions(-) diff --git a/internal-api/src/main/java/datadog/trace/api/Config.java b/internal-api/src/main/java/datadog/trace/api/Config.java index 6a54fb9808a..cfc3abe421f 100644 --- a/internal-api/src/main/java/datadog/trace/api/Config.java +++ b/internal-api/src/main/java/datadog/trace/api/Config.java @@ -759,7 +759,7 @@ private Config(final ConfigProvider configProvider, final InstrumenterConfig ins { final Map tags = new HashMap<>(configProvider.getMergedMap(GLOBAL_TAGS)); - tags.putAll(configProvider.getMergedMap(TRACE_TAGS, TAGS)); + tags.putAll(configProvider.getMergedTagsMap(TRACE_TAGS, TAGS)); if (serviceNameSetByUser) { // prioritize service name set by DD_SERVICE over DD_TAGS config tags.remove("service"); } diff --git a/internal-api/src/main/java/datadog/trace/bootstrap/config/provider/ConfigConverter.java b/internal-api/src/main/java/datadog/trace/bootstrap/config/provider/ConfigConverter.java index cd289a71a87..8a869ef9675 100644 --- a/internal-api/src/main/java/datadog/trace/bootstrap/config/provider/ConfigConverter.java +++ b/internal-api/src/main/java/datadog/trace/bootstrap/config/provider/ConfigConverter.java @@ -68,12 +68,7 @@ static List parseList(final String str, final String separator) { @Nonnull static Map parseMap(final String str, final String settingName) { - if (settingName.equals( - "trace.tags")) { // if we are parsing dd.tags, use the tags specific parser - return parseTraceTagsMap(str, settingName, ':', Arrays.asList(',', ' ')); - } else { - return parseMap(str, settingName, ':'); - } + return parseMap(str, settingName, ':'); } @Nonnull diff --git a/internal-api/src/main/java/datadog/trace/bootstrap/config/provider/ConfigProvider.java b/internal-api/src/main/java/datadog/trace/bootstrap/config/provider/ConfigProvider.java index d6418a56fef..89c41531642 100644 --- a/internal-api/src/main/java/datadog/trace/bootstrap/config/provider/ConfigProvider.java +++ b/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 getMergedMap(String key, String... aliases) { return merged; } + public Map getMergedTagsMap(String key, String... aliases) { + Map 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 parsedMap = + ConfigConverter.parseTraceTagsMap(value, key, ':', Arrays.asList(',', ' ')); + if (!parsedMap.isEmpty()) { + origin = sources[i].origin(); + } + merged.putAll(parsedMap); + } + if (collectConfig) { + ConfigCollector.get().put(key, merged, origin); + } + return merged; + } + public Map getOrderedMap(String key) { LinkedHashMap merged = new LinkedHashMap<>(); ConfigOrigin origin = ConfigOrigin.DEFAULT; diff --git a/internal-api/src/test/groovy/datadog/trace/bootstrap/config/provider/ConfigConverterTest.groovy b/internal-api/src/test/groovy/datadog/trace/bootstrap/config/provider/ConfigConverterTest.groovy index 47d32081914..ac73fd7ece4 100644 --- a/internal-api/src/test/groovy/datadog/trace/bootstrap/config/provider/ConfigConverterTest.groovy +++ b/internal-api/src/test/groovy/datadog/trace/bootstrap/config/provider/ConfigConverterTest.groovy @@ -102,7 +102,7 @@ class ConfigConverterTest extends DDSpecification { def "parsing map #mapString with List of arg separators for with key value separator #separator"() { //testing parsing for DD_TAGS setup: - def separatorList = [','.charAt(0), ' '.charAt(0)] + def separatorList = [',' as char, ' ' as char] when: def result = ConfigConverter.parseTraceTagsMap(mapString, "test", separator as char, separatorList as List) @@ -125,6 +125,11 @@ class ConfigConverterTest extends DDSpecification { "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 }