merge with configId changes made in master

Author: mtoffl01

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.

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

@@ -37,6 +37,20 @@ public void put(String key, Object value, ConfigOrigin origin, int seqId) {
     configMap.put(key, setting); // replaces any previous value for this key at origin
   }
 
+  public void put(String key, Object value, ConfigOrigin origin, String configId) {
+    ConfigSetting setting = ConfigSetting.of(key, value, origin, configId);
+    Map<String, ConfigSetting> configMap =
+        collected.computeIfAbsent(origin, k -> new ConcurrentHashMap<>());
+    configMap.put(key, setting); // replaces any previous value for this key at origin
+  }
+
+  public void put(String key, Object value, ConfigOrigin origin, int seqId, String configId) {
+    ConfigSetting setting = ConfigSetting.of(key, value, origin, seqId, configId);
+    Map<String, ConfigSetting> configMap =
+        collected.computeIfAbsent(origin, k -> new ConcurrentHashMap<>());
+    configMap.put(key, setting); // replaces any previous value for this key at origin
+  }
+
   public void putAll(Map<String, Object> keysAndValues, ConfigOrigin origin) {
     for (Map.Entry<String, Object> entry : keysAndValues.entrySet()) {
       put(entry.getKey(), entry.getValue(), origin);

internal-api/src/main/java/datadog/trace/api/ConfigSetting.java

@@ -11,28 +11,41 @@ public final class ConfigSetting {
   public final String key;
   public final Object value;
   public final ConfigOrigin origin;
-  public final int seqId;
 
+  public final int seqId;
   public static final int DEFAULT_SEQ_ID = 1;
   private static final int ABSENT_SEQ_ID = 0;
 
+  /** The config ID associated with this setting, or {@code null} if not applicable. */
+  public final String configId;
+
   private static final Set<String> 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, ABSENT_SEQ_ID);
+    return new ConfigSetting(key, value, origin, ABSENT_SEQ_ID, null);
   }
 
   public static ConfigSetting of(String key, Object value, ConfigOrigin origin, int seqId) {
-    return new ConfigSetting(key, value, origin, seqId);
+    return new ConfigSetting(key, value, origin, seqId, null);
+  }
+
+  public static ConfigSetting of(String key, Object value, ConfigOrigin origin, String configId) {
+    return new ConfigSetting(key, value, origin, ABSENT_SEQ_ID, configId);
+  }
+
+  public static ConfigSetting of(
+      String key, Object value, ConfigOrigin origin, int seqId, String configId) {
+    return new ConfigSetting(key, value, origin, seqId, configId);
   }
 
-  private ConfigSetting(String key, Object value, ConfigOrigin origin, int seqId) {
+  private ConfigSetting(String key, Object value, ConfigOrigin origin, int seqId, String configId) {
     this.key = key;
     this.value = CONFIG_FILTER_LIST.contains(key) ? "<hidden>" : value;
     this.origin = origin;
     this.seqId = seqId;
+    this.configId = configId;
   }
 
   public String normalizedKey() {
@@ -111,12 +124,13 @@ public boolean equals(Object o) {
     return key.equals(that.key)
         && Objects.equals(value, that.value)
         && origin == that.origin
-        && seqId == that.seqId;
+        && seqId == that.seqId
+        && Objects.equals(configId, that.configId);
   }
 
   @Override
   public int hashCode() {
-    return Objects.hash(key, value, origin, seqId);
+    return Objects.hash(key, value, origin, seqId, configId);
   }
 
   @Override
@@ -131,6 +145,8 @@ public String toString() {
         + origin
         + ", seqId="
         + seqId
+        + ", configId="
+        + configId
         + '}';
   }
 }

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

@@ -90,7 +90,8 @@ public String getString(String key, String defaultValue, String... aliases) {
       if (candidate != null) {
         value = candidate;
         if (collectConfig) {
-          ConfigCollector.get().put(key, candidate, source.origin(), seqId);
+          ConfigCollector.get()
+              .put(key, candidate, source.origin(), seqId, getConfigIdFromSource(source));
         }
       }
       seqId++;
@@ -108,23 +109,26 @@ public String getStringNotEmpty(String key, String defaultValue, String... alias
     }
     String value = null;
     ConfigOrigin origin = null;
+    String configId = null;
     int seqId = DEFAULT_SEQ_ID + 1;
     for (int i = sources.length - 1; i >= 0; i--) {
       ConfigProvider.Source source = sources[i];
-      String candidate = source.get(key, aliases);
+      String candidateValue = source.get(key, aliases);
+      String candidateConfigId = getConfigIdFromSource(source);
       if (collectConfig) {
-        ConfigCollector.get().put(key, candidate, source.origin(), seqId);
+        ConfigCollector.get().put(key, candidateValue, source.origin(), seqId, candidateConfigId);
       }
-      if (candidate != null && !candidate.trim().isEmpty()) {
-        value = candidate;
+      if (candidateValue != null && !candidateValue.trim().isEmpty()) {
+        value = candidateValue;
         origin = source.origin();
+        configId = candidateConfigId;
       }
       seqId++;
     }
     if (collectConfig) {
       // Re-report the chosen value post-trim, with the highest seqId
       if (value != null && origin != null) {
-        ConfigCollector.get().put(key, value, origin, seqId + 1);
+        ConfigCollector.get().put(key, value, origin, seqId + 1, configId);
       }
     }
     return value != null ? value : defaultValue;
@@ -142,16 +146,16 @@ public String getStringExcludingSource(
     int seqId = DEFAULT_SEQ_ID + 1;
     for (int i = sources.length - 1; i >= 0; i--) {
       ConfigProvider.Source source = sources[i];
-      // Do we still want to report telemetry in this case?
+      String candidate = source.get(key, aliases);
+      if (collectConfig) {
+        ConfigCollector.get()
+            .put(key, candidate, source.origin(), seqId, getConfigIdFromSource(source));
+      }
       if (excludedSource.isAssignableFrom(source.getClass())) {
         continue;
       }
-      String candidate = source.get(key, aliases);
       if (candidate != null) {
         value = candidate;
-        if (collectConfig) {
-          ConfigCollector.get().put(key, candidate, source.origin(), seqId);
-        }
       }
       seqId++;
     }
@@ -222,10 +226,12 @@ private <T> T get(String key, T defaultValue, Class<T> type, String... aliases)
     T value = null;
     ConfigOrigin origin = null;
     int seqId = DEFAULT_SEQ_ID + 1;
+    String configId = null;
     for (int i = sources.length - 1; i >= 0; i--) {
       String sourceValue = sources[i].get(key, aliases);
+      configId = getConfigIdFromSource(sources[i]);
       if (sourceValue != null && collectConfig) {
-        ConfigCollector.get().put(key, sourceValue, sources[i].origin(), seqId);
+        ConfigCollector.get().put(key, sourceValue, sources[i].origin(), seqId, configId);
       }
       try {
         T candidate = ConfigConverter.valueOf(sourceValue, type);
@@ -249,7 +255,7 @@ private <T> T get(String key, T defaultValue, Class<T> type, String... aliases)
     if (collectConfig) {
       // Re-report the chosen value and origin to ensure its seqId is higher than any error configs
       if (value != null && origin != null) {
-        ConfigCollector.get().put(key, value, origin, seqId + 1);
+        ConfigCollector.get().put(key, value, origin, seqId + 1, configId);
       }
     }
     return value != null ? value : defaultValue;
@@ -305,7 +311,8 @@ public Map<String, String> getMergedMap(String key, String... aliases) {
       if (!parsedMap.isEmpty()) {
         if (collectConfig) {
           seqId++;
-          ConfigCollector.get().put(key, parsedMap, sources[i].origin(), seqId);
+          ConfigCollector.get()
+              .put(key, parsedMap, sources[i].origin(), seqId, getConfigIdFromSource(sources[i]));
         }
         if (origin != ConfigOrigin.DEFAULT) {
           // if we already have a non-default origin, the value is calculated from multiple sources
@@ -340,7 +347,8 @@ public Map<String, String> getMergedTagsMap(String key, String... aliases) {
       if (!parsedMap.isEmpty()) {
         if (collectConfig) {
           seqId++;
-          ConfigCollector.get().put(key, parsedMap, sources[i].origin(), seqId);
+          ConfigCollector.get()
+              .put(key, parsedMap, sources[i].origin(), seqId, getConfigIdFromSource(sources[i]));
         }
         if (origin != ConfigOrigin.DEFAULT) {
           // if we already have a non-default origin, the value is calculated from multiple sources
@@ -375,7 +383,8 @@ public Map<String, String> getOrderedMap(String key) {
       if (!parsedMap.isEmpty()) {
         if (collectConfig) {
           seqId++;
-          ConfigCollector.get().put(key, parsedMap, sources[i].origin(), seqId);
+          ConfigCollector.get()
+              .put(key, parsedMap, sources[i].origin(), seqId, getConfigIdFromSource(sources[i]));
         }
         if (origin != ConfigOrigin.DEFAULT) {
           // if we already have a non-default origin, the value is calculated from multiple sources
@@ -412,7 +421,8 @@ public Map<String, String> getMergedMapWithOptionalMappings(
         if (!parsedMap.isEmpty()) {
           if (collectConfig) {
             seqId++;
-            ConfigCollector.get().put(key, parsedMap, sources[i].origin(), seqId);
+            ConfigCollector.get()
+                .put(key, parsedMap, sources[i].origin(), seqId, getConfigIdFromSource(sources[i]));
           }
           if (origin != ConfigOrigin.DEFAULT) {
             // if we already have a non-default origin, the value is calculated from multiple
@@ -603,6 +613,13 @@ private static Properties loadConfigurationFile(ConfigProvider configProvider) {
     return properties;
   }
 
+  private static String getConfigIdFromSource(Source source) {
+    if (source instanceof StableConfigSource) {
+      return ((StableConfigSource) source).getConfigId();
+    }
+    return null;
+  }
+
   public abstract static class Source {
     public final String get(String key, String... aliases) {
       String value = get(key);

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<Object, Object> map = (Map<Object, Object>) 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<String, Object>) map.getOrDefault("apm_configuration_default", emptyMap()));