From a03562cb89984dfe72bdc98fe56fb7fddad11e1d Mon Sep 17 00:00:00 2001 From: Mikayla Toffler Date: Fri, 7 Feb 2025 14:55:09 -0500 Subject: [PATCH] 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)