From 53b46a3204443c0d8be0e5161f9f4a59ff2f9144 Mon Sep 17 00:00:00 2001 From: Mikayla Toffler Date: Tue, 29 Jul 2025 15:41:10 -0400 Subject: [PATCH 01/11] initial introduction of config_id --- .../datadog/trace/api/ConfigCollector.java | 5 +++++ .../java/datadog/trace/api/ConfigSetting.java | 19 +++++++++++++++---- .../config/provider/ConfigProvider.java | 7 ++++++- 3 files changed, 26 insertions(+), 5 deletions(-) diff --git a/internal-api/src/main/java/datadog/trace/api/ConfigCollector.java b/internal-api/src/main/java/datadog/trace/api/ConfigCollector.java index f4cfda6877b..9923fc4739b 100644 --- a/internal-api/src/main/java/datadog/trace/api/ConfigCollector.java +++ b/internal-api/src/main/java/datadog/trace/api/ConfigCollector.java @@ -27,6 +27,11 @@ public void put(String key, Object value, ConfigOrigin origin) { collected.put(key, setting); } + public void put(String key, Object value, ConfigOrigin origin, String configId) { + ConfigSetting setting = ConfigSetting.of(key, value, origin); + collected.put(key, setting); + } + public void putAll(Map keysAndValues, ConfigOrigin origin) { // attempt merge+replace to avoid collector seeing partial update Map merged = diff --git a/internal-api/src/main/java/datadog/trace/api/ConfigSetting.java b/internal-api/src/main/java/datadog/trace/api/ConfigSetting.java index b688d5b477d..852380e146b 100644 --- a/internal-api/src/main/java/datadog/trace/api/ConfigSetting.java +++ b/internal-api/src/main/java/datadog/trace/api/ConfigSetting.java @@ -11,19 +11,25 @@ public final class ConfigSetting { public final String key; public final Object value; public final ConfigOrigin origin; + public final String configId; private static final Set CONFIG_FILTER_LIST = new HashSet<>( Arrays.asList("DD_API_KEY", "dd.api-key", "dd.profiling.api-key", "dd.profiling.apikey")); public static ConfigSetting of(String key, Object value, ConfigOrigin origin) { - return new ConfigSetting(key, value, origin); + return new ConfigSetting(key, value, origin, null); } - private ConfigSetting(String key, Object value, ConfigOrigin origin) { + public static ConfigSetting of(String key, Object value, ConfigOrigin origin, String configId) { + return new ConfigSetting(key, value, origin, configId); + } + + private ConfigSetting(String key, Object value, ConfigOrigin origin, String configId) { this.key = key; this.value = CONFIG_FILTER_LIST.contains(key) ? "" : value; this.origin = origin; + this.configId = configId; } public String normalizedKey() { @@ -99,12 +105,15 @@ public boolean equals(Object o) { if (this == o) return true; if (o == null || getClass() != o.getClass()) return false; ConfigSetting that = (ConfigSetting) o; - return key.equals(that.key) && Objects.equals(value, that.value) && origin == that.origin; + return key.equals(that.key) + && Objects.equals(value, that.value) + && origin == that.origin + && configId.equals(that.configId); } @Override public int hashCode() { - return Objects.hash(key, value, origin); + return Objects.hash(key, value, origin, configId); } @Override @@ -117,6 +126,8 @@ public String toString() { + stringValue() + ", origin=" + origin + + ", configId=" + + configId + '}'; } } diff --git a/internal-api/src/main/java/datadog/trace/bootstrap/config/provider/ConfigProvider.java b/internal-api/src/main/java/datadog/trace/bootstrap/config/provider/ConfigProvider.java index 0a2715dd11a..e6912e23d60 100644 --- a/internal-api/src/main/java/datadog/trace/bootstrap/config/provider/ConfigProvider.java +++ b/internal-api/src/main/java/datadog/trace/bootstrap/config/provider/ConfigProvider.java @@ -78,7 +78,12 @@ public String getString(String key, String defaultValue, String... aliases) { String value = source.get(key, aliases); if (value != null) { if (collectConfig) { - ConfigCollector.get().put(key, value, source.origin()); + if (source instanceof StableConfigSource) { + String configId = ((StableConfigSource) source).getConfigId(); + ConfigCollector.get().put(key, value, source.origin(), configId); + } else { + ConfigCollector.get().put(key, value, source.origin()); + } } return value; } From 27afb3a0e0603e408f91708cbea50e6a0351dc62 Mon Sep 17 00:00:00 2001 From: Mikayla Toffler Date: Mon, 4 Aug 2025 12:55:49 -0400 Subject: [PATCH 02/11] fix tests, fix NPE in ConfigSetting.equals, include configId in telemetryrequestbody when nonnull --- .../src/main/java/datadog/trace/api/ConfigSetting.java | 3 ++- .../src/main/java/datadog/telemetry/TelemetryRequestBody.java | 3 +++ 2 files changed, 5 insertions(+), 1 deletion(-) diff --git a/internal-api/src/main/java/datadog/trace/api/ConfigSetting.java b/internal-api/src/main/java/datadog/trace/api/ConfigSetting.java index 852380e146b..9b94b444a02 100644 --- a/internal-api/src/main/java/datadog/trace/api/ConfigSetting.java +++ b/internal-api/src/main/java/datadog/trace/api/ConfigSetting.java @@ -11,6 +11,7 @@ public final class ConfigSetting { public final String key; public final Object value; public final ConfigOrigin origin; + /** The config ID associated with this setting, or {@code null} if not applicable. */ public final String configId; private static final Set CONFIG_FILTER_LIST = @@ -108,7 +109,7 @@ public boolean equals(Object o) { return key.equals(that.key) && Objects.equals(value, that.value) && origin == that.origin - && configId.equals(that.configId); + && Objects.equals(configId, that.configId); } @Override diff --git a/telemetry/src/main/java/datadog/telemetry/TelemetryRequestBody.java b/telemetry/src/main/java/datadog/telemetry/TelemetryRequestBody.java index 9d9db047d24..b6679374862 100644 --- a/telemetry/src/main/java/datadog/telemetry/TelemetryRequestBody.java +++ b/telemetry/src/main/java/datadog/telemetry/TelemetryRequestBody.java @@ -230,6 +230,9 @@ public void writeConfiguration(ConfigSetting configSetting) throws IOException { bodyWriter.name("value").value(configSetting.stringValue()); bodyWriter.setSerializeNulls(false); bodyWriter.name("origin").value(configSetting.origin.value); + if (configSetting.configId != null) { + bodyWriter.name("configId").value(configSetting.configId); + } bodyWriter.endObject(); } From 6c9b852381a73cf64cc142848924e36f86306804 Mon Sep 17 00:00:00 2001 From: Mikayla Toffler Date: Wed, 13 Aug 2025 15:17:57 -0400 Subject: [PATCH 03/11] report config id for stableconfigsources in get --- buildSrc/.kotlin/errors/errors-1755104165118.log | 4 ++++ .../trace/bootstrap/config/provider/ConfigProvider.java | 7 ++++++- 2 files changed, 10 insertions(+), 1 deletion(-) create mode 100644 buildSrc/.kotlin/errors/errors-1755104165118.log diff --git a/buildSrc/.kotlin/errors/errors-1755104165118.log b/buildSrc/.kotlin/errors/errors-1755104165118.log new file mode 100644 index 00000000000..1219b509f09 --- /dev/null +++ b/buildSrc/.kotlin/errors/errors-1755104165118.log @@ -0,0 +1,4 @@ +kotlin version: 2.0.21 +error message: The daemon has terminated unexpectedly on startup attempt #1 with error code: 0. The daemon process output: + 1. Kotlin compile daemon is ready + diff --git a/internal-api/src/main/java/datadog/trace/bootstrap/config/provider/ConfigProvider.java b/internal-api/src/main/java/datadog/trace/bootstrap/config/provider/ConfigProvider.java index e6912e23d60..143a204f90a 100644 --- a/internal-api/src/main/java/datadog/trace/bootstrap/config/provider/ConfigProvider.java +++ b/internal-api/src/main/java/datadog/trace/bootstrap/config/provider/ConfigProvider.java @@ -202,7 +202,12 @@ private T get(String key, T defaultValue, Class type, String... aliases) T value = ConfigConverter.valueOf(sourceValue, type); if (value != null) { if (collectConfig) { - ConfigCollector.get().put(key, sourceValue, source.origin()); + if (source instanceof StableConfigSource) { + String configId = ((StableConfigSource) source).getConfigId(); + ConfigCollector.get().put(key, value, source.origin(), configId); + } else { + ConfigCollector.get().put(key, value, source.origin()); + } } return value; } From 3beff4ef0740293c5c87c0f90a95acb624a9699c Mon Sep 17 00:00:00 2001 From: Mikayla Toffler Date: Wed, 13 Aug 2025 15:26:57 -0400 Subject: [PATCH 04/11] Fix call to outdated ConfigSetting constructor in EventSourceTest --- .../src/test/groovy/datadog/telemetry/EventSourceTest.groovy | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/telemetry/src/test/groovy/datadog/telemetry/EventSourceTest.groovy b/telemetry/src/test/groovy/datadog/telemetry/EventSourceTest.groovy index 5a492c18171..5da3f0a3ea1 100644 --- a/telemetry/src/test/groovy/datadog/telemetry/EventSourceTest.groovy +++ b/telemetry/src/test/groovy/datadog/telemetry/EventSourceTest.groovy @@ -56,7 +56,7 @@ class EventSourceTest extends DDSpecification{ where: eventType | eventQueueName | eventInstance - "Config Change" | "configChangeQueue" | new ConfigSetting("key", "value", ConfigOrigin.ENV) + "Config Change" | "configChangeQueue" | ConfigSetting.of("key", "value", ConfigOrigin.ENV) "Integration" | "integrationQueue" | new Integration("name", true) "Dependency" | "dependencyQueue" | new Dependency("name", "version", "type", null) "Metric" | "metricQueue" | new Metric() From 9fb3500766da20adc20f9e496af0979d05a797d0 Mon Sep 17 00:00:00 2001 From: Mikayla Toffler Date: Wed, 13 Aug 2025 16:16:26 -0400 Subject: [PATCH 05/11] fix config_id report --- .../src/main/java/datadog/trace/api/ConfigCollector.java | 2 +- .../src/main/java/datadog/telemetry/TelemetryRequestBody.java | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/internal-api/src/main/java/datadog/trace/api/ConfigCollector.java b/internal-api/src/main/java/datadog/trace/api/ConfigCollector.java index 9923fc4739b..6e4ebc0969c 100644 --- a/internal-api/src/main/java/datadog/trace/api/ConfigCollector.java +++ b/internal-api/src/main/java/datadog/trace/api/ConfigCollector.java @@ -28,7 +28,7 @@ public void put(String key, Object value, ConfigOrigin origin) { } public void put(String key, Object value, ConfigOrigin origin, String configId) { - ConfigSetting setting = ConfigSetting.of(key, value, origin); + ConfigSetting setting = ConfigSetting.of(key, value, origin, configId); collected.put(key, setting); } diff --git a/telemetry/src/main/java/datadog/telemetry/TelemetryRequestBody.java b/telemetry/src/main/java/datadog/telemetry/TelemetryRequestBody.java index b6679374862..1e6424227fa 100644 --- a/telemetry/src/main/java/datadog/telemetry/TelemetryRequestBody.java +++ b/telemetry/src/main/java/datadog/telemetry/TelemetryRequestBody.java @@ -231,7 +231,7 @@ public void writeConfiguration(ConfigSetting configSetting) throws IOException { bodyWriter.setSerializeNulls(false); bodyWriter.name("origin").value(configSetting.origin.value); if (configSetting.configId != null) { - bodyWriter.name("configId").value(configSetting.configId); + bodyWriter.name("config_id").value(configSetting.configId); } bodyWriter.endObject(); } From 36e6e8953639723e988b6ddcff12d02427510e22 Mon Sep 17 00:00:00 2001 From: Mikayla Toffler Date: Wed, 13 Aug 2025 16:34:39 -0400 Subject: [PATCH 06/11] fix where null config id was getting treated as string 'null' --- .../bootstrap/config/provider/stableconfig/StableConfig.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/internal-api/src/main/java/datadog/trace/bootstrap/config/provider/stableconfig/StableConfig.java b/internal-api/src/main/java/datadog/trace/bootstrap/config/provider/stableconfig/StableConfig.java index 58fa3c4f826..f7129309102 100644 --- a/internal-api/src/main/java/datadog/trace/bootstrap/config/provider/stableconfig/StableConfig.java +++ b/internal-api/src/main/java/datadog/trace/bootstrap/config/provider/stableconfig/StableConfig.java @@ -17,7 +17,7 @@ public final class StableConfig { public StableConfig(Object yaml) { Map map = (Map) yaml; - this.configId = String.valueOf(map.get("config_id")); + this.configId = map.get("config_id") == null ? null : String.valueOf(map.get("config_id")); this.apmConfigurationDefault = unmodifiableMap( (Map) map.getOrDefault("apm_configuration_default", emptyMap())); From 66a7b9350cfd1319cd1a6839cb869e7640ca4d8d Mon Sep 17 00:00:00 2001 From: Mikayla Toffler Date: Wed, 13 Aug 2025 17:11:34 -0400 Subject: [PATCH 07/11] report config id for all get methods --- .../config/provider/ConfigProvider.java | 44 +++++++++++++++++-- 1 file changed, 40 insertions(+), 4 deletions(-) diff --git a/internal-api/src/main/java/datadog/trace/bootstrap/config/provider/ConfigProvider.java b/internal-api/src/main/java/datadog/trace/bootstrap/config/provider/ConfigProvider.java index 143a204f90a..9c7498369aa 100644 --- a/internal-api/src/main/java/datadog/trace/bootstrap/config/provider/ConfigProvider.java +++ b/internal-api/src/main/java/datadog/trace/bootstrap/config/provider/ConfigProvider.java @@ -256,6 +256,7 @@ public List getSpacedList(String key) { public Map getMergedMap(String key, String... aliases) { Map merged = new HashMap<>(); ConfigOrigin origin = ConfigOrigin.DEFAULT; + String configId = null; // System properties take precedence over env // prior art: // https://docs.spring.io/spring-boot/docs/1.5.6.RELEASE/reference/html/boot-features-external-config.html @@ -264,12 +265,19 @@ public Map getMergedMap(String key, String... aliases) { String value = sources[i].get(key, aliases); Map parsedMap = ConfigConverter.parseMap(value, key); if (!parsedMap.isEmpty()) { + if (sources[i] instanceof StableConfigSource) { + configId = ((StableConfigSource) sources[i]).getConfigId(); + } origin = sources[i].origin(); } merged.putAll(parsedMap); } if (collectConfig) { - ConfigCollector.get().put(key, merged, origin); + if (originReportsConfigId(origin)) { + ConfigCollector.get().put(key, merged, origin, configId); + } else { + ConfigCollector.get().put(key, merged, origin); + } } return merged; } @@ -277,6 +285,7 @@ public Map getMergedMap(String key, String... aliases) { public Map getMergedTagsMap(String key, String... aliases) { Map merged = new HashMap<>(); ConfigOrigin origin = ConfigOrigin.DEFAULT; + String configId = null; // System properties take precedence over env // prior art: // https://docs.spring.io/spring-boot/docs/1.5.6.RELEASE/reference/html/boot-features-external-config.html @@ -286,12 +295,19 @@ public Map getMergedTagsMap(String key, String... aliases) { Map parsedMap = ConfigConverter.parseTraceTagsMap(value, ':', Arrays.asList(',', ' ')); if (!parsedMap.isEmpty()) { + if (sources[i] instanceof StableConfigSource) { + configId = ((StableConfigSource) sources[i]).getConfigId(); + } origin = sources[i].origin(); } merged.putAll(parsedMap); } if (collectConfig) { - ConfigCollector.get().put(key, merged, origin); + if (originReportsConfigId(origin)) { + ConfigCollector.get().put(key, merged, origin, configId); + } else { + ConfigCollector.get().put(key, merged, origin); + } } return merged; } @@ -299,6 +315,7 @@ public Map getMergedTagsMap(String key, String... aliases) { public Map getOrderedMap(String key) { LinkedHashMap merged = new LinkedHashMap<>(); ConfigOrigin origin = ConfigOrigin.DEFAULT; + String configId = null; // System properties take precedence over env // prior art: // https://docs.spring.io/spring-boot/docs/1.5.6.RELEASE/reference/html/boot-features-external-config.html @@ -307,12 +324,19 @@ public Map getOrderedMap(String key) { String value = sources[i].get(key); Map parsedMap = ConfigConverter.parseOrderedMap(value, key); if (!parsedMap.isEmpty()) { + if (sources[i] instanceof StableConfigSource) { + configId = ((StableConfigSource) sources[i]).getConfigId(); + } origin = sources[i].origin(); } merged.putAll(parsedMap); } if (collectConfig) { - ConfigCollector.get().put(key, merged, origin); + if (originReportsConfigId(origin)) { + ConfigCollector.get().put(key, merged, origin, configId); + } else { + ConfigCollector.get().put(key, merged, origin); + } } return merged; } @@ -321,6 +345,7 @@ public Map getMergedMapWithOptionalMappings( String defaultPrefix, boolean lowercaseKeys, String... keys) { Map merged = new HashMap<>(); ConfigOrigin origin = ConfigOrigin.DEFAULT; + String configId = null; // System properties take precedence over env // prior art: // https://docs.spring.io/spring-boot/docs/1.5.6.RELEASE/reference/html/boot-features-external-config.html @@ -331,12 +356,19 @@ public Map getMergedMapWithOptionalMappings( Map parsedMap = ConfigConverter.parseMapWithOptionalMappings(value, key, defaultPrefix, lowercaseKeys); if (!parsedMap.isEmpty()) { + if (sources[i] instanceof StableConfigSource) { + configId = ((StableConfigSource) sources[i]).getConfigId(); + } origin = sources[i].origin(); } merged.putAll(parsedMap); } if (collectConfig) { - ConfigCollector.get().put(key, merged, origin); + if (originReportsConfigId(origin)) { + ConfigCollector.get().put(key, merged, origin, configId); + } else { + ConfigCollector.get().put(key, merged, origin); + } } } return merged; @@ -505,6 +537,10 @@ private static Properties loadConfigurationFile(ConfigProvider configProvider) { return properties; } + private static boolean originReportsConfigId(ConfigOrigin origin) { + return origin == ConfigOrigin.FLEET_STABLE_CONFIG || origin == ConfigOrigin.LOCAL_STABLE_CONFIG; + } + public abstract static class Source { public final String get(String key, String... aliases) { String value = get(key); From f11a0fa3229bd8f6b70e883c4f9d11200dba2943 Mon Sep 17 00:00:00 2001 From: Mikayla Toffler Date: Wed, 13 Aug 2025 17:15:05 -0400 Subject: [PATCH 08/11] remove superfluous file --- buildSrc/.kotlin/errors/errors-1755104165118.log | 4 ---- 1 file changed, 4 deletions(-) delete mode 100644 buildSrc/.kotlin/errors/errors-1755104165118.log diff --git a/buildSrc/.kotlin/errors/errors-1755104165118.log b/buildSrc/.kotlin/errors/errors-1755104165118.log deleted file mode 100644 index 1219b509f09..00000000000 --- a/buildSrc/.kotlin/errors/errors-1755104165118.log +++ /dev/null @@ -1,4 +0,0 @@ -kotlin version: 2.0.21 -error message: The daemon has terminated unexpectedly on startup attempt #1 with error code: 0. The daemon process output: - 1. Kotlin compile daemon is ready - From 831e115fc6bfc2a1f967805336ce323465e83935 Mon Sep 17 00:00:00 2001 From: Mikayla Toffler Date: Thu, 14 Aug 2025 14:49:57 -0400 Subject: [PATCH 09/11] simplify configId resolution logic in configProvider --- .../config/provider/ConfigProvider.java | 61 +++++-------------- 1 file changed, 15 insertions(+), 46 deletions(-) diff --git a/internal-api/src/main/java/datadog/trace/bootstrap/config/provider/ConfigProvider.java b/internal-api/src/main/java/datadog/trace/bootstrap/config/provider/ConfigProvider.java index d731ba1cae8..338cc40ff08 100644 --- a/internal-api/src/main/java/datadog/trace/bootstrap/config/provider/ConfigProvider.java +++ b/internal-api/src/main/java/datadog/trace/bootstrap/config/provider/ConfigProvider.java @@ -79,12 +79,7 @@ public String getString(String key, String defaultValue, String... aliases) { String value = source.get(key, aliases); if (value != null) { if (collectConfig) { - if (source instanceof StableConfigSource) { - String configId = ((StableConfigSource) source).getConfigId(); - ConfigCollector.get().put(key, value, source.origin(), configId); - } else { - ConfigCollector.get().put(key, value, source.origin()); - } + ConfigCollector.get().put(key, value, source.origin(), getConfigIdFromSource(source)); } return value; } @@ -203,12 +198,7 @@ private T get(String key, T defaultValue, Class type, String... aliases) T value = ConfigConverter.valueOf(sourceValue, type); if (value != null) { if (collectConfig) { - if (source instanceof StableConfigSource) { - String configId = ((StableConfigSource) source).getConfigId(); - ConfigCollector.get().put(key, value, source.origin(), configId); - } else { - ConfigCollector.get().put(key, value, source.origin()); - } + ConfigCollector.get().put(key, value, source.origin(), getConfigIdFromSource(source)); } return value; } @@ -266,19 +256,13 @@ public Map getMergedMap(String key, String... aliases) { String value = sources[i].get(key, aliases); Map parsedMap = ConfigConverter.parseMap(value, key); if (!parsedMap.isEmpty()) { - if (sources[i] instanceof StableConfigSource) { - configId = ((StableConfigSource) sources[i]).getConfigId(); - } origin = sources[i].origin(); + configId = getConfigIdFromSource(sources[i]); } merged.putAll(parsedMap); } if (collectConfig) { - if (originReportsConfigId(origin)) { - ConfigCollector.get().put(key, merged, origin, configId); - } else { - ConfigCollector.get().put(key, merged, origin); - } + ConfigCollector.get().put(key, merged, origin, configId); } return merged; } @@ -296,19 +280,13 @@ public Map getMergedTagsMap(String key, String... aliases) { Map parsedMap = ConfigConverter.parseTraceTagsMap(value, ':', Arrays.asList(',', ' ')); if (!parsedMap.isEmpty()) { - if (sources[i] instanceof StableConfigSource) { - configId = ((StableConfigSource) sources[i]).getConfigId(); - } origin = sources[i].origin(); + configId = getConfigIdFromSource(sources[i]); } merged.putAll(parsedMap); } if (collectConfig) { - if (originReportsConfigId(origin)) { - ConfigCollector.get().put(key, merged, origin, configId); - } else { - ConfigCollector.get().put(key, merged, origin); - } + ConfigCollector.get().put(key, merged, origin, configId); } return merged; } @@ -325,19 +303,13 @@ public Map getOrderedMap(String key) { String value = sources[i].get(key); Map parsedMap = ConfigConverter.parseOrderedMap(value, key); if (!parsedMap.isEmpty()) { - if (sources[i] instanceof StableConfigSource) { - configId = ((StableConfigSource) sources[i]).getConfigId(); - } origin = sources[i].origin(); + configId = getConfigIdFromSource(sources[i]); } merged.putAll(parsedMap); } if (collectConfig) { - if (originReportsConfigId(origin)) { - ConfigCollector.get().put(key, merged, origin, configId); - } else { - ConfigCollector.get().put(key, merged, origin); - } + ConfigCollector.get().put(key, merged, origin, configId); } return merged; } @@ -357,19 +329,13 @@ public Map getMergedMapWithOptionalMappings( Map parsedMap = ConfigConverter.parseMapWithOptionalMappings(value, key, defaultPrefix, lowercaseKeys); if (!parsedMap.isEmpty()) { - if (sources[i] instanceof StableConfigSource) { - configId = ((StableConfigSource) sources[i]).getConfigId(); - } origin = sources[i].origin(); + configId = getConfigIdFromSource(sources[i]); } merged.putAll(parsedMap); } if (collectConfig) { - if (originReportsConfigId(origin)) { - ConfigCollector.get().put(key, merged, origin, configId); - } else { - ConfigCollector.get().put(key, merged, origin); - } + ConfigCollector.get().put(key, merged, origin, configId); } } return merged; @@ -541,8 +507,11 @@ private static Properties loadConfigurationFile(ConfigProvider configProvider) { return properties; } - private static boolean originReportsConfigId(ConfigOrigin origin) { - return origin == ConfigOrigin.FLEET_STABLE_CONFIG || origin == ConfigOrigin.LOCAL_STABLE_CONFIG; + private static String getConfigIdFromSource(Source source) { + if (source instanceof StableConfigSource) { + return ((StableConfigSource) source).getConfigId(); + } + return null; } public abstract static class Source { From 3f5f00744cf1712edea3585f4a3ecc29956fcb05 Mon Sep 17 00:00:00 2001 From: Mikayla Toffler Date: Wed, 20 Aug 2025 10:58:03 -0400 Subject: [PATCH 10/11] report sourceValue instead of value to ConfigCollector in get method --- .../trace/bootstrap/config/provider/ConfigProvider.java | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/internal-api/src/main/java/datadog/trace/bootstrap/config/provider/ConfigProvider.java b/internal-api/src/main/java/datadog/trace/bootstrap/config/provider/ConfigProvider.java index 338cc40ff08..da336ffa8c2 100644 --- a/internal-api/src/main/java/datadog/trace/bootstrap/config/provider/ConfigProvider.java +++ b/internal-api/src/main/java/datadog/trace/bootstrap/config/provider/ConfigProvider.java @@ -198,7 +198,8 @@ private T get(String key, T defaultValue, Class type, String... aliases) T value = ConfigConverter.valueOf(sourceValue, type); if (value != null) { if (collectConfig) { - ConfigCollector.get().put(key, value, source.origin(), getConfigIdFromSource(source)); + ConfigCollector.get() + .put(key, sourceValue, source.origin(), getConfigIdFromSource(source)); } return value; } From 04eca28ea7a8946f9cf982e5363a5da11dd2671d Mon Sep 17 00:00:00 2001 From: Mikayla Toffler Date: Thu, 21 Aug 2025 09:27:10 -0400 Subject: [PATCH 11/11] add tests for configid and no configid --- boolean-conversion-proposal.md | 139 ++++++++++++++++++ .../trace/api/ConfigCollectorTest.groovy | 21 +++ .../provider/StableConfigSourceTest.groovy | 39 ++++- 3 files changed, 198 insertions(+), 1 deletion(-) create mode 100644 boolean-conversion-proposal.md diff --git a/boolean-conversion-proposal.md b/boolean-conversion-proposal.md new file mode 100644 index 00000000000..0f8fdd56b8f --- /dev/null +++ b/boolean-conversion-proposal.md @@ -0,0 +1,139 @@ +# Proposal: Standardize Configuration Type Conversion Behavior + +## Summary + +We propose standardizing configuration type conversion behavior across all data types in the next major release, specifically changing how invalid boolean values are handled to match the behavior of other numeric types. + +## Current Problem + +### Inconsistent Behavior Between Data Types + +Currently, our configuration system exhibits inconsistent behavior when handling invalid values: + +**Numeric Types (Integer, Float, Double, Long):** +```java +// Environment: DD_SOME_INT=invalid_number +int value = configProvider.getInteger("some.int", 42); +// Result: 42 (default value) ✅ +``` + +**Boolean Type:** +```java +// Environment: DD_SOME_BOOL=invalid_boolean +boolean value = configProvider.getBoolean("some.bool", true); +// Result: false (hardcoded fallback) ❌ +``` + +### Why This Is Problematic + +1. **Unexpected Behavior**: Users expect invalid values to fall back to their explicitly provided defaults +2. **Silent Failures**: Invalid boolean configurations silently become `false`, making debugging difficult +3. **API Inconsistency**: Different data types behave differently for the same logical error condition +4. **Code Complexity**: Requires custom exception handling and special-case logic + +## Current Implementation (Temporary Workaround) + +To maintain backward compatibility while fixing our test suite, we implemented a temporary solution: + +```java +// ConfigConverter.java +static class InvalidBooleanValueException extends IllegalArgumentException { + // Custom exception for backward compatibility +} + +public static Boolean booleanValueOf(String value) { + // ... validation logic ... + throw new InvalidBooleanValueException("Invalid boolean value: " + value); +} + +// ConfigProvider.java +catch (ConfigConverter.InvalidBooleanValueException ex) { + // Special case: return false instead of default for backward compatibility + return (T) Boolean.FALSE; +} +``` + +This approach works but adds complexity and maintains the inconsistent behavior. + +## Proposed Solution + +### For Next Major Release: Standardize All Type Conversions + +1. **Remove Custom Boolean Logic**: Eliminate `InvalidBooleanValueException` and special handling +2. **Consistent Exception Handling**: All invalid type conversions throw `IllegalArgumentException` +3. **Consistent Fallback Behavior**: All types fall back to user-provided defaults + +### After the Change + +```java +// All types will behave consistently: +int intValue = configProvider.getInteger("key", 42); // invalid → 42 +boolean boolValue = configProvider.getBoolean("key", true); // invalid → true +float floatValue = configProvider.getFloat("key", 3.14f); // invalid → 3.14f +``` + +## Implementation Plan + +### Phase 1: Next Major Release +- Remove `InvalidBooleanValueException` class +- Update `ConfigConverter.booleanValueOf()` to throw `IllegalArgumentException` +- Remove special boolean handling from `ConfigProvider.get()` +- Update documentation and migration guide + +### Phase 2: Communication +- Release notes highlighting the breaking change +- Update configuration documentation with examples +- Provide migration guidance for affected users + +## Breaking Change Impact + +### Who Is Affected +- Users who rely on invalid boolean values defaulting to `false` +- Applications that depend on the current behavior for error handling + +### Migration Path +Users can adapt by: +1. **Fixing Invalid Configurations**: Update configurations to use valid boolean values +2. **Adjusting Defaults**: If they want `false` as fallback, explicitly set `false` as the default +3. **Adding Validation**: Implement application-level validation if needed + +### Example Migration +```java +// Before (relied on invalid → false) +boolean feature = configProvider.getBoolean("feature.enabled", true); + +// After (explicit about wanting false for invalid values) +boolean feature = configProvider.getBoolean("feature.enabled", false); +``` + +## Benefits + +### For Users +- **Predictable Behavior**: Invalid values consistently fall back to provided defaults +- **Better Debugging**: Clear error signals when configurations are invalid +- **Explicit Intent**: Default values clearly express intended fallback behavior + +### For Maintainers +- **Simplified Codebase**: Remove custom exception types and special-case logic +- **Consistent Testing**: All type conversions can be tested with the same patterns +- **Reduced Complexity**: Fewer edge cases and branches to maintain + +## Recommendation + +We recommend implementing this change in the next major release (e.g., v2.0) because: + +1. **Improves User Experience**: More predictable and debuggable configuration behavior +2. **Reduces Technical Debt**: Eliminates custom workarounds and special cases +3. **Aligns with Principle of Least Surprise**: Consistent behavior across all data types +4. **Proper Breaking Change Window**: Major release is the appropriate time for behavior changes + +## Questions for Discussion + +1. Do we agree this inconsistency is worth fixing with a breaking change? +2. Should we provide any additional migration tooling or validation? +3. Are there other configuration behaviors we should standardize at the same time? +4. What timeline works best for communicating this change to users? + +--- + +**Next Steps**: If approved, we'll create implementation issues and begin updating documentation for the next major release cycle. diff --git a/internal-api/src/test/groovy/datadog/trace/api/ConfigCollectorTest.groovy b/internal-api/src/test/groovy/datadog/trace/api/ConfigCollectorTest.groovy index fd16d9b56bf..5e49e44d84b 100644 --- a/internal-api/src/test/groovy/datadog/trace/api/ConfigCollectorTest.groovy +++ b/internal-api/src/test/groovy/datadog/trace/api/ConfigCollectorTest.groovy @@ -9,6 +9,7 @@ import datadog.trace.api.config.TraceInstrumentationConfig import datadog.trace.api.config.TracerConfig import datadog.trace.api.iast.telemetry.Verbosity import datadog.trace.api.naming.SpanNaming +import datadog.trace.bootstrap.config.provider.ConfigProvider import datadog.trace.test.util.DDSpecification import datadog.trace.util.Strings @@ -228,4 +229,24 @@ class ConfigCollectorTest extends DDSpecification { "logs.injection.enabled" | "false" "trace.sample.rate" | "0.3" } + + def "config id is null for non-StableConfigSource"() { + setup: + def key = "test.key" + def value = "test-value" + injectSysConfig(key, value) + + when: + // Trigger config collection by getting a value + ConfigProvider.getInstance().getString(key) + def settings = ConfigCollector.get().collect() + + then: + // Verify the config was collected but without a config ID + def setting = settings.get(key) + setting != null + setting.configId == null + setting.value == value + setting.origin == ConfigOrigin.JVM_PROP + } } diff --git a/internal-api/src/test/groovy/datadog/trace/bootstrap/config/provider/StableConfigSourceTest.groovy b/internal-api/src/test/groovy/datadog/trace/bootstrap/config/provider/StableConfigSourceTest.groovy index 4dee86f5ae7..866158edad0 100644 --- a/internal-api/src/test/groovy/datadog/trace/bootstrap/config/provider/StableConfigSourceTest.groovy +++ b/internal-api/src/test/groovy/datadog/trace/bootstrap/config/provider/StableConfigSourceTest.groovy @@ -1,5 +1,7 @@ package datadog.trace.bootstrap.config.provider +import datadog.trace.api.ConfigCollector + import static java.util.Collections.singletonMap import datadog.trace.api.ConfigOrigin @@ -251,7 +253,42 @@ class StableConfigSourceTest extends DDSpecification { } } - // Use this if you want to explicitly write/test configId + def "test config id exists in ConfigCollector when using StableConfigSource"() { + given: + Path filePath = Files.createTempFile("testFile_", ".yaml") + String expectedConfigId = "123" + + // Create YAML content with config_id and some configuration + def yamlContent = """ +config_id: ${expectedConfigId} +apm_configuration_default: + DD_SERVICE: test-service + DD_ENV: test-env +""" + Files.write(filePath, yamlContent.getBytes()) + + // Clear any existing collected config + ConfigCollector.get().collect().clear() + + when: + // Create StableConfigSource and ConfigProvider + StableConfigSource stableConfigSource = new StableConfigSource(filePath.toString(), ConfigOrigin.LOCAL_STABLE_CONFIG) + ConfigProvider configProvider = new ConfigProvider(stableConfigSource) + + // Trigger config collection by getting a value + configProvider.getString("SERVICE", "default-service") + + then: + def collectedConfigs = ConfigCollector.get().collect() + def serviceSetting = collectedConfigs.get("SERVICE") + serviceSetting.configId == expectedConfigId + serviceSetting.value == "test-service" + serviceSetting.origin == ConfigOrigin.LOCAL_STABLE_CONFIG + + cleanup: + Files.delete(filePath) + } + def writeFileRaw(Path filePath, String configId, String data) { data = configId + "\n" + data StandardOpenOption[] openOpts = [StandardOpenOption.WRITE] as StandardOpenOption[]