Skip to content

Commit 1d77102

Browse files
mtoffl01AlexeyKuznetsov-DD
mtoffl01
and
AlexeyKuznetsov-DD
authored
Stable Config improvements (#9259)
* Refactor matchOperator to use method reference comparator for efficiency * Implement better exception handling and debug logging for StableConfig parsing errors * Add a debug log about no rules getting applied * Improve test file logic and cleanup in StableConfigSourceTest class * Github suggestions: Use static factory methods as constructors for Rule and Selector classes * Improve exception message: include value of object that caused the failure * Add more test cases to StableConfigSourceTest to improve codecov * Add test coverage for StableConfigMappingException * Update internal-api/src/main/java/datadog/trace/bootstrap/config/provider/stableconfig/Rule.java Co-authored-by: Alexey Kuznetsov <alexey.kuznetsov@datadoghq.com> * Update internal-api/src/main/java/datadog/trace/bootstrap/config/provider/stableconfig/Rule.java Co-authored-by: Alexey Kuznetsov <alexey.kuznetsov@datadoghq.com> * Use safeToString when printing stableconfig map * rearrange logic in selectors creation for better readability * delete superfluous else * Use constant to represent max length in safeToString * use throwStableConfigMappingException helper function * refactor exception catching in StableConfigSource * restore semicolon in calls to throwStableConfigMappingException * Use static import for throwStableConfigMappingException * test other kinds of exceptions in StableConfigSource * remove duplicate cfg assignments * fix missing operator test case * Update internal-api/src/main/java/datadog/trace/bootstrap/config/provider/stableconfig/Rule.java Co-authored-by: Alexey Kuznetsov <alexey.kuznetsov@datadoghq.com> * Improve readability of StableConfigMappingException and its function invocations * modify safeToString logic to print first 50 and last 50 chars * remove extra placeholder in string message when printing exception * Update internal-api/src/main/java/datadog/trace/bootstrap/config/provider/StableConfigSource.java Co-authored-by: Alexey Kuznetsov <alexey.kuznetsov@datadoghq.com> * refactor Rules.from to perform type assertions before heavyweight list manipulation * run spotless --------- Co-authored-by: Alexey Kuznetsov <alexey.kuznetsov@datadoghq.com>
1 parent 65b2349 commit 1d77102

File tree

8 files changed

+338
-45
lines changed

8 files changed

+338
-45
lines changed

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

Lines changed: 23 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -16,6 +16,7 @@
1616
import java.util.List;
1717
import java.util.Locale;
1818
import java.util.Map;
19+
import java.util.function.Predicate;
1920
import org.slf4j.Logger;
2021
import org.slf4j.LoggerFactory;
2122

@@ -75,6 +76,7 @@ public static StableConfigSource.StableConfig parse(String filePath) throws IOEx
7576
return new StableConfigSource.StableConfig(configId, mergedConfigMap);
7677
}
7778
}
79+
log.debug("No matching rule found in stable configuration file {}", filePath);
7880
}
7981
// If configs were found in apm_configuration_default, use them
8082
if (!configMap.isEmpty()) {
@@ -85,7 +87,6 @@ public static StableConfigSource.StableConfig parse(String filePath) throws IOEx
8587
if (configId != null) {
8688
return new StableConfigSource.StableConfig(configId, Collections.emptyMap());
8789
}
88-
8990
} catch (IOException e) {
9091
log.debug("Failed to read the stable configuration file: {}", filePath, e);
9192
}
@@ -117,22 +118,32 @@ private static boolean matchOperator(String value, String operator, List<String>
117118
return false;
118119
}
119120
value = value.toLowerCase(Locale.ROOT);
121+
122+
Predicate<String> comparator;
123+
switch (operator) {
124+
case "equals":
125+
comparator = value::equals;
126+
break;
127+
case "starts_with":
128+
comparator = value::startsWith;
129+
break;
130+
case "ends_with":
131+
comparator = value::endsWith;
132+
break;
133+
case "contains":
134+
comparator = value::contains;
135+
break;
136+
default:
137+
return false;
138+
}
139+
120140
for (String match : matches) {
121141
if (match == null) {
122142
continue;
123143
}
124144
match = match.toLowerCase(Locale.ROOT);
125-
switch (operator) {
126-
case "equals":
127-
return value.equals(match);
128-
case "starts_with":
129-
return value.startsWith(match);
130-
case "ends_with":
131-
return value.endsWith(match);
132-
case "contains":
133-
return value.contains(match);
134-
default:
135-
return false;
145+
if (comparator.test(match)) {
146+
return true;
136147
}
137148
}
138149
return false;

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

Lines changed: 18 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -3,6 +3,7 @@
33
import static datadog.trace.util.Strings.propertyNameToEnvironmentVariableName;
44

55
import datadog.trace.api.ConfigOrigin;
6+
import datadog.trace.bootstrap.config.provider.stableconfig.StableConfigMappingException;
67
import java.io.File;
78
import java.util.Collections;
89
import java.util.Map;
@@ -38,10 +39,23 @@ public final class StableConfigSource extends ConfigProvider.Source {
3839
log.debug("Stable configuration file found at path: {}", file);
3940
cfg = StableConfigParser.parse(filePath);
4041
} catch (Throwable e) {
41-
log.warn(
42-
"Encountered the following exception when attempting to read stable configuration file at path: {}, dropping configs.",
43-
file,
44-
e);
42+
if (e instanceof StableConfigMappingException
43+
|| e instanceof IllegalArgumentException
44+
|| e instanceof ClassCastException
45+
|| e instanceof NullPointerException) {
46+
log.warn(
47+
"YAML mapping error in stable configuration file: {}, error: {}",
48+
filePath,
49+
e.getMessage());
50+
} else if (log.isDebugEnabled()) {
51+
log.error("Unexpected error while reading stable configuration file: {}", filePath, e);
52+
} else {
53+
log.error(
54+
"Unexpected error while reading stable configuration file: {}, error: {}",
55+
filePath,
56+
e.getMessage());
57+
}
58+
4559
cfg = StableConfig.EMPTY;
4660
}
4761
this.config = cfg;

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

Lines changed: 42 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,6 @@
11
package datadog.trace.bootstrap.config.provider.stableconfig;
22

3+
import static datadog.trace.bootstrap.config.provider.stableconfig.StableConfigMappingException.throwStableConfigMappingException;
34
import static java.util.Collections.*;
45
import static java.util.stream.Collectors.toList;
56

@@ -25,13 +26,47 @@ public Rule(List<Selector> selectors, Map<String, Object> configuration) {
2526
this.configuration = configuration;
2627
}
2728

28-
public Rule(Object yaml) {
29-
Map map = (Map) yaml;
30-
selectors =
31-
unmodifiableList(
32-
((List<Object>) map.get("selectors"))
33-
.stream().filter(Objects::nonNull).map(Selector::new).collect(toList()));
34-
configuration = unmodifiableMap((Map<String, Object>) map.get("configuration"));
29+
public static Rule from(Map<?, ?> map) {
30+
Object selectorsObj = map.get("selectors");
31+
if (selectorsObj == null) {
32+
throwStableConfigMappingException("Missing 'selectors' in rule:", map);
33+
}
34+
35+
if (!(selectorsObj instanceof List)) {
36+
throwStableConfigMappingException(
37+
"'selectors' must be a list, but got: " + selectorsObj.getClass().getSimpleName() + ": ",
38+
selectorsObj);
39+
}
40+
41+
Object configObj = map.get("configuration");
42+
if (configObj == null) {
43+
throwStableConfigMappingException("Missing 'configuration' in rule:", map);
44+
}
45+
if (!(configObj instanceof Map)) {
46+
throwStableConfigMappingException(
47+
"'configuration' must be a map, but got: " + configObj.getClass().getSimpleName() + ": ",
48+
configObj);
49+
}
50+
51+
List<Selector> selectors =
52+
((List<?>) selectorsObj)
53+
.stream()
54+
.filter(Objects::nonNull)
55+
.map(
56+
s -> {
57+
if (!(s instanceof Map)) {
58+
throwStableConfigMappingException(
59+
"Each selector must be a map, but got: "
60+
+ s.getClass().getSimpleName()
61+
+ ": ",
62+
s);
63+
}
64+
65+
return Selector.from((Map<?, ?>) s);
66+
})
67+
.collect(toList());
68+
69+
return new Rule(unmodifiableList(selectors), (Map<String, Object>) configObj);
3570
}
3671

3772
public List<Selector> getSelectors() {

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

Lines changed: 38 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,7 @@
11
package datadog.trace.bootstrap.config.provider.stableconfig;
22

3+
import static datadog.trace.bootstrap.config.provider.stableconfig.StableConfigMappingException.throwStableConfigMappingException;
4+
35
import java.util.Collections;
46
import java.util.List;
57
import java.util.Map;
@@ -17,14 +19,43 @@ public Selector(String origin, String key, List<String> matches, String operator
1719
this.operator = operator;
1820
}
1921

20-
public Selector(Object yaml) {
21-
Map map = (Map) yaml;
22-
origin = (String) map.get("origin");
23-
key = (String) map.get("key");
24-
List<String> rawMatches = (List<String>) map.get("matches");
25-
matches =
22+
public static Selector from(Map<?, ?> map) {
23+
Object originObj = map.get("origin");
24+
if (originObj == null) {
25+
throwStableConfigMappingException("Missing 'origin' in selector:", map);
26+
}
27+
if (!(originObj instanceof String)) {
28+
throwStableConfigMappingException(
29+
"'origin' must be a string, but got: " + originObj.getClass().getSimpleName() + ": ",
30+
originObj);
31+
}
32+
String origin = (String) originObj;
33+
34+
Object keyObj = map.get("key");
35+
String key = (keyObj instanceof String) ? (String) keyObj : null;
36+
37+
Object matchesObj = map.get("matches");
38+
if (matchesObj != null && !(matchesObj instanceof List)) {
39+
throwStableConfigMappingException(
40+
"'matches' must be a list, but got: " + matchesObj.getClass().getSimpleName() + ": ",
41+
matchesObj);
42+
}
43+
List<String> rawMatches = (List<String>) matchesObj;
44+
List<String> matches =
2645
rawMatches != null ? Collections.unmodifiableList(rawMatches) : Collections.emptyList();
27-
operator = (String) map.get("operator");
46+
47+
Object operatorObj = map.get("operator");
48+
if (operatorObj == null) {
49+
throwStableConfigMappingException("Missing 'operator' in selector:", map);
50+
}
51+
if (!(operatorObj instanceof String)) {
52+
throwStableConfigMappingException(
53+
"'operator' must be a string, but got: " + operatorObj.getClass().getSimpleName() + ": ",
54+
operatorObj);
55+
}
56+
String operator = (String) operatorObj;
57+
58+
return new Selector(origin, key, matches, operator);
2859
}
2960

3061
public String getOrigin() {

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

Lines changed: 21 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -1,10 +1,10 @@
11
package datadog.trace.bootstrap.config.provider.stableconfig;
22

3+
import static datadog.trace.bootstrap.config.provider.stableconfig.StableConfigMappingException.throwStableConfigMappingException;
34
import static java.util.Collections.emptyList;
45
import static java.util.Collections.emptyMap;
56
import static java.util.Collections.unmodifiableList;
67
import static java.util.Collections.unmodifiableMap;
7-
import static java.util.stream.Collectors.toList;
88

99
import java.util.ArrayList;
1010
import java.util.List;
@@ -21,10 +21,7 @@ public StableConfig(Object yaml) {
2121
this.apmConfigurationDefault =
2222
unmodifiableMap(
2323
(Map<String, Object>) map.getOrDefault("apm_configuration_default", emptyMap()));
24-
this.apmConfigurationRules =
25-
unmodifiableList(
26-
((List<Object>) map.getOrDefault("apm_configuration_rules", emptyList()))
27-
.stream().map(Rule::new).collect(toList()));
24+
this.apmConfigurationRules = parseRules(map);
2825
}
2926

3027
// test only
@@ -45,4 +42,23 @@ public Map<String, Object> getApmConfigurationDefault() {
4542
public List<Rule> getApmConfigurationRules() {
4643
return apmConfigurationRules;
4744
}
45+
46+
private List<Rule> parseRules(Map<?, ?> map) {
47+
Object rulesObj = map.get("apm_configuration_rules");
48+
if (rulesObj instanceof List) {
49+
List<?> rulesList = (List<?>) rulesObj;
50+
List<Rule> rules = new ArrayList<>();
51+
for (Object ruleObj : rulesList) {
52+
if (ruleObj instanceof Map) {
53+
rules.add(Rule.from((Map<?, ?>) ruleObj));
54+
} else {
55+
throwStableConfigMappingException(
56+
"Rule must be a map, but got: " + ruleObj.getClass().getSimpleName() + ": ", ruleObj);
57+
return emptyList();
58+
}
59+
}
60+
return unmodifiableList(rules);
61+
}
62+
return emptyList();
63+
}
4864
}

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

Lines changed: 40 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,40 @@
1+
package datadog.trace.bootstrap.config.provider.stableconfig;
2+
3+
public class StableConfigMappingException extends RuntimeException {
4+
private static final int MAX_LEN = 100;
5+
6+
public StableConfigMappingException(String message) {
7+
super(message);
8+
}
9+
10+
/**
11+
* Safely converts an object to a string for error reporting, truncating the result if it exceeds
12+
* a maximum length.
13+
*
14+
* @param value the object to convert to a string
15+
* @return a string representation of the object, truncated if necessary
16+
*/
17+
static String safeToString(Object value) {
18+
if (value == null) return "null";
19+
String str = value.toString();
20+
if (str.length() > MAX_LEN) {
21+
int partLen = MAX_LEN / 2;
22+
return str.substring(0, partLen)
23+
+ "...(truncated)..."
24+
+ str.substring(str.length() - partLen);
25+
}
26+
return str;
27+
}
28+
29+
/**
30+
* Throws a StableConfigMappingException with a message that includes a safe string representation
31+
* of the provided value.
32+
*
33+
* @param message the error message to include
34+
* @param value the value to include in the error message, safely stringified
35+
* @throws StableConfigMappingException always thrown by this method
36+
*/
37+
public static void throwStableConfigMappingException(String message, Object value) {
38+
throw new StableConfigMappingException(message + " " + safeToString(value));
39+
}
40+
}

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

Lines changed: 35 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,35 @@
1+
package datadog.trace.bootstrap.config.provider
2+
3+
import datadog.trace.bootstrap.config.provider.stableconfig.StableConfigMappingException
4+
import spock.lang.Specification
5+
6+
class StableConfigMappingExceptionTest extends Specification {
7+
8+
def "constructors work as expected"() {
9+
when:
10+
def ex1 = new StableConfigMappingException("msg")
11+
def ex2 = new StableConfigMappingException("msg2")
12+
13+
then:
14+
ex1.message == "msg"
15+
ex1.cause == null
16+
ex2.message == "msg2"
17+
}
18+
19+
def "safeToString handles null"() {
20+
expect:
21+
StableConfigMappingException.safeToString(null) == "null"
22+
}
23+
24+
def "safeToString handles short string"() {
25+
expect:
26+
StableConfigMappingException.safeToString("short string") == "short string"
27+
}
28+
29+
def "safeToString handles long string"() {
30+
given:
31+
def longStr = "a" * 101
32+
expect:
33+
StableConfigMappingException.safeToString(longStr) == ("a" * 50) + "...(truncated)..." + ("a" * 51).substring(1)
34+
}
35+
}

0 commit comments

Comments
 (0)