Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add new parser for DD_TAGS and prioritizing DD_SERVICE #8296

Open
wants to merge 13 commits into
base: master
Choose a base branch
from
Open

Add new parser for DD_TAGS and prioritizing DD_SERVICE #8296

Show file tree
Hide file tree
Changes from 12 commits
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
04cc577
testing
mhlidd Jan 16, 2025
5cbca2c
cleanup
mhlidd Jan 16, 2025
96be6c8
first iteration on parser
mhlidd Jan 27, 2025
79e12ea
initial working parsing for DD_TAGS
mhlidd Jan 27, 2025
8c6cbaa
cleanup part 1
mhlidd Jan 27, 2025
f31e8e8
cleanup pt 2
mhlidd Jan 27, 2025
61d1d4d
adding traceTagsParser as separate parser due to different rule
mhlidd Jan 27, 2025
dee14ce
dd_service override
mhlidd Jan 28, 2025
bf49e20
reverting irrelevant changes
mhlidd Jan 28, 2025
8bef8d0
cleanup
mhlidd Jan 28, 2025
5dbe976
reverting irrelevant changes
mhlidd Jan 29, 2025
798b8d8
cleanup again
mhlidd Jan 30, 2025
32e9861
adding edge cases tests and creating specific tags mergedtags function
mhlidd Feb 4, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions internal-api/src/main/java/datadog/trace/api/Config.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -760,6 +760,9 @@ 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 (serviceNameSetByUser) { // prioritize service name set by DD_SERVICE over DD_TAGS config
tags.remove("service");
mhlidd marked this conversation as resolved.
Show resolved Hide resolved
}
this.tags = getMapWithPropertiesDefinedByEnvironment(tags, ENV, VERSION);
}

Expand Down
78 changes: 77 additions & 1 deletion internal-api/src/main/java/datadog/trace/bootstrap/config/provider/ConfigConverter.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -68,7 +68,12 @@ static List<String> parseList(final String str, final String separator) {

@Nonnull
static Map<String, String> parseMap(final String str, final String settingName) {
return parseMap(str, settingName, ':');
if (settingName.equals(
"trace.tags")) { // if we are parsing dd.tags, use the tags specific parser
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think putting tag name conditional logic in this method is not durable. Instead, what about just calling parseTraceTagsMap from the config class where those tags are extracted?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This would involve creating a new getMergedMap to let trace.tags take precedence over tags. This seems a little repetitive to me, but I agree that the conditional logic used currently isn't the best. I'm wondering if there is an alternate solution for this? WDYT?

return parseTraceTagsMap(str, settingName, ':', Arrays.asList(',', ' '));
} else {
return parseMap(str, settingName, ':');
}
}

@Nonnull
Expand All @@ -84,6 +89,22 @@ static Map<String, String> parseMap(
return map;
}

@Nonnull
static Map<String, String> parseTraceTagsMap(
final String str,
final String settingName,
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, settingName, 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
Expand Down Expand Up @@ -201,6 +222,61 @@ private static void loadMap(
}
}

private static void loadTraceTagsMap(
Map<String, String> map,
String str,
String settingName,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

that argument does not look used in that method

char keyValueSeparator,
final List<Character> 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;
Copy link
Collaborator

@amarziali amarziali Feb 3, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If I understood well the code I think you should retain the separator that comes first (the smaller indexof) and you should'n break here. For instance if you have mykeys abc,d the separator used is space but if the method is called with argSeparator = [',', ' '] This code will detect the comma as the separator while it does not look so.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The intended behavior is that if a comma exists at all within the input, we would want to split by comma, regardless of which separator comes first. That is why we iterate through an ordered list of separators that are ordered by priority of which separator we'd rather use.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it looks confusing since the space is allowed and nothing prevent to a value to contain a comma

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm I'm not sure what you mean here. This loop here is solely to determine whether we want to separate by comma or separate by space. When we iterate through the input, we take each substring (separated by the argSeparator) and then check for K/V then, meaning that the potential value would stop at the argSeparator. Let me know if that makes sense or if you have more questions!

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you run that code like this: loadTraceTagsMap(map, "k1=v1 k2=v2,v3", '=', List.of(',', ' ')); the result is {k1=v1 k2=v2, v3=}
The separator used is , but the first one would have been a space.
The comment in the code says: // find the first instance of the first possible separator.
I perhaps misunderstood, but the first possible would have been a space and not a comma.

}
}
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,
Expand Down
23 changes: 18 additions & 5 deletions internal-api/src/test/groovy/datadog/trace/api/ConfigTest.groovy
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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")
Expand All @@ -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']
}
Expand All @@ -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']
}
Expand All @@ -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")
Expand All @@ -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']
}
Expand Down
29 changes: 29 additions & 0 deletions ...al-api/src/test/groovy/datadog/trace/bootstrap/config/provider/ConfigConverterTest.groovy
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -99,6 +99,35 @@ 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 = [','.charAt(0), ' '.charAt(0)]
mhlidd marked this conversation as resolved.
Show resolved Hide resolved

when:
def result = ConfigConverter.parseTraceTagsMap(mapString, "test", separator as char, separatorList as List<Character>)

then:
result == expected

where:
// spotless:off
mapString | separator | expected
amarziali marked this conversation as resolved.
Show resolved Hide resolved
"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
}

def "test parseMapWithOptionalMappings"() {
when:
def result = ConfigConverter.parseMapWithOptionalMappings(mapString, "test", "", lowercaseKeys)
Expand Down
Loading