Skip to content

Commit 5c66a15

Browse files
mtoffl01mhliddmcculls
authored
ConfigProvider iterates over all sources and reports all non-null values to telemetry (#9404)
* apply all changes from PR #9327, accomodated to master * nits: add configId in ConfigProvider methods where it was missing; fix configId test to accommodate new ConfigCollector data structure * Fix 'test config id exists in ConfigCollector when using StableConfigSource' * introduce reportDefault * Call collect in BaseApplication.getLogInjectionEnabled; make getAppliedConfigSetting static function that takes in a map * Fix call to getAppliedConfigSetting in ConfigCollector test * Introduce ConfigValueResolver and ConfigMergeResolver * getString methods only report non-null values to telemetry * remove dependency on collectConfig for reReportToCollector and reReportFinalResult * updating config collector to not override existing configs * Adding putDefault and adding unit tests * relaxing reReportToCollector restraint and adding unit test * cleanup * updating failing unit test * abstracing ConfigCollector * adding support for proper seqId with Remote Config * abstracting away getString * Update internal-api/src/main/java/datadog/trace/bootstrap/config/provider/ConfigProvider.java * Update internal-api/src/main/java/datadog/trace/bootstrap/config/provider/ConfigProvider.java * Update internal-api/src/main/java/datadog/trace/bootstrap/config/provider/ConfigProvider.java * nit: fix javadoc comments * Modify getEnum, getList, getIntegerRange and getSet to report defaults before calling getString. And modify putDefault to treat null entries as a valid value * remove remoteConfig methods from ConfigCollector + highestSeqId * remove serializenulls comment in TelemetryRequestBody * remove 'no usages' comment above unused ConfigCollector put method * Simplify javadoc for putAll * remove javadoc for ABSENT_SEQ_ID * revert getStringInternal changes * Introduce new getStringInternal, for getting string from non-default sources * Deprecate ConfigCollector constructor without sequence ID * putRemote: ConfigCollector method for 'putting' from Remote Config origin; migrate AppSecConfigServiceImpl to use this API * ConfigSetting.NON_DEFAULT_SEQ_ID: introduce new constant, migrate all calls to 'DEFAULT_SEQ_ID + 1' to use new constant; move constants to top of ConfigSetting class * ConfigCollector.put: Delete unused function * Update internal-api/src/test/groovy/datadog/trace/api/ConfigCollectorTest.groovy Co-authored-by: Stuart McCulloch <[email protected]> * replace 'def origin' assignment in ConfigCollectorTest with in-line use * ConfigCollector.put: Remove deprecated function with zero uses * NEW_SUB_MAP: Define reusable lambda function as static field in ConfigCollector, used by putDefault and put * updateAll: rename putAll to updateAll, and scope to Remote origin, only * Fix updateAll * Align naming of methods that report remote config to the ConfigCollector * Avoid need to peek into ConfigCollector internals * Restore atomic reporting of updates from remote-config * Update utils/config-utils/src/main/java/datadog/trace/bootstrap/config/provider/ConfigProvider.java Co-authored-by: Stuart McCulloch <[email protected]> * Update utils/config-utils/src/main/java/datadog/trace/bootstrap/config/provider/ConfigProvider.java Co-authored-by: Stuart McCulloch <[email protected]> * Update utils/config-utils/src/main/java/datadog/trace/bootstrap/config/provider/ConfigProvider.java Co-authored-by: Stuart McCulloch <[email protected]> * Fix bug in reReportFinalResult that incorrectly reported CALCULATED for all non-default values --------- Co-authored-by: Matthew Li <[email protected]> Co-authored-by: Stuart McCulloch <[email protected]>
1 parent 27641db commit 5c66a15

File tree

16 files changed

+1023
-201
lines changed

16 files changed

+1023
-201
lines changed

‎dd-java-agent/appsec/src/main/java/com/datadog/appsec/config/AppSecConfigServiceImpl.java‎

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -50,7 +50,6 @@
5050
import datadog.remoteconfig.state.ProductListener;
5151
import datadog.trace.api.Config;
5252
import datadog.trace.api.ConfigCollector;
53-
import datadog.trace.api.ConfigOrigin;
5453
import datadog.trace.api.ProductActivation;
5554
import datadog.trace.api.UserIdCollectionMode;
5655
import datadog.trace.api.telemetry.LogCollector;
@@ -589,7 +588,7 @@ private void setAppSecActivation(final AppSecFeatures.Asm asm) {
589588
} else {
590589
newState = asm.enabled;
591590
// Report AppSec activation change via telemetry when modified via remote config
592-
ConfigCollector.get().put(APPSEC_ENABLED, asm.enabled, ConfigOrigin.REMOTE);
591+
ConfigCollector.get().putRemote(APPSEC_ENABLED, asm.enabled);
593592
}
594593
if (AppSecSystem.isActive() != newState) {
595594
log.info("AppSec {} (runtime)", newState ? "enabled" : "disabled");

‎dd-smoke-tests/log-injection/src/main/java/datadog/smoketest/loginjection/BaseApplication.java‎

Lines changed: 13 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -1,11 +1,11 @@
11
package datadog.smoketest.loginjection;
22

3-
import static datadog.trace.api.config.TraceInstrumentationConfig.LOGS_INJECTION_ENABLED;
4-
5-
import datadog.trace.api.ConfigCollector;
6-
import datadog.trace.api.ConfigSetting;
73
import datadog.trace.api.CorrelationIdentifier;
4+
import datadog.trace.api.GlobalTracer;
85
import datadog.trace.api.Trace;
6+
import datadog.trace.api.TraceConfig;
7+
import datadog.trace.api.Tracer;
8+
import java.lang.reflect.Method;
99
import java.util.concurrent.TimeUnit;
1010
import java.util.function.Supplier;
1111

@@ -23,13 +23,13 @@ public void run() throws InterruptedException {
2323

2424
secondTracedMethod();
2525

26-
if (!waitForCondition(() -> Boolean.FALSE.equals(getLogInjectionEnabled()))) {
26+
if (!waitForCondition(() -> !getLogInjectionEnabled())) {
2727
throw new RuntimeException("Logs injection config was never updated");
2828
}
2929

3030
thirdTracedMethod();
3131

32-
if (!waitForCondition(() -> Boolean.TRUE.equals(getLogInjectionEnabled()))) {
32+
if (!waitForCondition(() -> getLogInjectionEnabled())) {
3333
throw new RuntimeException("Logs injection config was never updated a second time");
3434
}
3535

@@ -43,12 +43,14 @@ public void run() throws InterruptedException {
4343
Thread.sleep(400);
4444
}
4545

46-
private static Object getLogInjectionEnabled() {
47-
ConfigSetting configSetting = ConfigCollector.get().collect().get(LOGS_INJECTION_ENABLED);
48-
if (configSetting == null) {
49-
return null;
46+
private static boolean getLogInjectionEnabled() {
47+
try {
48+
Tracer tracer = GlobalTracer.get();
49+
Method captureTraceConfig = tracer.getClass().getMethod("captureTraceConfig");
50+
return ((TraceConfig) captureTraceConfig.invoke(tracer)).isLogsInjectionEnabled();
51+
} catch (ReflectiveOperationException e) {
52+
throw new RuntimeException(e);
5053
}
51-
return configSetting.value;
5254
}
5355

5456
@Trace

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

Lines changed: 5 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -170,6 +170,7 @@
170170
import static datadog.trace.api.ConfigDefaults.DEFAULT_WEBSOCKET_MESSAGES_SEPARATE_TRACES;
171171
import static datadog.trace.api.ConfigDefaults.DEFAULT_WEBSOCKET_TAG_SESSION_ID;
172172
import static datadog.trace.api.ConfigDefaults.DEFAULT_WRITER_BAGGAGE_INJECT;
173+
import static datadog.trace.api.ConfigSetting.NON_DEFAULT_SEQ_ID;
173174
import static datadog.trace.api.DDTags.APM_ENABLED;
174175
import static datadog.trace.api.DDTags.HOST_TAG;
175176
import static datadog.trace.api.DDTags.INTERNAL_HOST_NAME;
@@ -5300,7 +5301,8 @@ private static boolean isWindowsOS() {
53005301
private static String getEnv(String name) {
53015302
String value = EnvironmentVariables.get(name);
53025303
if (value != null) {
5303-
ConfigCollector.get().put(name, value, ConfigOrigin.ENV);
5304+
// Report non-default sequence id for consistency
5305+
ConfigCollector.get().put(name, value, ConfigOrigin.ENV, NON_DEFAULT_SEQ_ID);
53045306
}
53055307
return value;
53065308
}
@@ -5323,7 +5325,8 @@ private static String getProp(String name) {
53235325
private static String getProp(String name, String def) {
53245326
String value = SystemProperties.getOrDefault(name, def);
53255327
if (value != null) {
5326-
ConfigCollector.get().put(name, value, ConfigOrigin.JVM_PROP);
5328+
// Report non-default sequence id for consistency
5329+
ConfigCollector.get().put(name, value, ConfigOrigin.JVM_PROP, NON_DEFAULT_SEQ_ID);
53275330
}
53285331
return value;
53295332
}

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

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -294,7 +294,7 @@ static void reportConfigChange(Snapshot newSnapshot) {
294294
update.put(TRACE_SAMPLING_RULES, newSnapshot.traceSamplingRulesJson);
295295
maybePut(update, TRACE_SAMPLE_RATE, newSnapshot.traceSampleRate);
296296

297-
ConfigCollector.get().putAll(update, ConfigOrigin.REMOTE);
297+
ConfigCollector.get().putRemote(update);
298298
}
299299

300300
@SuppressWarnings("SameParameterValue")

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

Lines changed: 96 additions & 30 deletions
Original file line numberDiff line numberDiff line change
@@ -15,6 +15,7 @@ import datadog.trace.util.ConfigStrings
1515

1616
import static datadog.trace.api.ConfigDefaults.DEFAULT_IAST_WEAK_HASH_ALGORITHMS
1717
import static datadog.trace.api.ConfigDefaults.DEFAULT_TELEMETRY_HEARTBEAT_INTERVAL
18+
import static datadog.trace.api.ConfigSetting.ABSENT_SEQ_ID
1819

1920
class ConfigCollectorTest extends DDSpecification {
2021

@@ -23,9 +24,10 @@ class ConfigCollectorTest extends DDSpecification {
2324
injectEnvConfig(ConfigStrings.toEnvVar(configKey), configValue)
2425

2526
expect:
26-
def setting = ConfigCollector.get().collect().get(configKey)
27-
setting.stringValue() == configValue
28-
setting.origin == ConfigOrigin.ENV
27+
def envConfigByKey = ConfigCollector.get().collect().get(ConfigOrigin.ENV)
28+
def config = envConfigByKey.get(configKey)
29+
config.stringValue() == configValue
30+
config.origin == ConfigOrigin.ENV
2931

3032
where:
3133
configKey | configValue
@@ -65,18 +67,31 @@ class ConfigCollectorTest extends DDSpecification {
6567

6668
def "should collect merged data from multiple sources"() {
6769
setup:
68-
injectEnvConfig(ConfigStrings.toEnvVar(configKey), envValue)
69-
if (jvmValue != null) {
70-
injectSysConfig(configKey, jvmValue)
70+
injectEnvConfig(ConfigStrings.toEnvVar(configKey), envConfigValue)
71+
if (jvmConfigValue != null) {
72+
injectSysConfig(configKey, jvmConfigValue)
73+
}
74+
75+
when:
76+
def collected = ConfigCollector.get().collect()
77+
78+
then:
79+
def envSetting = collected.get(ConfigOrigin.ENV)
80+
def envConfig = envSetting.get(configKey)
81+
envConfig.stringValue() == envConfigValue
82+
envConfig.origin == ConfigOrigin.ENV
83+
if (jvmConfigValue != null ) {
84+
def jvmSetting = collected.get(ConfigOrigin.JVM_PROP)
85+
def jvmConfig = jvmSetting.get(configKey)
86+
jvmConfig.stringValue().split(',') as Set == jvmConfigValue.split(',') as Set
87+
jvmConfig.origin == ConfigOrigin.JVM_PROP
7188
}
7289

73-
expect:
74-
def setting = ConfigCollector.get().collect().get(configKey)
75-
setting.stringValue() == expectedValue
76-
setting.origin == expectedOrigin
90+
91+
// TODO: Add a check for which setting the collector recognizes as highest precedence
7792

7893
where:
79-
configKey | envValue | jvmValue | expectedValue | expectedOrigin
94+
configKey | envConfigValue | jvmConfigValue | expectedValue | expectedOrigin
8095
// ConfigProvider.getMergedMap
8196
TracerConfig.TRACE_PEER_SERVICE_MAPPING | "service1:best_service,userService:my_service" | "service2:backup_service" | "service2:backup_service,service1:best_service,userService:my_service" | ConfigOrigin.CALCULATED
8297
// ConfigProvider.getOrderedMap
@@ -89,7 +104,8 @@ class ConfigCollectorTest extends DDSpecification {
89104

90105
def "default not-null config settings are collected"() {
91106
expect:
92-
def setting = ConfigCollector.get().collect().get(configKey)
107+
def defaultConfigByKey = ConfigCollector.get().collect().get(ConfigOrigin.DEFAULT)
108+
def setting = defaultConfigByKey.get(configKey)
93109
setting.origin == ConfigOrigin.DEFAULT
94110
setting.stringValue() == defaultValue
95111

@@ -105,7 +121,8 @@ class ConfigCollectorTest extends DDSpecification {
105121

106122
def "default null config settings are also collected"() {
107123
when:
108-
ConfigSetting cs = ConfigCollector.get().collect().get(configKey)
124+
def defaultConfigByKey = ConfigCollector.get().collect().get(ConfigOrigin.DEFAULT)
125+
ConfigSetting cs = defaultConfigByKey.get(configKey)
109126

110127
then:
111128
cs.key == configKey
@@ -126,7 +143,8 @@ class ConfigCollectorTest extends DDSpecification {
126143

127144
def "default empty maps and list config settings are collected as empty strings"() {
128145
when:
129-
ConfigSetting cs = ConfigCollector.get().collect().get(configKey)
146+
def defaultConfigByKey = ConfigCollector.get().collect().get(ConfigOrigin.DEFAULT)
147+
ConfigSetting cs = defaultConfigByKey.get(configKey)
130148

131149
then:
132150
cs.key == configKey
@@ -146,17 +164,17 @@ class ConfigCollectorTest extends DDSpecification {
146164
ConfigCollector.get().collect()
147165

148166
when:
149-
ConfigCollector.get().put('key1', 'value1', ConfigOrigin.DEFAULT)
150-
ConfigCollector.get().put('key2', 'value2', ConfigOrigin.ENV)
151-
ConfigCollector.get().put('key1', 'replaced', ConfigOrigin.REMOTE)
152-
ConfigCollector.get().put('key3', 'value3', ConfigOrigin.JVM_PROP)
167+
ConfigCollector.get().put('key1', 'value1', ConfigOrigin.DEFAULT, ABSENT_SEQ_ID)
168+
ConfigCollector.get().put('key2', 'value2', ConfigOrigin.ENV, ABSENT_SEQ_ID)
169+
ConfigCollector.get().put('key1', 'value4', ConfigOrigin.REMOTE, ABSENT_SEQ_ID)
170+
ConfigCollector.get().put('key3', 'value3', ConfigOrigin.JVM_PROP, ABSENT_SEQ_ID)
153171

154172
then:
155-
ConfigCollector.get().collect().values().toSet() == [
156-
ConfigSetting.of('key1', 'replaced', ConfigOrigin.REMOTE),
157-
ConfigSetting.of('key2', 'value2', ConfigOrigin.ENV),
158-
ConfigSetting.of('key3', 'value3', ConfigOrigin.JVM_PROP)
159-
] as Set
173+
def collected = ConfigCollector.get().collect()
174+
collected.get(ConfigOrigin.REMOTE).get('key1') == ConfigSetting.of('key1', 'value4', ConfigOrigin.REMOTE)
175+
collected.get(ConfigOrigin.ENV).get('key2') == ConfigSetting.of('key2', 'value2', ConfigOrigin.ENV)
176+
collected.get(ConfigOrigin.JVM_PROP).get('key3') == ConfigSetting.of('key3', 'value3', ConfigOrigin.JVM_PROP)
177+
collected.get(ConfigOrigin.DEFAULT).get('key1') == ConfigSetting.of('key1', 'value1', ConfigOrigin.DEFAULT)
160178
}
161179

162180

@@ -165,18 +183,19 @@ class ConfigCollectorTest extends DDSpecification {
165183
ConfigCollector.get().collect()
166184

167185
when:
168-
ConfigCollector.get().put('DD_API_KEY', 'sensitive data', ConfigOrigin.ENV)
186+
ConfigCollector.get().put('DD_API_KEY', 'sensitive data', ConfigOrigin.ENV, ABSENT_SEQ_ID)
169187

170188
then:
171-
ConfigCollector.get().collect().get('DD_API_KEY').stringValue() == '<hidden>'
189+
def collected = ConfigCollector.get().collect()
190+
collected.get(ConfigOrigin.ENV).get('DD_API_KEY').stringValue() == '<hidden>'
172191
}
173192

174193
def "collects common setting default values"() {
175194
when:
176-
def settings = ConfigCollector.get().collect()
195+
def defaultConfigByKey = ConfigCollector.get().collect().get(ConfigOrigin.DEFAULT)
177196

178197
then:
179-
def setting = settings.get(key)
198+
def setting = defaultConfigByKey.get(key)
180199

181200
setting.key == key
182201
setting.stringValue() == value
@@ -207,10 +226,10 @@ class ConfigCollectorTest extends DDSpecification {
207226
injectEnvConfig("DD_TRACE_SAMPLE_RATE", "0.3")
208227

209228
when:
210-
def settings = ConfigCollector.get().collect()
229+
def envConfigByKey = ConfigCollector.get().collect().get(ConfigOrigin.ENV)
211230

212231
then:
213-
def setting = settings.get(key)
232+
def setting = envConfigByKey.get(key)
214233

215234
setting.key == key
216235
setting.stringValue() == value
@@ -230,6 +249,31 @@ class ConfigCollectorTest extends DDSpecification {
230249
"trace.sample.rate" | "0.3"
231250
}
232251

252+
def "config collector creates ConfigSettings with correct seqId"() {
253+
setup:
254+
ConfigCollector.get().collect() // clear previous state
255+
256+
when:
257+
// Simulate sources with increasing precedence and a default
258+
ConfigCollector.get().put("test.key", "default", ConfigOrigin.DEFAULT, ConfigSetting.DEFAULT_SEQ_ID)
259+
ConfigCollector.get().put("test.key", "env", ConfigOrigin.ENV, 2)
260+
ConfigCollector.get().put("test.key", "jvm", ConfigOrigin.JVM_PROP, 3)
261+
ConfigCollector.get().put("test.key", "remote", ConfigOrigin.REMOTE, 4)
262+
263+
then:
264+
def collected = ConfigCollector.get().collect()
265+
def defaultSetting = collected.get(ConfigOrigin.DEFAULT).get("test.key")
266+
def envSetting = collected.get(ConfigOrigin.ENV).get("test.key")
267+
def jvmSetting = collected.get(ConfigOrigin.JVM_PROP).get("test.key")
268+
def remoteSetting = collected.get(ConfigOrigin.REMOTE).get("test.key")
269+
270+
defaultSetting.seqId == ConfigSetting.DEFAULT_SEQ_ID
271+
// Higher precedence = higher seqId
272+
defaultSetting.seqId < envSetting.seqId
273+
envSetting.seqId < jvmSetting.seqId
274+
jvmSetting.seqId < remoteSetting.seqId
275+
}
276+
233277
def "config id is null for non-StableConfigSource"() {
234278
setup:
235279
def key = "test.key"
@@ -243,10 +287,32 @@ class ConfigCollectorTest extends DDSpecification {
243287

244288
then:
245289
// Verify the config was collected but without a config ID
246-
def setting = settings.get(key)
290+
def setting = settings.get(ConfigOrigin.JVM_PROP).get(key)
247291
setting != null
248292
setting.configId == null
249293
setting.value == value
250294
setting.origin == ConfigOrigin.JVM_PROP
251295
}
296+
297+
def "default sources cannot be overridden"() {
298+
setup:
299+
def key = "test.key"
300+
def value = "test-value"
301+
def overrideVal = "override-value"
302+
def defaultConfigByKey
303+
ConfigSetting cs
304+
305+
when:
306+
// Need to make 2 calls in a row because collect() will empty the map
307+
ConfigCollector.get().putDefault(key, value)
308+
ConfigCollector.get().putDefault(key, overrideVal)
309+
defaultConfigByKey = ConfigCollector.get().collect().get(ConfigOrigin.DEFAULT)
310+
cs = defaultConfigByKey.get(key)
311+
312+
then:
313+
cs.key == key
314+
cs.stringValue() == value
315+
cs.origin == ConfigOrigin.DEFAULT
316+
cs.seqId == ConfigSetting.DEFAULT_SEQ_ID
317+
}
252318
}

0 commit comments

Comments
 (0)