-
Notifications
You must be signed in to change notification settings - Fork 324
ConfigProvider iterates over all sources and reports all non-null values to telemetry #9404
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
Changes from 34 commits
08c80b4
c62688b
125744a
8becbc5
db60faf
750ba64
3006e7c
bee84bf
bfcfd7e
b411ea9
d097300
1ca7fd9
81db67d
1b81e8a
9b22670
cdf3a12
7053f6c
ff95cdc
4094c53
1d70667
bdab76d
2482933
bc64730
11b61f1
d7dc744
700f837
b190961
779551d
94f6485
e796896
3997f56
9794631
84ab423
a9c1f7a
ff39a53
0408e49
e3b307d
81187aa
0ffe413
b5e1414
b048c91
0ef089d
7ef2a9f
7508fc7
49d3ced
fe9b376
d9dac26
474cd40
8e29e03
365f5d6
4a91a8e
d9fe333
2626c62
5b5ad0d
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -44,7 +44,9 @@ public void run() throws InterruptedException { | |
| } | ||
|
|
||
| private static Object getLogInjectionEnabled() { | ||
| ConfigSetting configSetting = ConfigCollector.get().collect().get(LOGS_INJECTION_ENABLED); | ||
| ConfigSetting configSetting = | ||
| ConfigCollector.getAppliedConfigSetting( | ||
| LOGS_INJECTION_ENABLED, ConfigCollector.get().collect()); | ||
|
||
| if (configSetting == null) { | ||
| return null; | ||
| } | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -170,6 +170,7 @@ | |
| import static datadog.trace.api.ConfigDefaults.DEFAULT_WEBSOCKET_MESSAGES_SEPARATE_TRACES; | ||
| import static datadog.trace.api.ConfigDefaults.DEFAULT_WEBSOCKET_TAG_SESSION_ID; | ||
| import static datadog.trace.api.ConfigDefaults.DEFAULT_WRITER_BAGGAGE_INJECT; | ||
| import static datadog.trace.api.ConfigSetting.DEFAULT_SEQ_ID; | ||
| import static datadog.trace.api.DDTags.APM_ENABLED; | ||
| import static datadog.trace.api.DDTags.HOST_TAG; | ||
| import static datadog.trace.api.DDTags.INTERNAL_HOST_NAME; | ||
|
|
@@ -5293,7 +5294,8 @@ private static boolean isWindowsOS() { | |
| private static String getEnv(String name) { | ||
| String value = EnvironmentVariables.get(name); | ||
| if (value != null) { | ||
| ConfigCollector.get().put(name, value, ConfigOrigin.ENV); | ||
| // Report non-default sequence id for consistency | ||
| ConfigCollector.get().put(name, value, ConfigOrigin.ENV, DEFAULT_SEQ_ID + 1); | ||
|
||
| } | ||
| return value; | ||
| } | ||
|
|
@@ -5316,7 +5318,8 @@ private static String getProp(String name) { | |
| private static String getProp(String name, String def) { | ||
| String value = SystemProperties.getOrDefault(name, def); | ||
| if (value != null) { | ||
| ConfigCollector.get().put(name, value, ConfigOrigin.JVM_PROP); | ||
| // Report non-default sequence id for consistency | ||
| ConfigCollector.get().put(name, value, ConfigOrigin.JVM_PROP, DEFAULT_SEQ_ID + 1); | ||
|
||
| } | ||
| return value; | ||
| } | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -15,17 +15,20 @@ import datadog.trace.util.ConfigStrings | |
|
|
||
| import static datadog.trace.api.ConfigDefaults.DEFAULT_IAST_WEAK_HASH_ALGORITHMS | ||
| import static datadog.trace.api.ConfigDefaults.DEFAULT_TELEMETRY_HEARTBEAT_INTERVAL | ||
| import static datadog.trace.api.ConfigSetting.ABSENT_SEQ_ID | ||
|
|
||
| class ConfigCollectorTest extends DDSpecification { | ||
|
|
||
| def "non-default config settings get collected"() { | ||
| setup: | ||
| injectEnvConfig(ConfigStrings.toEnvVar(configKey), configValue) | ||
| def origin = ConfigOrigin.ENV | ||
|
||
|
|
||
| expect: | ||
| def setting = ConfigCollector.get().collect().get(configKey) | ||
| setting.stringValue() == configValue | ||
| setting.origin == ConfigOrigin.ENV | ||
| def envConfigByKey = ConfigCollector.get().collect().get(origin) | ||
| def config = envConfigByKey.get(configKey) | ||
| config.stringValue() == configValue | ||
| config.origin == ConfigOrigin.ENV | ||
|
|
||
| where: | ||
| configKey | configValue | ||
|
|
@@ -65,18 +68,31 @@ class ConfigCollectorTest extends DDSpecification { | |
|
|
||
| def "should collect merged data from multiple sources"() { | ||
| setup: | ||
| injectEnvConfig(ConfigStrings.toEnvVar(configKey), envValue) | ||
| if (jvmValue != null) { | ||
| injectSysConfig(configKey, jvmValue) | ||
| injectEnvConfig(ConfigStrings.toEnvVar(configKey), envConfigValue) | ||
| if (jvmConfigValue != null) { | ||
| injectSysConfig(configKey, jvmConfigValue) | ||
| } | ||
|
|
||
| expect: | ||
| def setting = ConfigCollector.get().collect().get(configKey) | ||
| setting.stringValue() == expectedValue | ||
| setting.origin == expectedOrigin | ||
| when: | ||
| def collected = ConfigCollector.get().collect() | ||
|
|
||
| then: | ||
| def envSetting = collected.get(ConfigOrigin.ENV) | ||
| def envConfig = envSetting.get(configKey) | ||
| envConfig.stringValue() == envConfigValue | ||
| envConfig.origin == ConfigOrigin.ENV | ||
| if (jvmConfigValue != null ) { | ||
| def jvmSetting = collected.get(ConfigOrigin.JVM_PROP) | ||
| def jvmConfig = jvmSetting.get(configKey) | ||
| jvmConfig.stringValue().split(',').toList().toSet() == jvmConfigValue.split(',').toList().toSet() | ||
mtoffl01 marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
| jvmConfig.origin == ConfigOrigin.JVM_PROP | ||
| } | ||
|
|
||
|
|
||
| // TODO: Add a check for which setting the collector recognizes as highest precedence | ||
|
|
||
| where: | ||
| configKey | envValue | jvmValue | expectedValue | expectedOrigin | ||
| configKey | envConfigValue | jvmConfigValue | expectedValue | expectedOrigin | ||
| // ConfigProvider.getMergedMap | ||
| 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 | ||
| // ConfigProvider.getOrderedMap | ||
|
|
@@ -89,7 +105,8 @@ class ConfigCollectorTest extends DDSpecification { | |
|
|
||
| def "default not-null config settings are collected"() { | ||
| expect: | ||
| def setting = ConfigCollector.get().collect().get(configKey) | ||
| def defaultConfigByKey = ConfigCollector.get().collect().get(ConfigOrigin.DEFAULT) | ||
| def setting = defaultConfigByKey.get(configKey) | ||
| setting.origin == ConfigOrigin.DEFAULT | ||
| setting.stringValue() == defaultValue | ||
|
|
||
|
|
@@ -105,7 +122,8 @@ class ConfigCollectorTest extends DDSpecification { | |
|
|
||
| def "default null config settings are also collected"() { | ||
| when: | ||
| ConfigSetting cs = ConfigCollector.get().collect().get(configKey) | ||
| def defaultConfigByKey = ConfigCollector.get().collect().get(ConfigOrigin.DEFAULT) | ||
| ConfigSetting cs = defaultConfigByKey.get(configKey) | ||
|
|
||
| then: | ||
| cs.key == configKey | ||
|
|
@@ -126,7 +144,8 @@ class ConfigCollectorTest extends DDSpecification { | |
|
|
||
| def "default empty maps and list config settings are collected as empty strings"() { | ||
| when: | ||
| ConfigSetting cs = ConfigCollector.get().collect().get(configKey) | ||
| def defaultConfigByKey = ConfigCollector.get().collect().get(ConfigOrigin.DEFAULT) | ||
| ConfigSetting cs = defaultConfigByKey.get(configKey) | ||
|
|
||
| then: | ||
| cs.key == configKey | ||
|
|
@@ -146,17 +165,17 @@ class ConfigCollectorTest extends DDSpecification { | |
| ConfigCollector.get().collect() | ||
|
|
||
| when: | ||
| ConfigCollector.get().put('key1', 'value1', ConfigOrigin.DEFAULT) | ||
| ConfigCollector.get().put('key2', 'value2', ConfigOrigin.ENV) | ||
| ConfigCollector.get().put('key1', 'replaced', ConfigOrigin.REMOTE) | ||
| ConfigCollector.get().put('key3', 'value3', ConfigOrigin.JVM_PROP) | ||
| ConfigCollector.get().put('key1', 'value1', ConfigOrigin.DEFAULT, ABSENT_SEQ_ID) | ||
| ConfigCollector.get().put('key2', 'value2', ConfigOrigin.ENV, ABSENT_SEQ_ID) | ||
| ConfigCollector.get().put('key1', 'value4', ConfigOrigin.REMOTE, ABSENT_SEQ_ID) | ||
| ConfigCollector.get().put('key3', 'value3', ConfigOrigin.JVM_PROP, ABSENT_SEQ_ID) | ||
|
|
||
| then: | ||
| ConfigCollector.get().collect().values().toSet() == [ | ||
| ConfigSetting.of('key1', 'replaced', ConfigOrigin.REMOTE), | ||
| ConfigSetting.of('key2', 'value2', ConfigOrigin.ENV), | ||
| ConfigSetting.of('key3', 'value3', ConfigOrigin.JVM_PROP) | ||
| ] as Set | ||
| def collected = ConfigCollector.get().collect() | ||
| collected.get(ConfigOrigin.REMOTE).get('key1') == ConfigSetting.of('key1', 'value4', ConfigOrigin.REMOTE) | ||
| collected.get(ConfigOrigin.ENV).get('key2') == ConfigSetting.of('key2', 'value2', ConfigOrigin.ENV) | ||
| collected.get(ConfigOrigin.JVM_PROP).get('key3') == ConfigSetting.of('key3', 'value3', ConfigOrigin.JVM_PROP) | ||
| collected.get(ConfigOrigin.DEFAULT).get('key1') == ConfigSetting.of('key1', 'value1', ConfigOrigin.DEFAULT) | ||
| } | ||
|
|
||
|
|
||
|
|
@@ -165,18 +184,19 @@ class ConfigCollectorTest extends DDSpecification { | |
| ConfigCollector.get().collect() | ||
|
|
||
| when: | ||
| ConfigCollector.get().put('DD_API_KEY', 'sensitive data', ConfigOrigin.ENV) | ||
| ConfigCollector.get().put('DD_API_KEY', 'sensitive data', ConfigOrigin.ENV, ABSENT_SEQ_ID) | ||
|
|
||
| then: | ||
| ConfigCollector.get().collect().get('DD_API_KEY').stringValue() == '<hidden>' | ||
| def collected = ConfigCollector.get().collect() | ||
| collected.get(ConfigOrigin.ENV).get('DD_API_KEY').stringValue() == '<hidden>' | ||
| } | ||
|
|
||
| def "collects common setting default values"() { | ||
| when: | ||
| def settings = ConfigCollector.get().collect() | ||
| def defaultConfigByKey = ConfigCollector.get().collect().get(ConfigOrigin.DEFAULT) | ||
|
|
||
| then: | ||
| def setting = settings.get(key) | ||
| def setting = defaultConfigByKey.get(key) | ||
|
|
||
| setting.key == key | ||
| setting.stringValue() == value | ||
|
|
@@ -207,10 +227,10 @@ class ConfigCollectorTest extends DDSpecification { | |
| injectEnvConfig("DD_TRACE_SAMPLE_RATE", "0.3") | ||
|
|
||
| when: | ||
| def settings = ConfigCollector.get().collect() | ||
| def envConfigByKey = ConfigCollector.get().collect().get(ConfigOrigin.ENV) | ||
|
|
||
| then: | ||
| def setting = settings.get(key) | ||
| def setting = envConfigByKey.get(key) | ||
|
|
||
| setting.key == key | ||
| setting.stringValue() == value | ||
|
|
@@ -230,6 +250,67 @@ class ConfigCollectorTest extends DDSpecification { | |
| "trace.sample.rate" | "0.3" | ||
| } | ||
|
|
||
| def "config collector creates ConfigSettings with correct seqId"() { | ||
| setup: | ||
| ConfigCollector.get().collect() // clear previous state | ||
|
|
||
| when: | ||
| // Simulate sources with increasing precedence and a default | ||
| ConfigCollector.get().put("test.key", "default", ConfigOrigin.DEFAULT, ConfigSetting.DEFAULT_SEQ_ID) | ||
| ConfigCollector.get().put("test.key", "env", ConfigOrigin.ENV, 2) | ||
| ConfigCollector.get().put("test.key", "jvm", ConfigOrigin.JVM_PROP, 3) | ||
| ConfigCollector.get().put("test.key", "remote", ConfigOrigin.REMOTE, 4) | ||
|
|
||
| then: | ||
| def collected = ConfigCollector.get().collect() | ||
| def defaultSetting = collected.get(ConfigOrigin.DEFAULT).get("test.key") | ||
| def envSetting = collected.get(ConfigOrigin.ENV).get("test.key") | ||
| def jvmSetting = collected.get(ConfigOrigin.JVM_PROP).get("test.key") | ||
| def remoteSetting = collected.get(ConfigOrigin.REMOTE).get("test.key") | ||
|
|
||
| defaultSetting.seqId == ConfigSetting.DEFAULT_SEQ_ID | ||
| // Higher precedence = higher seqId | ||
| defaultSetting.seqId < envSetting.seqId | ||
| envSetting.seqId < jvmSetting.seqId | ||
| jvmSetting.seqId < remoteSetting.seqId | ||
| } | ||
|
|
||
| def "getAppliedConfigSetting returns highest precedence setting"() { | ||
| setup: | ||
| ConfigCollector.get().collect() // clear previous state | ||
|
|
||
| when: | ||
| // Add multiple settings for the same key with different seqIds | ||
| ConfigCollector.get().put("test.key", "default-value", ConfigOrigin.DEFAULT, ConfigSetting.DEFAULT_SEQ_ID) | ||
| ConfigCollector.get().put("test.key", "env-value", ConfigOrigin.ENV, 2) | ||
| ConfigCollector.get().put("test.key", "jvm-value", ConfigOrigin.JVM_PROP, 5) | ||
| ConfigCollector.get().put("test.key", "remote-value", ConfigOrigin.REMOTE, 3) | ||
|
|
||
| // Add another key with only one setting | ||
| ConfigCollector.get().put("single.key", "only-value", ConfigOrigin.ENV, 1) | ||
|
|
||
| def collected = ConfigCollector.get().collect() | ||
|
|
||
| then: | ||
| // Should return the setting with highest seqId (5) | ||
| def appliedSetting = ConfigCollector.getAppliedConfigSetting("test.key", collected) | ||
| appliedSetting != null | ||
| appliedSetting.value == "jvm-value" | ||
| appliedSetting.origin == ConfigOrigin.JVM_PROP | ||
| appliedSetting.seqId == 5 | ||
|
|
||
| // Should return the only setting for single.key | ||
| def singleSetting = ConfigCollector.getAppliedConfigSetting("single.key", collected) | ||
| singleSetting != null | ||
| singleSetting.value == "only-value" | ||
| singleSetting.origin == ConfigOrigin.ENV | ||
| singleSetting.seqId == 1 | ||
|
|
||
| // Should return null for non-existent key | ||
| def nonExistentSetting = ConfigCollector.getAppliedConfigSetting("non.existent.key", collected) | ||
| nonExistentSetting == null | ||
| } | ||
|
|
||
| def "config id is null for non-StableConfigSource"() { | ||
| setup: | ||
| def key = "test.key" | ||
|
|
@@ -243,10 +324,32 @@ class ConfigCollectorTest extends DDSpecification { | |
|
|
||
| then: | ||
| // Verify the config was collected but without a config ID | ||
| def setting = settings.get(key) | ||
| def setting = settings.get(ConfigOrigin.JVM_PROP).get(key) | ||
| setting != null | ||
| setting.configId == null | ||
| setting.value == value | ||
| setting.origin == ConfigOrigin.JVM_PROP | ||
| } | ||
|
|
||
| def "default sources cannot be overridden"() { | ||
| setup: | ||
| def key = "test.key" | ||
| def value = "test-value" | ||
| def overrideVal = "override-value" | ||
| def defaultConfigByKey | ||
| ConfigSetting cs | ||
|
|
||
| when: | ||
| // Need to make 2 calls in a row because collect() will empty the map | ||
| ConfigCollector.get().putDefault(key, value) | ||
| ConfigCollector.get().putDefault(key, overrideVal) | ||
| defaultConfigByKey = ConfigCollector.get().collect().get(ConfigOrigin.DEFAULT) | ||
| cs = defaultConfigByKey.get(key) | ||
|
|
||
| then: | ||
| cs.key == key | ||
| cs.stringValue() == value | ||
| cs.origin == ConfigOrigin.DEFAULT | ||
| cs.seqId == ConfigSetting.DEFAULT_SEQ_ID | ||
| } | ||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What do you think about adding an
updatemethod toConfigCollector? or mayberemotePut?The new method would just take a key and new value - the origin would always be
ConfigOrigin.REMOTEand the sequence id would be absent.This simplifies remote-config calls to
ConfigCollectorand also provides a clear code path for such calls...There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
great idea