From c512b782c61771b3006c5de8dcb7480d1e3bdaca Mon Sep 17 00:00:00 2001 From: paullegranddc Date: Mon, 3 Feb 2025 15:59:00 +0100 Subject: [PATCH 01/41] Initial commit * Add Stable config source * Add snakeyaml to parse file --- internal-api/build.gradle | 2 + .../java/datadog/trace/api/ConfigOrigin.java | 4 ++ .../config/provider/StableConfigSource.java | 63 +++++++++++++++++++ 3 files changed, 69 insertions(+) create mode 100644 internal-api/src/main/java/datadog/trace/bootstrap/config/provider/StableConfigSource.java diff --git a/internal-api/build.gradle b/internal-api/build.gradle index 1867ac148ba..3b3047448b9 100644 --- a/internal-api/build.gradle +++ b/internal-api/build.gradle @@ -221,6 +221,8 @@ dependencies { // it contains annotations that are also present in the instrumented application classes api "com.datadoghq:dd-javac-plugin-client:0.2.2" + implementation 'org.yaml:snakeyaml:2.0' + testImplementation project(":utils:test-utils") testImplementation("org.assertj:assertj-core:3.20.2") testImplementation libs.bundles.junit5 diff --git a/internal-api/src/main/java/datadog/trace/api/ConfigOrigin.java b/internal-api/src/main/java/datadog/trace/api/ConfigOrigin.java index 3f46985758a..dcd3a9edd49 100644 --- a/internal-api/src/main/java/datadog/trace/api/ConfigOrigin.java +++ b/internal-api/src/main/java/datadog/trace/api/ConfigOrigin.java @@ -7,6 +7,10 @@ public enum ConfigOrigin { REMOTE("remote_config"), /** configurations that are set through JVM properties */ JVM_PROP("jvm_prop"), + /** configuration read in the stable config file, managed by customers */ + CUSTOMER_STABLE_CONFIG("customer_stable_config"), + /** configuration read in the stable config file, managed by fleet */ + FLEET_STABLE_CONFIG("fleet_stable_config"), /** set when the user has not set any configuration for the key (defaults to a value) */ DEFAULT("default"); diff --git a/internal-api/src/main/java/datadog/trace/bootstrap/config/provider/StableConfigSource.java b/internal-api/src/main/java/datadog/trace/bootstrap/config/provider/StableConfigSource.java new file mode 100644 index 00000000000..a238133852d --- /dev/null +++ b/internal-api/src/main/java/datadog/trace/bootstrap/config/provider/StableConfigSource.java @@ -0,0 +1,63 @@ +package datadog.trace.bootstrap.config.provider; + +import java.io.File; +import java.io.FileInputStream; +import java.io.InputStream; +import java.util.HashMap; + +import static datadog.trace.util.Strings.propertyNameToEnvironmentVariableName; + +import datadog.trace.api.ConfigOrigin; +import datadog.trace.bootstrap.config.provider.ConfigProvider; + +import org.yaml.snakeyaml.Yaml; +import org.yaml.snakeyaml.LoaderOptions; +import org.yaml.snakeyaml.constructor.SafeConstructor; + +final public class StableConfigSource extends ConfigProvider.Source { + static final String MANAGED_STABLE_CONFIGURATION_PATH = "/etc/datadog-agent/managed/datadog-agent/stable/datadog_apm.yaml"; + static final String LOCAL_STABLE_CONFIGURATION_PATH = "/etc/datadog-agent/datadog_apm.yaml"; + + final ConfigOrigin fileOrigin; + HashMap configuration; + + StableConfigSource(String file, ConfigOrigin origin) { + this.fileOrigin = origin; + try { + configuration = parseStableConfig(file); + } catch (Exception e) { + configuration = new HashMap(); + } + } + + private static final HashMap parseStableConfig(String filePath) { + Yaml yaml = new Yaml(new SafeConstructor(new LoaderOptions())); + InputStream input = new FileInputStream(new File(filePath)); + Object data = yaml.load(input); + + HashMap config = new HashMap(); + + return config; + }; + + public final String get(String key) { + return configuration.get(propertyNameToEnvironmentVariableName(key)); + } + + public final ConfigOrigin origin() { + return fileOrigin; + } + + private class StableConfig { + private String config_id; + private HashMap apm_configuration_default; + + private void setApmConfigurationDefault(HashMap m) { + apm_configuration_default = m; + } + + private void setConfigId(String i) { + config_id = i; + } + } +} From dc449e01dcc82f3d1bf8a3f5c8f168407c7dd508 Mon Sep 17 00:00:00 2001 From: Mikayla Toffler Date: Tue, 4 Feb 2025 16:37:32 -0500 Subject: [PATCH 02/41] Initial tests for StableConfigSource --- .../java/datadog/trace/api/ConfigOrigin.java | 6 +- .../config/provider/StableConfigSource.java | 77 +++++++++++++------ .../provider/StableConfigSourceTest.groovy | 68 ++++++++++++++++ 3 files changed, 125 insertions(+), 26 deletions(-) create mode 100644 internal-api/src/test/groovy/datadog/trace/bootstrap/config/provider/StableConfigSourceTest.groovy diff --git a/internal-api/src/main/java/datadog/trace/api/ConfigOrigin.java b/internal-api/src/main/java/datadog/trace/api/ConfigOrigin.java index dcd3a9edd49..78cf4e20e2b 100644 --- a/internal-api/src/main/java/datadog/trace/api/ConfigOrigin.java +++ b/internal-api/src/main/java/datadog/trace/api/ConfigOrigin.java @@ -7,10 +7,10 @@ public enum ConfigOrigin { REMOTE("remote_config"), /** configurations that are set through JVM properties */ JVM_PROP("jvm_prop"), - /** configuration read in the stable config file, managed by customers */ - CUSTOMER_STABLE_CONFIG("customer_stable_config"), + /** configuration read in the stable config file, managed by users */ + USER_STABLE_CONFIG("user_stable_config"), /** configuration read in the stable config file, managed by fleet */ - FLEET_STABLE_CONFIG("fleet_stable_config"), + MANAGED_STABLE_CONFIG("managed_stable_config"), /** set when the user has not set any configuration for the key (defaults to a value) */ DEFAULT("default"); diff --git a/internal-api/src/main/java/datadog/trace/bootstrap/config/provider/StableConfigSource.java b/internal-api/src/main/java/datadog/trace/bootstrap/config/provider/StableConfigSource.java index a238133852d..57c1ba78b28 100644 --- a/internal-api/src/main/java/datadog/trace/bootstrap/config/provider/StableConfigSource.java +++ b/internal-api/src/main/java/datadog/trace/bootstrap/config/provider/StableConfigSource.java @@ -1,54 +1,85 @@ package datadog.trace.bootstrap.config.provider; -import java.io.File; -import java.io.FileInputStream; -import java.io.InputStream; -import java.util.HashMap; - import static datadog.trace.util.Strings.propertyNameToEnvironmentVariableName; import datadog.trace.api.ConfigOrigin; -import datadog.trace.bootstrap.config.provider.ConfigProvider; - -import org.yaml.snakeyaml.Yaml; +import java.io.File; +import java.io.IOException; +import java.io.InputStream; +import java.nio.file.Files; +import java.util.HashMap; +import java.util.Map; +import java.util.Set; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; import org.yaml.snakeyaml.LoaderOptions; +import org.yaml.snakeyaml.Yaml; import org.yaml.snakeyaml.constructor.SafeConstructor; -final public class StableConfigSource extends ConfigProvider.Source { - static final String MANAGED_STABLE_CONFIGURATION_PATH = "/etc/datadog-agent/managed/datadog-agent/stable/datadog_apm.yaml"; - static final String LOCAL_STABLE_CONFIGURATION_PATH = "/etc/datadog-agent/datadog_apm.yaml"; +public final class StableConfigSource extends ConfigProvider.Source { + static final String USER_STABLE_CONFIG_PATH = "/etc/datadog-agent/application_monitoring.yaml"; + static final String MANAGED_STABLE_CONFIG_PATH = + "/etc/datadog-agent/managed/datadog-apm-libraries/stable/application_monitoring.yaml "; + private static final Logger log = LoggerFactory.getLogger(StableConfigSource.class); final ConfigOrigin fileOrigin; - HashMap configuration; + HashMap configuration; StableConfigSource(String file, ConfigOrigin origin) { this.fileOrigin = origin; try { configuration = parseStableConfig(file); } catch (Exception e) { - configuration = new HashMap(); + configuration = new HashMap<>(); } } - private static final HashMap parseStableConfig(String filePath) { + private static HashMap parseStableConfig(String filePath) throws IOException { + HashMap config = new HashMap<>(); + + // Check if the file exists + File file = new File(filePath); + if (!file.exists()) { + log.error("Stable configuration file does not exist at the specified path: {}", filePath); + return config; // Exit early or take other action as necessary + } + Yaml yaml = new Yaml(new SafeConstructor(new LoaderOptions())); - InputStream input = new FileInputStream(new File(filePath)); - Object data = yaml.load(input); + InputStream input = Files.newInputStream(new File(filePath).toPath()); + HashMap data = yaml.load(input); - HashMap config = new HashMap(); - - return config; + Object apmConfig = data.get("apm_configuration_default"); + if (apmConfig instanceof HashMap) { + HashMap tempConfig = (HashMap) apmConfig; + for (Map.Entry entry : tempConfig.entrySet()) { + if (entry.getKey() instanceof String && entry.getValue() != null) { + String key = String.valueOf(entry.getKey()); + Object value = entry.getValue(); + config.put(key, value); + } else { + log.debug("Configuration {} in unexpected format", entry.getKey()); + } + } + } else { + // do something + log.debug("File {} in unexpected format", filePath); + } + return config; }; - public final String get(String key) { - return configuration.get(propertyNameToEnvironmentVariableName(key)); + public String get(String key) { + return (String) configuration.get(propertyNameToEnvironmentVariableName(key)); + } + + public Set getKeys() { + return this.configuration.keySet(); } - public final ConfigOrigin origin() { + public ConfigOrigin origin() { return fileOrigin; } - private class StableConfig { + private static class StableConfig { private String config_id; private HashMap apm_configuration_default; 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 new file mode 100644 index 00000000000..dd429e97799 --- /dev/null +++ b/internal-api/src/test/groovy/datadog/trace/bootstrap/config/provider/StableConfigSourceTest.groovy @@ -0,0 +1,68 @@ +package datadog.trace.bootstrap.config.provider + +import datadog.trace.api.ConfigOrigin +import datadog.trace.test.util.DDSpecification +import org.yaml.snakeyaml.DumperOptions +import org.yaml.snakeyaml.Yaml + +class StableConfigSourceTest extends DDSpecification { + + def "test file doesn't exist"() { + setup: + StableConfigSource config = new StableConfigSource(StableConfigSource.USER_STABLE_CONFIG_PATH, ConfigOrigin.USER_STABLE_CONFIG) + + expect: + config.getKeys().size() == 0 + // How to check that the "file does not exist" error was logged? + } + + def "test valid file"() { + // test empty file + when: + def path = StableConfigSource.USER_STABLE_CONFIG_PATH + try { + File file = new File(path) + file.createNewFile() + } catch (IOException e) { + // fail fast? + System.out.println("Error creating file") + e.printStackTrace() + } + StableConfigSource config = new StableConfigSource(path, ConfigOrigin.USER_STABLE_CONFIG) + + then: + config.getKeys().size() == 0 + + // test populated file + // when: + // def key1 = "dd_first_key" + // def val1 = "dd_first_val" + // def key2 = "dd_second_key" + // def val2 = "dd_second_val" + // // Create the map that will be used to populate the config file + // Map data = new HashMap<>(); + // data.put("apm_configuration_default", new HashMap() {{ + // put(key1, val1); + // put(key2, val2); + // }}) + // + // DumperOptions options = new DumperOptions(); + // options.setDefaultFlowStyle(DumperOptions.FlowStyle.BLOCK); + // + // // Prepare to write the data map to the file in yaml format + // Yaml yaml = new Yaml(options); + // + // try (FileWriter writer = new FileWriter(path)) { + // yaml.dump(data, writer); + // } catch (IOException e) { + // System.err.println("Error writing to file: " + e.getMessage()); + // // fail fast? + // } + // + // then: + // StableConfigSource config2 = new StableConfigSource(StableConfigSource.USER_STABLE_CONFIG_PATH, ConfigOrigin.USER_STABLE_CONFIG); + // config2.getKeys().size() == 2 + // config2.get(key1) == val1 + // config2.get(key2) == val2 + } +} From 6d72f818f03c0f9170a89a81751205e1565c980b Mon Sep 17 00:00:00 2001 From: Mikayla Toffler Date: Tue, 4 Feb 2025 17:03:33 -0500 Subject: [PATCH 03/41] Add @Override for stableconfig get and configorigin methods --- .../bootstrap/config/provider/StableConfigSource.java | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/internal-api/src/main/java/datadog/trace/bootstrap/config/provider/StableConfigSource.java b/internal-api/src/main/java/datadog/trace/bootstrap/config/provider/StableConfigSource.java index 57c1ba78b28..2c3cb7f7248 100644 --- a/internal-api/src/main/java/datadog/trace/bootstrap/config/provider/StableConfigSource.java +++ b/internal-api/src/main/java/datadog/trace/bootstrap/config/provider/StableConfigSource.java @@ -67,18 +67,20 @@ private static HashMap parseStableConfig(String filePath) throws return config; }; + @Override public String get(String key) { return (String) configuration.get(propertyNameToEnvironmentVariableName(key)); } - public Set getKeys() { - return this.configuration.keySet(); - } - + @Override public ConfigOrigin origin() { return fileOrigin; } + public Set getKeys() { + return this.configuration.keySet(); + } + private static class StableConfig { private String config_id; private HashMap apm_configuration_default; From 92c81fdc3e4f9d9915b8b840938127a9808f0479 Mon Sep 17 00:00:00 2001 From: Mikayla Toffler Date: Tue, 4 Feb 2025 17:06:58 -0500 Subject: [PATCH 04/41] apply github code qual suggestions --- .../config/provider/StableConfigSource.java | 28 +++++++++---------- 1 file changed, 14 insertions(+), 14 deletions(-) diff --git a/internal-api/src/main/java/datadog/trace/bootstrap/config/provider/StableConfigSource.java b/internal-api/src/main/java/datadog/trace/bootstrap/config/provider/StableConfigSource.java index 2c3cb7f7248..6622eb0a802 100644 --- a/internal-api/src/main/java/datadog/trace/bootstrap/config/provider/StableConfigSource.java +++ b/internal-api/src/main/java/datadog/trace/bootstrap/config/provider/StableConfigSource.java @@ -23,7 +23,7 @@ public final class StableConfigSource extends ConfigProvider.Source { private static final Logger log = LoggerFactory.getLogger(StableConfigSource.class); final ConfigOrigin fileOrigin; - HashMap configuration; + Map configuration; StableConfigSource(String file, ConfigOrigin origin) { this.fileOrigin = origin; @@ -34,7 +34,7 @@ public final class StableConfigSource extends ConfigProvider.Source { } } - private static HashMap parseStableConfig(String filePath) throws IOException { + private static Map parseStableConfig(String filePath) throws IOException { HashMap config = new HashMap<>(); // Check if the file exists @@ -81,16 +81,16 @@ public Set getKeys() { return this.configuration.keySet(); } - private static class StableConfig { - private String config_id; - private HashMap apm_configuration_default; - - private void setApmConfigurationDefault(HashMap m) { - apm_configuration_default = m; - } - - private void setConfigId(String i) { - config_id = i; - } - } + // private static class StableConfig { + // private String config_id; + // private Map apm_configuration_default; + // + // private void setApmConfigurationDefault(HashMap m) { + // apm_configuration_default = m; + // } + // + // private void setConfigId(String i) { + // config_id = i; + // } + // } } From 635ba8a8e3ea23b52c4dfac8bf9d0387a1149f41 Mon Sep 17 00:00:00 2001 From: Mikayla Toffler Date: Wed, 5 Feb 2025 11:49:51 -0500 Subject: [PATCH 05/41] passing initial tests --- .../config/provider/StableConfigSource.java | 6 +- .../provider/StableConfigSourceTest.groovy | 84 +++++++++++-------- 2 files changed, 50 insertions(+), 40 deletions(-) diff --git a/internal-api/src/main/java/datadog/trace/bootstrap/config/provider/StableConfigSource.java b/internal-api/src/main/java/datadog/trace/bootstrap/config/provider/StableConfigSource.java index 6622eb0a802..4cb36dc6c7d 100644 --- a/internal-api/src/main/java/datadog/trace/bootstrap/config/provider/StableConfigSource.java +++ b/internal-api/src/main/java/datadog/trace/bootstrap/config/provider/StableConfigSource.java @@ -1,7 +1,5 @@ package datadog.trace.bootstrap.config.provider; -import static datadog.trace.util.Strings.propertyNameToEnvironmentVariableName; - import datadog.trace.api.ConfigOrigin; import java.io.File; import java.io.IOException; @@ -41,7 +39,7 @@ private static Map parseStableConfig(String filePath) throws IOE File file = new File(filePath); if (!file.exists()) { log.error("Stable configuration file does not exist at the specified path: {}", filePath); - return config; // Exit early or take other action as necessary + return config; } Yaml yaml = new Yaml(new SafeConstructor(new LoaderOptions())); @@ -69,7 +67,7 @@ private static Map parseStableConfig(String filePath) throws IOE @Override public String get(String key) { - return (String) configuration.get(propertyNameToEnvironmentVariableName(key)); + return (String) configuration.get(key); } @Override 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 dd429e97799..0a6abf29206 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 @@ -5,6 +5,10 @@ import datadog.trace.test.util.DDSpecification import org.yaml.snakeyaml.DumperOptions import org.yaml.snakeyaml.Yaml +import java.nio.file.Path +import java.nio.file.Files +import java.nio.file.StandardOpenOption + class StableConfigSourceTest extends DDSpecification { def "test file doesn't exist"() { @@ -19,50 +23,58 @@ class StableConfigSourceTest extends DDSpecification { def "test valid file"() { // test empty file when: - def path = StableConfigSource.USER_STABLE_CONFIG_PATH + Path filePath = null + StableConfigSource config = null try { - File file = new File(path) - file.createNewFile() + filePath = Files.createTempFile("testFile_", ".yaml") } catch (IOException e) { - // fail fast? - System.out.println("Error creating file") + println "Error creating file: ${e.message}" e.printStackTrace() + return // or throw new RuntimeException("File creation failed", e) + } + if (filePath != null) { + config = new StableConfigSource(filePath.toString(), ConfigOrigin.USER_STABLE_CONFIG) + } else { + return } - StableConfigSource config = new StableConfigSource(path, ConfigOrigin.USER_STABLE_CONFIG) then: config.getKeys().size() == 0 // test populated file - // when: - // def key1 = "dd_first_key" - // def val1 = "dd_first_val" - // def key2 = "dd_second_key" - // def val2 = "dd_second_val" - // // Create the map that will be used to populate the config file - // Map data = new HashMap<>(); - // data.put("apm_configuration_default", new HashMap() {{ - // put(key1, val1); - // put(key2, val2); - // }}) - // - // DumperOptions options = new DumperOptions(); - // options.setDefaultFlowStyle(DumperOptions.FlowStyle.BLOCK); - // - // // Prepare to write the data map to the file in yaml format - // Yaml yaml = new Yaml(options); - // - // try (FileWriter writer = new FileWriter(path)) { - // yaml.dump(data, writer); - // } catch (IOException e) { - // System.err.println("Error writing to file: " + e.getMessage()); - // // fail fast? - // } - // - // then: - // StableConfigSource config2 = new StableConfigSource(StableConfigSource.USER_STABLE_CONFIG_PATH, ConfigOrigin.USER_STABLE_CONFIG); - // config2.getKeys().size() == 2 - // config2.get(key1) == val1 - // config2.get(key2) == val2 + when: + def key1 = "dd_first_key" + def val1 = "dd_first_val" + def key2 = "dd_second_key" + def val2 = "dd_second_val" + // Create the map that will be used to populate the config file + Map data = new HashMap<>() + data.put("apm_configuration_default", new HashMap() {{ + put(key1, val1) + put(key2, val2) + }}) + + DumperOptions options = new DumperOptions() + options.setDefaultFlowStyle(DumperOptions.FlowStyle.BLOCK) + + // Prepare to write the data map to the file in yaml format + Yaml yaml = new Yaml(options) + String yamlString = yaml.dump(data) + + try { + StandardOpenOption[] openOpts = [StandardOpenOption.WRITE] as StandardOpenOption[] + Files.write(filePath, yamlString.getBytes(), openOpts) + println "YAML written to: $filePath" + } catch (IOException e) { + println "Error writing to file: ${e.message}" + // fail fast? + } + + then: + StableConfigSource config2 = new StableConfigSource(filePath.toString(), ConfigOrigin.USER_STABLE_CONFIG) + config2.getKeys().size() == 2 + config2.get(key1) == val1 + config2.get(key2) == val2 + Files.delete(filePath) } } From d5bf9638cfd1abd568e9a8be5cab9f23426b637e Mon Sep 17 00:00:00 2001 From: Mikayla Toffler Date: Wed, 5 Feb 2025 15:07:22 -0500 Subject: [PATCH 06/41] support config_id --- .../config/provider/StableConfigSource.java | 45 +++++++++++++------ .../provider/StableConfigSourceTest.groovy | 9 +++- 2 files changed, 39 insertions(+), 15 deletions(-) diff --git a/internal-api/src/main/java/datadog/trace/bootstrap/config/provider/StableConfigSource.java b/internal-api/src/main/java/datadog/trace/bootstrap/config/provider/StableConfigSource.java index 4cb36dc6c7d..8bba499dc9f 100644 --- a/internal-api/src/main/java/datadog/trace/bootstrap/config/provider/StableConfigSource.java +++ b/internal-api/src/main/java/datadog/trace/bootstrap/config/provider/StableConfigSource.java @@ -22,30 +22,45 @@ public final class StableConfigSource extends ConfigProvider.Source { final ConfigOrigin fileOrigin; Map configuration; + String configId; - StableConfigSource(String file, ConfigOrigin origin) { + StableConfigSource(String file, ConfigOrigin origin) throws IOException { this.fileOrigin = origin; - try { - configuration = parseStableConfig(file); - } catch (Exception e) { - configuration = new HashMap<>(); + HashMap data = readFile(file); + if (data == null) { + this.configuration = new HashMap<>(); + this.configId = null; + } else { + this.configId = (String) data.get("config_id"); // could be "" + this.configuration = parseStableConfig(data); } } - private static Map parseStableConfig(String filePath) throws IOException { - HashMap config = new HashMap<>(); - + private static HashMap readFile(String filePath) { // Check if the file exists File file = new File(filePath); if (!file.exists()) { - log.error("Stable configuration file does not exist at the specified path: {}", filePath); - return config; + log.debug( + "Stable configuration file does not exist at the specified path: {}, ignoring", filePath); + return new HashMap<>(); } Yaml yaml = new Yaml(new SafeConstructor(new LoaderOptions())); - InputStream input = Files.newInputStream(new File(filePath).toPath()); - HashMap data = yaml.load(input); + InputStream input; + try { + input = Files.newInputStream(new File(filePath).toPath()); + } catch (IOException e) { + // throw new RuntimeException(e); // Do we want to do this? Or fail more gracefully? + log.error("Unable to read from stable config file {}, dropping input", filePath); + return new HashMap<>(); + } + return yaml.load(input); + } + + private static Map parseStableConfig(HashMap data) + throws IOException { + HashMap config = new HashMap<>(); Object apmConfig = data.get("apm_configuration_default"); if (apmConfig instanceof HashMap) { HashMap tempConfig = (HashMap) apmConfig; @@ -60,7 +75,7 @@ private static Map parseStableConfig(String filePath) throws IOE } } else { // do something - log.debug("File {} in unexpected format", filePath); + log.debug("File in unexpected format"); } return config; }; @@ -79,6 +94,10 @@ public Set getKeys() { return this.configuration.keySet(); } + public String getConfigId() { + return this.configId; + } + // private static class StableConfig { // private String config_id; // private Map apm_configuration_default; 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 0a6abf29206..452ceb98124 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 @@ -17,14 +17,15 @@ class StableConfigSourceTest extends DDSpecification { expect: config.getKeys().size() == 0 + config.getConfigId() == null // How to check that the "file does not exist" error was logged? } def "test valid file"() { // test empty file when: - Path filePath = null - StableConfigSource config = null + Path filePath + StableConfigSource config try { filePath = Files.createTempFile("testFile_", ".yaml") } catch (IOException e) { @@ -40,15 +41,18 @@ class StableConfigSourceTest extends DDSpecification { then: config.getKeys().size() == 0 + config.getConfigId() == null // test populated file when: + def id = "12345" def key1 = "dd_first_key" def val1 = "dd_first_val" def key2 = "dd_second_key" def val2 = "dd_second_val" // Create the map that will be used to populate the config file Map data = new HashMap<>() + data.put("config_id", id) data.put("apm_configuration_default", new HashMap() {{ put(key1, val1) put(key2, val2) @@ -72,6 +76,7 @@ class StableConfigSourceTest extends DDSpecification { then: StableConfigSource config2 = new StableConfigSource(filePath.toString(), ConfigOrigin.USER_STABLE_CONFIG) + config2.getConfigId() == id config2.getKeys().size() == 2 config2.get(key1) == val1 config2.get(key2) == val2 From 9f03e0414308e61abbf517eee8f964b35ffc1093 Mon Sep 17 00:00:00 2001 From: Mikayla Toffler Date: Wed, 5 Feb 2025 16:11:38 -0500 Subject: [PATCH 07/41] Additional tests for invalid files --- .../config/provider/StableConfigSource.java | 14 +++- .../provider/StableConfigSourceTest.groovy | 73 ++++++++++++++----- 2 files changed, 63 insertions(+), 24 deletions(-) diff --git a/internal-api/src/main/java/datadog/trace/bootstrap/config/provider/StableConfigSource.java b/internal-api/src/main/java/datadog/trace/bootstrap/config/provider/StableConfigSource.java index 8bba499dc9f..e84fa6857fd 100644 --- a/internal-api/src/main/java/datadog/trace/bootstrap/config/provider/StableConfigSource.java +++ b/internal-api/src/main/java/datadog/trace/bootstrap/config/provider/StableConfigSource.java @@ -54,14 +54,20 @@ private static HashMap readFile(String filePath) { log.error("Unable to read from stable config file {}, dropping input", filePath); return new HashMap<>(); } - - return yaml.load(input); + try { + return yaml.load(input); + } catch (Exception e) { + log.error("YAML parsing error in stable config file {}: {}", filePath, e.getMessage()); + return new HashMap<>(); + } } - private static Map parseStableConfig(HashMap data) - throws IOException { + private static Map parseStableConfig(HashMap data) { HashMap config = new HashMap<>(); Object apmConfig = data.get("apm_configuration_default"); + if (apmConfig == null) { + return config; + } if (apmConfig instanceof HashMap) { HashMap tempConfig = (HashMap) apmConfig; for (Map.Entry entry : tempConfig.entrySet()) { 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 452ceb98124..f5c05959f39 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 @@ -21,7 +21,7 @@ class StableConfigSourceTest extends DDSpecification { // How to check that the "file does not exist" error was logged? } - def "test valid file"() { + def "test empty file"() { // test empty file when: Path filePath @@ -42,28 +42,47 @@ class StableConfigSourceTest extends DDSpecification { then: config.getKeys().size() == 0 config.getConfigId() == null + } - // test populated file + def "test populated file"() { when: - def id = "12345" - def key1 = "dd_first_key" - def val1 = "dd_first_val" - def key2 = "dd_second_key" - def val2 = "dd_second_val" - // Create the map that will be used to populate the config file - Map data = new HashMap<>() - data.put("config_id", id) - data.put("apm_configuration_default", new HashMap() {{ - put(key1, val1) - put(key2, val2) - }}) + Path filePath + StableConfigSource stableCfg + try { + filePath = Files.createTempFile("testFile_", ".yaml") + } catch (IOException e) { + println "Error creating file: ${e.message}" + e.printStackTrace() + return // or throw new RuntimeException("File creation failed", e) + } + if (filePath == null) { + return + } DumperOptions options = new DumperOptions() options.setDefaultFlowStyle(DumperOptions.FlowStyle.BLOCK) // Prepare to write the data map to the file in yaml format Yaml yaml = new Yaml(options) - String yamlString = yaml.dump(data) + String yamlString + if (corrupt == true) { + yamlString = ''' + abc: 123 + def + ghi "jkl" + lmn: 456 + ''' + } else { + Map data = new HashMap<>() + if (configId != null) { + data.put("config_id", configId) + } + if (configs != null) { + data.put("apm_configuration_default", configs) + } + + yamlString = yaml.dump(data) + } try { StandardOpenOption[] openOpts = [StandardOpenOption.WRITE] as StandardOpenOption[] @@ -74,12 +93,26 @@ class StableConfigSourceTest extends DDSpecification { // fail fast? } + stableCfg = new StableConfigSource(filePath.toString(), ConfigOrigin.USER_STABLE_CONFIG) + then: - StableConfigSource config2 = new StableConfigSource(filePath.toString(), ConfigOrigin.USER_STABLE_CONFIG) - config2.getConfigId() == id - config2.getKeys().size() == 2 - config2.get(key1) == val1 - config2.get(key2) == val2 + stableCfg.getConfigId() == configId + if (configs == null) { + stableCfg.getKeys().size() == 0 + } else { + stableCfg.getKeys().size() == configs.size() + } Files.delete(filePath) + + where: + key_one = "key_one" + val_one = "val_one" + key_two = "key_two" + val_two = "val_2" + configId | configs | corrupt + null | null | true + "" | new HashMap<>() | false + "12345" | new HashMap<>() << ["key_one": "val_one", "key_two": "val_two"] | false + } } From 06d572d8e7e2c7b711033b4516e7e47fe0642b98 Mon Sep 17 00:00:00 2001 From: Mikayla Toffler Date: Thu, 6 Feb 2025 16:13:09 -0500 Subject: [PATCH 08/41] Introduce StableConfigSource to the list of ConfigProvider sources --- .../config/provider/ConfigProvider.java | 27 +++- .../config/provider/StableConfigSource.java | 53 +++--- .../provider/StableConfigSourceTest.groovy | 153 ++++++++++++------ 3 files changed, 155 insertions(+), 78 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 d6418a56fef..49de5abc53c 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 @@ -353,19 +353,29 @@ public static ConfigProvider createDefault() { Properties configProperties = loadConfigurationFile( new ConfigProvider(new SystemPropertiesConfigSource(), new EnvironmentConfigSource())); + // MIKAYLA: What is the significance of configProperties, of this isEmpty check? if (configProperties.isEmpty()) { return new ConfigProvider( new SystemPropertiesConfigSource(), + new StableConfigSource( + StableConfigSource.MANAGED_STABLE_CONFIG_PATH, ConfigOrigin.MANAGED_STABLE_CONFIG), new EnvironmentConfigSource(), new OtelEnvironmentConfigSource(), - new CapturedEnvironmentConfigSource()); + new CapturedEnvironmentConfigSource(), // MIKAYLA: what is + // CapturedEnvironmentConfigSource? + new StableConfigSource( + StableConfigSource.USER_STABLE_CONFIG_PATH, ConfigOrigin.USER_STABLE_CONFIG)); } else { return new ConfigProvider( new SystemPropertiesConfigSource(), + new StableConfigSource( + StableConfigSource.MANAGED_STABLE_CONFIG_PATH, ConfigOrigin.MANAGED_STABLE_CONFIG), new EnvironmentConfigSource(), new PropertiesConfigSource(configProperties, true), new OtelEnvironmentConfigSource(configProperties), - new CapturedEnvironmentConfigSource()); + new CapturedEnvironmentConfigSource(), + new StableConfigSource( + StableConfigSource.USER_STABLE_CONFIG_PATH, ConfigOrigin.USER_STABLE_CONFIG)); } } @@ -378,20 +388,29 @@ public static ConfigProvider withoutCollector() { return new ConfigProvider( false, new SystemPropertiesConfigSource(), + new StableConfigSource( + StableConfigSource.MANAGED_STABLE_CONFIG_PATH, ConfigOrigin.MANAGED_STABLE_CONFIG), new EnvironmentConfigSource(), new OtelEnvironmentConfigSource(), - new CapturedEnvironmentConfigSource()); + new CapturedEnvironmentConfigSource(), + new StableConfigSource( + StableConfigSource.USER_STABLE_CONFIG_PATH, ConfigOrigin.USER_STABLE_CONFIG)); } else { return new ConfigProvider( false, new SystemPropertiesConfigSource(), + new StableConfigSource( + StableConfigSource.MANAGED_STABLE_CONFIG_PATH, ConfigOrigin.MANAGED_STABLE_CONFIG), new EnvironmentConfigSource(), new PropertiesConfigSource(configProperties, true), new OtelEnvironmentConfigSource(configProperties), - new CapturedEnvironmentConfigSource()); + new CapturedEnvironmentConfigSource(), + new StableConfigSource( + StableConfigSource.USER_STABLE_CONFIG_PATH, ConfigOrigin.USER_STABLE_CONFIG)); } } + // MIKAYLA: What is providedConfigSource, and how should it stand up against stableconfig? public static ConfigProvider withPropertiesOverride(Properties properties) { PropertiesConfigSource providedConfigSource = new PropertiesConfigSource(properties, false); Properties configProperties = diff --git a/internal-api/src/main/java/datadog/trace/bootstrap/config/provider/StableConfigSource.java b/internal-api/src/main/java/datadog/trace/bootstrap/config/provider/StableConfigSource.java index e84fa6857fd..8ada8f2c4bd 100644 --- a/internal-api/src/main/java/datadog/trace/bootstrap/config/provider/StableConfigSource.java +++ b/internal-api/src/main/java/datadog/trace/bootstrap/config/provider/StableConfigSource.java @@ -1,5 +1,7 @@ package datadog.trace.bootstrap.config.provider; +import static datadog.trace.util.Strings.propertyNameToEnvironmentVariableName; + import datadog.trace.api.ConfigOrigin; import java.io.File; import java.io.IOException; @@ -20,29 +22,40 @@ public final class StableConfigSource extends ConfigProvider.Source { "/etc/datadog-agent/managed/datadog-apm-libraries/stable/application_monitoring.yaml "; private static final Logger log = LoggerFactory.getLogger(StableConfigSource.class); - final ConfigOrigin fileOrigin; - Map configuration; - String configId; + private final ConfigOrigin fileOrigin; + private final Map configuration; + private final String configId; - StableConfigSource(String file, ConfigOrigin origin) throws IOException { + StableConfigSource(String file, ConfigOrigin origin) { this.fileOrigin = origin; - HashMap data = readFile(file); + HashMap data = readYamlFromFile(file); if (data == null) { this.configuration = new HashMap<>(); this.configId = null; } else { - this.configId = (String) data.get("config_id"); // could be "" + this.configId = (String) data.get("config_id"); this.configuration = parseStableConfig(data); } } - private static HashMap readFile(String filePath) { - // Check if the file exists + /** + * Reads configuration data from the YAML file located at the specified file path. + * + *

If the file is in a valid YAML format, this method returns a {@link HashMap} containing the + * configuration information. + * + *

If the file is in an invalid format, the method returns null. + * + * @param filePath The path to the YAML file to be read. + * @return A {@link HashMap} containing the configuration data if the file is valid, or null + * if the file is in an invalid format. + */ + private static HashMap readYamlFromFile(String filePath) { File file = new File(filePath); if (!file.exists()) { log.debug( "Stable configuration file does not exist at the specified path: {}, ignoring", filePath); - return new HashMap<>(); + return null; } Yaml yaml = new Yaml(new SafeConstructor(new LoaderOptions())); @@ -52,13 +65,13 @@ private static HashMap readFile(String filePath) { } catch (IOException e) { // throw new RuntimeException(e); // Do we want to do this? Or fail more gracefully? log.error("Unable to read from stable config file {}, dropping input", filePath); - return new HashMap<>(); + return null; } try { return yaml.load(input); } catch (Exception e) { log.error("YAML parsing error in stable config file {}: {}", filePath, e.getMessage()); - return new HashMap<>(); + return null; } } @@ -66,6 +79,7 @@ private static Map parseStableConfig(HashMap dat HashMap config = new HashMap<>(); Object apmConfig = data.get("apm_configuration_default"); if (apmConfig == null) { + return config; } if (apmConfig instanceof HashMap) { @@ -76,7 +90,7 @@ private static Map parseStableConfig(HashMap dat Object value = entry.getValue(); config.put(key, value); } else { - log.debug("Configuration {} in unexpected format", entry.getKey()); + log.debug("Config key {} in unexpected format", entry.getKey()); } } } else { @@ -88,7 +102,7 @@ private static Map parseStableConfig(HashMap dat @Override public String get(String key) { - return (String) configuration.get(key); + return (String) this.configuration.get(propertyNameToEnvironmentVariableName(key)); } @Override @@ -103,17 +117,4 @@ public Set getKeys() { public String getConfigId() { return this.configId; } - - // private static class StableConfig { - // private String config_id; - // private Map apm_configuration_default; - // - // private void setApmConfigurationDefault(HashMap m) { - // apm_configuration_default = m; - // } - // - // private void setConfigId(String i) { - // config_id = i; - // } - // } } 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 f5c05959f39..474a0d343eb 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 @@ -4,6 +4,7 @@ import datadog.trace.api.ConfigOrigin import datadog.trace.test.util.DDSpecification import org.yaml.snakeyaml.DumperOptions import org.yaml.snakeyaml.Yaml +import spock.lang.Shared import java.nio.file.Path import java.nio.file.Files @@ -18,7 +19,6 @@ class StableConfigSourceTest extends DDSpecification { expect: config.getKeys().size() == 0 config.getConfigId() == null - // How to check that the "file does not exist" error was logged? } def "test empty file"() { @@ -44,75 +44,132 @@ class StableConfigSourceTest extends DDSpecification { config.getConfigId() == null } - def "test populated file"() { + def "test get"() { when: - Path filePath - StableConfigSource stableCfg + Path filePath = tempFile() + if (filePath == null) { + return // fail? + } + + def configs = new HashMap<>() << ["DD_SERVICE": "svc", "DD_ENV": "env", "CONFIG_NO_DD": "value123"] + try { - filePath = Files.createTempFile("testFile_", ".yaml") + writeFileYaml(filePath, "12345", configs) } catch (IOException e) { - println "Error creating file: ${e.message}" - e.printStackTrace() - return // or throw new RuntimeException("File creation failed", e) + println "Error writing to file: ${e.message}" + return // fail? } + + StableConfigSource cfg = new StableConfigSource(filePath.toString(), ConfigOrigin.USER_STABLE_CONFIG) + + then: + cfg.getKeys().size() == 3 + cfg.get("service") == "svc" + cfg.get("env") == "env" + cfg.get("config_no_dd") == null + cfg.get("config_nonexistent") == null + } + + def "test file invalid format"() { + when: + Path filePath = tempFile() if (filePath == null) { - return + return // fail? } - DumperOptions options = new DumperOptions() - options.setDefaultFlowStyle(DumperOptions.FlowStyle.BLOCK) + try { + writeFileRaw(filePath, configId, configs) + } catch (IOException e) { + println "Error writing to file: ${e.message}" + return // fail? + } - // Prepare to write the data map to the file in yaml format - Yaml yaml = new Yaml(options) - String yamlString - if (corrupt == true) { - yamlString = ''' - abc: 123 - def - ghi "jkl" - lmn: 456 - ''' - } else { - Map data = new HashMap<>() - if (configId != null) { - data.put("config_id", configId) - } - if (configs != null) { - data.put("apm_configuration_default", configs) - } - - yamlString = yaml.dump(data) + StableConfigSource stableCfg = new StableConfigSource(filePath.toString(), ConfigOrigin.USER_STABLE_CONFIG) + + then: + stableCfg.getConfigId() == null + stableCfg.getKeys().size() == 0 + Files.delete(filePath) + + where: + configId | configs + null | corruptYaml + "12345" | "this is not yaml format!" + } + + def "test file valid format"() { + when: + Path filePath = tempFile() + if (filePath == null) { + return // fail? } try { - StandardOpenOption[] openOpts = [StandardOpenOption.WRITE] as StandardOpenOption[] - Files.write(filePath, yamlString.getBytes(), openOpts) - println "YAML written to: $filePath" + writeFileYaml(filePath, configId, configs) } catch (IOException e) { println "Error writing to file: ${e.message}" - // fail fast? + return // fail? } - stableCfg = new StableConfigSource(filePath.toString(), ConfigOrigin.USER_STABLE_CONFIG) + StableConfigSource stableCfg = new StableConfigSource(filePath.toString(), ConfigOrigin.USER_STABLE_CONFIG) then: stableCfg.getConfigId() == configId - if (configs == null) { - stableCfg.getKeys().size() == 0 - } else { - stableCfg.getKeys().size() == configs.size() + stableCfg.getKeys().size() == configs.size() + for (key in stableCfg.getKeys()) { + key = key.substring(4) // Cut `DD_` + stableCfg.get(key) == configs.get(key) } Files.delete(filePath) where: - key_one = "key_one" - val_one = "val_one" - key_two = "key_two" - val_two = "val_2" - configId | configs | corrupt - null | null | true - "" | new HashMap<>() | false - "12345" | new HashMap<>() << ["key_one": "val_one", "key_two": "val_two"] | false + configId | configs + "" | new HashMap<>() + "12345" | new HashMap<>() << ["DD_KEY_ONE": "one", "DD_KEY_TWO": "two"] + } + // Corrupt YAML string variable used for testing, defined outside the 'where' block for readability + @Shared + def corruptYaml = ''' + abc: 123 + def: + ghi: "jkl" + lmn: 456 + ''' + + Path tempFile() { + try { + return Files.createTempFile("testFile_", ".yaml") + } catch (IOException e) { + println "Error creating file: ${e.message}" + e.printStackTrace() + return null // or throw new RuntimeException("File creation failed", e) + } + } + + def writeFileYaml(Path filePath, String configId, Map configs) { + DumperOptions options = new DumperOptions() + options.setDefaultFlowStyle(DumperOptions.FlowStyle.BLOCK) + + // Prepare to write the data map to the file in yaml format + Yaml yaml = new Yaml(options) + String yamlString + Map data = new HashMap<>() + if (configId != null) { + data.put("config_id", configId) + } + if (configs instanceof HashMap) { + data.put("apm_configuration_default", configs) + } + + yamlString = yaml.dump(data) + + StandardOpenOption[] openOpts = [StandardOpenOption.WRITE] as StandardOpenOption[] + Files.write(filePath, yamlString.getBytes(), openOpts) + } + def writeFileRaw(Path filePath, String configId, String configs) { + String data = configId + "\n" + configs + StandardOpenOption[] openOpts = [StandardOpenOption.WRITE] as StandardOpenOption[] + Files.write(filePath, data.getBytes(), openOpts) } } From a03562cb89984dfe72bdc98fe56fb7fddad11e1d Mon Sep 17 00:00:00 2001 From: Mikayla Toffler Date: Fri, 7 Feb 2025 14:55:09 -0500 Subject: [PATCH 09/41] add comments to current tests --- .../java/datadog/trace/bootstrap/Agent.java | 21 ++++++ .../config/provider/ConfigProvider.java | 28 +++++--- .../config/provider/StableConfigSource.java | 19 +++-- .../datadog/trace/api/ConfigTest.groovy | 72 +++++++++++++++++++ .../provider/StableConfigSourceTest.groovy | 4 +- 5 files changed, 126 insertions(+), 18 deletions(-) diff --git a/dd-java-agent/agent-bootstrap/src/main/java/datadog/trace/bootstrap/Agent.java b/dd-java-agent/agent-bootstrap/src/main/java/datadog/trace/bootstrap/Agent.java index 1e60a79034b..2d88879947b 100644 --- a/dd-java-agent/agent-bootstrap/src/main/java/datadog/trace/bootstrap/Agent.java +++ b/dd-java-agent/agent-bootstrap/src/main/java/datadog/trace/bootstrap/Agent.java @@ -33,6 +33,7 @@ import datadog.trace.api.profiling.ProfilingEnablement; import datadog.trace.api.scopemanager.ScopeListener; import datadog.trace.bootstrap.benchmark.StaticEventLogger; +import datadog.trace.bootstrap.config.provider.StableConfigSource; import datadog.trace.bootstrap.instrumentation.api.AgentTracer; import datadog.trace.bootstrap.instrumentation.api.AgentTracer.TracerAPI; import datadog.trace.bootstrap.instrumentation.api.ProfilingContextIntegration; @@ -49,6 +50,7 @@ import java.net.URL; import java.security.CodeSource; import java.util.EnumSet; +import java.util.Map; import java.util.concurrent.TimeUnit; import java.util.concurrent.atomic.AtomicBoolean; import java.util.regex.PatternSyntaxException; @@ -1203,9 +1205,18 @@ private static boolean isFeatureEnabled(AgentFeature feature) { // must be kept in sync with logic from Config! final String featureEnabledSysprop = feature.getSystemProp(); String featureEnabled = System.getProperty(featureEnabledSysprop); + // MIKAYLA: Where to write tests for this? + if (featureEnabled == null) { + featureEnabled = + getStableConfig(featureEnabledSysprop, StableConfigSource.MANAGED_STABLE_CONFIG_PATH); + } if (featureEnabled == null) { featureEnabled = ddGetEnv(featureEnabledSysprop); } + if (featureEnabled == null) { + featureEnabled = + getStableConfig(featureEnabledSysprop, StableConfigSource.USER_STABLE_CONFIG_PATH); + } if (feature.isEnabledByDefault()) { // true unless it's explicitly set to "false" @@ -1215,6 +1226,7 @@ private static boolean isFeatureEnabled(AgentFeature feature) { // We need this hack because profiling in SSI can receive 'auto' value in // the enablement config return ProfilingEnablement.of(featureEnabled).isActive(); + // MIKAYLA: How does this order of precedence compete with stable config? } // false unless it's explicitly set to "true" return Boolean.parseBoolean(featureEnabled) || "1".equals(featureEnabled); @@ -1342,6 +1354,15 @@ private static String ddGetProperty(final String sysProp) { return value; } + private static String getStableConfig(final String sysProp, String filepath) { + String key = toEnvVar(sysProp); + Map data = StableConfigSource.readYamlFromFile(filepath); + if (data != null) { + return (String) data.get(key); + } + return null; + } + /** Looks for the "DD_" environment variable equivalent of the given "dd." system property. */ private static String ddGetEnv(final String sysProp) { return System.getenv(toEnvVar(sysProp)); 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 49de5abc53c..260cd261f85 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 @@ -353,7 +353,6 @@ public static ConfigProvider createDefault() { Properties configProperties = loadConfigurationFile( new ConfigProvider(new SystemPropertiesConfigSource(), new EnvironmentConfigSource())); - // MIKAYLA: What is the significance of configProperties, of this isEmpty check? if (configProperties.isEmpty()) { return new ConfigProvider( new SystemPropertiesConfigSource(), @@ -361,10 +360,9 @@ public static ConfigProvider createDefault() { StableConfigSource.MANAGED_STABLE_CONFIG_PATH, ConfigOrigin.MANAGED_STABLE_CONFIG), new EnvironmentConfigSource(), new OtelEnvironmentConfigSource(), - new CapturedEnvironmentConfigSource(), // MIKAYLA: what is - // CapturedEnvironmentConfigSource? new StableConfigSource( - StableConfigSource.USER_STABLE_CONFIG_PATH, ConfigOrigin.USER_STABLE_CONFIG)); + StableConfigSource.USER_STABLE_CONFIG_PATH, ConfigOrigin.USER_STABLE_CONFIG), + new CapturedEnvironmentConfigSource()); } else { return new ConfigProvider( new SystemPropertiesConfigSource(), @@ -373,9 +371,9 @@ public static ConfigProvider createDefault() { new EnvironmentConfigSource(), new PropertiesConfigSource(configProperties, true), new OtelEnvironmentConfigSource(configProperties), - new CapturedEnvironmentConfigSource(), new StableConfigSource( - StableConfigSource.USER_STABLE_CONFIG_PATH, ConfigOrigin.USER_STABLE_CONFIG)); + StableConfigSource.USER_STABLE_CONFIG_PATH, ConfigOrigin.USER_STABLE_CONFIG), + new CapturedEnvironmentConfigSource()); } } @@ -392,9 +390,9 @@ public static ConfigProvider withoutCollector() { StableConfigSource.MANAGED_STABLE_CONFIG_PATH, ConfigOrigin.MANAGED_STABLE_CONFIG), new EnvironmentConfigSource(), new OtelEnvironmentConfigSource(), - new CapturedEnvironmentConfigSource(), new StableConfigSource( - StableConfigSource.USER_STABLE_CONFIG_PATH, ConfigOrigin.USER_STABLE_CONFIG)); + StableConfigSource.USER_STABLE_CONFIG_PATH, ConfigOrigin.USER_STABLE_CONFIG), + new CapturedEnvironmentConfigSource()); } else { return new ConfigProvider( false, @@ -404,35 +402,43 @@ public static ConfigProvider withoutCollector() { new EnvironmentConfigSource(), new PropertiesConfigSource(configProperties, true), new OtelEnvironmentConfigSource(configProperties), - new CapturedEnvironmentConfigSource(), new StableConfigSource( - StableConfigSource.USER_STABLE_CONFIG_PATH, ConfigOrigin.USER_STABLE_CONFIG)); + StableConfigSource.USER_STABLE_CONFIG_PATH, ConfigOrigin.USER_STABLE_CONFIG), + new CapturedEnvironmentConfigSource()); } } - // MIKAYLA: What is providedConfigSource, and how should it stand up against stableconfig? public static ConfigProvider withPropertiesOverride(Properties properties) { PropertiesConfigSource providedConfigSource = new PropertiesConfigSource(properties, false); Properties configProperties = loadConfigurationFile( new ConfigProvider( new SystemPropertiesConfigSource(), + // MIKAYLA: To add StableConfig? new EnvironmentConfigSource(), providedConfigSource)); if (configProperties.isEmpty()) { return new ConfigProvider( new SystemPropertiesConfigSource(), + new StableConfigSource( + StableConfigSource.MANAGED_STABLE_CONFIG_PATH, ConfigOrigin.MANAGED_STABLE_CONFIG), new EnvironmentConfigSource(), providedConfigSource, new OtelEnvironmentConfigSource(), + new StableConfigSource( + StableConfigSource.USER_STABLE_CONFIG_PATH, ConfigOrigin.USER_STABLE_CONFIG), new CapturedEnvironmentConfigSource()); } else { return new ConfigProvider( providedConfigSource, new SystemPropertiesConfigSource(), + new StableConfigSource( + StableConfigSource.MANAGED_STABLE_CONFIG_PATH, ConfigOrigin.MANAGED_STABLE_CONFIG), new EnvironmentConfigSource(), new PropertiesConfigSource(configProperties, true), new OtelEnvironmentConfigSource(configProperties), + new StableConfigSource( + StableConfigSource.USER_STABLE_CONFIG_PATH, ConfigOrigin.USER_STABLE_CONFIG), new CapturedEnvironmentConfigSource()); } } diff --git a/internal-api/src/main/java/datadog/trace/bootstrap/config/provider/StableConfigSource.java b/internal-api/src/main/java/datadog/trace/bootstrap/config/provider/StableConfigSource.java index 8ada8f2c4bd..8e1c4d45616 100644 --- a/internal-api/src/main/java/datadog/trace/bootstrap/config/provider/StableConfigSource.java +++ b/internal-api/src/main/java/datadog/trace/bootstrap/config/provider/StableConfigSource.java @@ -17,15 +17,20 @@ import org.yaml.snakeyaml.constructor.SafeConstructor; public final class StableConfigSource extends ConfigProvider.Source { - static final String USER_STABLE_CONFIG_PATH = "/etc/datadog-agent/application_monitoring.yaml"; - static final String MANAGED_STABLE_CONFIG_PATH = - "/etc/datadog-agent/managed/datadog-apm-libraries/stable/application_monitoring.yaml "; + public static String USER_STABLE_CONFIG_PATH = + "/etc/datadog-agent/application_monitoring.yaml"; // MIKAYLA: this should be final, but I need + // to modify it in my tests because I can't + // write to /etc/ without sudo persmission. + public static String MANAGED_STABLE_CONFIG_PATH = + "/etc/datadog-agent/managed/datadog-apm-libraries/stable/application_monitoring.yaml "; // MIKAYLA: Same for this var. private static final Logger log = LoggerFactory.getLogger(StableConfigSource.class); private final ConfigOrigin fileOrigin; private final Map configuration; private final String configId; + // MIKAYLA: improvement - if we see that some cached map is already not null by the name the + // StableConfigurationSource constructor is called, we can skip calling it again. StableConfigSource(String file, ConfigOrigin origin) { this.fileOrigin = origin; HashMap data = readYamlFromFile(file); @@ -50,14 +55,18 @@ public final class StableConfigSource extends ConfigProvider.Source { * @return A {@link HashMap} containing the configuration data if the file is valid, or null * if the file is in an invalid format. */ - private static HashMap readYamlFromFile(String filePath) { + // MIKAYLA: Should improve this so that it caches the resulting map the first time it's called, + // that way we don't need to call readFromYaml twice; + // if we see that said cached map is already not null by the name the StableConfigurationSource + // constructor is called, we can skip calling it again. + public static HashMap readYamlFromFile(String filePath) { File file = new File(filePath); if (!file.exists()) { log.debug( "Stable configuration file does not exist at the specified path: {}, ignoring", filePath); return null; } - + System.out.println("file exists"); Yaml yaml = new Yaml(new SafeConstructor(new LoaderOptions())); InputStream input; try { diff --git a/internal-api/src/test/groovy/datadog/trace/api/ConfigTest.groovy b/internal-api/src/test/groovy/datadog/trace/api/ConfigTest.groovy index ae07c2d5cd6..a1f1b6da485 100644 --- a/internal-api/src/test/groovy/datadog/trace/api/ConfigTest.groovy +++ b/internal-api/src/test/groovy/datadog/trace/api/ConfigTest.groovy @@ -4,10 +4,14 @@ import datadog.trace.api.env.FixedCapturedEnvironment import datadog.trace.bootstrap.config.provider.AgentArgsInjector import datadog.trace.bootstrap.config.provider.ConfigConverter import datadog.trace.bootstrap.config.provider.ConfigProvider +import datadog.trace.bootstrap.config.provider.StableConfigSource +import datadog.trace.bootstrap.config.provider.StableConfigSourceTest import datadog.trace.test.util.DDSpecification import datadog.trace.util.throwable.FatalAgentMisconfigurationError import org.junit.Rule +import java.nio.file.Path + import static datadog.trace.api.ConfigDefaults.DEFAULT_HTTP_CLIENT_ERROR_STATUSES import static datadog.trace.api.ConfigDefaults.DEFAULT_HTTP_SERVER_ERROR_STATUSES import static datadog.trace.api.ConfigDefaults.DEFAULT_PARTIAL_FLUSH_MIN_SPANS @@ -26,6 +30,8 @@ import static datadog.trace.api.TracePropagationStyle.B3SINGLE import static datadog.trace.api.TracePropagationStyle.DATADOG import static datadog.trace.api.TracePropagationStyle.HAYSTACK import static datadog.trace.api.TracePropagationStyle.TRACECONTEXT +import static datadog.trace.api.config.AppSecConfig.APPSEC_ENABLED +import static datadog.trace.api.config.AppSecConfig.APPSEC_SCA_ENABLED import static datadog.trace.api.config.CiVisibilityConfig.CIVISIBILITY_AGENTLESS_ENABLED import static datadog.trace.api.config.CiVisibilityConfig.CIVISIBILITY_ENABLED import static datadog.trace.api.config.DebuggerConfig.DEBUGGER_CLASSFILE_DUMP_ENABLED @@ -56,6 +62,9 @@ import static datadog.trace.api.config.GeneralConfig.SITE import static datadog.trace.api.config.GeneralConfig.TAGS import static datadog.trace.api.config.GeneralConfig.TRACER_METRICS_IGNORED_RESOURCES import static datadog.trace.api.config.GeneralConfig.VERSION +import static datadog.trace.api.config.GeneralConfig.DATA_STREAMS_ENABLED +import static datadog.trace.api.config.GeneralConfig.DATA_JOBS_ENABLED +import static datadog.trace.api.config.IastConfig.IAST_ENABLED import static datadog.trace.api.config.JmxFetchConfig.JMX_FETCH_CHECK_PERIOD import static datadog.trace.api.config.JmxFetchConfig.JMX_FETCH_ENABLED import static datadog.trace.api.config.JmxFetchConfig.JMX_FETCH_METRICS_CONFIGS @@ -97,6 +106,7 @@ import static datadog.trace.api.config.TraceInstrumentationConfig.DB_CLIENT_HOST import static datadog.trace.api.config.TraceInstrumentationConfig.HTTP_CLIENT_HOST_SPLIT_BY_DOMAIN import static datadog.trace.api.config.TraceInstrumentationConfig.RUNTIME_CONTEXT_FIELD_INJECTION import static datadog.trace.api.config.TraceInstrumentationConfig.TRACE_ENABLED +import static datadog.trace.api.config.TraceInstrumentationConfig.LOGS_INJECTION import static datadog.trace.api.config.TracerConfig.AGENT_HOST import static datadog.trace.api.config.TracerConfig.AGENT_PORT_LEGACY import static datadog.trace.api.config.TracerConfig.AGENT_UNIX_DOMAIN_SOCKET @@ -567,6 +577,68 @@ class ConfigTest extends DDSpecification { config.getLongRunningTraceFlushInterval() == 81 } + def "specify overrides stable config"() { + setup: + Path filePath = StableConfigSourceTest.tempFile() + if (filePath == null) { + // fail fast? + return + } + // Override for testing + String originalPath + if (user == true) { + originalPath = StableConfigSource.USER_STABLE_CONFIG_PATH + StableConfigSource.USER_STABLE_CONFIG_PATH = filePath.toAbsolutePath().toString() + } else if (managed == true) { + originalPath = StableConfigSource.MANAGED_STABLE_CONFIG_PATH + StableConfigSource.MANAGED_STABLE_CONFIG_PATH = filePath.toAbsolutePath().toString() + } + def configs = new HashMap<>() + configs.put(TRACE_ENABLED, "false") + // configs.put(DD_RUNTIME_METRICS_ENABLED_ENV, "false") + // configs.put(LOGS_INJECTION, "false") + // configs.put(PROFILING_ENABLED, "true") + // configs.put(DATA_STREAMS_ENABLED, "true") + // configs.put(APPSEC_ENABLED, "true") + // configs.put(IAST_ENABLED, "true") + // configs.put(DEBUGGER_ENABLED, "true") + // configs.put(DATA_JOBS_ENABLED, "true") + // configs.put(APPSEC_SCA_ENABLED, "true") + + try { + StableConfigSourceTest.writeFileYaml(filePath, "12345", configs) + } catch (IOException e) { + println "Error writing to file: ${e.message}" + return // fail? + } + + when: + def config = new Config() + + then: + !config.traceEnabled + // !config.runtimeMetricsEnabled + // !config.logsInjectionEnabled + // config.profilingEnabled + // config.dataStreamsEnabled + // //config.appSecEnabled? + // config.iastDebugEnabled + // config.debuggerEnabled + // config.dataJobsEnabled + // config.appSecScaEnabled + if (user == true) { + StableConfigSource.USER_STABLE_CONFIG_PATH = originalPath + } else if (managed == true) { + StableConfigSource.MANAGED_STABLE_CONFIG_PATH = originalPath + } + + where: + user | managed + true | false + // false | true + + } + def "sys props override env vars"() { setup: environmentVariables.set(DD_SERVICE_NAME_ENV, "still something else") 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 474a0d343eb..31143149f92 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 @@ -136,7 +136,7 @@ class StableConfigSourceTest extends DDSpecification { lmn: 456 ''' - Path tempFile() { + static Path tempFile() { try { return Files.createTempFile("testFile_", ".yaml") } catch (IOException e) { @@ -146,7 +146,7 @@ class StableConfigSourceTest extends DDSpecification { } } - def writeFileYaml(Path filePath, String configId, Map configs) { + static writeFileYaml(Path filePath, String configId, Map configs) { DumperOptions options = new DumperOptions() options.setDefaultFlowStyle(DumperOptions.FlowStyle.BLOCK) From ac7b387f3c1f864777bbc519e8a90e19b16dc10e Mon Sep 17 00:00:00 2001 From: Mikayla Toffler <46911781+mtoffl01@users.noreply.github.com> Date: Fri, 7 Feb 2025 15:47:25 -0500 Subject: [PATCH 10/41] Update internal-api/src/main/java/datadog/trace/bootstrap/config/provider/StableConfigSource.java Co-authored-by: datadog-datadog-prod-us1[bot] <88084959+datadog-datadog-prod-us1[bot]@users.noreply.github.com> --- .../trace/bootstrap/config/provider/StableConfigSource.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/internal-api/src/main/java/datadog/trace/bootstrap/config/provider/StableConfigSource.java b/internal-api/src/main/java/datadog/trace/bootstrap/config/provider/StableConfigSource.java index 8e1c4d45616..0a6a6388486 100644 --- a/internal-api/src/main/java/datadog/trace/bootstrap/config/provider/StableConfigSource.java +++ b/internal-api/src/main/java/datadog/trace/bootstrap/config/provider/StableConfigSource.java @@ -59,7 +59,7 @@ public final class StableConfigSource extends ConfigProvider.Source { // that way we don't need to call readFromYaml twice; // if we see that said cached map is already not null by the name the StableConfigurationSource // constructor is called, we can skip calling it again. - public static HashMap readYamlFromFile(String filePath) { + public static Map readYamlFromFile(String filePath) { File file = new File(filePath); if (!file.exists()) { log.debug( From 908a72da3e3db92915f5065d11a6b19cc9f83f24 Mon Sep 17 00:00:00 2001 From: Mikayla Toffler Date: Fri, 7 Feb 2025 15:48:17 -0500 Subject: [PATCH 11/41] fix typo --- .../trace/bootstrap/config/provider/StableConfigSource.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/internal-api/src/main/java/datadog/trace/bootstrap/config/provider/StableConfigSource.java b/internal-api/src/main/java/datadog/trace/bootstrap/config/provider/StableConfigSource.java index 8e1c4d45616..3223632b8fc 100644 --- a/internal-api/src/main/java/datadog/trace/bootstrap/config/provider/StableConfigSource.java +++ b/internal-api/src/main/java/datadog/trace/bootstrap/config/provider/StableConfigSource.java @@ -29,7 +29,7 @@ public final class StableConfigSource extends ConfigProvider.Source { private final Map configuration; private final String configId; - // MIKAYLA: improvement - if we see that some cached map is already not null by the name the + // MIKAYLA: improvement - if we see that some cached map is already not null by the time the // StableConfigurationSource constructor is called, we can skip calling it again. StableConfigSource(String file, ConfigOrigin origin) { this.fileOrigin = origin; From 38355fe6a16be81b5dd259c089c3a562cc07ed28 Mon Sep 17 00:00:00 2001 From: Mikayla Toffler Date: Mon, 10 Feb 2025 14:36:45 -0500 Subject: [PATCH 12/41] Refactor: Introduce StableConfigSource sub-class, StableConfig --- .../java/datadog/trace/bootstrap/Agent.java | 30 ++-- .../config/provider/StableConfigSource.java | 141 +++++++++++------- .../provider/StableConfigSourceTest.groovy | 1 - 3 files changed, 101 insertions(+), 71 deletions(-) diff --git a/dd-java-agent/agent-bootstrap/src/main/java/datadog/trace/bootstrap/Agent.java b/dd-java-agent/agent-bootstrap/src/main/java/datadog/trace/bootstrap/Agent.java index 2d88879947b..e5d3e2ff722 100644 --- a/dd-java-agent/agent-bootstrap/src/main/java/datadog/trace/bootstrap/Agent.java +++ b/dd-java-agent/agent-bootstrap/src/main/java/datadog/trace/bootstrap/Agent.java @@ -41,6 +41,7 @@ import datadog.trace.util.AgentTaskScheduler; import datadog.trace.util.AgentThreadFactory.AgentThread; import datadog.trace.util.throwable.FatalAgentMisconfigurationError; +import java.io.IOException; import java.lang.instrument.Instrumentation; import java.lang.reflect.InvocationTargetException; import java.lang.reflect.Method; @@ -50,7 +51,6 @@ import java.net.URL; import java.security.CodeSource; import java.util.EnumSet; -import java.util.Map; import java.util.concurrent.TimeUnit; import java.util.concurrent.atomic.AtomicBoolean; import java.util.regex.PatternSyntaxException; @@ -1206,17 +1206,18 @@ private static boolean isFeatureEnabled(AgentFeature feature) { final String featureEnabledSysprop = feature.getSystemProp(); String featureEnabled = System.getProperty(featureEnabledSysprop); // MIKAYLA: Where to write tests for this? - if (featureEnabled == null) { - featureEnabled = - getStableConfig(featureEnabledSysprop, StableConfigSource.MANAGED_STABLE_CONFIG_PATH); - } + // if (featureEnabled == null) { + // featureEnabled = + // getStableConfig(featureEnabledSysprop, + // StableConfigSource.MANAGED_STABLE_CONFIG_PATH); + // } if (featureEnabled == null) { featureEnabled = ddGetEnv(featureEnabledSysprop); } - if (featureEnabled == null) { - featureEnabled = - getStableConfig(featureEnabledSysprop, StableConfigSource.USER_STABLE_CONFIG_PATH); - } + // if (featureEnabled == null) { + // featureEnabled = + // getStableConfig(featureEnabledSysprop, StableConfigSource.USER_STABLE_CONFIG_PATH); + // } if (feature.isEnabledByDefault()) { // true unless it's explicitly set to "false" @@ -1355,10 +1356,13 @@ private static String ddGetProperty(final String sysProp) { } private static String getStableConfig(final String sysProp, String filepath) { - String key = toEnvVar(sysProp); - Map data = StableConfigSource.readYamlFromFile(filepath); - if (data != null) { - return (String) data.get(key); + try { + return StableConfigSource.findAndReturnEarly( + StableConfigSource.readYamlFromFile(filepath), toEnvVar(sysProp)); + } catch (IOException e) { + log.error("Error reading from file: {}", e.getMessage()); + } catch (Exception e) { + log.error("Error processing file: {}", e.getMessage()); } return null; } diff --git a/internal-api/src/main/java/datadog/trace/bootstrap/config/provider/StableConfigSource.java b/internal-api/src/main/java/datadog/trace/bootstrap/config/provider/StableConfigSource.java index 3cb7b2cd03f..37976251065 100644 --- a/internal-api/src/main/java/datadog/trace/bootstrap/config/provider/StableConfigSource.java +++ b/internal-api/src/main/java/datadog/trace/bootstrap/config/provider/StableConfigSource.java @@ -17,80 +17,58 @@ import org.yaml.snakeyaml.constructor.SafeConstructor; public final class StableConfigSource extends ConfigProvider.Source { - public static String USER_STABLE_CONFIG_PATH = - "/etc/datadog-agent/application_monitoring.yaml"; // MIKAYLA: this should be final, but I need - // to modify it in my tests because I can't - // write to /etc/ without sudo persmission. - public static String MANAGED_STABLE_CONFIG_PATH = - "/etc/datadog-agent/managed/datadog-apm-libraries/stable/application_monitoring.yaml "; // MIKAYLA: Same for this var. + public static final String USER_STABLE_CONFIG_PATH = + "/etc/datadog-agent/application_monitoring.yaml"; + public static final String MANAGED_STABLE_CONFIG_PATH = + "/etc/datadog-agent/managed/datadog-apm-libraries/stable/application_monitoring.yaml "; + private static final Logger log = LoggerFactory.getLogger(StableConfigSource.class); + private final StableConfig config; private final ConfigOrigin fileOrigin; - private final Map configuration; - private final String configId; - // MIKAYLA: improvement - if we see that some cached map is already not null by the time the - // StableConfigurationSource constructor is called, we can skip calling it again. StableConfigSource(String file, ConfigOrigin origin) { this.fileOrigin = origin; - HashMap data = readYamlFromFile(file); - if (data == null) { - this.configuration = new HashMap<>(); - this.configId = null; - } else { - this.configId = (String) data.get("config_id"); - this.configuration = parseStableConfig(data); + this.config = newStableConfigFromFile(file); + } + + private static StableConfig newStableConfigFromFile(String file) { + try { + HashMap data = readYamlFromFile(file); + return buildStableConfig(data); + } catch (IOException e) { + log.error("Error reading from file: {}", e.getMessage()); + } catch (Exception e) { + log.error("Error processing file: {}", e.getMessage()); } + return new StableConfig(); } - /** - * Reads configuration data from the YAML file located at the specified file path. - * - *

If the file is in a valid YAML format, this method returns a {@link HashMap} containing the - * configuration information. - * - *

If the file is in an invalid format, the method returns null. - * - * @param filePath The path to the YAML file to be read. - * @return A {@link HashMap} containing the configuration data if the file is valid, or null - * if the file is in an invalid format. - */ - // MIKAYLA: Should improve this so that it caches the resulting map the first time it's called, - // that way we don't need to call readFromYaml twice; - // if we see that said cached map is already not null by the name the StableConfigurationSource - // constructor is called, we can skip calling it again. - public static Map readYamlFromFile(String filePath) { + public static HashMap readYamlFromFile(String filePath) + throws IOException, Exception { File file = new File(filePath); if (!file.exists()) { - log.debug( - "Stable configuration file does not exist at the specified path: {}, ignoring", filePath); - return null; + throw new IOException( + "Stable configuration file does not exist at the specified path: " + filePath); } - System.out.println("file exists"); Yaml yaml = new Yaml(new SafeConstructor(new LoaderOptions())); - InputStream input; - try { - input = Files.newInputStream(new File(filePath).toPath()); - } catch (IOException e) { - // throw new RuntimeException(e); // Do we want to do this? Or fail more gracefully? - log.error("Unable to read from stable config file {}, dropping input", filePath); - return null; - } + InputStream input = Files.newInputStream(new File(filePath).toPath()); try { return yaml.load(input); } catch (Exception e) { - log.error("YAML parsing error in stable config file {}: {}", filePath, e.getMessage()); - return null; + throw new Exception( + "YAML parsing error in stable config file " + filePath + ": " + e.getMessage(), e); } } - private static Map parseStableConfig(HashMap data) { - HashMap config = new HashMap<>(); + private static StableConfig buildStableConfig(HashMap data) { + if (data == null) { + return new StableConfig(); + } + String configId = (String) data.get("config_id"); Object apmConfig = data.get("apm_configuration_default"); - if (apmConfig == null) { - return config; - } + HashMap config = new HashMap<>(); if (apmConfig instanceof HashMap) { HashMap tempConfig = (HashMap) apmConfig; for (Map.Entry entry : tempConfig.entrySet()) { @@ -102,16 +80,33 @@ private static Map parseStableConfig(HashMap dat log.debug("Config key {} in unexpected format", entry.getKey()); } } + return new StableConfig(configId, config); } else { // do something log.debug("File in unexpected format"); + return new StableConfig(configId); } - return config; }; + public static String findAndReturnEarly(HashMap data, String key) { + if (data == null) { + return null; + } + Object apmConfig = data.get("apm_configuration_default"); + if (apmConfig instanceof HashMap) { + HashMap tempConfig = (HashMap) apmConfig; + for (Map.Entry entry : tempConfig.entrySet()) { + if (entry.getKey() == key) { + return (String) entry.getValue(); + } + } + } + return null; + } + @Override public String get(String key) { - return (String) this.configuration.get(propertyNameToEnvironmentVariableName(key)); + return this.config.get(propertyNameToEnvironmentVariableName(key)); } @Override @@ -120,10 +115,42 @@ public ConfigOrigin origin() { } public Set getKeys() { - return this.configuration.keySet(); + return this.config.getKeys(); } public String getConfigId() { - return this.configId; + return this.config.getConfigId(); + } + + private static class StableConfig { + private final Map apmConfiguration; + private final String configId; + + StableConfig() { + this.apmConfiguration = new HashMap<>(); + this.configId = null; + } + + StableConfig(String configId) { + this.apmConfiguration = new HashMap<>(); + this.configId = configId; + } + + StableConfig(String configId, Map config) { + this.apmConfiguration = config; + this.configId = configId; + } + + private String get(String key) { + return (String) this.apmConfiguration.get(key); + } + + private Set getKeys() { + return this.apmConfiguration.keySet(); + } + + private String getConfigId() { + return this.configId; + } } } 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 31143149f92..7b22ceb99bb 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 @@ -59,7 +59,6 @@ class StableConfigSourceTest extends DDSpecification { println "Error writing to file: ${e.message}" return // fail? } - StableConfigSource cfg = new StableConfigSource(filePath.toString(), ConfigOrigin.USER_STABLE_CONFIG) then: From aebebbe1e9f1bd8417aece7098c8acfafb11db66 Mon Sep 17 00:00:00 2001 From: Mikayla Toffler Date: Mon, 10 Feb 2025 14:51:43 -0500 Subject: [PATCH 13/41] nits: docs and cleanup --- .../java/datadog/trace/bootstrap/Agent.java | 17 ++- .../config/provider/StableConfigSource.java | 42 +++++- .../datadog/trace/api/ConfigTest.groovy | 122 +++++++++--------- 3 files changed, 106 insertions(+), 75 deletions(-) diff --git a/dd-java-agent/agent-bootstrap/src/main/java/datadog/trace/bootstrap/Agent.java b/dd-java-agent/agent-bootstrap/src/main/java/datadog/trace/bootstrap/Agent.java index e5d3e2ff722..ed8b5f903e1 100644 --- a/dd-java-agent/agent-bootstrap/src/main/java/datadog/trace/bootstrap/Agent.java +++ b/dd-java-agent/agent-bootstrap/src/main/java/datadog/trace/bootstrap/Agent.java @@ -1206,18 +1206,17 @@ private static boolean isFeatureEnabled(AgentFeature feature) { final String featureEnabledSysprop = feature.getSystemProp(); String featureEnabled = System.getProperty(featureEnabledSysprop); // MIKAYLA: Where to write tests for this? - // if (featureEnabled == null) { - // featureEnabled = - // getStableConfig(featureEnabledSysprop, - // StableConfigSource.MANAGED_STABLE_CONFIG_PATH); - // } + if (featureEnabled == null) { + featureEnabled = + getStableConfig(featureEnabledSysprop, StableConfigSource.MANAGED_STABLE_CONFIG_PATH); + } if (featureEnabled == null) { featureEnabled = ddGetEnv(featureEnabledSysprop); } - // if (featureEnabled == null) { - // featureEnabled = - // getStableConfig(featureEnabledSysprop, StableConfigSource.USER_STABLE_CONFIG_PATH); - // } + if (featureEnabled == null) { + featureEnabled = + getStableConfig(featureEnabledSysprop, StableConfigSource.USER_STABLE_CONFIG_PATH); + } if (feature.isEnabledByDefault()) { // true unless it's explicitly set to "false" diff --git a/internal-api/src/main/java/datadog/trace/bootstrap/config/provider/StableConfigSource.java b/internal-api/src/main/java/datadog/trace/bootstrap/config/provider/StableConfigSource.java index 37976251065..fac9df8b784 100644 --- a/internal-api/src/main/java/datadog/trace/bootstrap/config/provider/StableConfigSource.java +++ b/internal-api/src/main/java/datadog/trace/bootstrap/config/provider/StableConfigSource.java @@ -44,8 +44,17 @@ private static StableConfig newStableConfigFromFile(String file) { return new StableConfig(); } - public static HashMap readYamlFromFile(String filePath) - throws IOException, Exception { + /** + * Reads a YAML file from the specified file path and returns its contents as a HashMap. + * + * @param filePath The path to the YAML file to be read. It must be a valid path to an existing + * file. + * @return A HashMap containing the parsed data from the YAML file. + * @throws IOException If the specified file does not exist or cannot be accessed. + * @throws Exception If there is an error during YAML parsing, including invalid formatting or + * structure. + */ + public static HashMap readYamlFromFile(String filePath) throws Exception { File file = new File(filePath); if (!file.exists()) { throw new IOException( @@ -61,10 +70,23 @@ public static HashMap readYamlFromFile(String filePath) } } + /** + * Creates a StableConfig object based on the provided data map. + * + *

This method extracts the "config_id" and "apm_configuration_default" values from the input + * data map. If "apm_configuration_default" is a valid HashMap, it processes the entries and + * constructs a new configuration map. + * + * @param data A HashMap containing the configuration data. It should contain keys + * such as "config_id" and "apm_configuration_default". + * @return A StableConfig object constructed with the "config_id" if available and, if applicable, + * the parsed configuration data. + */ private static StableConfig buildStableConfig(HashMap data) { if (data == null) { return new StableConfig(); } + String configId = (String) data.get("config_id"); Object apmConfig = data.get("apm_configuration_default"); @@ -73,9 +95,7 @@ private static StableConfig buildStableConfig(HashMap data) { HashMap tempConfig = (HashMap) apmConfig; for (Map.Entry entry : tempConfig.entrySet()) { if (entry.getKey() instanceof String && entry.getValue() != null) { - String key = String.valueOf(entry.getKey()); - Object value = entry.getValue(); - config.put(key, value); + config.put(String.valueOf(entry.getKey()), entry.getValue()); } else { log.debug("Config key {} in unexpected format", entry.getKey()); } @@ -88,6 +108,18 @@ private static StableConfig buildStableConfig(HashMap data) { } }; + /** + * Searches for a specific key in the provided map and returns its associated value if found. + * + *

This method is designed to quickly check if a specific key exists within the + * "apm_configuration_default" section of the provided data map and returns its associated value + * as a string if found. + * + * @param data A HashMap containing the configuration data. The key + * "apm_configuration_default" should be present, containing a nested map. + * @param key The key to search for within the "apm_configuration_default" section. + * @return The value associated with the specified key as a String if found; otherwise, null. + */ public static String findAndReturnEarly(HashMap data, String key) { if (data == null) { return null; diff --git a/internal-api/src/test/groovy/datadog/trace/api/ConfigTest.groovy b/internal-api/src/test/groovy/datadog/trace/api/ConfigTest.groovy index a1f1b6da485..01fa80850f4 100644 --- a/internal-api/src/test/groovy/datadog/trace/api/ConfigTest.groovy +++ b/internal-api/src/test/groovy/datadog/trace/api/ConfigTest.groovy @@ -577,67 +577,67 @@ class ConfigTest extends DDSpecification { config.getLongRunningTraceFlushInterval() == 81 } - def "specify overrides stable config"() { - setup: - Path filePath = StableConfigSourceTest.tempFile() - if (filePath == null) { - // fail fast? - return - } - // Override for testing - String originalPath - if (user == true) { - originalPath = StableConfigSource.USER_STABLE_CONFIG_PATH - StableConfigSource.USER_STABLE_CONFIG_PATH = filePath.toAbsolutePath().toString() - } else if (managed == true) { - originalPath = StableConfigSource.MANAGED_STABLE_CONFIG_PATH - StableConfigSource.MANAGED_STABLE_CONFIG_PATH = filePath.toAbsolutePath().toString() - } - def configs = new HashMap<>() - configs.put(TRACE_ENABLED, "false") - // configs.put(DD_RUNTIME_METRICS_ENABLED_ENV, "false") - // configs.put(LOGS_INJECTION, "false") - // configs.put(PROFILING_ENABLED, "true") - // configs.put(DATA_STREAMS_ENABLED, "true") - // configs.put(APPSEC_ENABLED, "true") - // configs.put(IAST_ENABLED, "true") - // configs.put(DEBUGGER_ENABLED, "true") - // configs.put(DATA_JOBS_ENABLED, "true") - // configs.put(APPSEC_SCA_ENABLED, "true") - - try { - StableConfigSourceTest.writeFileYaml(filePath, "12345", configs) - } catch (IOException e) { - println "Error writing to file: ${e.message}" - return // fail? - } - - when: - def config = new Config() - - then: - !config.traceEnabled - // !config.runtimeMetricsEnabled - // !config.logsInjectionEnabled - // config.profilingEnabled - // config.dataStreamsEnabled - // //config.appSecEnabled? - // config.iastDebugEnabled - // config.debuggerEnabled - // config.dataJobsEnabled - // config.appSecScaEnabled - if (user == true) { - StableConfigSource.USER_STABLE_CONFIG_PATH = originalPath - } else if (managed == true) { - StableConfigSource.MANAGED_STABLE_CONFIG_PATH = originalPath - } - - where: - user | managed - true | false - // false | true - - } + // def "specify overrides stable config"() { + // setup: + // Path filePath = StableConfigSourceTest.tempFile() + // if (filePath == null) { + // // fail fast? + // return + // } + // // Override for testing + // String originalPath + // if (user == true) { + // originalPath = StableConfigSource.USER_STABLE_CONFIG_PATH + // StableConfigSource.USER_STABLE_CONFIG_PATH = filePath.toAbsolutePath().toString() + // } else if (managed == true) { + // originalPath = StableConfigSource.MANAGED_STABLE_CONFIG_PATH + // StableConfigSource.MANAGED_STABLE_CONFIG_PATH = filePath.toAbsolutePath().toString() + // } + // def configs = new HashMap<>() + // configs.put(TRACE_ENABLED, "false") + // configs.put(DD_RUNTIME_METRICS_ENABLED_ENV, "false") + // configs.put(LOGS_INJECTION, "false") + // configs.put(PROFILING_ENABLED, "true") + // configs.put(DATA_STREAMS_ENABLED, "true") + // configs.put(APPSEC_ENABLED, "true") + // configs.put(IAST_ENABLED, "true") + // configs.put(DEBUGGER_ENABLED, "true") + // configs.put(DATA_JOBS_ENABLED, "true") + // configs.put(APPSEC_SCA_ENABLED, "true") + // + // try { + // StableConfigSourceTest.writeFileYaml(filePath, "12345", configs) + // } catch (IOException e) { + // println "Error writing to file: ${e.message}" + // return // fail? + // } + // + // when: + // def config = new Config() + // + // then: + // !config.traceEnabled + // !config.runtimeMetricsEnabled + // !config.logsInjectionEnabled + // config.profilingEnabled + // config.dataStreamsEnabled + // //config.appSecEnabled? + // config.iastDebugEnabled + // config.debuggerEnabled + // config.dataJobsEnabled + // config.appSecScaEnabled + // if (user == true) { + // StableConfigSource.USER_STABLE_CONFIG_PATH = originalPath + // } else if (managed == true) { + // StableConfigSource.MANAGED_STABLE_CONFIG_PATH = originalPath + // } + // + // where: + // user | managed + // true | false + // false | true + // + // } def "sys props override env vars"() { setup: From 90b52e9d64f9c0857d68f6e80d16cfdd2354ff5a Mon Sep 17 00:00:00 2001 From: Mikayla Toffler <46911781+mtoffl01@users.noreply.github.com> Date: Mon, 10 Feb 2025 14:52:36 -0500 Subject: [PATCH 14/41] Update internal-api/src/main/java/datadog/trace/bootstrap/config/provider/StableConfigSource.java Co-authored-by: datadog-datadog-prod-us1[bot] <88084959+datadog-datadog-prod-us1[bot]@users.noreply.github.com> --- .../trace/bootstrap/config/provider/StableConfigSource.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/internal-api/src/main/java/datadog/trace/bootstrap/config/provider/StableConfigSource.java b/internal-api/src/main/java/datadog/trace/bootstrap/config/provider/StableConfigSource.java index fac9df8b784..e3468b569d6 100644 --- a/internal-api/src/main/java/datadog/trace/bootstrap/config/provider/StableConfigSource.java +++ b/internal-api/src/main/java/datadog/trace/bootstrap/config/provider/StableConfigSource.java @@ -82,7 +82,7 @@ public static HashMap readYamlFromFile(String filePath) throws E * @return A StableConfig object constructed with the "config_id" if available and, if applicable, * the parsed configuration data. */ - private static StableConfig buildStableConfig(HashMap data) { + private static StableConfig buildStableConfig(Map data) { if (data == null) { return new StableConfig(); } From cdd8df811070c4610fa3c1d917850d22010f763e Mon Sep 17 00:00:00 2001 From: Mikayla Toffler <46911781+mtoffl01@users.noreply.github.com> Date: Mon, 10 Feb 2025 14:52:51 -0500 Subject: [PATCH 15/41] Update internal-api/src/main/java/datadog/trace/bootstrap/config/provider/StableConfigSource.java Co-authored-by: datadog-datadog-prod-us1[bot] <88084959+datadog-datadog-prod-us1[bot]@users.noreply.github.com> --- .../trace/bootstrap/config/provider/StableConfigSource.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/internal-api/src/main/java/datadog/trace/bootstrap/config/provider/StableConfigSource.java b/internal-api/src/main/java/datadog/trace/bootstrap/config/provider/StableConfigSource.java index e3468b569d6..3d843c3812c 100644 --- a/internal-api/src/main/java/datadog/trace/bootstrap/config/provider/StableConfigSource.java +++ b/internal-api/src/main/java/datadog/trace/bootstrap/config/provider/StableConfigSource.java @@ -120,7 +120,7 @@ private static StableConfig buildStableConfig(Map data) { * @param key The key to search for within the "apm_configuration_default" section. * @return The value associated with the specified key as a String if found; otherwise, null. */ - public static String findAndReturnEarly(HashMap data, String key) { + public static String findAndReturnEarly(Map data, String key) { if (data == null) { return null; } From 1fed1da89447d6ebbf388f731896120f686632af Mon Sep 17 00:00:00 2001 From: Mikayla Toffler Date: Mon, 10 Feb 2025 15:25:13 -0500 Subject: [PATCH 16/41] add doc for getStableConfig --- .../src/main/java/datadog/trace/bootstrap/Agent.java | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/dd-java-agent/agent-bootstrap/src/main/java/datadog/trace/bootstrap/Agent.java b/dd-java-agent/agent-bootstrap/src/main/java/datadog/trace/bootstrap/Agent.java index ed8b5f903e1..3f6ab9fcf31 100644 --- a/dd-java-agent/agent-bootstrap/src/main/java/datadog/trace/bootstrap/Agent.java +++ b/dd-java-agent/agent-bootstrap/src/main/java/datadog/trace/bootstrap/Agent.java @@ -1205,7 +1205,6 @@ private static boolean isFeatureEnabled(AgentFeature feature) { // must be kept in sync with logic from Config! final String featureEnabledSysprop = feature.getSystemProp(); String featureEnabled = System.getProperty(featureEnabledSysprop); - // MIKAYLA: Where to write tests for this? if (featureEnabled == null) { featureEnabled = getStableConfig(featureEnabledSysprop, StableConfigSource.MANAGED_STABLE_CONFIG_PATH); @@ -1354,6 +1353,10 @@ private static String ddGetProperty(final String sysProp) { return value; } + /** + * Looks for the "DD_" environment variable equivalent of the given "dd." system property in the + * Stable Configuration input + */ private static String getStableConfig(final String sysProp, String filepath) { try { return StableConfigSource.findAndReturnEarly( From ae6036b7fbcb1e725cf48ba0d2ae76a9f3716a1d Mon Sep 17 00:00:00 2001 From: Mikayla Toffler Date: Tue, 11 Feb 2025 21:16:33 -0500 Subject: [PATCH 17/41] Optimize: lazily load configuration information from file, cache results --- .../java/datadog/trace/bootstrap/Agent.java | 18 +- .../config/provider/ConfigProvider.java | 36 ++-- .../config/provider/StableConfigSource.java | 202 ++++++++++-------- .../provider/StableConfigSourceSingleton.java | 23 ++ .../provider/StableConfigSourceTest.groovy | 5 +- 5 files changed, 158 insertions(+), 126 deletions(-) create mode 100644 internal-api/src/main/java/datadog/trace/bootstrap/config/provider/StableConfigSourceSingleton.java diff --git a/dd-java-agent/agent-bootstrap/src/main/java/datadog/trace/bootstrap/Agent.java b/dd-java-agent/agent-bootstrap/src/main/java/datadog/trace/bootstrap/Agent.java index 87557090840..e10dab2d6f0 100644 --- a/dd-java-agent/agent-bootstrap/src/main/java/datadog/trace/bootstrap/Agent.java +++ b/dd-java-agent/agent-bootstrap/src/main/java/datadog/trace/bootstrap/Agent.java @@ -34,6 +34,7 @@ import datadog.trace.api.scopemanager.ScopeListener; import datadog.trace.bootstrap.benchmark.StaticEventLogger; import datadog.trace.bootstrap.config.provider.StableConfigSource; +import datadog.trace.bootstrap.config.provider.StableConfigSourceSingleton; import datadog.trace.bootstrap.instrumentation.api.AgentTracer; import datadog.trace.bootstrap.instrumentation.api.AgentTracer.TracerAPI; import datadog.trace.bootstrap.instrumentation.api.ProfilingContextIntegration; @@ -41,7 +42,6 @@ import datadog.trace.util.AgentTaskScheduler; import datadog.trace.util.AgentThreadFactory.AgentThread; import datadog.trace.util.throwable.FatalAgentMisconfigurationError; -import java.io.IOException; import java.lang.instrument.Instrumentation; import java.lang.reflect.InvocationTargetException; import java.lang.reflect.Method; @@ -1209,14 +1209,14 @@ private static boolean isFeatureEnabled(AgentFeature feature) { String featureEnabled = System.getProperty(featureEnabledSysprop); if (featureEnabled == null) { featureEnabled = - getStableConfig(featureEnabledSysprop, StableConfigSource.MANAGED_STABLE_CONFIG_PATH); + getStableConfig(StableConfigSourceSingleton.getManaged(), featureEnabledSysprop); } if (featureEnabled == null) { featureEnabled = ddGetEnv(featureEnabledSysprop); } if (featureEnabled == null) { featureEnabled = - getStableConfig(featureEnabledSysprop, StableConfigSource.USER_STABLE_CONFIG_PATH); + getStableConfig(StableConfigSourceSingleton.getUser(), featureEnabledSysprop); } if (feature.isEnabledByDefault()) { @@ -1359,16 +1359,8 @@ private static String ddGetProperty(final String sysProp) { * Looks for the "DD_" environment variable equivalent of the given "dd." system property in the * Stable Configuration input */ - private static String getStableConfig(final String sysProp, String filepath) { - try { - return StableConfigSource.findAndReturnEarly( - StableConfigSource.readYamlFromFile(filepath), toEnvVar(sysProp)); - } catch (IOException e) { - log.error("Error reading from file: {}", e.getMessage()); - } catch (Exception e) { - log.error("Error processing file: {}", e.getMessage()); - } - return null; + private static String getStableConfig(StableConfigSource source, final String sysProp) { + return source.get(toEnvVar(sysProp)); } /** Looks for the "DD_" environment variable equivalent of the given "dd." system property. */ 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 260cd261f85..6beddb1a9e8 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 @@ -356,23 +356,19 @@ public static ConfigProvider createDefault() { if (configProperties.isEmpty()) { return new ConfigProvider( new SystemPropertiesConfigSource(), - new StableConfigSource( - StableConfigSource.MANAGED_STABLE_CONFIG_PATH, ConfigOrigin.MANAGED_STABLE_CONFIG), + StableConfigSourceSingleton.getManaged(), new EnvironmentConfigSource(), new OtelEnvironmentConfigSource(), - new StableConfigSource( - StableConfigSource.USER_STABLE_CONFIG_PATH, ConfigOrigin.USER_STABLE_CONFIG), + StableConfigSourceSingleton.getUser(), new CapturedEnvironmentConfigSource()); } else { return new ConfigProvider( new SystemPropertiesConfigSource(), - new StableConfigSource( - StableConfigSource.MANAGED_STABLE_CONFIG_PATH, ConfigOrigin.MANAGED_STABLE_CONFIG), + StableConfigSourceSingleton.getManaged(), new EnvironmentConfigSource(), new PropertiesConfigSource(configProperties, true), new OtelEnvironmentConfigSource(configProperties), - new StableConfigSource( - StableConfigSource.USER_STABLE_CONFIG_PATH, ConfigOrigin.USER_STABLE_CONFIG), + StableConfigSourceSingleton.getUser(), new CapturedEnvironmentConfigSource()); } } @@ -386,24 +382,20 @@ public static ConfigProvider withoutCollector() { return new ConfigProvider( false, new SystemPropertiesConfigSource(), - new StableConfigSource( - StableConfigSource.MANAGED_STABLE_CONFIG_PATH, ConfigOrigin.MANAGED_STABLE_CONFIG), + StableConfigSourceSingleton.getManaged(), new EnvironmentConfigSource(), new OtelEnvironmentConfigSource(), - new StableConfigSource( - StableConfigSource.USER_STABLE_CONFIG_PATH, ConfigOrigin.USER_STABLE_CONFIG), + StableConfigSourceSingleton.getUser(), new CapturedEnvironmentConfigSource()); } else { return new ConfigProvider( false, new SystemPropertiesConfigSource(), - new StableConfigSource( - StableConfigSource.MANAGED_STABLE_CONFIG_PATH, ConfigOrigin.MANAGED_STABLE_CONFIG), + StableConfigSourceSingleton.getManaged(), new EnvironmentConfigSource(), new PropertiesConfigSource(configProperties, true), new OtelEnvironmentConfigSource(configProperties), - new StableConfigSource( - StableConfigSource.USER_STABLE_CONFIG_PATH, ConfigOrigin.USER_STABLE_CONFIG), + StableConfigSourceSingleton.getUser(), new CapturedEnvironmentConfigSource()); } } @@ -420,25 +412,21 @@ public static ConfigProvider withPropertiesOverride(Properties properties) { if (configProperties.isEmpty()) { return new ConfigProvider( new SystemPropertiesConfigSource(), - new StableConfigSource( - StableConfigSource.MANAGED_STABLE_CONFIG_PATH, ConfigOrigin.MANAGED_STABLE_CONFIG), + StableConfigSourceSingleton.getManaged(), new EnvironmentConfigSource(), providedConfigSource, new OtelEnvironmentConfigSource(), - new StableConfigSource( - StableConfigSource.USER_STABLE_CONFIG_PATH, ConfigOrigin.USER_STABLE_CONFIG), + StableConfigSourceSingleton.getUser(), new CapturedEnvironmentConfigSource()); } else { return new ConfigProvider( providedConfigSource, new SystemPropertiesConfigSource(), - new StableConfigSource( - StableConfigSource.MANAGED_STABLE_CONFIG_PATH, ConfigOrigin.MANAGED_STABLE_CONFIG), + StableConfigSourceSingleton.getManaged(), new EnvironmentConfigSource(), new PropertiesConfigSource(configProperties, true), new OtelEnvironmentConfigSource(configProperties), - new StableConfigSource( - StableConfigSource.USER_STABLE_CONFIG_PATH, ConfigOrigin.USER_STABLE_CONFIG), + StableConfigSourceSingleton.getUser(), new CapturedEnvironmentConfigSource()); } } diff --git a/internal-api/src/main/java/datadog/trace/bootstrap/config/provider/StableConfigSource.java b/internal-api/src/main/java/datadog/trace/bootstrap/config/provider/StableConfigSource.java index 3d843c3812c..e3f18bf584c 100644 --- a/internal-api/src/main/java/datadog/trace/bootstrap/config/provider/StableConfigSource.java +++ b/internal-api/src/main/java/datadog/trace/bootstrap/config/provider/StableConfigSource.java @@ -8,6 +8,7 @@ import java.io.InputStream; import java.nio.file.Files; import java.util.HashMap; +import java.util.Iterator; import java.util.Map; import java.util.Set; import org.slf4j.Logger; @@ -26,22 +27,98 @@ public final class StableConfigSource extends ConfigProvider.Source { private final StableConfig config; private final ConfigOrigin fileOrigin; + private final String file; + private Map configEntries; + private Iterator> iterator; + private Boolean fileReadComplete; + + StableConfigSource() { + this.fileOrigin = null; + this.file = null; + this.config = new StableConfig(); + } StableConfigSource(String file, ConfigOrigin origin) { this.fileOrigin = origin; - this.config = newStableConfigFromFile(file); + this.file = file; + this.config = new StableConfig(); + // The below are explicitly initialized for clarity + this.fileReadComplete = false; + this.iterator = null; + } + + @Override + public String get(String key) { + if (this.fileReadComplete) { + return this.config.get(key); + } + return this.findAndReturnEarly(key); + } + + @Override + public ConfigOrigin origin() { + return fileOrigin; + } + + public Set getKeys() { + return this.config.getKeys(); + } + + public String getConfigId() { + return this.config.getConfigId(); + } + + /** + * Searches for a specific key and quick returns its associated value if found. This function + * first checks if the configuration entries have already been loaded from the yaml file. If not, + * it attempts to load the configuration from the file and process it, returning the key's value + * if found. + * + * @param key the key to search for in the configuration. + * @return the configuration value corresponding to the given key, or null if not found or if an + * error occurs. + */ + private String findAndReturnEarly(String key) { + if (this.configEntries != null) { + return iterateAndReturnEarly(key); + } + + HashMap data = loadConfigDataFromFile(); + if (data == null || data.isEmpty()) { + this.fileReadComplete = true; + return null; + } + + this.config.setConfigId((String) data.get("config_id")); + Object apmConfig = data.get("apm_configuration_default"); + + // If the key apm_configuration_default is not present in the file, or if the value of + // apm_configuration_default is not convertible to a hashmap, there's nothing to process; return + // nothing. + if (!(apmConfig instanceof HashMap)) { + this.fileReadComplete = true; + return null; + } + + this.configEntries = (HashMap) apmConfig; + return iterateAndReturnEarly(key); } - private static StableConfig newStableConfigFromFile(String file) { + /** + * Attempts to load configuration data from the file. This function reads the file in YAML format + * and returns the contents as a HashMap. If an error occurs during the reading or processing of + * the file, it logs the error and returns null. + * + * @return a HashMap containing the configuration data, or null if an error occurs. + */ + private HashMap loadConfigDataFromFile() { try { - HashMap data = readYamlFromFile(file); - return buildStableConfig(data); - } catch (IOException e) { - log.error("Error reading from file: {}", e.getMessage()); + return readYamlFromFile(this.file); } catch (Exception e) { + this.fileReadComplete = true; log.error("Error processing file: {}", e.getMessage()); + return null; } - return new StableConfig(); } /** @@ -71,116 +148,65 @@ public static HashMap readYamlFromFile(String filePath) throws E } /** - * Creates a StableConfig object based on the provided data map. - * - *

This method extracts the "config_id" and "apm_configuration_default" values from the input - * data map. If "apm_configuration_default" is a valid HashMap, it processes the entries and - * constructs a new configuration map. - * - * @param data A HashMap containing the configuration data. It should contain keys - * such as "config_id" and "apm_configuration_default". - * @return A StableConfig object constructed with the "config_id" if available and, if applicable, - * the parsed configuration data. - */ - private static StableConfig buildStableConfig(Map data) { - if (data == null) { - return new StableConfig(); - } - - String configId = (String) data.get("config_id"); - Object apmConfig = data.get("apm_configuration_default"); - - HashMap config = new HashMap<>(); - if (apmConfig instanceof HashMap) { - HashMap tempConfig = (HashMap) apmConfig; - for (Map.Entry entry : tempConfig.entrySet()) { - if (entry.getKey() instanceof String && entry.getValue() != null) { - config.put(String.valueOf(entry.getKey()), entry.getValue()); - } else { - log.debug("Config key {} in unexpected format", entry.getKey()); - } - } - return new StableConfig(configId, config); - } else { - // do something - log.debug("File in unexpected format"); - return new StableConfig(configId); - } - }; - - /** - * Searches for a specific key in the provided map and returns its associated value if found. + * Iterates over the configuration entries and attempts to find the value for the given key. The + * method reuses an existing iterator to avoid re-iterating over the entire set of entries every + * time it is called, thus optimizing the processing. If the key is found during iteration, the + * corresponding value is returned immediately. * - *

This method is designed to quickly check if a specific key exists within the - * "apm_configuration_default" section of the provided data map and returns its associated value - * as a string if found. + *

The function also populates the StableConfig `config` map with valid key-value pairs during + * the iteration. If no value is found for the key, or if the iteration completes without finding + * the key, the function marks the file as read and returns null. * - * @param data A HashMap containing the configuration data. The key - * "apm_configuration_default" should be present, containing a nested map. - * @param key The key to search for within the "apm_configuration_default" section. - * @return The value associated with the specified key as a String if found; otherwise, null. + * @param key the key to search for in the configuration entries. + * @return the configuration value associated with the key, or null if not found. */ - public static String findAndReturnEarly(Map data, String key) { - if (data == null) { - return null; + private String iterateAndReturnEarly(String key) { + if (this.iterator == null) { + this.iterator = this.configEntries.entrySet().iterator(); } - Object apmConfig = data.get("apm_configuration_default"); - if (apmConfig instanceof HashMap) { - HashMap tempConfig = (HashMap) apmConfig; - for (Map.Entry entry : tempConfig.entrySet()) { - if (entry.getKey() == key) { - return (String) entry.getValue(); + while (this.iterator.hasNext()) { + Map.Entry entry = iterator.next(); + Object entryKey = entry.getKey(); + Object entryValue = entry.getValue(); + if (entryKey instanceof String && entryValue != null) { + this.config.put(String.valueOf(entryKey), entryValue); + String configValue = this.config.get(key); + if (configValue != null) { + return configValue; } } } + this.fileReadComplete = true; return null; } - @Override - public String get(String key) { - return this.config.get(propertyNameToEnvironmentVariableName(key)); - } - - @Override - public ConfigOrigin origin() { - return fileOrigin; - } - - public Set getKeys() { - return this.config.getKeys(); - } - - public String getConfigId() { - return this.config.getConfigId(); - } - private static class StableConfig { - private final Map apmConfiguration; - private final String configId; + private Map apmConfiguration; + private String configId; StableConfig() { this.apmConfiguration = new HashMap<>(); this.configId = null; } - StableConfig(String configId) { - this.apmConfiguration = new HashMap<>(); - this.configId = configId; + private void put(String key, Object value) { + this.apmConfiguration.put(key, value); } - StableConfig(String configId, Map config) { - this.apmConfiguration = config; + private void setConfigId(String configId) { this.configId = configId; } + // TODO: Make this.apmConfiguration a Map instead private String get(String key) { - return (String) this.apmConfiguration.get(key); + return (String) this.apmConfiguration.get(propertyNameToEnvironmentVariableName(key)); } private Set getKeys() { return this.apmConfiguration.keySet(); } + // Might be null private String getConfigId() { return this.configId; } diff --git a/internal-api/src/main/java/datadog/trace/bootstrap/config/provider/StableConfigSourceSingleton.java b/internal-api/src/main/java/datadog/trace/bootstrap/config/provider/StableConfigSourceSingleton.java new file mode 100644 index 00000000000..80e711df680 --- /dev/null +++ b/internal-api/src/main/java/datadog/trace/bootstrap/config/provider/StableConfigSourceSingleton.java @@ -0,0 +1,23 @@ +package datadog.trace.bootstrap.config.provider; + +import datadog.trace.api.ConfigOrigin; + +public class StableConfigSourceSingleton { + private static final StableConfigSource USER = + new StableConfigSource( + StableConfigSource.USER_STABLE_CONFIG_PATH, ConfigOrigin.USER_STABLE_CONFIG); + private static final StableConfigSource MANAGED = + new StableConfigSource( + StableConfigSource.MANAGED_STABLE_CONFIG_PATH, ConfigOrigin.MANAGED_STABLE_CONFIG); + + // Private constructor to prevent instantiation + private StableConfigSourceSingleton() {} + + public static StableConfigSource getUser() { + return USER; + } + + public static StableConfigSource getManaged() { + return MANAGED; + } +} 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 7b22ceb99bb..50f5e499578 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 @@ -62,11 +62,13 @@ class StableConfigSourceTest extends DDSpecification { StableConfigSource cfg = new StableConfigSource(filePath.toString(), ConfigOrigin.USER_STABLE_CONFIG) then: - cfg.getKeys().size() == 3 cfg.get("service") == "svc" cfg.get("env") == "env" cfg.get("config_no_dd") == null cfg.get("config_nonexistent") == null + cfg.getKeys().size() == 3 + cfg.getConfigId() == "12345" + } def "test file invalid format"() { @@ -126,6 +128,7 @@ class StableConfigSourceTest extends DDSpecification { "" | new HashMap<>() "12345" | new HashMap<>() << ["DD_KEY_ONE": "one", "DD_KEY_TWO": "two"] } + // Corrupt YAML string variable used for testing, defined outside the 'where' block for readability @Shared def corruptYaml = ''' From e3d6c8057f369ad61c6547e77cec6c3e53f7fc11 Mon Sep 17 00:00:00 2001 From: Mikayla Toffler Date: Tue, 11 Feb 2025 21:31:07 -0500 Subject: [PATCH 18/41] nits: variable renaming, comments to describe variables --- .../config/provider/StableConfigSource.java | 29 +++++++++---------- 1 file changed, 14 insertions(+), 15 deletions(-) diff --git a/internal-api/src/main/java/datadog/trace/bootstrap/config/provider/StableConfigSource.java b/internal-api/src/main/java/datadog/trace/bootstrap/config/provider/StableConfigSource.java index e3f18bf584c..a0c99b69637 100644 --- a/internal-api/src/main/java/datadog/trace/bootstrap/config/provider/StableConfigSource.java +++ b/internal-api/src/main/java/datadog/trace/bootstrap/config/provider/StableConfigSource.java @@ -25,24 +25,23 @@ public final class StableConfigSource extends ConfigProvider.Source { private static final Logger log = LoggerFactory.getLogger(StableConfigSource.class); - private final StableConfig config; private final ConfigOrigin fileOrigin; - private final String file; - private Map configEntries; - private Iterator> iterator; + private final String filePath; + + private Map + loadedConfig; // Holds the configuration entries lazily loaded from the configuration file + private Iterator> + iterator; // used to traverse through the entries of the loadedConfig map as processing + // progresses private Boolean fileReadComplete; - StableConfigSource() { - this.fileOrigin = null; - this.file = null; - this.config = new StableConfig(); - } + private final StableConfig config; StableConfigSource(String file, ConfigOrigin origin) { this.fileOrigin = origin; - this.file = file; + this.filePath = file; this.config = new StableConfig(); - // The below are explicitly initialized for clarity + // The below are initialized explicitly for clarity this.fileReadComplete = false; this.iterator = null; } @@ -79,7 +78,7 @@ public String getConfigId() { * error occurs. */ private String findAndReturnEarly(String key) { - if (this.configEntries != null) { + if (this.loadedConfig != null) { return iterateAndReturnEarly(key); } @@ -100,7 +99,7 @@ private String findAndReturnEarly(String key) { return null; } - this.configEntries = (HashMap) apmConfig; + this.loadedConfig = (HashMap) apmConfig; return iterateAndReturnEarly(key); } @@ -113,7 +112,7 @@ private String findAndReturnEarly(String key) { */ private HashMap loadConfigDataFromFile() { try { - return readYamlFromFile(this.file); + return readYamlFromFile(this.filePath); } catch (Exception e) { this.fileReadComplete = true; log.error("Error processing file: {}", e.getMessage()); @@ -162,7 +161,7 @@ public static HashMap readYamlFromFile(String filePath) throws E */ private String iterateAndReturnEarly(String key) { if (this.iterator == null) { - this.iterator = this.configEntries.entrySet().iterator(); + this.iterator = this.loadedConfig.entrySet().iterator(); } while (this.iterator.hasNext()) { Map.Entry entry = iterator.next(); From a7bf5ba499a9ca9ea4cbcaecdfcf3df2d995e8c7 Mon Sep 17 00:00:00 2001 From: Mikayla Toffler Date: Tue, 11 Feb 2025 21:33:59 -0500 Subject: [PATCH 19/41] nit: move comment above variable --- .../bootstrap/config/provider/StableConfigSource.java | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/internal-api/src/main/java/datadog/trace/bootstrap/config/provider/StableConfigSource.java b/internal-api/src/main/java/datadog/trace/bootstrap/config/provider/StableConfigSource.java index a0c99b69637..c359e7624a9 100644 --- a/internal-api/src/main/java/datadog/trace/bootstrap/config/provider/StableConfigSource.java +++ b/internal-api/src/main/java/datadog/trace/bootstrap/config/provider/StableConfigSource.java @@ -28,11 +28,11 @@ public final class StableConfigSource extends ConfigProvider.Source { private final ConfigOrigin fileOrigin; private final String filePath; - private Map - loadedConfig; // Holds the configuration entries lazily loaded from the configuration file - private Iterator> - iterator; // used to traverse through the entries of the loadedConfig map as processing + // loadedConfig holds the configuration entries lazily loaded from the configuration file + private Map loadedConfig; + // iterator is used to traverse through the entries of the loadedConfig map as processing // progresses + private Iterator> iterator; private Boolean fileReadComplete; private final StableConfig config; From 0c8ef6a08bba040ee3662e508c58ede52c9361fa Mon Sep 17 00:00:00 2001 From: Mikayla Toffler Date: Thu, 13 Feb 2025 11:28:57 -0500 Subject: [PATCH 20/41] fix typo in managed file path --- .../trace/bootstrap/config/provider/StableConfigSource.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/internal-api/src/main/java/datadog/trace/bootstrap/config/provider/StableConfigSource.java b/internal-api/src/main/java/datadog/trace/bootstrap/config/provider/StableConfigSource.java index c359e7624a9..c68ee927772 100644 --- a/internal-api/src/main/java/datadog/trace/bootstrap/config/provider/StableConfigSource.java +++ b/internal-api/src/main/java/datadog/trace/bootstrap/config/provider/StableConfigSource.java @@ -21,7 +21,7 @@ public final class StableConfigSource extends ConfigProvider.Source { public static final String USER_STABLE_CONFIG_PATH = "/etc/datadog-agent/application_monitoring.yaml"; public static final String MANAGED_STABLE_CONFIG_PATH = - "/etc/datadog-agent/managed/datadog-apm-libraries/stable/application_monitoring.yaml "; + "/etc/datadog-agent/managed/datadog-apm-libraries/stable/application_monitoring.yaml"; private static final Logger log = LoggerFactory.getLogger(StableConfigSource.class); From 049da8bace7e021453857ed0a7e75751932e983d Mon Sep 17 00:00:00 2001 From: Mikayla Toffler Date: Thu, 13 Feb 2025 13:49:18 -0500 Subject: [PATCH 21/41] Initial StableConfigParser implementation and testing --- .../config/provider/StableConfigParser.java | 61 +++++++++++++++++++ .../config/provider/StableConfigSource.java | 12 ++-- .../provider/StableConfigParserTest.groovy | 51 ++++++++++++++++ .../provider/StableConfigSourceTest.groovy | 26 ++++++-- 4 files changed, 139 insertions(+), 11 deletions(-) create mode 100644 internal-api/src/main/java/datadog/trace/bootstrap/config/provider/StableConfigParser.java create mode 100644 internal-api/src/test/groovy/datadog/trace/bootstrap/config/provider/StableConfigParserTest.groovy diff --git a/internal-api/src/main/java/datadog/trace/bootstrap/config/provider/StableConfigParser.java b/internal-api/src/main/java/datadog/trace/bootstrap/config/provider/StableConfigParser.java new file mode 100644 index 00000000000..e58959f9657 --- /dev/null +++ b/internal-api/src/main/java/datadog/trace/bootstrap/config/provider/StableConfigParser.java @@ -0,0 +1,61 @@ +package datadog.trace.bootstrap.config.provider; + +import java.io.IOException; +import java.nio.file.Files; +import java.nio.file.Paths; +import java.util.concurrent.atomic.AtomicBoolean; +import java.util.regex.Matcher; +import java.util.regex.Pattern; +import java.util.stream.Stream; + +public class StableConfigParser { + + private static final Pattern idPattern = + Pattern.compile("^config_id\\s*:(.*)$"); // Match config_id: + private static final Pattern apmConfigPattern = + Pattern.compile("^apm_configuration_default:$"); // Match 'apm_configuration_default:' + private static final Pattern keyValPattern = + Pattern.compile( + "^\\s{2}([^:]+):\\s*(\"[^\"]*\"|[^\"\\n]*)$");; // Match indented (2 spaces) key-value + // pairs, either with double quotes or + // without + + public static StableConfigSource.StableConfig parse(String filePath) throws IOException { + try (Stream lines = Files.lines(Paths.get("file.txt"))) { + StableConfigSource.StableConfig cfg = new StableConfigSource.StableConfig(); + // Use AtomicBoolean because it's mutable within a lambda expression to + AtomicBoolean apmConfigFound = + new AtomicBoolean(false); // Track if we've found 'apm_configuration_default:' + lines.forEach( + line -> { + Matcher matcher = idPattern.matcher(line); + if (matcher.find()) { + cfg.setConfigId(matcher.group(2).trim()); + return; // go to next line + } + + if (!apmConfigFound.get() && apmConfigPattern.matcher(line).matches()) { + apmConfigFound.set(true); + return; // go to next line + } + + if (apmConfigFound.get()) { + Matcher keyValueMatcher = keyValPattern.matcher(line); + if (keyValueMatcher.matches()) { + String key = keyValueMatcher.group(1).trim(); + String value = keyValueMatcher.group(2).trim(); + // If value has quotes, remove them + if (value.startsWith("\"") && value.endsWith("\"")) { + value = value.substring(1, value.length() - 1); + } + cfg.put(key, value); // Store key-value pair in map + } else { + // If we encounter a non-indented or non-key-value line, stop processing + apmConfigFound.set(false); + } + } + }); + return cfg; + } + } +} diff --git a/internal-api/src/main/java/datadog/trace/bootstrap/config/provider/StableConfigSource.java b/internal-api/src/main/java/datadog/trace/bootstrap/config/provider/StableConfigSource.java index c68ee927772..2dbff1e7223 100644 --- a/internal-api/src/main/java/datadog/trace/bootstrap/config/provider/StableConfigSource.java +++ b/internal-api/src/main/java/datadog/trace/bootstrap/config/provider/StableConfigSource.java @@ -179,7 +179,7 @@ private String iterateAndReturnEarly(String key) { return null; } - private static class StableConfig { + /*public?*/ static class StableConfig { private Map apmConfiguration; private String configId; @@ -188,25 +188,25 @@ private static class StableConfig { this.configId = null; } - private void put(String key, Object value) { + void put(String key, Object value) { this.apmConfiguration.put(key, value); } - private void setConfigId(String configId) { + void setConfigId(String configId) { this.configId = configId; } // TODO: Make this.apmConfiguration a Map instead - private String get(String key) { + public String get(String key) { return (String) this.apmConfiguration.get(propertyNameToEnvironmentVariableName(key)); } - private Set getKeys() { + public Set getKeys() { return this.apmConfiguration.keySet(); } // Might be null - private String getConfigId() { + public String getConfigId() { return this.configId; } } diff --git a/internal-api/src/test/groovy/datadog/trace/bootstrap/config/provider/StableConfigParserTest.groovy b/internal-api/src/test/groovy/datadog/trace/bootstrap/config/provider/StableConfigParserTest.groovy new file mode 100644 index 00000000000..3676464066b --- /dev/null +++ b/internal-api/src/test/groovy/datadog/trace/bootstrap/config/provider/StableConfigParserTest.groovy @@ -0,0 +1,51 @@ +package datadog.trace.bootstrap.config.provider + +import datadog.trace.test.util.DDSpecification + +import java.nio.file.Path + +class StableConfigParserTest extends DDSpecification { + + def "test parser"() { + when: + Path filePath = StableConfigSourceTest.tempFile() + if (filePath == null) { + return // fail? + } + HashMap configs = new HashMap<>() + configs.put("KEY_ONE", "VALUE_ONE") + configs.put("KEY_TWO", "VALUE_TWO") + configs.put("KEY_THREE", "VALUE_THREE") + + HashMap fileContent = new HashMap<>() + fileContent.put("something-irrelevant-to-apm", "") + fileContent.put("config_id", "12345") + fileContent.put("something-else-irrelevant", "value-irrelevant") + fileContent.put("apm_configuration_default", configs) + + try { + StableConfigSourceTest.writeFileYaml(filePath, fileContent) + } catch (IOException e) { + println "Error writing to file: ${e.message}" + return // fail? + } + + StableConfigSource.StableConfig cfg + try { + cfg = StableConfigParser.parse(filePath.toString()) + } catch (Exception e) { + println "Error parsing file: ${e.message}" + return // fail? + } + + then: + def keys = cfg.getKeys() + keys.size() == 3 + !keys.contains("something-irrelevant-to-apm") + !keys.contains("something-else-irrelevant") + cfg.getConfigId() == "12345" + cfg.get("KEY_ONE") == "VALUE_ONE" + cfg.get("KEY_TWO") == "VALUE_TWO" + cfg.get("KEY_THREE") == "VALUE_THREE" + } +} 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 50f5e499578..0a68707e8ac 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 @@ -115,11 +115,12 @@ class StableConfigSourceTest extends DDSpecification { StableConfigSource stableCfg = new StableConfigSource(filePath.toString(), ConfigOrigin.USER_STABLE_CONFIG) then: - stableCfg.getConfigId() == configId - stableCfg.getKeys().size() == configs.size() - for (key in stableCfg.getKeys()) { - key = key.substring(4) // Cut `DD_` - stableCfg.get(key) == configs.get(key) + // FIXME: This is failing because configId is not loaded until stableCfg.get is invoked + // stableCfg.getConfigId() == configId + for (key in configs.keySet()) { + String keyString = (String) key + keyString = keyString.substring(4) // Cut `DD_` + stableCfg.get(keyString) == configs.get(key) } Files.delete(filePath) @@ -148,6 +149,7 @@ class StableConfigSourceTest extends DDSpecification { } } + // Use this if you want to explicitly write/test configId static writeFileYaml(Path filePath, String configId, Map configs) { DumperOptions options = new DumperOptions() options.setDefaultFlowStyle(DumperOptions.FlowStyle.BLOCK) @@ -169,6 +171,20 @@ class StableConfigSourceTest extends DDSpecification { Files.write(filePath, yamlString.getBytes(), openOpts) } + static writeFileYaml(Path filePath, Map fileContent) { + DumperOptions options = new DumperOptions() + options.setDefaultFlowStyle(DumperOptions.FlowStyle.BLOCK) + + // Prepare to write the data map to the file in yaml format + Yaml yaml = new Yaml(options) + String yamlString + + yamlString = yaml.dump(fileContent) + + StandardOpenOption[] openOpts = [StandardOpenOption.WRITE] as StandardOpenOption[] + Files.write(filePath, yamlString.getBytes(), openOpts) + } + def writeFileRaw(Path filePath, String configId, String configs) { String data = configId + "\n" + configs StandardOpenOption[] openOpts = [StandardOpenOption.WRITE] as StandardOpenOption[] From 39fa0e22e8603f8070d0e79334044e90d4b30e03 Mon Sep 17 00:00:00 2001 From: Mikayla Toffler Date: Thu, 13 Feb 2025 14:22:47 -0500 Subject: [PATCH 22/41] Finalize StableConfigParser and tests --- .../config/provider/StableConfigParser.java | 21 ++++++++++++------- .../config/provider/StableConfigSource.java | 5 ++--- .../provider/StableConfigParserTest.groovy | 9 ++++---- 3 files changed, 19 insertions(+), 16 deletions(-) diff --git a/internal-api/src/main/java/datadog/trace/bootstrap/config/provider/StableConfigParser.java b/internal-api/src/main/java/datadog/trace/bootstrap/config/provider/StableConfigParser.java index e58959f9657..3094cbb746f 100644 --- a/internal-api/src/main/java/datadog/trace/bootstrap/config/provider/StableConfigParser.java +++ b/internal-api/src/main/java/datadog/trace/bootstrap/config/provider/StableConfigParser.java @@ -21,7 +21,7 @@ public class StableConfigParser { // without public static StableConfigSource.StableConfig parse(String filePath) throws IOException { - try (Stream lines = Files.lines(Paths.get("file.txt"))) { + try (Stream lines = Files.lines(Paths.get(filePath))) { StableConfigSource.StableConfig cfg = new StableConfigSource.StableConfig(); // Use AtomicBoolean because it's mutable within a lambda expression to AtomicBoolean apmConfigFound = @@ -30,7 +30,7 @@ public static StableConfigSource.StableConfig parse(String filePath) throws IOEx line -> { Matcher matcher = idPattern.matcher(line); if (matcher.find()) { - cfg.setConfigId(matcher.group(2).trim()); + cfg.setConfigId(trimQuotes(matcher.group(1).trim())); return; // go to next line } @@ -42,13 +42,10 @@ public static StableConfigSource.StableConfig parse(String filePath) throws IOEx if (apmConfigFound.get()) { Matcher keyValueMatcher = keyValPattern.matcher(line); if (keyValueMatcher.matches()) { - String key = keyValueMatcher.group(1).trim(); - String value = keyValueMatcher.group(2).trim(); // If value has quotes, remove them - if (value.startsWith("\"") && value.endsWith("\"")) { - value = value.substring(1, value.length() - 1); - } - cfg.put(key, value); // Store key-value pair in map + cfg.put( + keyValueMatcher.group(1).trim(), + trimQuotes(keyValueMatcher.group(2).trim())); // Store key-value pair in map } else { // If we encounter a non-indented or non-key-value line, stop processing apmConfigFound.set(false); @@ -58,4 +55,12 @@ public static StableConfigSource.StableConfig parse(String filePath) throws IOEx return cfg; } } + + private static String trimQuotes(String value) { + if ((value.startsWith("'") && value.endsWith("'")) + || (value.startsWith("\"") && value.endsWith("\""))) { + return value.substring(1, value.length() - 1); + } + return value; + } } diff --git a/internal-api/src/main/java/datadog/trace/bootstrap/config/provider/StableConfigSource.java b/internal-api/src/main/java/datadog/trace/bootstrap/config/provider/StableConfigSource.java index 2dbff1e7223..ad7e503880e 100644 --- a/internal-api/src/main/java/datadog/trace/bootstrap/config/provider/StableConfigSource.java +++ b/internal-api/src/main/java/datadog/trace/bootstrap/config/provider/StableConfigSource.java @@ -1,7 +1,5 @@ package datadog.trace.bootstrap.config.provider; -import static datadog.trace.util.Strings.propertyNameToEnvironmentVariableName; - import datadog.trace.api.ConfigOrigin; import java.io.File; import java.io.IOException; @@ -198,7 +196,8 @@ void setConfigId(String configId) { // TODO: Make this.apmConfiguration a Map instead public String get(String key) { - return (String) this.apmConfiguration.get(propertyNameToEnvironmentVariableName(key)); + // return (String) this.apmConfiguration.get(propertyNameToEnvironmentVariableName(key)); + return (String) this.apmConfiguration.get(key); } public Set getKeys() { diff --git a/internal-api/src/test/groovy/datadog/trace/bootstrap/config/provider/StableConfigParserTest.groovy b/internal-api/src/test/groovy/datadog/trace/bootstrap/config/provider/StableConfigParserTest.groovy index 3676464066b..c4bee83ec9b 100644 --- a/internal-api/src/test/groovy/datadog/trace/bootstrap/config/provider/StableConfigParserTest.groovy +++ b/internal-api/src/test/groovy/datadog/trace/bootstrap/config/provider/StableConfigParserTest.groovy @@ -10,7 +10,7 @@ class StableConfigParserTest extends DDSpecification { when: Path filePath = StableConfigSourceTest.tempFile() if (filePath == null) { - return // fail? + throw new AssertionError("Failed to create test file: ${e.message}") } HashMap configs = new HashMap<>() configs.put("KEY_ONE", "VALUE_ONE") @@ -27,15 +27,14 @@ class StableConfigParserTest extends DDSpecification { StableConfigSourceTest.writeFileYaml(filePath, fileContent) } catch (IOException e) { println "Error writing to file: ${e.message}" - return // fail? + throw new AssertionError("Failed to write to file: ${e.message}") } StableConfigSource.StableConfig cfg try { cfg = StableConfigParser.parse(filePath.toString()) } catch (Exception e) { - println "Error parsing file: ${e.message}" - return // fail? + throw new AssertionError("Failed to parse the file: ${e.message}") } then: @@ -43,7 +42,7 @@ class StableConfigParserTest extends DDSpecification { keys.size() == 3 !keys.contains("something-irrelevant-to-apm") !keys.contains("something-else-irrelevant") - cfg.getConfigId() == "12345" + cfg.getConfigId().trim() == ("12345") cfg.get("KEY_ONE") == "VALUE_ONE" cfg.get("KEY_TWO") == "VALUE_TWO" cfg.get("KEY_THREE") == "VALUE_THREE" From 53dec929932ab24cf6b21dee1ac4d5cdec2ea593 Mon Sep 17 00:00:00 2001 From: Mikayla Toffler Date: Thu, 13 Feb 2025 14:56:04 -0500 Subject: [PATCH 23/41] User parser in StableconfigSource; no more lazy loading --- .../java/datadog/trace/bootstrap/Agent.java | 7 +- .../config/provider/ConfigProvider.java | 24 +-- .../config/provider/StableConfigSource.java | 164 ++---------------- .../provider/StableConfigSourceSingleton.java | 23 --- .../provider/StableConfigParserTest.groovy | 5 +- .../provider/StableConfigSourceTest.groovy | 38 ++-- 6 files changed, 47 insertions(+), 214 deletions(-) delete mode 100644 internal-api/src/main/java/datadog/trace/bootstrap/config/provider/StableConfigSourceSingleton.java diff --git a/dd-java-agent/agent-bootstrap/src/main/java/datadog/trace/bootstrap/Agent.java b/dd-java-agent/agent-bootstrap/src/main/java/datadog/trace/bootstrap/Agent.java index e10dab2d6f0..0bdf2c9fc2b 100644 --- a/dd-java-agent/agent-bootstrap/src/main/java/datadog/trace/bootstrap/Agent.java +++ b/dd-java-agent/agent-bootstrap/src/main/java/datadog/trace/bootstrap/Agent.java @@ -34,7 +34,6 @@ import datadog.trace.api.scopemanager.ScopeListener; import datadog.trace.bootstrap.benchmark.StaticEventLogger; import datadog.trace.bootstrap.config.provider.StableConfigSource; -import datadog.trace.bootstrap.config.provider.StableConfigSourceSingleton; import datadog.trace.bootstrap.instrumentation.api.AgentTracer; import datadog.trace.bootstrap.instrumentation.api.AgentTracer.TracerAPI; import datadog.trace.bootstrap.instrumentation.api.ProfilingContextIntegration; @@ -1208,15 +1207,13 @@ private static boolean isFeatureEnabled(AgentFeature feature) { final String featureEnabledSysprop = feature.getSystemProp(); String featureEnabled = System.getProperty(featureEnabledSysprop); if (featureEnabled == null) { - featureEnabled = - getStableConfig(StableConfigSourceSingleton.getManaged(), featureEnabledSysprop); + featureEnabled = getStableConfig(StableConfigSource.MANAGED, featureEnabledSysprop); } if (featureEnabled == null) { featureEnabled = ddGetEnv(featureEnabledSysprop); } if (featureEnabled == null) { - featureEnabled = - getStableConfig(StableConfigSourceSingleton.getUser(), featureEnabledSysprop); + featureEnabled = getStableConfig(StableConfigSource.USER, featureEnabledSysprop); } if (feature.isEnabledByDefault()) { 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 6beddb1a9e8..aa81cd4dd3b 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 @@ -356,19 +356,19 @@ public static ConfigProvider createDefault() { if (configProperties.isEmpty()) { return new ConfigProvider( new SystemPropertiesConfigSource(), - StableConfigSourceSingleton.getManaged(), + StableConfigSource.MANAGED, new EnvironmentConfigSource(), new OtelEnvironmentConfigSource(), - StableConfigSourceSingleton.getUser(), + StableConfigSource.USER, new CapturedEnvironmentConfigSource()); } else { return new ConfigProvider( new SystemPropertiesConfigSource(), - StableConfigSourceSingleton.getManaged(), + StableConfigSource.MANAGED, new EnvironmentConfigSource(), new PropertiesConfigSource(configProperties, true), new OtelEnvironmentConfigSource(configProperties), - StableConfigSourceSingleton.getUser(), + StableConfigSource.USER, new CapturedEnvironmentConfigSource()); } } @@ -382,20 +382,20 @@ public static ConfigProvider withoutCollector() { return new ConfigProvider( false, new SystemPropertiesConfigSource(), - StableConfigSourceSingleton.getManaged(), + StableConfigSource.MANAGED, new EnvironmentConfigSource(), new OtelEnvironmentConfigSource(), - StableConfigSourceSingleton.getUser(), + StableConfigSource.USER, new CapturedEnvironmentConfigSource()); } else { return new ConfigProvider( false, new SystemPropertiesConfigSource(), - StableConfigSourceSingleton.getManaged(), + StableConfigSource.MANAGED, new EnvironmentConfigSource(), new PropertiesConfigSource(configProperties, true), new OtelEnvironmentConfigSource(configProperties), - StableConfigSourceSingleton.getUser(), + StableConfigSource.USER, new CapturedEnvironmentConfigSource()); } } @@ -412,21 +412,21 @@ public static ConfigProvider withPropertiesOverride(Properties properties) { if (configProperties.isEmpty()) { return new ConfigProvider( new SystemPropertiesConfigSource(), - StableConfigSourceSingleton.getManaged(), + StableConfigSource.MANAGED, new EnvironmentConfigSource(), providedConfigSource, new OtelEnvironmentConfigSource(), - StableConfigSourceSingleton.getUser(), + StableConfigSource.USER, new CapturedEnvironmentConfigSource()); } else { return new ConfigProvider( providedConfigSource, new SystemPropertiesConfigSource(), - StableConfigSourceSingleton.getManaged(), + StableConfigSource.MANAGED, new EnvironmentConfigSource(), new PropertiesConfigSource(configProperties, true), new OtelEnvironmentConfigSource(configProperties), - StableConfigSourceSingleton.getUser(), + StableConfigSource.USER, new CapturedEnvironmentConfigSource()); } } diff --git a/internal-api/src/main/java/datadog/trace/bootstrap/config/provider/StableConfigSource.java b/internal-api/src/main/java/datadog/trace/bootstrap/config/provider/StableConfigSource.java index ad7e503880e..560145da742 100644 --- a/internal-api/src/main/java/datadog/trace/bootstrap/config/provider/StableConfigSource.java +++ b/internal-api/src/main/java/datadog/trace/bootstrap/config/provider/StableConfigSource.java @@ -1,55 +1,42 @@ package datadog.trace.bootstrap.config.provider; +import static datadog.trace.util.Strings.propertyNameToEnvironmentVariableName; + import datadog.trace.api.ConfigOrigin; -import java.io.File; import java.io.IOException; -import java.io.InputStream; -import java.nio.file.Files; import java.util.HashMap; -import java.util.Iterator; import java.util.Map; import java.util.Set; -import org.slf4j.Logger; -import org.slf4j.LoggerFactory; -import org.yaml.snakeyaml.LoaderOptions; -import org.yaml.snakeyaml.Yaml; -import org.yaml.snakeyaml.constructor.SafeConstructor; public final class StableConfigSource extends ConfigProvider.Source { public static final String USER_STABLE_CONFIG_PATH = "/etc/datadog-agent/application_monitoring.yaml"; public static final String MANAGED_STABLE_CONFIG_PATH = "/etc/datadog-agent/managed/datadog-apm-libraries/stable/application_monitoring.yaml"; - - private static final Logger log = LoggerFactory.getLogger(StableConfigSource.class); + public static StableConfigSource USER = + new StableConfigSource(USER_STABLE_CONFIG_PATH, ConfigOrigin.USER_STABLE_CONFIG); + public static final StableConfigSource MANAGED = + new StableConfigSource( + StableConfigSource.MANAGED_STABLE_CONFIG_PATH, ConfigOrigin.MANAGED_STABLE_CONFIG); private final ConfigOrigin fileOrigin; - private final String filePath; - - // loadedConfig holds the configuration entries lazily loaded from the configuration file - private Map loadedConfig; - // iterator is used to traverse through the entries of the loadedConfig map as processing - // progresses - private Iterator> iterator; - private Boolean fileReadComplete; private final StableConfig config; StableConfigSource(String file, ConfigOrigin origin) { this.fileOrigin = origin; - this.filePath = file; - this.config = new StableConfig(); - // The below are initialized explicitly for clarity - this.fileReadComplete = false; - this.iterator = null; + StableConfig cfg; + try { + cfg = StableConfigParser.parse(file); + } catch (IOException e) { + cfg = new StableConfig(); + } + this.config = cfg; } @Override public String get(String key) { - if (this.fileReadComplete) { - return this.config.get(key); - } - return this.findAndReturnEarly(key); + return this.config.get(propertyNameToEnvironmentVariableName(key)); } @Override @@ -65,120 +52,8 @@ public String getConfigId() { return this.config.getConfigId(); } - /** - * Searches for a specific key and quick returns its associated value if found. This function - * first checks if the configuration entries have already been loaded from the yaml file. If not, - * it attempts to load the configuration from the file and process it, returning the key's value - * if found. - * - * @param key the key to search for in the configuration. - * @return the configuration value corresponding to the given key, or null if not found or if an - * error occurs. - */ - private String findAndReturnEarly(String key) { - if (this.loadedConfig != null) { - return iterateAndReturnEarly(key); - } - - HashMap data = loadConfigDataFromFile(); - if (data == null || data.isEmpty()) { - this.fileReadComplete = true; - return null; - } - - this.config.setConfigId((String) data.get("config_id")); - Object apmConfig = data.get("apm_configuration_default"); - - // If the key apm_configuration_default is not present in the file, or if the value of - // apm_configuration_default is not convertible to a hashmap, there's nothing to process; return - // nothing. - if (!(apmConfig instanceof HashMap)) { - this.fileReadComplete = true; - return null; - } - - this.loadedConfig = (HashMap) apmConfig; - return iterateAndReturnEarly(key); - } - - /** - * Attempts to load configuration data from the file. This function reads the file in YAML format - * and returns the contents as a HashMap. If an error occurs during the reading or processing of - * the file, it logs the error and returns null. - * - * @return a HashMap containing the configuration data, or null if an error occurs. - */ - private HashMap loadConfigDataFromFile() { - try { - return readYamlFromFile(this.filePath); - } catch (Exception e) { - this.fileReadComplete = true; - log.error("Error processing file: {}", e.getMessage()); - return null; - } - } - - /** - * Reads a YAML file from the specified file path and returns its contents as a HashMap. - * - * @param filePath The path to the YAML file to be read. It must be a valid path to an existing - * file. - * @return A HashMap containing the parsed data from the YAML file. - * @throws IOException If the specified file does not exist or cannot be accessed. - * @throws Exception If there is an error during YAML parsing, including invalid formatting or - * structure. - */ - public static HashMap readYamlFromFile(String filePath) throws Exception { - File file = new File(filePath); - if (!file.exists()) { - throw new IOException( - "Stable configuration file does not exist at the specified path: " + filePath); - } - Yaml yaml = new Yaml(new SafeConstructor(new LoaderOptions())); - InputStream input = Files.newInputStream(new File(filePath).toPath()); - try { - return yaml.load(input); - } catch (Exception e) { - throw new Exception( - "YAML parsing error in stable config file " + filePath + ": " + e.getMessage(), e); - } - } - - /** - * Iterates over the configuration entries and attempts to find the value for the given key. The - * method reuses an existing iterator to avoid re-iterating over the entire set of entries every - * time it is called, thus optimizing the processing. If the key is found during iteration, the - * corresponding value is returned immediately. - * - *

The function also populates the StableConfig `config` map with valid key-value pairs during - * the iteration. If no value is found for the key, or if the iteration completes without finding - * the key, the function marks the file as read and returns null. - * - * @param key the key to search for in the configuration entries. - * @return the configuration value associated with the key, or null if not found. - */ - private String iterateAndReturnEarly(String key) { - if (this.iterator == null) { - this.iterator = this.loadedConfig.entrySet().iterator(); - } - while (this.iterator.hasNext()) { - Map.Entry entry = iterator.next(); - Object entryKey = entry.getKey(); - Object entryValue = entry.getValue(); - if (entryKey instanceof String && entryValue != null) { - this.config.put(String.valueOf(entryKey), entryValue); - String configValue = this.config.get(key); - if (configValue != null) { - return configValue; - } - } - } - this.fileReadComplete = true; - return null; - } - /*public?*/ static class StableConfig { - private Map apmConfiguration; + private final Map apmConfiguration; private String configId; StableConfig() { @@ -186,7 +61,7 @@ private String iterateAndReturnEarly(String key) { this.configId = null; } - void put(String key, Object value) { + void put(String key, String value) { this.apmConfiguration.put(key, value); } @@ -194,17 +69,14 @@ void setConfigId(String configId) { this.configId = configId; } - // TODO: Make this.apmConfiguration a Map instead public String get(String key) { - // return (String) this.apmConfiguration.get(propertyNameToEnvironmentVariableName(key)); - return (String) this.apmConfiguration.get(key); + return this.apmConfiguration.get(key); } public Set getKeys() { return this.apmConfiguration.keySet(); } - // Might be null public String getConfigId() { return this.configId; } diff --git a/internal-api/src/main/java/datadog/trace/bootstrap/config/provider/StableConfigSourceSingleton.java b/internal-api/src/main/java/datadog/trace/bootstrap/config/provider/StableConfigSourceSingleton.java deleted file mode 100644 index 80e711df680..00000000000 --- a/internal-api/src/main/java/datadog/trace/bootstrap/config/provider/StableConfigSourceSingleton.java +++ /dev/null @@ -1,23 +0,0 @@ -package datadog.trace.bootstrap.config.provider; - -import datadog.trace.api.ConfigOrigin; - -public class StableConfigSourceSingleton { - private static final StableConfigSource USER = - new StableConfigSource( - StableConfigSource.USER_STABLE_CONFIG_PATH, ConfigOrigin.USER_STABLE_CONFIG); - private static final StableConfigSource MANAGED = - new StableConfigSource( - StableConfigSource.MANAGED_STABLE_CONFIG_PATH, ConfigOrigin.MANAGED_STABLE_CONFIG); - - // Private constructor to prevent instantiation - private StableConfigSourceSingleton() {} - - public static StableConfigSource getUser() { - return USER; - } - - public static StableConfigSource getManaged() { - return MANAGED; - } -} diff --git a/internal-api/src/test/groovy/datadog/trace/bootstrap/config/provider/StableConfigParserTest.groovy b/internal-api/src/test/groovy/datadog/trace/bootstrap/config/provider/StableConfigParserTest.groovy index c4bee83ec9b..a414fa3f8df 100644 --- a/internal-api/src/test/groovy/datadog/trace/bootstrap/config/provider/StableConfigParserTest.groovy +++ b/internal-api/src/test/groovy/datadog/trace/bootstrap/config/provider/StableConfigParserTest.groovy @@ -2,6 +2,7 @@ package datadog.trace.bootstrap.config.provider import datadog.trace.test.util.DDSpecification +import java.nio.file.Files import java.nio.file.Path class StableConfigParserTest extends DDSpecification { @@ -10,7 +11,7 @@ class StableConfigParserTest extends DDSpecification { when: Path filePath = StableConfigSourceTest.tempFile() if (filePath == null) { - throw new AssertionError("Failed to create test file: ${e.message}") + throw new AssertionError("Failed to create test file") } HashMap configs = new HashMap<>() configs.put("KEY_ONE", "VALUE_ONE") @@ -26,7 +27,6 @@ class StableConfigParserTest extends DDSpecification { try { StableConfigSourceTest.writeFileYaml(filePath, fileContent) } catch (IOException e) { - println "Error writing to file: ${e.message}" throw new AssertionError("Failed to write to file: ${e.message}") } @@ -46,5 +46,6 @@ class StableConfigParserTest extends DDSpecification { cfg.get("KEY_ONE") == "VALUE_ONE" cfg.get("KEY_TWO") == "VALUE_TWO" cfg.get("KEY_THREE") == "VALUE_THREE" + Files.delete(filePath) } } 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 0a68707e8ac..14a734c2e50 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 @@ -22,22 +22,12 @@ class StableConfigSourceTest extends DDSpecification { } def "test empty file"() { - // test empty file when: - Path filePath - StableConfigSource config - try { - filePath = Files.createTempFile("testFile_", ".yaml") - } catch (IOException e) { - println "Error creating file: ${e.message}" - e.printStackTrace() - return // or throw new RuntimeException("File creation failed", e) - } - if (filePath != null) { - config = new StableConfigSource(filePath.toString(), ConfigOrigin.USER_STABLE_CONFIG) - } else { - return + Path filePath = tempFile() + if (filePath == null) { + throw new AssertionError("Failed to create test file") } + StableConfigSource config = new StableConfigSource(filePath.toString(), ConfigOrigin.USER_STABLE_CONFIG) then: config.getKeys().size() == 0 @@ -48,7 +38,7 @@ class StableConfigSourceTest extends DDSpecification { when: Path filePath = tempFile() if (filePath == null) { - return // fail? + throw new AssertionError("Failed to create test file") } def configs = new HashMap<>() << ["DD_SERVICE": "svc", "DD_ENV": "env", "CONFIG_NO_DD": "value123"] @@ -56,9 +46,9 @@ class StableConfigSourceTest extends DDSpecification { try { writeFileYaml(filePath, "12345", configs) } catch (IOException e) { - println "Error writing to file: ${e.message}" - return // fail? + throw new AssertionError("Failed to write to file: ${e.message}") } + StableConfigSource cfg = new StableConfigSource(filePath.toString(), ConfigOrigin.USER_STABLE_CONFIG) then: @@ -68,21 +58,20 @@ class StableConfigSourceTest extends DDSpecification { cfg.get("config_nonexistent") == null cfg.getKeys().size() == 3 cfg.getConfigId() == "12345" - + Files.delete(filePath) } def "test file invalid format"() { when: Path filePath = tempFile() if (filePath == null) { - return // fail? + throw new AssertionError("Failed to create test file") } try { writeFileRaw(filePath, configId, configs) } catch (IOException e) { - println "Error writing to file: ${e.message}" - return // fail? + throw new AssertionError("Failed to write to file: ${e.message}") } StableConfigSource stableCfg = new StableConfigSource(filePath.toString(), ConfigOrigin.USER_STABLE_CONFIG) @@ -102,21 +91,18 @@ class StableConfigSourceTest extends DDSpecification { when: Path filePath = tempFile() if (filePath == null) { - return // fail? + throw new AssertionError("Failed to create test file") } try { writeFileYaml(filePath, configId, configs) } catch (IOException e) { - println "Error writing to file: ${e.message}" - return // fail? + throw new AssertionError("Failed to write to file: ${e.message}") } StableConfigSource stableCfg = new StableConfigSource(filePath.toString(), ConfigOrigin.USER_STABLE_CONFIG) then: - // FIXME: This is failing because configId is not loaded until stableCfg.get is invoked - // stableCfg.getConfigId() == configId for (key in configs.keySet()) { String keyString = (String) key keyString = keyString.substring(4) // Cut `DD_` From 96881d0da149bc3f45f0a3cb1b56132e59e89f11 Mon Sep 17 00:00:00 2001 From: Mikayla Toffler Date: Thu, 13 Feb 2025 15:38:47 -0500 Subject: [PATCH 24/41] Add new logs for testing --- .../trace/bootstrap/config/provider/StableConfigParser.java | 2 ++ .../trace/bootstrap/config/provider/StableConfigSource.java | 5 +++++ 2 files changed, 7 insertions(+) diff --git a/internal-api/src/main/java/datadog/trace/bootstrap/config/provider/StableConfigParser.java b/internal-api/src/main/java/datadog/trace/bootstrap/config/provider/StableConfigParser.java index 3094cbb746f..d036a7a8d4e 100644 --- a/internal-api/src/main/java/datadog/trace/bootstrap/config/provider/StableConfigParser.java +++ b/internal-api/src/main/java/datadog/trace/bootstrap/config/provider/StableConfigParser.java @@ -20,6 +20,8 @@ public class StableConfigParser { // pairs, either with double quotes or // without + // TODO: Determine whether parse should be re-tried by StableConfigSource some number of times to + // account for cases where the file might not be available, or complete, by application startup public static StableConfigSource.StableConfig parse(String filePath) throws IOException { try (Stream lines = Files.lines(Paths.get(filePath))) { StableConfigSource.StableConfig cfg = new StableConfigSource.StableConfig(); diff --git a/internal-api/src/main/java/datadog/trace/bootstrap/config/provider/StableConfigSource.java b/internal-api/src/main/java/datadog/trace/bootstrap/config/provider/StableConfigSource.java index 560145da742..65428af67b2 100644 --- a/internal-api/src/main/java/datadog/trace/bootstrap/config/provider/StableConfigSource.java +++ b/internal-api/src/main/java/datadog/trace/bootstrap/config/provider/StableConfigSource.java @@ -7,6 +7,8 @@ import java.util.HashMap; import java.util.Map; import java.util.Set; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; public final class StableConfigSource extends ConfigProvider.Source { public static final String USER_STABLE_CONFIG_PATH = @@ -19,6 +21,8 @@ public final class StableConfigSource extends ConfigProvider.Source { new StableConfigSource( StableConfigSource.MANAGED_STABLE_CONFIG_PATH, ConfigOrigin.MANAGED_STABLE_CONFIG); + private static final Logger log = LoggerFactory.getLogger(StableConfigSource.class); + private final ConfigOrigin fileOrigin; private final StableConfig config; @@ -29,6 +33,7 @@ public final class StableConfigSource extends ConfigProvider.Source { try { cfg = StableConfigParser.parse(file); } catch (IOException e) { + log.debug("Stable configuration file not available at specified path: {}", file); cfg = new StableConfig(); } this.config = cfg; From 09e22a735167f06d246d50149ce73555dc501bba Mon Sep 17 00:00:00 2001 From: Mikayla Toffler Date: Thu, 13 Feb 2025 15:48:23 -0500 Subject: [PATCH 25/41] remove snakeyaml from dependency tree in internal-api --- internal-api/build.gradle | 2 -- 1 file changed, 2 deletions(-) diff --git a/internal-api/build.gradle b/internal-api/build.gradle index b44301d460f..b0886477765 100644 --- a/internal-api/build.gradle +++ b/internal-api/build.gradle @@ -226,8 +226,6 @@ dependencies { // it contains annotations that are also present in the instrumented application classes api "com.datadoghq:dd-javac-plugin-client:0.2.2" - implementation 'org.yaml:snakeyaml:2.0' - testImplementation project(":utils:test-utils") testImplementation("org.assertj:assertj-core:3.20.2") testImplementation libs.bundles.junit5 From 519c443748ff0926239c8a1485ae89b1d0ece740 Mon Sep 17 00:00:00 2001 From: Stuart McCulloch Date: Fri, 14 Feb 2025 18:41:59 +0000 Subject: [PATCH 26/41] Fix class initialization order --- .../trace/bootstrap/config/provider/StableConfigSource.java | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/internal-api/src/main/java/datadog/trace/bootstrap/config/provider/StableConfigSource.java b/internal-api/src/main/java/datadog/trace/bootstrap/config/provider/StableConfigSource.java index 65428af67b2..0795c3bc7ad 100644 --- a/internal-api/src/main/java/datadog/trace/bootstrap/config/provider/StableConfigSource.java +++ b/internal-api/src/main/java/datadog/trace/bootstrap/config/provider/StableConfigSource.java @@ -11,6 +11,8 @@ import org.slf4j.LoggerFactory; public final class StableConfigSource extends ConfigProvider.Source { + private static final Logger log = LoggerFactory.getLogger(StableConfigSource.class); + public static final String USER_STABLE_CONFIG_PATH = "/etc/datadog-agent/application_monitoring.yaml"; public static final String MANAGED_STABLE_CONFIG_PATH = @@ -21,8 +23,6 @@ public final class StableConfigSource extends ConfigProvider.Source { new StableConfigSource( StableConfigSource.MANAGED_STABLE_CONFIG_PATH, ConfigOrigin.MANAGED_STABLE_CONFIG); - private static final Logger log = LoggerFactory.getLogger(StableConfigSource.class); - private final ConfigOrigin fileOrigin; private final StableConfig config; From 9a519e1fbafddb336a7adbbbaf74d3647953c44b Mon Sep 17 00:00:00 2001 From: Mikayla Toffler Date: Sat, 15 Feb 2025 12:45:53 -0500 Subject: [PATCH 27/41] update parser test to use raw file input as opposed to map --- internal-api/build.gradle | 2 + .../config/provider/StableConfigParser.java | 1 - .../provider/StableConfigParserTest.groovy | 39 ++++++++++--------- .../provider/StableConfigSourceTest.groovy | 19 +++------ 4 files changed, 28 insertions(+), 33 deletions(-) diff --git a/internal-api/build.gradle b/internal-api/build.gradle index b0886477765..b44301d460f 100644 --- a/internal-api/build.gradle +++ b/internal-api/build.gradle @@ -226,6 +226,8 @@ dependencies { // it contains annotations that are also present in the instrumented application classes api "com.datadoghq:dd-javac-plugin-client:0.2.2" + implementation 'org.yaml:snakeyaml:2.0' + testImplementation project(":utils:test-utils") testImplementation("org.assertj:assertj-core:3.20.2") testImplementation libs.bundles.junit5 diff --git a/internal-api/src/main/java/datadog/trace/bootstrap/config/provider/StableConfigParser.java b/internal-api/src/main/java/datadog/trace/bootstrap/config/provider/StableConfigParser.java index d036a7a8d4e..278f961d971 100644 --- a/internal-api/src/main/java/datadog/trace/bootstrap/config/provider/StableConfigParser.java +++ b/internal-api/src/main/java/datadog/trace/bootstrap/config/provider/StableConfigParser.java @@ -44,7 +44,6 @@ public static StableConfigSource.StableConfig parse(String filePath) throws IOEx if (apmConfigFound.get()) { Matcher keyValueMatcher = keyValPattern.matcher(line); if (keyValueMatcher.matches()) { - // If value has quotes, remove them cfg.put( keyValueMatcher.group(1).trim(), trimQuotes(keyValueMatcher.group(2).trim())); // Store key-value pair in map diff --git a/internal-api/src/test/groovy/datadog/trace/bootstrap/config/provider/StableConfigParserTest.groovy b/internal-api/src/test/groovy/datadog/trace/bootstrap/config/provider/StableConfigParserTest.groovy index a414fa3f8df..3eaa67f5e1b 100644 --- a/internal-api/src/test/groovy/datadog/trace/bootstrap/config/provider/StableConfigParserTest.groovy +++ b/internal-api/src/test/groovy/datadog/trace/bootstrap/config/provider/StableConfigParserTest.groovy @@ -7,25 +7,25 @@ import java.nio.file.Path class StableConfigParserTest extends DDSpecification { - def "test parser"() { + def "test valid file format"() { when: Path filePath = StableConfigSourceTest.tempFile() if (filePath == null) { throw new AssertionError("Failed to create test file") } - HashMap configs = new HashMap<>() - configs.put("KEY_ONE", "VALUE_ONE") - configs.put("KEY_TWO", "VALUE_TWO") - configs.put("KEY_THREE", "VALUE_THREE") - - HashMap fileContent = new HashMap<>() - fileContent.put("something-irrelevant-to-apm", "") - fileContent.put("config_id", "12345") - fileContent.put("something-else-irrelevant", "value-irrelevant") - fileContent.put("apm_configuration_default", configs) - + String yaml = """ +something-irrelevant: "" +config_id: 12345 +apm_configuration_default: + KEY_ONE: value_one + KEY_TWO: "value_two" + KEY_THREE: 100 + KEY_FOUR: true + KEY_FIVE: [a,b,c,d] +something-else-irrelevant: value-irrelevant +""" try { - StableConfigSourceTest.writeFileYaml(filePath, fileContent) + StableConfigSourceTest.writeFileRaw(filePath, yaml) } catch (IOException e) { throw new AssertionError("Failed to write to file: ${e.message}") } @@ -39,13 +39,16 @@ class StableConfigParserTest extends DDSpecification { then: def keys = cfg.getKeys() - keys.size() == 3 - !keys.contains("something-irrelevant-to-apm") + keys.size() == 5 + !keys.contains("something-irrelevant") !keys.contains("something-else-irrelevant") cfg.getConfigId().trim() == ("12345") - cfg.get("KEY_ONE") == "VALUE_ONE" - cfg.get("KEY_TWO") == "VALUE_TWO" - cfg.get("KEY_THREE") == "VALUE_THREE" + cfg.get("KEY_ONE") == "value_one" + cfg.get("KEY_TWO") == "value_two" + cfg.get("KEY_THREE") == "100" + cfg.get("KEY_FOUR") == "true" + cfg.get("KEY_FIVE") == "[a,b,c,d]" Files.delete(filePath) } +// def "test parser invalid file format"() {} } 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 14a734c2e50..ad0bbb252b9 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 @@ -135,7 +135,6 @@ class StableConfigSourceTest extends DDSpecification { } } - // Use this if you want to explicitly write/test configId static writeFileYaml(Path filePath, String configId, Map configs) { DumperOptions options = new DumperOptions() options.setDefaultFlowStyle(DumperOptions.FlowStyle.BLOCK) @@ -157,22 +156,14 @@ class StableConfigSourceTest extends DDSpecification { Files.write(filePath, yamlString.getBytes(), openOpts) } - static writeFileYaml(Path filePath, Map fileContent) { - DumperOptions options = new DumperOptions() - options.setDefaultFlowStyle(DumperOptions.FlowStyle.BLOCK) - - // Prepare to write the data map to the file in yaml format - Yaml yaml = new Yaml(options) - String yamlString - - yamlString = yaml.dump(fileContent) - + // Use this if you want to explicitly write/test configId + def writeFileRaw(Path filePath, String configId, String configs) { + String data = configId + "\n" + configs StandardOpenOption[] openOpts = [StandardOpenOption.WRITE] as StandardOpenOption[] - Files.write(filePath, yamlString.getBytes(), openOpts) + Files.write(filePath, data.getBytes(), openOpts) } - def writeFileRaw(Path filePath, String configId, String configs) { - String data = configId + "\n" + configs + static writeFileRaw(Path filePath, String data) { StandardOpenOption[] openOpts = [StandardOpenOption.WRITE] as StandardOpenOption[] Files.write(filePath, data.getBytes(), openOpts) } From a9f968f3c7ce8347a601db403e766bb60e5527d4 Mon Sep 17 00:00:00 2001 From: Mikayla Toffler Date: Tue, 18 Feb 2025 10:00:58 -0500 Subject: [PATCH 28/41] make ParserTest handle unexpected input format as well --- .../config/provider/StableConfigParser.java | 1 + .../config/provider/StableConfigSource.java | 32 +++++++++++++++++-- .../main/java/datadog/trace/util/Strings.java | 5 +++ .../provider/StableConfigParserTest.groovy | 9 ++++-- 4 files changed, 43 insertions(+), 4 deletions(-) diff --git a/internal-api/src/main/java/datadog/trace/bootstrap/config/provider/StableConfigParser.java b/internal-api/src/main/java/datadog/trace/bootstrap/config/provider/StableConfigParser.java index 278f961d971..62a12d48906 100644 --- a/internal-api/src/main/java/datadog/trace/bootstrap/config/provider/StableConfigParser.java +++ b/internal-api/src/main/java/datadog/trace/bootstrap/config/provider/StableConfigParser.java @@ -30,6 +30,7 @@ public static StableConfigSource.StableConfig parse(String filePath) throws IOEx new AtomicBoolean(false); // Track if we've found 'apm_configuration_default:' lines.forEach( line -> { + System.out.println("MIKAYLA: FILE LINE IS: " + line); Matcher matcher = idPattern.matcher(line); if (matcher.find()) { cfg.setConfigId(trimQuotes(matcher.group(1).trim())); diff --git a/internal-api/src/main/java/datadog/trace/bootstrap/config/provider/StableConfigSource.java b/internal-api/src/main/java/datadog/trace/bootstrap/config/provider/StableConfigSource.java index 0795c3bc7ad..a82abf132a0 100644 --- a/internal-api/src/main/java/datadog/trace/bootstrap/config/provider/StableConfigSource.java +++ b/internal-api/src/main/java/datadog/trace/bootstrap/config/provider/StableConfigSource.java @@ -25,23 +25,51 @@ public final class StableConfigSource extends ConfigProvider.Source { private final ConfigOrigin fileOrigin; - private final StableConfig config; + private StableConfig config; + + private final String filePath; + private boolean fileFound; + private static final int maxRetries = 3; + private int fileRetries; StableConfigSource(String file, ConfigOrigin origin) { this.fileOrigin = origin; + this.filePath = file; StableConfig cfg; try { cfg = StableConfigParser.parse(file); + this.fileFound = true; + // for testing + System.out.println("MIKAYLA: FILE AVAILABLE"); } catch (IOException e) { log.debug("Stable configuration file not available at specified path: {}", file); cfg = new StableConfig(); + this.fileFound = false; } this.config = cfg; } @Override public String get(String key) { - return this.config.get(propertyNameToEnvironmentVariableName(key)); + if (this.fileFound) { + return this.config.get(propertyNameToEnvironmentVariableName(key)); + } + if (this.fileRetries < maxRetries) { + this.fileRetries++; + try { + this.config = StableConfigParser.parse(this.filePath); + this.fileFound = true; + // for testing + System.out.println("MIKAYLA: FILE AVAILABLE"); + return this.config.get(propertyNameToEnvironmentVariableName(key)); + } catch (IOException e) { + log.debug( + "Retry number {}, Stable configuration file not available at specified path: {}", + this.fileRetries, + filePath); + } + } + return null; } @Override diff --git a/internal-api/src/main/java/datadog/trace/util/Strings.java b/internal-api/src/main/java/datadog/trace/util/Strings.java index 2fea0e107e5..188e89a4c2e 100644 --- a/internal-api/src/main/java/datadog/trace/util/Strings.java +++ b/internal-api/src/main/java/datadog/trace/util/Strings.java @@ -79,6 +79,11 @@ public static String replaceFirst(String str, String delimiter, String replaceme */ @Nonnull public static String propertyNameToEnvironmentVariableName(final String setting) { + // FOR TESTING + if (setting.equals("data.streams.enabled")) { + System.out.println( + "MIKAYLA in propertynametoenvvar; returning: " + "DD_" + toEnvVar(setting)); + } return "DD_" + toEnvVar(setting); } diff --git a/internal-api/src/test/groovy/datadog/trace/bootstrap/config/provider/StableConfigParserTest.groovy b/internal-api/src/test/groovy/datadog/trace/bootstrap/config/provider/StableConfigParserTest.groovy index 3eaa67f5e1b..4e0be2a4f0d 100644 --- a/internal-api/src/test/groovy/datadog/trace/bootstrap/config/provider/StableConfigParserTest.groovy +++ b/internal-api/src/test/groovy/datadog/trace/bootstrap/config/provider/StableConfigParserTest.groovy @@ -7,7 +7,7 @@ import java.nio.file.Path class StableConfigParserTest extends DDSpecification { - def "test valid file format"() { + def "test parser"() { when: Path filePath = StableConfigSourceTest.tempFile() if (filePath == null) { @@ -16,6 +16,12 @@ class StableConfigParserTest extends DDSpecification { String yaml = """ something-irrelevant: "" config_id: 12345 +something : not : expected << and weird format + inufjka << + [a, + b, + c, + d] apm_configuration_default: KEY_ONE: value_one KEY_TWO: "value_two" @@ -50,5 +56,4 @@ something-else-irrelevant: value-irrelevant cfg.get("KEY_FIVE") == "[a,b,c,d]" Files.delete(filePath) } -// def "test parser invalid file format"() {} } From 7c93f15621fddf0c012e7f42adb3f5f7a351215e Mon Sep 17 00:00:00 2001 From: Mikayla Toffler Date: Tue, 18 Feb 2025 10:56:22 -0500 Subject: [PATCH 29/41] Revert fileretries stuff --- .../config/provider/StableConfigSource.java | 30 ++----------------- 1 file changed, 2 insertions(+), 28 deletions(-) diff --git a/internal-api/src/main/java/datadog/trace/bootstrap/config/provider/StableConfigSource.java b/internal-api/src/main/java/datadog/trace/bootstrap/config/provider/StableConfigSource.java index a82abf132a0..5865488253c 100644 --- a/internal-api/src/main/java/datadog/trace/bootstrap/config/provider/StableConfigSource.java +++ b/internal-api/src/main/java/datadog/trace/bootstrap/config/provider/StableConfigSource.java @@ -25,51 +25,25 @@ public final class StableConfigSource extends ConfigProvider.Source { private final ConfigOrigin fileOrigin; - private StableConfig config; - - private final String filePath; - private boolean fileFound; - private static final int maxRetries = 3; - private int fileRetries; + private final StableConfig config; StableConfigSource(String file, ConfigOrigin origin) { this.fileOrigin = origin; - this.filePath = file; StableConfig cfg; try { cfg = StableConfigParser.parse(file); - this.fileFound = true; // for testing System.out.println("MIKAYLA: FILE AVAILABLE"); } catch (IOException e) { log.debug("Stable configuration file not available at specified path: {}", file); cfg = new StableConfig(); - this.fileFound = false; } this.config = cfg; } @Override public String get(String key) { - if (this.fileFound) { - return this.config.get(propertyNameToEnvironmentVariableName(key)); - } - if (this.fileRetries < maxRetries) { - this.fileRetries++; - try { - this.config = StableConfigParser.parse(this.filePath); - this.fileFound = true; - // for testing - System.out.println("MIKAYLA: FILE AVAILABLE"); - return this.config.get(propertyNameToEnvironmentVariableName(key)); - } catch (IOException e) { - log.debug( - "Retry number {}, Stable configuration file not available at specified path: {}", - this.fileRetries, - filePath); - } - } - return null; + return this.config.get(propertyNameToEnvironmentVariableName(key)); } @Override From 36a7e8aef1044150ae1a66520b606474375c7962 Mon Sep 17 00:00:00 2001 From: Mikayla Toffler Date: Tue, 18 Feb 2025 11:26:04 -0500 Subject: [PATCH 30/41] Change managed path to reflect system tests --- .../trace/bootstrap/config/provider/StableConfigSource.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/internal-api/src/main/java/datadog/trace/bootstrap/config/provider/StableConfigSource.java b/internal-api/src/main/java/datadog/trace/bootstrap/config/provider/StableConfigSource.java index 5865488253c..e5ea9a348b4 100644 --- a/internal-api/src/main/java/datadog/trace/bootstrap/config/provider/StableConfigSource.java +++ b/internal-api/src/main/java/datadog/trace/bootstrap/config/provider/StableConfigSource.java @@ -16,7 +16,7 @@ public final class StableConfigSource extends ConfigProvider.Source { public static final String USER_STABLE_CONFIG_PATH = "/etc/datadog-agent/application_monitoring.yaml"; public static final String MANAGED_STABLE_CONFIG_PATH = - "/etc/datadog-agent/managed/datadog-apm-libraries/stable/application_monitoring.yaml"; + "/etc/datadog-agent/managed/datadog-agent/stable/application_monitoring.yaml"; public static StableConfigSource USER = new StableConfigSource(USER_STABLE_CONFIG_PATH, ConfigOrigin.USER_STABLE_CONFIG); public static final StableConfigSource MANAGED = From 9598e1143cb6be49fa353f71d67968e52f02b844 Mon Sep 17 00:00:00 2001 From: Mikayla Toffler Date: Tue, 18 Feb 2025 11:32:46 -0500 Subject: [PATCH 31/41] remove superfluous comments/logs used for testing --- .../config/provider/StableConfigParser.java | 3 - .../config/provider/StableConfigSource.java | 2 - .../main/java/datadog/trace/util/Strings.java | 5 -- .../datadog/trace/api/ConfigTest.groovy | 72 ------------------- 4 files changed, 82 deletions(-) diff --git a/internal-api/src/main/java/datadog/trace/bootstrap/config/provider/StableConfigParser.java b/internal-api/src/main/java/datadog/trace/bootstrap/config/provider/StableConfigParser.java index 62a12d48906..0265b9a8b22 100644 --- a/internal-api/src/main/java/datadog/trace/bootstrap/config/provider/StableConfigParser.java +++ b/internal-api/src/main/java/datadog/trace/bootstrap/config/provider/StableConfigParser.java @@ -20,8 +20,6 @@ public class StableConfigParser { // pairs, either with double quotes or // without - // TODO: Determine whether parse should be re-tried by StableConfigSource some number of times to - // account for cases where the file might not be available, or complete, by application startup public static StableConfigSource.StableConfig parse(String filePath) throws IOException { try (Stream lines = Files.lines(Paths.get(filePath))) { StableConfigSource.StableConfig cfg = new StableConfigSource.StableConfig(); @@ -30,7 +28,6 @@ public static StableConfigSource.StableConfig parse(String filePath) throws IOEx new AtomicBoolean(false); // Track if we've found 'apm_configuration_default:' lines.forEach( line -> { - System.out.println("MIKAYLA: FILE LINE IS: " + line); Matcher matcher = idPattern.matcher(line); if (matcher.find()) { cfg.setConfigId(trimQuotes(matcher.group(1).trim())); diff --git a/internal-api/src/main/java/datadog/trace/bootstrap/config/provider/StableConfigSource.java b/internal-api/src/main/java/datadog/trace/bootstrap/config/provider/StableConfigSource.java index e5ea9a348b4..245e75264cd 100644 --- a/internal-api/src/main/java/datadog/trace/bootstrap/config/provider/StableConfigSource.java +++ b/internal-api/src/main/java/datadog/trace/bootstrap/config/provider/StableConfigSource.java @@ -32,8 +32,6 @@ public final class StableConfigSource extends ConfigProvider.Source { StableConfig cfg; try { cfg = StableConfigParser.parse(file); - // for testing - System.out.println("MIKAYLA: FILE AVAILABLE"); } catch (IOException e) { log.debug("Stable configuration file not available at specified path: {}", file); cfg = new StableConfig(); diff --git a/internal-api/src/main/java/datadog/trace/util/Strings.java b/internal-api/src/main/java/datadog/trace/util/Strings.java index 188e89a4c2e..2fea0e107e5 100644 --- a/internal-api/src/main/java/datadog/trace/util/Strings.java +++ b/internal-api/src/main/java/datadog/trace/util/Strings.java @@ -79,11 +79,6 @@ public static String replaceFirst(String str, String delimiter, String replaceme */ @Nonnull public static String propertyNameToEnvironmentVariableName(final String setting) { - // FOR TESTING - if (setting.equals("data.streams.enabled")) { - System.out.println( - "MIKAYLA in propertynametoenvvar; returning: " + "DD_" + toEnvVar(setting)); - } return "DD_" + toEnvVar(setting); } diff --git a/internal-api/src/test/groovy/datadog/trace/api/ConfigTest.groovy b/internal-api/src/test/groovy/datadog/trace/api/ConfigTest.groovy index fc7d86560d1..b3e2a3d9b09 100644 --- a/internal-api/src/test/groovy/datadog/trace/api/ConfigTest.groovy +++ b/internal-api/src/test/groovy/datadog/trace/api/ConfigTest.groovy @@ -4,14 +4,10 @@ import datadog.trace.api.env.FixedCapturedEnvironment import datadog.trace.bootstrap.config.provider.AgentArgsInjector import datadog.trace.bootstrap.config.provider.ConfigConverter import datadog.trace.bootstrap.config.provider.ConfigProvider -import datadog.trace.bootstrap.config.provider.StableConfigSource -import datadog.trace.bootstrap.config.provider.StableConfigSourceTest import datadog.trace.test.util.DDSpecification import datadog.trace.util.throwable.FatalAgentMisconfigurationError import org.junit.Rule -import java.nio.file.Path - import static datadog.trace.api.ConfigDefaults.DEFAULT_HTTP_CLIENT_ERROR_STATUSES import static datadog.trace.api.ConfigDefaults.DEFAULT_HTTP_SERVER_ERROR_STATUSES import static datadog.trace.api.ConfigDefaults.DEFAULT_PARTIAL_FLUSH_MIN_SPANS @@ -30,8 +26,6 @@ import static datadog.trace.api.TracePropagationStyle.B3SINGLE import static datadog.trace.api.TracePropagationStyle.DATADOG import static datadog.trace.api.TracePropagationStyle.HAYSTACK import static datadog.trace.api.TracePropagationStyle.TRACECONTEXT -import static datadog.trace.api.config.AppSecConfig.APPSEC_ENABLED -import static datadog.trace.api.config.AppSecConfig.APPSEC_SCA_ENABLED import static datadog.trace.api.config.CiVisibilityConfig.CIVISIBILITY_AGENTLESS_ENABLED import static datadog.trace.api.config.CiVisibilityConfig.CIVISIBILITY_ENABLED import static datadog.trace.api.config.DebuggerConfig.DYNAMIC_INSTRUMENTATION_CLASSFILE_DUMP_ENABLED @@ -62,9 +56,6 @@ import static datadog.trace.api.config.GeneralConfig.SITE import static datadog.trace.api.config.GeneralConfig.TAGS import static datadog.trace.api.config.GeneralConfig.TRACER_METRICS_IGNORED_RESOURCES import static datadog.trace.api.config.GeneralConfig.VERSION -import static datadog.trace.api.config.GeneralConfig.DATA_STREAMS_ENABLED -import static datadog.trace.api.config.GeneralConfig.DATA_JOBS_ENABLED -import static datadog.trace.api.config.IastConfig.IAST_ENABLED import static datadog.trace.api.config.JmxFetchConfig.JMX_FETCH_CHECK_PERIOD import static datadog.trace.api.config.JmxFetchConfig.JMX_FETCH_ENABLED import static datadog.trace.api.config.JmxFetchConfig.JMX_FETCH_METRICS_CONFIGS @@ -106,7 +97,6 @@ import static datadog.trace.api.config.TraceInstrumentationConfig.DB_CLIENT_HOST import static datadog.trace.api.config.TraceInstrumentationConfig.HTTP_CLIENT_HOST_SPLIT_BY_DOMAIN import static datadog.trace.api.config.TraceInstrumentationConfig.RUNTIME_CONTEXT_FIELD_INJECTION import static datadog.trace.api.config.TraceInstrumentationConfig.TRACE_ENABLED -import static datadog.trace.api.config.TraceInstrumentationConfig.LOGS_INJECTION import static datadog.trace.api.config.TracerConfig.AGENT_HOST import static datadog.trace.api.config.TracerConfig.AGENT_PORT_LEGACY import static datadog.trace.api.config.TracerConfig.AGENT_UNIX_DOMAIN_SOCKET @@ -577,68 +567,6 @@ class ConfigTest extends DDSpecification { config.getLongRunningTraceFlushInterval() == 81 } - // def "specify overrides stable config"() { - // setup: - // Path filePath = StableConfigSourceTest.tempFile() - // if (filePath == null) { - // // fail fast? - // return - // } - // // Override for testing - // String originalPath - // if (user == true) { - // originalPath = StableConfigSource.USER_STABLE_CONFIG_PATH - // StableConfigSource.USER_STABLE_CONFIG_PATH = filePath.toAbsolutePath().toString() - // } else if (managed == true) { - // originalPath = StableConfigSource.MANAGED_STABLE_CONFIG_PATH - // StableConfigSource.MANAGED_STABLE_CONFIG_PATH = filePath.toAbsolutePath().toString() - // } - // def configs = new HashMap<>() - // configs.put(TRACE_ENABLED, "false") - // configs.put(DD_RUNTIME_METRICS_ENABLED_ENV, "false") - // configs.put(LOGS_INJECTION, "false") - // configs.put(PROFILING_ENABLED, "true") - // configs.put(DATA_STREAMS_ENABLED, "true") - // configs.put(APPSEC_ENABLED, "true") - // configs.put(IAST_ENABLED, "true") - // configs.put(DEBUGGER_ENABLED, "true") - // configs.put(DATA_JOBS_ENABLED, "true") - // configs.put(APPSEC_SCA_ENABLED, "true") - // - // try { - // StableConfigSourceTest.writeFileYaml(filePath, "12345", configs) - // } catch (IOException e) { - // println "Error writing to file: ${e.message}" - // return // fail? - // } - // - // when: - // def config = new Config() - // - // then: - // !config.traceEnabled - // !config.runtimeMetricsEnabled - // !config.logsInjectionEnabled - // config.profilingEnabled - // config.dataStreamsEnabled - // //config.appSecEnabled? - // config.iastDebugEnabled - // config.debuggerEnabled - // config.dataJobsEnabled - // config.appSecScaEnabled - // if (user == true) { - // StableConfigSource.USER_STABLE_CONFIG_PATH = originalPath - // } else if (managed == true) { - // StableConfigSource.MANAGED_STABLE_CONFIG_PATH = originalPath - // } - // - // where: - // user | managed - // true | false - // false | true - // - // } - def "sys props override env vars"() { setup: environmentVariables.set(DD_SERVICE_NAME_ENV, "still something else") From a34a54665cfab864b31f95c9d3a198efd5bc7ee5 Mon Sep 17 00:00:00 2001 From: Stuart McCulloch Date: Wed, 19 Feb 2025 10:32:46 +0000 Subject: [PATCH 32/41] snakeyaml is only used for testing now --- internal-api/build.gradle | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/internal-api/build.gradle b/internal-api/build.gradle index b1ee206a77a..19b1eded1a6 100644 --- a/internal-api/build.gradle +++ b/internal-api/build.gradle @@ -225,8 +225,6 @@ dependencies { // it contains annotations that are also present in the instrumented application classes api "com.datadoghq:dd-javac-plugin-client:0.2.2" - implementation 'org.yaml:snakeyaml:2.0' - testImplementation project(":utils:test-utils") testImplementation("org.assertj:assertj-core:3.20.2") testImplementation libs.bundles.junit5 @@ -234,6 +232,7 @@ dependencies { testImplementation libs.commons.math testImplementation libs.bundles.mockito testImplementation libs.truth + testImplementation 'org.yaml:snakeyaml:2.0' } jmh { From a3be88def564cfab143df3849d14e5e712f046d1 Mon Sep 17 00:00:00 2001 From: Stuart McCulloch Date: Wed, 19 Feb 2025 10:41:04 +0000 Subject: [PATCH 33/41] Register StableConfigSource with native-image since we need it at build time --- .../nativeimage/NativeImageGeneratorRunnerInstrumentation.java | 3 +++ 1 file changed, 3 insertions(+) diff --git a/dd-java-agent/instrumentation/graal/native-image/src/main/java/datadog/trace/instrumentation/graal/nativeimage/NativeImageGeneratorRunnerInstrumentation.java b/dd-java-agent/instrumentation/graal/native-image/src/main/java/datadog/trace/instrumentation/graal/nativeimage/NativeImageGeneratorRunnerInstrumentation.java index 3994ff991b5..eea77052645 100644 --- a/dd-java-agent/instrumentation/graal/native-image/src/main/java/datadog/trace/instrumentation/graal/nativeimage/NativeImageGeneratorRunnerInstrumentation.java +++ b/dd-java-agent/instrumentation/graal/native-image/src/main/java/datadog/trace/instrumentation/graal/nativeimage/NativeImageGeneratorRunnerInstrumentation.java @@ -78,6 +78,7 @@ public static void onEnter(@Advice.Argument(value = 0, readOnly = false) String[ + "datadog.trace.api.env.CapturedEnvironment:build_time," + "datadog.trace.api.ConfigCollector:rerun," + "datadog.trace.api.ConfigDefaults:build_time," + + "datadog.trace.api.ConfigOrigin:build_time," + "datadog.trace.api.ConfigSetting:build_time," + "datadog.trace.api.EventTracker:build_time," + "datadog.trace.api.InstrumenterConfig:build_time," @@ -106,6 +107,8 @@ public static void onEnter(@Advice.Argument(value = 0, readOnly = false) String[ + "datadog.trace.bootstrap.config.provider.EnvironmentConfigSource:build_time," + "datadog.trace.bootstrap.config.provider.OtelEnvironmentConfigSource:build_time," + "datadog.trace.bootstrap.config.provider.SystemPropertiesConfigSource:build_time," + + "datadog.trace.bootstrap.config.provider.StableConfigSource:build_time," + + "datadog.trace.bootstrap.config.provider.StableConfigSource$StableConfig:build_time," + "datadog.trace.bootstrap.Agent:build_time," + "datadog.trace.bootstrap.BootstrapProxy:build_time," + "datadog.trace.bootstrap.CallDepthThreadLocalMap:build_time," From 99cb07b7a293bc93aa6d1e8114cf56d00e9ae329 Mon Sep 17 00:00:00 2001 From: Mikayla Toffler <46911781+mtoffl01@users.noreply.github.com> Date: Thu, 20 Feb 2025 11:29:26 -0500 Subject: [PATCH 34/41] Update internal-api/src/main/java/datadog/trace/bootstrap/config/provider/StableConfigSource.java Co-authored-by: Stuart McCulloch --- .../trace/bootstrap/config/provider/StableConfigSource.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/internal-api/src/main/java/datadog/trace/bootstrap/config/provider/StableConfigSource.java b/internal-api/src/main/java/datadog/trace/bootstrap/config/provider/StableConfigSource.java index 245e75264cd..8eb106ad3fc 100644 --- a/internal-api/src/main/java/datadog/trace/bootstrap/config/provider/StableConfigSource.java +++ b/internal-api/src/main/java/datadog/trace/bootstrap/config/provider/StableConfigSource.java @@ -32,7 +32,7 @@ public final class StableConfigSource extends ConfigProvider.Source { StableConfig cfg; try { cfg = StableConfigParser.parse(file); - } catch (IOException e) { + } catch (Throwable e) { log.debug("Stable configuration file not available at specified path: {}", file); cfg = new StableConfig(); } From cf99c4f34a1e4195977e6fc0d303b2325fcbc5a6 Mon Sep 17 00:00:00 2001 From: Mikayla Toffler Date: Thu, 20 Feb 2025 14:38:45 -0500 Subject: [PATCH 35/41] Address PR comments --- .../java/datadog/trace/bootstrap/Agent.java | 3 +- .../config/provider/ConfigProvider.java | 1 - .../config/provider/StableConfigParser.java | 58 ++++++++++++------- .../config/provider/StableConfigSource.java | 14 +++-- 4 files changed, 47 insertions(+), 29 deletions(-) diff --git a/dd-java-agent/agent-bootstrap/src/main/java/datadog/trace/bootstrap/Agent.java b/dd-java-agent/agent-bootstrap/src/main/java/datadog/trace/bootstrap/Agent.java index 05fca84da12..19f9d291636 100644 --- a/dd-java-agent/agent-bootstrap/src/main/java/datadog/trace/bootstrap/Agent.java +++ b/dd-java-agent/agent-bootstrap/src/main/java/datadog/trace/bootstrap/Agent.java @@ -1233,7 +1233,6 @@ private static boolean isFeatureEnabled(AgentFeature feature) { // We need this hack because profiling in SSI can receive 'auto' value in // the enablement config return ProfilingEnablement.of(featureEnabled).isActive(); - // MIKAYLA: How does this order of precedence compete with stable config? } // false unless it's explicitly set to "true" return Boolean.parseBoolean(featureEnabled) || "1".equals(featureEnabled); @@ -1366,7 +1365,7 @@ private static String ddGetProperty(final String sysProp) { * Stable Configuration input */ private static String getStableConfig(StableConfigSource source, final String sysProp) { - return source.get(toEnvVar(sysProp)); + return source.get((sysProp)); } /** Looks for the "DD_" environment variable equivalent of the given "dd." system property. */ 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 aa81cd4dd3b..60acd7330ae 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 @@ -406,7 +406,6 @@ public static ConfigProvider withPropertiesOverride(Properties properties) { loadConfigurationFile( new ConfigProvider( new SystemPropertiesConfigSource(), - // MIKAYLA: To add StableConfig? new EnvironmentConfigSource(), providedConfigSource)); if (configProperties.isEmpty()) { diff --git a/internal-api/src/main/java/datadog/trace/bootstrap/config/provider/StableConfigParser.java b/internal-api/src/main/java/datadog/trace/bootstrap/config/provider/StableConfigParser.java index 0265b9a8b22..f0b3dc77052 100644 --- a/internal-api/src/main/java/datadog/trace/bootstrap/config/provider/StableConfigParser.java +++ b/internal-api/src/main/java/datadog/trace/bootstrap/config/provider/StableConfigParser.java @@ -1,62 +1,76 @@ package datadog.trace.bootstrap.config.provider; +import java.io.File; import java.io.IOException; import java.nio.file.Files; import java.nio.file.Paths; -import java.util.concurrent.atomic.AtomicBoolean; +import java.util.HashMap; +import java.util.Map; import java.util.regex.Matcher; import java.util.regex.Pattern; import java.util.stream.Stream; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; public class StableConfigParser { - - private static final Pattern idPattern = - Pattern.compile("^config_id\\s*:(.*)$"); // Match config_id: - private static final Pattern apmConfigPattern = - Pattern.compile("^apm_configuration_default:$"); // Match 'apm_configuration_default:' + // Match config_id: + private static final Pattern idPattern = Pattern.compile("^config_id\\s*:(.*)$"); + // Match 'apm_configuration_default:' + private static final Pattern apmConfigPattern = Pattern.compile("^apm_configuration_default:$"); + // Match indented (2 spaces) key-value pairs, either with double quotes or without private static final Pattern keyValPattern = - Pattern.compile( - "^\\s{2}([^:]+):\\s*(\"[^\"]*\"|[^\"\\n]*)$");; // Match indented (2 spaces) key-value - // pairs, either with double quotes or - // without + Pattern.compile("^\\s{2}([^:]+):\\s*(\"[^\"]*\"|[^\"\\n]*)$");; + private static final Logger log = LoggerFactory.getLogger(StableConfigParser.class); public static StableConfigSource.StableConfig parse(String filePath) throws IOException { + File file = new File(filePath); + if (!file.exists()) { + log.debug("Stable configuration file not available at specified path: {}", file); + return StableConfigSource.StableConfig.EMPTY; + } + Map configMap = new HashMap<>(); + String[] configId = new String[1]; try (Stream lines = Files.lines(Paths.get(filePath))) { - StableConfigSource.StableConfig cfg = new StableConfigSource.StableConfig(); // Use AtomicBoolean because it's mutable within a lambda expression to - AtomicBoolean apmConfigFound = - new AtomicBoolean(false); // Track if we've found 'apm_configuration_default:' + boolean[] apmConfigFound = + new boolean[1]; // Track if we've found 'apm_configuration_default:' + boolean[] configIdFound = new boolean[1]; // Track if we've found 'config_id:' lines.forEach( line -> { Matcher matcher = idPattern.matcher(line); if (matcher.find()) { - cfg.setConfigId(trimQuotes(matcher.group(1).trim())); + // Do not allow duplicate config_id keys + if (configIdFound[0]) { + throw new RuntimeException("Duplicate config_id keys found; file may be malformed"); + } + configId[0] = trimQuotes(matcher.group(1).trim()); + configIdFound[0] = true; return; // go to next line } - - if (!apmConfigFound.get() && apmConfigPattern.matcher(line).matches()) { - apmConfigFound.set(true); + // TODO: Do not allow duplicate apm_configuration_default keys? Or just return early. + if (!apmConfigFound[0] && apmConfigPattern.matcher(line).matches()) { + apmConfigFound[0] = true; return; // go to next line } - if (apmConfigFound.get()) { + if (apmConfigFound[0]) { Matcher keyValueMatcher = keyValPattern.matcher(line); if (keyValueMatcher.matches()) { - cfg.put( + configMap.put( keyValueMatcher.group(1).trim(), trimQuotes(keyValueMatcher.group(2).trim())); // Store key-value pair in map } else { // If we encounter a non-indented or non-key-value line, stop processing - apmConfigFound.set(false); + apmConfigFound[0] = false; } } }); - return cfg; + return new StableConfigSource.StableConfig(configId[0], configMap); } } private static String trimQuotes(String value) { - if ((value.startsWith("'") && value.endsWith("'")) + if (value.length() > 1 && (value.startsWith("'") && value.endsWith("'")) || (value.startsWith("\"") && value.endsWith("\""))) { return value.substring(1, value.length() - 1); } diff --git a/internal-api/src/main/java/datadog/trace/bootstrap/config/provider/StableConfigSource.java b/internal-api/src/main/java/datadog/trace/bootstrap/config/provider/StableConfigSource.java index 8eb106ad3fc..408652815e1 100644 --- a/internal-api/src/main/java/datadog/trace/bootstrap/config/provider/StableConfigSource.java +++ b/internal-api/src/main/java/datadog/trace/bootstrap/config/provider/StableConfigSource.java @@ -3,7 +3,7 @@ import static datadog.trace.util.Strings.propertyNameToEnvironmentVariableName; import datadog.trace.api.ConfigOrigin; -import java.io.IOException; +import java.util.Collections; import java.util.HashMap; import java.util.Map; import java.util.Set; @@ -17,7 +17,7 @@ public final class StableConfigSource extends ConfigProvider.Source { "/etc/datadog-agent/application_monitoring.yaml"; public static final String MANAGED_STABLE_CONFIG_PATH = "/etc/datadog-agent/managed/datadog-agent/stable/application_monitoring.yaml"; - public static StableConfigSource USER = + public static final StableConfigSource USER = new StableConfigSource(USER_STABLE_CONFIG_PATH, ConfigOrigin.USER_STABLE_CONFIG); public static final StableConfigSource MANAGED = new StableConfigSource( @@ -33,7 +33,7 @@ public final class StableConfigSource extends ConfigProvider.Source { try { cfg = StableConfigParser.parse(file); } catch (Throwable e) { - log.debug("Stable configuration file not available at specified path: {}", file); + log.debug("Stable configuration file not readable at specified path: {}", file); cfg = new StableConfig(); } this.config = cfg; @@ -57,7 +57,8 @@ public String getConfigId() { return this.config.getConfigId(); } - /*public?*/ static class StableConfig { + public static class StableConfig { + public static final StableConfig EMPTY = new StableConfig(null, Collections.emptyMap()); private final Map apmConfiguration; private String configId; @@ -66,6 +67,11 @@ public String getConfigId() { this.configId = null; } + StableConfig(String configId, Map configMap) { + this.configId = configId; + this.apmConfiguration = configMap; + } + void put(String key, String value) { this.apmConfiguration.put(key, value); } From 3d1a1d8688d96532a587b1632e9c4df76b944b05 Mon Sep 17 00:00:00 2001 From: Mikayla Toffler Date: Thu, 20 Feb 2025 14:54:58 -0500 Subject: [PATCH 36/41] Change javadoc on getStableConfig function --- .../src/main/java/datadog/trace/bootstrap/Agent.java | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/dd-java-agent/agent-bootstrap/src/main/java/datadog/trace/bootstrap/Agent.java b/dd-java-agent/agent-bootstrap/src/main/java/datadog/trace/bootstrap/Agent.java index 19f9d291636..9c2a7e2f51b 100644 --- a/dd-java-agent/agent-bootstrap/src/main/java/datadog/trace/bootstrap/Agent.java +++ b/dd-java-agent/agent-bootstrap/src/main/java/datadog/trace/bootstrap/Agent.java @@ -1360,10 +1360,7 @@ private static String ddGetProperty(final String sysProp) { return value; } - /** - * Looks for the "DD_" environment variable equivalent of the given "dd." system property in the - * Stable Configuration input - */ + /** Looks for sysProp in the Stable Configuration input */ private static String getStableConfig(StableConfigSource source, final String sysProp) { return source.get((sysProp)); } From 41fd3bc2a5e36d4f6b7acfa973e6f87b3bfe3b8f Mon Sep 17 00:00:00 2001 From: Mikayla Toffler <46911781+mtoffl01@users.noreply.github.com> Date: Thu, 20 Feb 2025 15:58:40 -0500 Subject: [PATCH 37/41] Update dd-java-agent/agent-bootstrap/src/main/java/datadog/trace/bootstrap/Agent.java Co-authored-by: Stuart McCulloch --- .../src/main/java/datadog/trace/bootstrap/Agent.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/dd-java-agent/agent-bootstrap/src/main/java/datadog/trace/bootstrap/Agent.java b/dd-java-agent/agent-bootstrap/src/main/java/datadog/trace/bootstrap/Agent.java index 9c2a7e2f51b..e9db18f9eb4 100644 --- a/dd-java-agent/agent-bootstrap/src/main/java/datadog/trace/bootstrap/Agent.java +++ b/dd-java-agent/agent-bootstrap/src/main/java/datadog/trace/bootstrap/Agent.java @@ -1362,7 +1362,7 @@ private static String ddGetProperty(final String sysProp) { /** Looks for sysProp in the Stable Configuration input */ private static String getStableConfig(StableConfigSource source, final String sysProp) { - return source.get((sysProp)); + return source.get(sysProp); } /** Looks for the "DD_" environment variable equivalent of the given "dd." system property. */ From 9dd51fe652999b73b6a5b4aff44f17a39b774cd0 Mon Sep 17 00:00:00 2001 From: Mikayla Toffler <46911781+mtoffl01@users.noreply.github.com> Date: Thu, 20 Feb 2025 16:03:29 -0500 Subject: [PATCH 38/41] Update internal-api/src/main/java/datadog/trace/bootstrap/config/provider/StableConfigSource.java Co-authored-by: Stuart McCulloch --- .../trace/bootstrap/config/provider/StableConfigSource.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/internal-api/src/main/java/datadog/trace/bootstrap/config/provider/StableConfigSource.java b/internal-api/src/main/java/datadog/trace/bootstrap/config/provider/StableConfigSource.java index 408652815e1..ea3fa4110fd 100644 --- a/internal-api/src/main/java/datadog/trace/bootstrap/config/provider/StableConfigSource.java +++ b/internal-api/src/main/java/datadog/trace/bootstrap/config/provider/StableConfigSource.java @@ -34,7 +34,7 @@ public final class StableConfigSource extends ConfigProvider.Source { cfg = StableConfigParser.parse(file); } catch (Throwable e) { log.debug("Stable configuration file not readable at specified path: {}", file); - cfg = new StableConfig(); + cfg = StableConfig.EMPTY; } this.config = cfg; } From f143632d7bfe91978d111d3d5ef4561f1e142b7e Mon Sep 17 00:00:00 2001 From: Mikayla Toffler <46911781+mtoffl01@users.noreply.github.com> Date: Thu, 20 Feb 2025 16:03:44 -0500 Subject: [PATCH 39/41] Update internal-api/src/main/java/datadog/trace/bootstrap/config/provider/StableConfigSource.java Co-authored-by: Stuart McCulloch --- .../trace/bootstrap/config/provider/StableConfigSource.java | 3 +++ 1 file changed, 3 insertions(+) diff --git a/internal-api/src/main/java/datadog/trace/bootstrap/config/provider/StableConfigSource.java b/internal-api/src/main/java/datadog/trace/bootstrap/config/provider/StableConfigSource.java index ea3fa4110fd..7ed52b173dc 100644 --- a/internal-api/src/main/java/datadog/trace/bootstrap/config/provider/StableConfigSource.java +++ b/internal-api/src/main/java/datadog/trace/bootstrap/config/provider/StableConfigSource.java @@ -41,6 +41,9 @@ public final class StableConfigSource extends ConfigProvider.Source { @Override public String get(String key) { + if (this.config == StableConfig.EMPTY) { + return null; + } return this.config.get(propertyNameToEnvironmentVariableName(key)); } From ea8d9f7307e680c356ddb7a28a4d6ab99d8607bd Mon Sep 17 00:00:00 2001 From: Mikayla Toffler Date: Thu, 20 Feb 2025 16:04:55 -0500 Subject: [PATCH 40/41] Apply PR suggestions round 2 --- .../config/provider/StableConfigParser.java | 2 +- .../config/provider/StableConfigSource.java | 16 +--------------- 2 files changed, 2 insertions(+), 16 deletions(-) diff --git a/internal-api/src/main/java/datadog/trace/bootstrap/config/provider/StableConfigParser.java b/internal-api/src/main/java/datadog/trace/bootstrap/config/provider/StableConfigParser.java index f0b3dc77052..c065d69ee5d 100644 --- a/internal-api/src/main/java/datadog/trace/bootstrap/config/provider/StableConfigParser.java +++ b/internal-api/src/main/java/datadog/trace/bootstrap/config/provider/StableConfigParser.java @@ -13,6 +13,7 @@ import org.slf4j.LoggerFactory; public class StableConfigParser { + private static final Logger log = LoggerFactory.getLogger(StableConfigParser.class); // Match config_id: private static final Pattern idPattern = Pattern.compile("^config_id\\s*:(.*)$"); // Match 'apm_configuration_default:' @@ -20,7 +21,6 @@ public class StableConfigParser { // Match indented (2 spaces) key-value pairs, either with double quotes or without private static final Pattern keyValPattern = Pattern.compile("^\\s{2}([^:]+):\\s*(\"[^\"]*\"|[^\"\\n]*)$");; - private static final Logger log = LoggerFactory.getLogger(StableConfigParser.class); public static StableConfigSource.StableConfig parse(String filePath) throws IOException { File file = new File(filePath); diff --git a/internal-api/src/main/java/datadog/trace/bootstrap/config/provider/StableConfigSource.java b/internal-api/src/main/java/datadog/trace/bootstrap/config/provider/StableConfigSource.java index 7ed52b173dc..f95dd71d33d 100644 --- a/internal-api/src/main/java/datadog/trace/bootstrap/config/provider/StableConfigSource.java +++ b/internal-api/src/main/java/datadog/trace/bootstrap/config/provider/StableConfigSource.java @@ -4,7 +4,6 @@ import datadog.trace.api.ConfigOrigin; import java.util.Collections; -import java.util.HashMap; import java.util.Map; import java.util.Set; import org.slf4j.Logger; @@ -63,26 +62,13 @@ public String getConfigId() { public static class StableConfig { public static final StableConfig EMPTY = new StableConfig(null, Collections.emptyMap()); private final Map apmConfiguration; - private String configId; - - StableConfig() { - this.apmConfiguration = new HashMap<>(); - this.configId = null; - } + private final String configId; StableConfig(String configId, Map configMap) { this.configId = configId; this.apmConfiguration = configMap; } - void put(String key, String value) { - this.apmConfiguration.put(key, value); - } - - void setConfigId(String configId) { - this.configId = configId; - } - public String get(String key) { return this.apmConfiguration.get(key); } From 4366cab02b0037f5a7006c14009b2073e28d9d6a Mon Sep 17 00:00:00 2001 From: Mikayla Toffler Date: Fri, 21 Feb 2025 12:14:22 -0500 Subject: [PATCH 41/41] Apply PR suggestions round 3 --- .../config/provider/StableConfigParser.java | 22 +++--- .../provider/StableConfigParserTest.groovy | 68 +++++++++++++++++++ 2 files changed, 78 insertions(+), 12 deletions(-) diff --git a/internal-api/src/main/java/datadog/trace/bootstrap/config/provider/StableConfigParser.java b/internal-api/src/main/java/datadog/trace/bootstrap/config/provider/StableConfigParser.java index c065d69ee5d..bb651e11588 100644 --- a/internal-api/src/main/java/datadog/trace/bootstrap/config/provider/StableConfigParser.java +++ b/internal-api/src/main/java/datadog/trace/bootstrap/config/provider/StableConfigParser.java @@ -31,29 +31,27 @@ public static StableConfigSource.StableConfig parse(String filePath) throws IOEx Map configMap = new HashMap<>(); String[] configId = new String[1]; try (Stream lines = Files.lines(Paths.get(filePath))) { - // Use AtomicBoolean because it's mutable within a lambda expression to - boolean[] apmConfigFound = - new boolean[1]; // Track if we've found 'apm_configuration_default:' - boolean[] configIdFound = new boolean[1]; // Track if we've found 'config_id:' + int apmConfigNotFound = -1, apmConfigStarted = 0, apmConfigComplete = 1; + int[] apmConfigFound = {apmConfigNotFound}; lines.forEach( line -> { Matcher matcher = idPattern.matcher(line); if (matcher.find()) { // Do not allow duplicate config_id keys - if (configIdFound[0]) { + if (configId[0] != null) { throw new RuntimeException("Duplicate config_id keys found; file may be malformed"); } configId[0] = trimQuotes(matcher.group(1).trim()); - configIdFound[0] = true; return; // go to next line } - // TODO: Do not allow duplicate apm_configuration_default keys? Or just return early. - if (!apmConfigFound[0] && apmConfigPattern.matcher(line).matches()) { - apmConfigFound[0] = true; + // TODO: Do not allow duplicate apm_configuration_default keys; and/or return early once + // apmConfigFound[0] == apmConfigComplete + if (apmConfigFound[0] == apmConfigNotFound + && apmConfigPattern.matcher(line).matches()) { + apmConfigFound[0] = apmConfigStarted; return; // go to next line } - - if (apmConfigFound[0]) { + if (apmConfigFound[0] == apmConfigStarted) { Matcher keyValueMatcher = keyValPattern.matcher(line); if (keyValueMatcher.matches()) { configMap.put( @@ -61,7 +59,7 @@ public static StableConfigSource.StableConfig parse(String filePath) throws IOEx trimQuotes(keyValueMatcher.group(2).trim())); // Store key-value pair in map } else { // If we encounter a non-indented or non-key-value line, stop processing - apmConfigFound[0] = false; + apmConfigFound[0] = apmConfigComplete; } } }); diff --git a/internal-api/src/test/groovy/datadog/trace/bootstrap/config/provider/StableConfigParserTest.groovy b/internal-api/src/test/groovy/datadog/trace/bootstrap/config/provider/StableConfigParserTest.groovy index 4e0be2a4f0d..f7cc3a0101c 100644 --- a/internal-api/src/test/groovy/datadog/trace/bootstrap/config/provider/StableConfigParserTest.groovy +++ b/internal-api/src/test/groovy/datadog/trace/bootstrap/config/provider/StableConfigParserTest.groovy @@ -56,4 +56,72 @@ something-else-irrelevant: value-irrelevant cfg.get("KEY_FIVE") == "[a,b,c,d]" Files.delete(filePath) } + + def "test duplicate config_id"() { + when: + Path filePath = StableConfigSourceTest.tempFile() + if (filePath == null) { + throw new AssertionError("Failed to create test file") + } + String yaml = """ +config_id: 12345 +something-irrelevant: "" +apm_configuration_default: + DD_KEY: value +config_id: 67890 +""" + + try { + StableConfigSourceTest.writeFileRaw(filePath, yaml) + } catch (IOException e) { + throw new AssertionError("Failed to write to file: ${e.message}") + } + + Exception exception + StableConfigSource.StableConfig cfg + try { + cfg = StableConfigParser.parse(filePath.toString()) + } catch (Exception e) { + exception = e + } + + then: + cfg == null + exception != null + exception.getMessage() == "Duplicate config_id keys found; file may be malformed" + } + + def "test duplicate apm_configuration_default"() { + // Assert that only the first entry is used + when: + Path filePath = StableConfigSourceTest.tempFile() + if (filePath == null) { + throw new AssertionError("Failed to create test file") + } + String yaml = """ +apm_configuration_default: + KEY_1: value_1 +something-else-irrelevant: value-irrelevant +apm_configuration_default: + KEY_2: value_2 +""" + try { + StableConfigSourceTest.writeFileRaw(filePath, yaml) + } catch (IOException e) { + throw new AssertionError("Failed to write to file: ${e.message}") + } + + StableConfigSource.StableConfig cfg + try { + cfg = StableConfigParser.parse(filePath.toString()) + } catch (Exception e) { + throw new AssertionError("Failed to parse the file: ${e.message}") + } + + then: + def keys = cfg.getKeys() + keys.size() == 1 + !keys.contains("KEY_2") + cfg.get("KEY_1") == "value_1" + } }