Skip to content

Stable Config improvements #9259

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.

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

Merged
merged 31 commits into from
Aug 15, 2025
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
31 commits
Select commit Hold shift + click to select a range
370e894
Refactor matchOperator to use method reference comparator for efficiency
mtoffl01 Jul 28, 2025
df4da2f
Implement better exception handling and debug logging for StableConfi…
mtoffl01 Jul 28, 2025
7049d36
Add a debug log about no rules getting applied
mtoffl01 Jul 28, 2025
e730cbe
Improve test file logic and cleanup in StableConfigSourceTest class
mtoffl01 Jul 29, 2025
5b639aa
Github suggestions: Use static factory methods as constructors for R…
mtoffl01 Jul 31, 2025
85d11cf
Improve exception message: include value of object that caused the fa…
mtoffl01 Aug 1, 2025
6ab4b6c
Add more test cases to StableConfigSourceTest to improve codecov
mtoffl01 Aug 1, 2025
007f58a
Add test coverage for StableConfigMappingException
mtoffl01 Aug 1, 2025
ac69f2d
Merge branch 'master' into mtoff/scfg-improvements
mtoffl01 Aug 4, 2025
473c59b
Update internal-api/src/main/java/datadog/trace/bootstrap/config/prov…
mtoffl01 Aug 8, 2025
feb1ba0
Update internal-api/src/main/java/datadog/trace/bootstrap/config/prov…
mtoffl01 Aug 8, 2025
f93546a
Use safeToString when printing stableconfig map
mtoffl01 Aug 8, 2025
780aee3
rearrange logic in selectors creation for better readability
mtoffl01 Aug 8, 2025
ac5e05a
delete superfluous else
mtoffl01 Aug 8, 2025
bcebbcb
Use constant to represent max length in safeToString
mtoffl01 Aug 8, 2025
0baccfd
use throwStableConfigMappingException helper function
mtoffl01 Aug 11, 2025
957d9c9
refactor exception catching in StableConfigSource
mtoffl01 Aug 11, 2025
b9870bd
restore semicolon in calls to throwStableConfigMappingException
mtoffl01 Aug 11, 2025
5019584
Use static import for throwStableConfigMappingException
mtoffl01 Aug 11, 2025
e8d6e8f
test other kinds of exceptions in StableConfigSource
mtoffl01 Aug 11, 2025
1fefb17
remove duplicate cfg assignments
mtoffl01 Aug 11, 2025
058cbda
fix missing operator test case
mtoffl01 Aug 12, 2025
5460c8a
Update internal-api/src/main/java/datadog/trace/bootstrap/config/prov…
mtoffl01 Aug 12, 2025
37da711
Improve readability of StableConfigMappingException and its function …
mtoffl01 Aug 12, 2025
5dac7f8
modify safeToString logic to print first 50 and last 50 chars
mtoffl01 Aug 12, 2025
5104ac5
Merge branch 'mtoff/scfg-improvements' of github.com:DataDog/dd-trace…
mtoffl01 Aug 12, 2025
13bef8d
remove extra placeholder in string message when printing exception
mtoffl01 Aug 12, 2025
2706bca
Update internal-api/src/main/java/datadog/trace/bootstrap/config/prov…
mtoffl01 Aug 12, 2025
4dde216
refactor Rules.from to perform type assertions before heavyweight lis…
mtoffl01 Aug 14, 2025
bfd5d7b
Merge branch 'mtoff/scfg-improvements' of github.com:DataDog/dd-trace…
mtoffl01 Aug 14, 2025
16d5f36
run spotless
mtoffl01 Aug 15, 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
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@
import java.util.List;
import java.util.Locale;
import java.util.Map;
import java.util.function.Predicate;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;

Expand Down Expand Up @@ -83,7 +84,6 @@ public static StableConfigSource.StableConfig parse(String filePath) throws IOEx
if (configId != null) {
return new StableConfigSource.StableConfig(configId, Collections.emptyMap());
}

} catch (IOException e) {
log.debug("Failed to read the stable configuration file: {}", filePath, e);
}
Expand Down Expand Up @@ -115,22 +115,32 @@ private static boolean matchOperator(String value, String operator, List<String>
return false;
}
value = value.toLowerCase(Locale.ROOT);

Predicate<String> comparator;
switch (operator) {
case "equals":
comparator = value::equals;
break;
case "starts_with":
comparator = value::startsWith;
break;
case "ends_with":
comparator = value::endsWith;
break;
case "contains":
comparator = value::contains;
break;
default:
return false;
}

for (String match : matches) {
if (match == null) {
continue;
}
match = match.toLowerCase(Locale.ROOT);
switch (operator) {
case "equals":
return value.equals(match);
case "starts_with":
return value.startsWith(match);
case "ends_with":
return value.endsWith(match);
case "contains":
return value.contains(match);
default:
return false;
if (comparator.test(match)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

👍

return true;
}
}
return false;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
import static datadog.trace.util.Strings.propertyNameToEnvironmentVariableName;

import datadog.trace.api.ConfigOrigin;
import datadog.trace.bootstrap.config.provider.stableconfig.StableConfigMappingException;
import java.io.File;
import java.util.Collections;
import java.util.Map;
Expand Down Expand Up @@ -37,11 +38,21 @@ public final class StableConfigSource extends ConfigProvider.Source {
try {
log.debug("Stable configuration file found at path: {}", file);
cfg = StableConfigParser.parse(filePath);
} catch (Throwable e) {
log.warn(
"Encountered the following exception when attempting to read stable configuration file at path: {}, dropping configs.",
file,
e);
} catch (StableConfigMappingException
| IllegalArgumentException
| ClassCastException
| NullPointerException e) {
log.warn("YAML mapping error in stable configuration file {}: {}", filePath, e.getMessage());
cfg = StableConfig.EMPTY;
} catch (Exception e) {
if (log.isDebugEnabled()) {
log.error("Unexpected error while reading stable configuration file {}: {}", filePath, e);
} else {
log.error(
"Unexpected error while reading stable configuration file {}: {}",
filePath,
e.getMessage());
}
cfg = StableConfig.EMPTY;
}
this.config = cfg;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -26,12 +26,34 @@ public Rule(List<Selector> selectors, Map<String, Object> configuration) {
}

public Rule(Object yaml) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Let me know if this constructor is too hard to read and if you recommend I move the "require fields" logic into helper functions (in a utils class to be used by both Rule and Selector?)

Copy link
Contributor

Choose a reason for hiding this comment

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

Overall, I would rather try to bring type safety to the Java API as soon as possible even if SnakeYaml is returning loose Object.

An idiomatic Java would be to have a static builder like static Rule from(Map<?, ?> rules) and check that its parameters is the right time before calling it.
-- Note that a better type for rules would be Map<String, ?> but there is no way to enforce / test it due to type erasure in Java.

So far example, I would check the type ahead in StableConfig before trying to create rule using its fromMap:

this.apmConfigurationRules = parseRules(yaml);

// with some helper methods in StableConfig

private List<Rule> parseRules(Map<?, ?> yaml) {
    Object apmConfigurationDefaultObject = map.get("apm_configuration_default");
    if (apmConfigurationDefaultObject instanceof List) {
      return ((List<?>) apmConfigurationDefaultObject).stream()
          .filter(r -> r instanceof Map)
          .map(r -> (Map<?, ?>) r)
          .map(Rule::fromMap)
          .collect(toList());
    } else {
      return emptyList();
    }
  }

if (!(yaml instanceof Map)) {
throw new StableConfigMappingException(
"Rule must be a map, but got: " + yaml.getClass().getSimpleName());
}
Map map = (Map) yaml;

Object selectorsObj = map.get("selectors");
if (selectorsObj == null) {
throw new StableConfigMappingException("Missing 'selectors' in rule: " + map);
}
if (!(selectorsObj instanceof List)) {
throw new StableConfigMappingException(
"'selectors' must be a list, but got: " + selectorsObj.getClass().getSimpleName());
}
selectors =
unmodifiableList(
((List<Object>) map.get("selectors"))
((List<Object>) selectorsObj)
.stream().filter(Objects::nonNull).map(Selector::new).collect(toList()));
configuration = unmodifiableMap((Map<String, Object>) map.get("configuration"));

Object configObj = map.get("configuration");
if (configObj == null) {
throw new StableConfigMappingException("Missing 'configuration' in rule: " + map);
}
if (!(configObj instanceof Map)) {
throw new StableConfigMappingException(
"'configuration' must be a map, but got: " + configObj.getClass().getSimpleName());
}
configuration = unmodifiableMap((Map<String, Object>) configObj);
}

public List<Selector> getSelectors() {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -18,13 +18,43 @@ public Selector(String origin, String key, List<String> matches, String operator
}

public Selector(Object yaml) {
if (!(yaml instanceof Map)) {
throw new StableConfigMappingException(
"Selector must be a map, but got: " + yaml.getClass().getSimpleName());
}
Map map = (Map) yaml;
origin = (String) map.get("origin");
key = (String) map.get("key");
List<String> rawMatches = (List<String>) map.get("matches");

Object originObj = map.get("origin");
if (originObj == null) {
throw new StableConfigMappingException("Missing 'origin' in selector: " + map);
}
if (!(originObj instanceof String)) {
throw new StableConfigMappingException(
"'origin' must be a string, but got: " + originObj.getClass().getSimpleName());
}
origin = (String) originObj;

Object keyObj = map.get("key");
key = (keyObj instanceof String) ? (String) keyObj : null;

Object matchesObj = map.get("matches");
if (matchesObj != null && !(matchesObj instanceof List)) {
throw new StableConfigMappingException(
"'matches' must be a list, but got: " + matchesObj.getClass().getSimpleName());
}
List<String> rawMatches = (List<String>) matchesObj;
matches =
rawMatches != null ? Collections.unmodifiableList(rawMatches) : Collections.emptyList();
operator = (String) map.get("operator");

Object operatorObj = map.get("operator");
if (operatorObj == null) {
throw new StableConfigMappingException("Missing 'operator' in selector: " + map);
}
if (!(operatorObj instanceof String)) {
throw new StableConfigMappingException(
"'operator' must be a string, but got: " + operatorObj.getClass().getSimpleName());
}
operator = (String) operatorObj;
}

public String getOrigin() {
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
package datadog.trace.bootstrap.config.provider.stableconfig;

public class StableConfigMappingException extends RuntimeException {
public StableConfigMappingException(String message) {
super(message);
}

public StableConfigMappingException(String message, Throwable cause) {
super(message, cause);
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,10 @@ import datadog.trace.bootstrap.config.provider.stableconfig.StableConfig
import datadog.trace.test.util.DDSpecification
import org.snakeyaml.engine.v2.api.Dump
import org.snakeyaml.engine.v2.api.DumpSettings
import ch.qos.logback.classic.Logger
import ch.qos.logback.classic.spi.ILoggingEvent
import ch.qos.logback.core.read.ListAppender
import org.slf4j.LoggerFactory

import java.nio.file.Files
import java.nio.file.Path
Expand Down Expand Up @@ -108,6 +112,78 @@ class StableConfigSourceTest extends DDSpecification {
"12345" | ["DD_KEY_ONE": "one", "DD_KEY_TWO": "two"] | Arrays.asList(sampleMatchingRule, sampleNonMatchingRule)
}

def "test parse invalid logs mapping errors"() {
given:
Logger logbackLogger = (Logger) LoggerFactory.getLogger(datadog.trace.bootstrap.config.provider.StableConfigSource)
def listAppender = new ListAppender<ILoggingEvent>()
listAppender.start()
logbackLogger.addAppender(listAppender)

def tempFile = File.createTempFile("testFile_", ".yaml")
tempFile.text = yaml

when:
def stableCfg = new StableConfigSource(tempFile.absolutePath, ConfigOrigin.LOCAL_STABLE_CONFIG)

then:
stableCfg.config == StableConfigSource.StableConfig.EMPTY
def warnLogs = listAppender.list.findAll { it.level.toString() == 'WARN' }
warnLogs.any { it.formattedMessage.contains(expectedLogSubstring) }

cleanup:
tempFile.delete()
logbackLogger.detachAppender(listAppender)

where:
yaml | expectedLogSubstring
// Missing 'origin' in selector
'''apm_configuration_rules:
- selectors:
- key: "someKey"
matches: ["someValue"]
operator: equals
configuration:
DD_SERVICE: "test"
''' | "Missing 'origin' in selector"
// Missing 'configuration' in rule
'''apm_configuration_rules:
- selectors:
- origin: process_arguments
key: "-Dfoo"
matches: ["bar"]
operator: equals
''' | "Missing 'configuration' in rule"
// Missing 'selectors' in rule
'''apm_configuration_rules:
- configuration:
DD_SERVICE: "test"
''' | "Missing 'selectors' in rule"
// Triggers ClassCastException (selectors should be a list, not a string)
'''apm_configuration_rules:
- selectors: "not-a-list"
configuration:
DD_SERVICE: "test"
''' | "'selectors' must be a list, but got: String"
// configuration present but not a map
'''apm_configuration_rules:
- selectors:
- origin: process_arguments
key: "-Dfoo"
matches: ["bar"]
operator: equals
configuration: "not-a-map"
''' | "'configuration' must be a map, but got: String"
// configuration present but not a map (integer)
'''apm_configuration_rules:
- selectors:
- origin: process_arguments
key: "-Dfoo"
matches: ["bar"]
operator: equals
configuration: 12345
''' | "'configuration' must be a map, but got: Integer"
}

// Corrupt YAML string variable used for testing, defined outside the 'where' block for readability
def static corruptYaml = '''
abc: 123
Expand Down
Loading