Implement better exception handling and debug logging for StableConfig parsing errors

Author: mtoffl01

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

@@ -84,7 +84,6 @@ public static StableConfigSource.StableConfig parse(String filePath) throws IOEx
       if (configId != null) {
         return new StableConfigSource.StableConfig(configId, Collections.emptyMap());
       }
-
     } catch (IOException e) {
       log.debug("Failed to read the stable configuration file: {}", filePath, e);
     }

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

@@ -3,6 +3,7 @@
 import static datadog.trace.util.Strings.propertyNameToEnvironmentVariableName;
 
 import datadog.trace.api.ConfigOrigin;
+import datadog.trace.bootstrap.config.provider.stableconfig.StableConfigMappingException;
 import java.io.File;
 import java.util.Collections;
 import java.util.Map;
@@ -37,11 +38,21 @@ public final class StableConfigSource extends ConfigProvider.Source {
     try {
       log.debug("Stable configuration file found at path: {}", file);
       cfg = StableConfigParser.parse(filePath);
-    } catch (Throwable e) {
-      log.warn(
-          "Encountered the following exception when attempting to read stable configuration file at path: {}, dropping configs.",
-          file,
-          e);
+    } catch (StableConfigMappingException
+        | IllegalArgumentException
+        | ClassCastException
+        | NullPointerException e) {
+      log.warn("YAML mapping error in stable configuration file {}: {}", filePath, e.getMessage());
+      cfg = StableConfig.EMPTY;
+    } catch (Exception e) {
+      if (log.isDebugEnabled()) {
+        log.error("Unexpected error while reading stable configuration file {}: {}", filePath, e);
+      } else {
+        log.error(
+            "Unexpected error while reading stable configuration file {}: {}",
+            filePath,
+            e.getMessage());
+      }
       cfg = StableConfig.EMPTY;
     }
     this.config = cfg;

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

@@ -26,12 +26,34 @@ public Rule(List<Selector> selectors, Map<String, Object> configuration) {
   }
 
   public Rule(Object yaml) {
+    if (!(yaml instanceof Map)) {
+      throw new StableConfigMappingException(
+          "Rule must be a map, but got: " + yaml.getClass().getSimpleName());
+    }
     Map map = (Map) yaml;
+
+    Object selectorsObj = map.get("selectors");
+    if (selectorsObj == null) {
+      throw new StableConfigMappingException("Missing 'selectors' in rule: " + map);
+    }
+    if (!(selectorsObj instanceof List)) {
+      throw new StableConfigMappingException(
+          "'selectors' must be a list, but got: " + selectorsObj.getClass().getSimpleName());
+    }
     selectors =
         unmodifiableList(
-            ((List<Object>) map.get("selectors"))
+            ((List<Object>) selectorsObj)
                 .stream().filter(Objects::nonNull).map(Selector::new).collect(toList()));
-    configuration = unmodifiableMap((Map<String, Object>) map.get("configuration"));
+
+    Object configObj = map.get("configuration");
+    if (configObj == null) {
+      throw new StableConfigMappingException("Missing 'configuration' in rule: " + map);
+    }
+    if (!(configObj instanceof Map)) {
+      throw new StableConfigMappingException(
+          "'configuration' must be a map, but got: " + configObj.getClass().getSimpleName());
+    }
+    configuration = unmodifiableMap((Map<String, Object>) configObj);
   }
 
   public List<Selector> getSelectors() {

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

@@ -18,13 +18,43 @@ public Selector(String origin, String key, List<String> matches, String operator
   }
 
   public Selector(Object yaml) {
+    if (!(yaml instanceof Map)) {
+      throw new StableConfigMappingException(
+          "Selector must be a map, but got: " + yaml.getClass().getSimpleName());
+    }
     Map map = (Map) yaml;
-    origin = (String) map.get("origin");
-    key = (String) map.get("key");
-    List<String> rawMatches = (List<String>) map.get("matches");
+
+    Object originObj = map.get("origin");
+    if (originObj == null) {
+      throw new StableConfigMappingException("Missing 'origin' in selector: " + map);
+    }
+    if (!(originObj instanceof String)) {
+      throw new StableConfigMappingException(
+          "'origin' must be a string, but got: " + originObj.getClass().getSimpleName());
+    }
+    origin = (String) originObj;
+
+    Object keyObj = map.get("key");
+    key = (keyObj instanceof String) ? (String) keyObj : null;
+
+    Object matchesObj = map.get("matches");
+    if (matchesObj != null && !(matchesObj instanceof List)) {
+      throw new StableConfigMappingException(
+          "'matches' must be a list, but got: " + matchesObj.getClass().getSimpleName());
+    }
+    List<String> rawMatches = (List<String>) matchesObj;
     matches =
         rawMatches != null ? Collections.unmodifiableList(rawMatches) : Collections.emptyList();
-    operator = (String) map.get("operator");
+
+    Object operatorObj = map.get("operator");
+    if (operatorObj == null) {
+      throw new StableConfigMappingException("Missing 'operator' in selector: " + map);
+    }
+    if (!(operatorObj instanceof String)) {
+      throw new StableConfigMappingException(
+          "'operator' must be a string, but got: " + operatorObj.getClass().getSimpleName());
+    }
+    operator = (String) operatorObj;
   }
 
   public String getOrigin() {

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

@@ -0,0 +1,11 @@
+package datadog.trace.bootstrap.config.provider.stableconfig;
+
+public class StableConfigMappingException extends RuntimeException {
+  public StableConfigMappingException(String message) {
+    super(message);
+  }
+
+  public StableConfigMappingException(String message, Throwable cause) {
+    super(message, cause);
+  }
+}

internal-api/src/test/groovy/datadog/trace/bootstrap/config/provider/StableConfigSourceTest.groovy

@@ -9,6 +9,10 @@ import datadog.trace.bootstrap.config.provider.stableconfig.StableConfig
 import datadog.trace.test.util.DDSpecification
 import org.snakeyaml.engine.v2.api.Dump
 import org.snakeyaml.engine.v2.api.DumpSettings
+import ch.qos.logback.classic.Logger
+import ch.qos.logback.classic.spi.ILoggingEvent
+import ch.qos.logback.core.read.ListAppender
+import org.slf4j.LoggerFactory
 
 import java.nio.file.Files
 import java.nio.file.Path
@@ -108,6 +112,78 @@ class StableConfigSourceTest extends DDSpecification {
     "12345"  | ["DD_KEY_ONE": "one", "DD_KEY_TWO": "two"] | Arrays.asList(sampleMatchingRule, sampleNonMatchingRule)
   }
 
+  def "test parse invalid logs mapping errors"() {
+    given:
+    Logger logbackLogger = (Logger) LoggerFactory.getLogger(datadog.trace.bootstrap.config.provider.StableConfigSource)
+    def listAppender = new ListAppender<ILoggingEvent>()
+    listAppender.start()
+    logbackLogger.addAppender(listAppender)
+
+    def tempFile = File.createTempFile("testFile_", ".yaml")
+    tempFile.text = yaml
+
+    when:
+    def stableCfg = new StableConfigSource(tempFile.absolutePath, ConfigOrigin.LOCAL_STABLE_CONFIG)
+
+    then:
+    stableCfg.config == StableConfigSource.StableConfig.EMPTY
+    def warnLogs = listAppender.list.findAll { it.level.toString() == 'WARN' }
+    warnLogs.any { it.formattedMessage.contains(expectedLogSubstring) }
+
+    cleanup:
+    tempFile.delete()
+    logbackLogger.detachAppender(listAppender)
+
+    where:
+    yaml                                                                                           | expectedLogSubstring
+    // Missing 'origin' in selector
+    '''apm_configuration_rules:
+         - selectors:
+             - key: "someKey"
+               matches: ["someValue"]
+               operator: equals
+           configuration:
+             DD_SERVICE: "test"
+    '''                                                                                             | "Missing 'origin' in selector"
+    // Missing 'configuration' in rule
+    '''apm_configuration_rules:
+         - selectors:
+             - origin: process_arguments
+               key: "-Dfoo"
+               matches: ["bar"]
+               operator: equals
+    '''                                                                                             | "Missing 'configuration' in rule"
+    // Missing 'selectors' in rule
+    '''apm_configuration_rules:
+         - configuration:
+             DD_SERVICE: "test"
+    '''                                                                                             | "Missing 'selectors' in rule"
+    // Triggers ClassCastException (selectors should be a list, not a string)
+    '''apm_configuration_rules:
+         - selectors: "not-a-list"
+           configuration:
+             DD_SERVICE: "test"
+    '''                                                                                             | "'selectors' must be a list, but got: String"
+    // configuration present but not a map
+    '''apm_configuration_rules:
+         - selectors:
+             - origin: process_arguments
+               key: "-Dfoo"
+               matches: ["bar"]
+               operator: equals
+           configuration: "not-a-map"
+    '''                                                                                             | "'configuration' must be a map, but got: String"
+    // configuration present but not a map (integer)
+    '''apm_configuration_rules:
+         - selectors:
+             - origin: process_arguments
+               key: "-Dfoo"
+               matches: ["bar"]
+               operator: equals
+           configuration: 12345
+    '''                                                                                             | "'configuration' must be a map, but got: Integer"
+  }
+
   // Corrupt YAML string variable used for testing, defined outside the 'where' block for readability
   def static corruptYaml = ''' 
         abc: 123