Skip to content

Commit 076e9e1

Browse files
mtoffl01mtoffl01
committed
Modify ConfigCollector to support multiple configs for same key, grouped by origin
1 parent 7933b4e commit 076e9e1

File tree

2 files changed

+133
-68
lines changed

2 files changed

+133
-68
lines changed

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

Lines changed: 29 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -16,47 +16,58 @@ public class ConfigCollector {
1616
private static final AtomicReferenceFieldUpdater<ConfigCollector, Map> COLLECTED_UPDATER =
1717
AtomicReferenceFieldUpdater.newUpdater(ConfigCollector.class, Map.class, "collected");
1818

19-
private volatile Map<String, ConfigSetting> collected = new ConcurrentHashMap<>();
19+
private volatile Map<String, Map<ConfigOrigin, ConfigSetting>> collected =
20+
new ConcurrentHashMap<>();
2021

2122
public static ConfigCollector get() {
2223
return INSTANCE;
2324
}
2425

26+
/**
27+
* Records the latest ConfigSetting for the given key and origin, replacing any previous value for
28+
* that (key, origin) pair.
29+
*/
2530
public void put(String key, Object value, ConfigOrigin origin) {
2631
ConfigSetting setting = ConfigSetting.of(key, value, origin);
27-
collected.put(key, setting);
32+
Map<ConfigOrigin, ConfigSetting> originMap =
33+
collected.computeIfAbsent(key, k -> new ConcurrentHashMap<>());
34+
originMap.put(origin, setting); // replaces any previous value for this origin
2835
}
2936

3037
public void put(String key, Object value, ConfigOrigin origin, int seqId) {
3138
ConfigSetting setting = ConfigSetting.of(key, value, origin, seqId);
32-
collected.put(key, setting);
39+
Map<ConfigOrigin, ConfigSetting> originMap =
40+
collected.computeIfAbsent(key, k -> new ConcurrentHashMap<>());
41+
originMap.put(origin, setting); // replaces any previous value for this origin
3342
}
3443

44+
// TODO: Does this need a counterpart with seqId?
3545
public void putAll(Map<String, Object> keysAndValues, ConfigOrigin origin) {
36-
// attempt merge+replace to avoid collector seeing partial update
37-
Map<String, ConfigSetting> merged =
38-
new ConcurrentHashMap<>(keysAndValues.size() + collected.size());
3946
for (Map.Entry<String, Object> entry : keysAndValues.entrySet()) {
40-
ConfigSetting setting = ConfigSetting.of(entry.getKey(), entry.getValue(), origin);
41-
merged.put(entry.getKey(), setting);
42-
}
43-
while (true) {
44-
Map<String, ConfigSetting> current = collected;
45-
current.forEach(merged::putIfAbsent);
46-
if (COLLECTED_UPDATER.compareAndSet(this, current, merged)) {
47-
break; // success
48-
}
49-
// roll back to original update before next attempt
50-
merged.keySet().retainAll(keysAndValues.keySet());
47+
put(entry.getKey(), entry.getValue(), origin);
5148
}
5249
}
5350

5451
@SuppressWarnings("unchecked")
55-
public Map<String, ConfigSetting> collect() {
52+
public Map<String, Map<ConfigOrigin, ConfigSetting>> collect() {
5653
if (!collected.isEmpty()) {
5754
return COLLECTED_UPDATER.getAndSet(this, new ConcurrentHashMap<>());
5855
} else {
5956
return Collections.emptyMap();
6057
}
6158
}
59+
60+
/**
61+
* Returns the {@link ConfigSetting} with the highest precedence for the given key, or {@code
62+
* null} if no setting exists for that key.
63+
*/
64+
public ConfigSetting getAppliedConfigSetting(String key) {
65+
Map<ConfigOrigin, ConfigSetting> originMap = collected.get(key);
66+
if (originMap == null || originMap.isEmpty()) {
67+
return null;
68+
}
69+
return originMap.values().stream()
70+
.max(java.util.Comparator.comparingInt(setting -> setting.seqId))
71+
.orElse(null);
72+
}
6273
}

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

Lines changed: 104 additions & 50 deletions
Original file line numberDiff line numberDiff line change
@@ -22,44 +22,53 @@ class ConfigCollectorTest extends DDSpecification {
2222
injectEnvConfig(Strings.toEnvVar(configKey), configValue)
2323

2424
expect:
25-
def setting = ConfigCollector.get().collect().get(configKey)
26-
setting.stringValue() == configValue
27-
setting.origin == ConfigOrigin.ENV
25+
def configsByOrigin = ConfigCollector.get().collect().get(configKey)
26+
configsByOrigin != null
27+
configsByOrigin.size() == 2 // default and env origins
28+
29+
// Check that the ENV value is present and correct
30+
def envSetting = configsByOrigin.get(ConfigOrigin.ENV)
31+
assert envSetting != null
32+
assert envSetting.stringValue() == configValue
33+
34+
// Check the default is present with non-null value
35+
def defaultSetting = configsByOrigin.get(ConfigOrigin.DEFAULT)
36+
assert defaultSetting != null
2837

2938
where:
3039
configKey | configValue
31-
// // ConfigProvider.getEnum
32-
// IastConfig.IAST_TELEMETRY_VERBOSITY | Verbosity.DEBUG.toString()
33-
// // ConfigProvider.getString
34-
// TracerConfig.TRACE_SPAN_ATTRIBUTE_SCHEMA | "v1"
40+
// ConfigProvider.getEnum
41+
IastConfig.IAST_TELEMETRY_VERBOSITY | Verbosity.DEBUG.toString()
42+
// ConfigProvider.getString
43+
TracerConfig.TRACE_SPAN_ATTRIBUTE_SCHEMA | "v1"
3544
// ConfigProvider.getStringNotEmpty
3645
AppSecConfig.APPSEC_AUTOMATED_USER_EVENTS_TRACKING | UserEventTrackingMode.EXTENDED.toString()
3746
// ConfigProvider.getStringExcludingSource
38-
// GeneralConfig.APPLICATION_KEY | "app-key"
39-
// // ConfigProvider.getBoolean
40-
// TraceInstrumentationConfig.RESOLVER_USE_URL_CACHES | "true"
41-
// // ConfigProvider.getInteger
42-
// JmxFetchConfig.JMX_FETCH_CHECK_PERIOD | "60"
43-
// // ConfigProvider.getLong
44-
// CiVisibilityConfig.CIVISIBILITY_GIT_COMMAND_TIMEOUT_MILLIS | "450273"
45-
// // ConfigProvider.getFloat
46-
// GeneralConfig.TELEMETRY_HEARTBEAT_INTERVAL | "1.5"
47-
// // ConfigProvider.getDouble
48-
// TracerConfig.TRACE_SAMPLE_RATE | "2.2"
49-
// // ConfigProvider.getList
50-
// TraceInstrumentationConfig.JMS_PROPAGATION_DISABLED_TOPICS | "someTopic,otherTopic"
51-
// // ConfigProvider.getSet
52-
// IastConfig.IAST_WEAK_HASH_ALGORITHMS | "SHA1,SHA-1"
53-
// // ConfigProvider.getSpacedList
54-
// TracerConfig.PROXY_NO_PROXY | "a b c"
55-
// // ConfigProvider.getMergedMap
56-
// TracerConfig.TRACE_PEER_SERVICE_MAPPING | "service1:best_service,userService:my_service"
57-
// // ConfigProvider.getOrderedMap
58-
// TracerConfig.TRACE_HTTP_SERVER_PATH_RESOURCE_NAME_MAPPING | "/asdf/*:/test"
59-
// // ConfigProvider.getMergedMapWithOptionalMappings
60-
// TracerConfig.HEADER_TAGS | "e:five"
61-
// // ConfigProvider.getIntegerRange
62-
// TracerConfig.TRACE_HTTP_CLIENT_ERROR_STATUSES | "400-402"
47+
GeneralConfig.APPLICATION_KEY | "app-key"
48+
// ConfigProvider.getBoolean
49+
TraceInstrumentationConfig.RESOLVER_USE_URL_CACHES | "true"
50+
// ConfigProvider.getInteger
51+
JmxFetchConfig.JMX_FETCH_CHECK_PERIOD | "60"
52+
// ConfigProvider.getLong
53+
CiVisibilityConfig.CIVISIBILITY_GIT_COMMAND_TIMEOUT_MILLIS | "450273"
54+
// ConfigProvider.getFloat
55+
GeneralConfig.TELEMETRY_HEARTBEAT_INTERVAL | "1.5"
56+
// ConfigProvider.getDouble
57+
TracerConfig.TRACE_SAMPLE_RATE | "2.2"
58+
// ConfigProvider.getList
59+
TraceInstrumentationConfig.JMS_PROPAGATION_DISABLED_TOPICS | "someTopic,otherTopic"
60+
// ConfigProvider.getSet
61+
IastConfig.IAST_WEAK_HASH_ALGORITHMS | "SHA1,SHA-1"
62+
// ConfigProvider.getSpacedList
63+
TracerConfig.PROXY_NO_PROXY | "a b c"
64+
// ConfigProvider.getMergedMap
65+
TracerConfig.TRACE_PEER_SERVICE_MAPPING | "service1:best_service,userService:my_service"
66+
// ConfigProvider.getOrderedMap
67+
TracerConfig.TRACE_HTTP_SERVER_PATH_RESOURCE_NAME_MAPPING | "/asdf/*:/test"
68+
// ConfigProvider.getMergedMapWithOptionalMappings
69+
TracerConfig.HEADER_TAGS | "e:five"
70+
// ConfigProvider.getIntegerRange
71+
TracerConfig.TRACE_HTTP_CLIENT_ERROR_STATUSES | "400-402"
6372
}
6473

6574
def "should collect merged data from multiple sources"() {
@@ -68,9 +77,13 @@ class ConfigCollectorTest extends DDSpecification {
6877
injectSysConfig(configKey, configValue2)
6978

7079
expect:
71-
def setting = ConfigCollector.get().collect().get(configKey)
80+
def configsByOrigin = ConfigCollector.get().collect().get(configKey)
81+
configsByOrigin != null
82+
def setting = configsByOrigin.get(ConfigOrigin.JVM_PROP)
83+
setting != null
7284
setting.stringValue() == expectedValue
7385
setting.origin == ConfigOrigin.JVM_PROP
86+
// TODO: Add check for env origin as well
7487

7588
where:
7689
configKey | configValue1 | configValue2 | expectedValue
@@ -84,9 +97,10 @@ class ConfigCollectorTest extends DDSpecification {
8497

8598
def "default not-null config settings are collected"() {
8699
expect:
87-
def setting = ConfigCollector.get().collect().get(configKey)
88-
setting.origin == ConfigOrigin.DEFAULT
89-
setting.stringValue() == defaultValue
100+
def configsByOrigin = ConfigCollector.get().collect().get(configKey)
101+
configsByOrigin != null
102+
def setting = configsByOrigin.get(ConfigOrigin.DEFAULT)
103+
setting != null
90104

91105
where:
92106
configKey | defaultValue
@@ -100,12 +114,15 @@ class ConfigCollectorTest extends DDSpecification {
100114

101115
def "default null config settings are also collected"() {
102116
when:
103-
ConfigSetting cs = ConfigCollector.get().collect().get(configKey)
117+
def configsByOrigin = ConfigCollector.get().collect().get(configKey)
118+
configsByOrigin != null
119+
def setting = configsByOrigin.get(ConfigOrigin.DEFAULT)
120+
setting != null
104121

105122
then:
106-
cs.key == configKey
107-
cs.stringValue() == null
108-
cs.origin == ConfigOrigin.DEFAULT
123+
setting.key == configKey
124+
setting.stringValue() == null
125+
setting.origin == ConfigOrigin.DEFAULT
109126

110127
where:
111128
configKey << [
@@ -121,12 +138,14 @@ class ConfigCollectorTest extends DDSpecification {
121138

122139
def "default empty maps and list config settings are collected as empty strings"() {
123140
when:
124-
ConfigSetting cs = ConfigCollector.get().collect().get(configKey)
141+
def configsByOrigin = ConfigCollector.get().collect().get(configKey)
142+
def cs = configsByOrigin?.get(ConfigOrigin.DEFAULT)
125143

126144
then:
127-
cs.key == configKey
128-
cs.stringValue() == ""
129-
cs.origin == ConfigOrigin.DEFAULT
145+
assert cs != null
146+
assert cs.key == configKey
147+
assert cs.stringValue() == ""
148+
assert cs.origin == ConfigOrigin.DEFAULT
130149

131150
where:
132151
configKey << [
@@ -143,12 +162,14 @@ class ConfigCollectorTest extends DDSpecification {
143162
when:
144163
ConfigCollector.get().put('key1', 'value1', ConfigOrigin.DEFAULT)
145164
ConfigCollector.get().put('key2', 'value2', ConfigOrigin.ENV)
146-
ConfigCollector.get().put('key1', 'replaced', ConfigOrigin.REMOTE)
165+
ConfigCollector.get().put('key1', 'value11', ConfigOrigin.REMOTE)
147166
ConfigCollector.get().put('key3', 'value3', ConfigOrigin.JVM_PROP)
148167

149168
then:
150-
ConfigCollector.get().collect().values().toSet() == [
151-
ConfigSetting.of('key1', 'replaced', ConfigOrigin.REMOTE),
169+
def allSettings = ConfigCollector.get().collect().values().collectMany { it.values() }.toSet()
170+
allSettings == [
171+
ConfigSetting.of('key1', 'value1', ConfigOrigin.DEFAULT),
172+
ConfigSetting.of('key1', 'value11', ConfigOrigin.REMOTE),
152173
ConfigSetting.of('key2', 'value2', ConfigOrigin.ENV),
153174
ConfigSetting.of('key3', 'value3', ConfigOrigin.JVM_PROP)
154175
] as Set
@@ -163,15 +184,20 @@ class ConfigCollectorTest extends DDSpecification {
163184
ConfigCollector.get().put('DD_API_KEY', 'sensitive data', ConfigOrigin.ENV)
164185

165186
then:
166-
ConfigCollector.get().collect().get('DD_API_KEY').stringValue() == '<hidden>'
187+
def configsByOrigin = ConfigCollector.get().collect().get('DD_API_KEY')
188+
configsByOrigin != null
189+
configsByOrigin.get(ConfigOrigin.ENV).stringValue() == '<hidden>'
167190
}
168191

169192
def "collects common setting default values"() {
170193
when:
171194
def settings = ConfigCollector.get().collect()
172195

173196
then:
174-
def setting = settings.get(key)
197+
def configsByOrigin = settings.get(key)
198+
configsByOrigin != null
199+
def setting = configsByOrigin.get(ConfigOrigin.DEFAULT)
200+
setting != null
175201

176202
setting.key == key
177203
setting.stringValue() == value
@@ -205,7 +231,10 @@ class ConfigCollectorTest extends DDSpecification {
205231
def settings = ConfigCollector.get().collect()
206232

207233
then:
208-
def setting = settings.get(key)
234+
def configsByOrigin = settings.get(key)
235+
configsByOrigin != null
236+
def setting = configsByOrigin.get(ConfigOrigin.ENV)
237+
setting != null
209238

210239
setting.key == key
211240
setting.stringValue() == value
@@ -224,4 +253,29 @@ class ConfigCollectorTest extends DDSpecification {
224253
"logs.injection.enabled" | "false"
225254
"trace.sample.rate" | "0.3"
226255
}
256+
257+
def "getAppliedConfigSetting returns the setting with the highest seqId for a key"() {
258+
setup:
259+
def collector = ConfigCollector.get()
260+
collector.collect() // clear previous state
261+
collector.put('test.key', 'default', ConfigOrigin.DEFAULT, 1)
262+
collector.put('test.key', 'env', ConfigOrigin.ENV, 2)
263+
collector.put('test.key', 'remote', ConfigOrigin.REMOTE, 4)
264+
collector.put('test.key', 'jvm', ConfigOrigin.JVM_PROP, 3)
265+
266+
when:
267+
def applied = collector.getAppliedConfigSetting('test.key')
268+
269+
then:
270+
applied != null
271+
applied.key == 'test.key'
272+
applied.value == 'remote'
273+
applied.origin == ConfigOrigin.REMOTE
274+
275+
when: "no settings for a key"
276+
def none = collector.getAppliedConfigSetting('nonexistent.key')
277+
278+
then:
279+
none == null
280+
}
227281
}

0 commit comments

Comments
 (0)