From 901efee50e37f6f303d40b5c6108e00f03fa2d72 Mon Sep 17 00:00:00 2001 From: Luca Abbati Date: Tue, 4 Jun 2019 15:13:40 +0200 Subject: [PATCH] Remove static usage of low level config methods from outside the config class --- .../trace/agent/jmxfetch/JMXFetch.java | 2 +- .../trace/agent/decorator/BaseDecorator.java | 7 +- .../agent/decorator/HttpServerDecorator.java | 2 +- .../trace/agent/tooling/Instrumenter.java | 4 +- .../AbstractExecutorInstrumentation.java | 6 +- .../mdc/MDCInjectionInstrumentation.java | 3 +- .../TraceAnnotationsInstrumentation.java | 2 +- .../TraceConfigInstrumentation.java | 2 +- .../main/java/datadog/trace/api/Config.java | 70 ++++++++++++++++++- .../datadog/trace/api/ConfigTest.groovy | 6 +- 10 files changed, 83 insertions(+), 21 deletions(-) diff --git a/dd-java-agent/agent-jmxfetch/src/main/java/datadog/trace/agent/jmxfetch/JMXFetch.java b/dd-java-agent/agent-jmxfetch/src/main/java/datadog/trace/agent/jmxfetch/JMXFetch.java index 462e9a9192..321e626314 100644 --- a/dd-java-agent/agent-jmxfetch/src/main/java/datadog/trace/agent/jmxfetch/JMXFetch.java +++ b/dd-java-agent/agent-jmxfetch/src/main/java/datadog/trace/agent/jmxfetch/JMXFetch.java @@ -151,7 +151,7 @@ public class JMXFetch { for (final String config : split) { integrationName.clear(); integrationName.add(config.replace(".yaml", "")); - if (Config.jmxFetchIntegrationEnabled(integrationName, false)) { + if (Config.get().isJmxFetchIntegrationEnabled(integrationName, false)) { final URL resource = JMXFetch.class.getResource("metricconfigs/" + config); result.add(resource.getPath().split("\\.jar!/")[1]); } diff --git a/dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/decorator/BaseDecorator.java b/dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/decorator/BaseDecorator.java index 0f24cf4d05..258aa50d11 100644 --- a/dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/decorator/BaseDecorator.java +++ b/dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/decorator/BaseDecorator.java @@ -26,11 +26,12 @@ public abstract class BaseDecorator { final String[] instrumentationNames = instrumentationNames(); traceAnalyticsEnabled = instrumentationNames.length > 0 - && Config.traceAnalyticsIntegrationEnabled( - new TreeSet<>(Arrays.asList(instrumentationNames)), traceAnalyticsDefault()); + && Config.get() + .isTraceAnalyticsIntegrationEnabled( + new TreeSet<>(Arrays.asList(instrumentationNames)), traceAnalyticsDefault()); float rate = 1.0f; for (final String name : instrumentationNames) { - rate = Config.getFloatSettingFromEnvironment(name + ".analytics.sample-rate", rate); + rate = Config.get().getInstrumentationAnalyticsSampleRate(name); } traceAnalyticsSampleRate = rate; } diff --git a/dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/decorator/HttpServerDecorator.java b/dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/decorator/HttpServerDecorator.java index 9e9f7c56c7..20f9c68157 100644 --- a/dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/decorator/HttpServerDecorator.java +++ b/dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/decorator/HttpServerDecorator.java @@ -35,7 +35,7 @@ public abstract class HttpServerDecorator extends @Override protected boolean traceAnalyticsDefault() { - return Config.getBooleanSettingFromEnvironment(Config.TRACE_ANALYTICS_ENABLED, false); + return Config.get().isTraceAnalyticsEnabled(); } public Span onRequest(final Span span, final REQUEST request) { diff --git a/dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/tooling/Instrumenter.java b/dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/tooling/Instrumenter.java index 5f6be30f90..6f77481f66 100644 --- a/dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/tooling/Instrumenter.java +++ b/dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/tooling/Instrumenter.java @@ -52,7 +52,7 @@ public interface Instrumenter { instrumentationNames.add(instrumentationName); instrumentationPrimaryName = instrumentationName; - enabled = Config.integrationEnabled(instrumentationNames, defaultEnabled()); + enabled = Config.get().isIntegrationEnabled(instrumentationNames, defaultEnabled()); contextProvider = new FieldBackedProvider(this); } @@ -212,7 +212,7 @@ public interface Instrumenter { } protected boolean defaultEnabled() { - return Config.getBooleanSettingFromEnvironment("integrations.enabled", true); + return Config.get().isIntegrationsEnabled(); } } } diff --git a/dd-java-agent/instrumentation/java-concurrent/src/main/java/datadog/trace/instrumentation/java/concurrent/AbstractExecutorInstrumentation.java b/dd-java-agent/instrumentation/java-concurrent/src/main/java/datadog/trace/instrumentation/java/concurrent/AbstractExecutorInstrumentation.java index 3532dc0835..e94de3dd1e 100644 --- a/dd-java-agent/instrumentation/java-concurrent/src/main/java/datadog/trace/instrumentation/java/concurrent/AbstractExecutorInstrumentation.java +++ b/dd-java-agent/instrumentation/java-concurrent/src/main/java/datadog/trace/instrumentation/java/concurrent/AbstractExecutorInstrumentation.java @@ -22,8 +22,7 @@ public abstract class AbstractExecutorInstrumentation extends Instrumenter.Defau public static final String EXEC_NAME = "java_concurrent"; - private static final boolean TRACE_ALL_EXECUTORS = - Config.getBooleanSettingFromEnvironment("trace.executors.all", false); + private static final boolean TRACE_ALL_EXECUTORS = Config.get().isTraceExecutorsAll(); /** * Only apply executor instrumentation to whitelisted executors. To apply to all executors, use @@ -85,8 +84,7 @@ public abstract class AbstractExecutorInstrumentation extends Instrumenter.Defau "com.google.common.util.concurrent.MoreExecutors$ScheduledListeningDecorator", }; - final Set executors = - new HashSet<>(Config.getListSettingFromEnvironment("trace.executors", "")); + final Set executors = new HashSet<>(Config.get().getTraceExecutors()); executors.addAll(Arrays.asList(whitelist)); WHITELISTED_EXECUTORS = Collections.unmodifiableSet(executors); diff --git a/dd-java-agent/instrumentation/slf4j-mdc/src/main/java/datadog/trace/instrumentation/slf4j/mdc/MDCInjectionInstrumentation.java b/dd-java-agent/instrumentation/slf4j-mdc/src/main/java/datadog/trace/instrumentation/slf4j/mdc/MDCInjectionInstrumentation.java index ca8c98f4e0..992923d30c 100644 --- a/dd-java-agent/instrumentation/slf4j-mdc/src/main/java/datadog/trace/instrumentation/slf4j/mdc/MDCInjectionInstrumentation.java +++ b/dd-java-agent/instrumentation/slf4j-mdc/src/main/java/datadog/trace/instrumentation/slf4j/mdc/MDCInjectionInstrumentation.java @@ -34,8 +34,7 @@ public class MDCInjectionInstrumentation extends Instrumenter.Default { @Override protected boolean defaultEnabled() { - return Config.getBooleanSettingFromEnvironment( - Config.LOGS_INJECTION_ENABLED, Config.DEFAULT_LOGS_INJECTION_ENABLED); + return Config.get().isLogsInjectionEnabled(); } @Override diff --git a/dd-java-agent/instrumentation/trace-annotation/src/main/java/datadog/trace/instrumentation/trace_annotation/TraceAnnotationsInstrumentation.java b/dd-java-agent/instrumentation/trace-annotation/src/main/java/datadog/trace/instrumentation/trace_annotation/TraceAnnotationsInstrumentation.java index ac250de924..f2c1516957 100644 --- a/dd-java-agent/instrumentation/trace-annotation/src/main/java/datadog/trace/instrumentation/trace_annotation/TraceAnnotationsInstrumentation.java +++ b/dd-java-agent/instrumentation/trace-annotation/src/main/java/datadog/trace/instrumentation/trace_annotation/TraceAnnotationsInstrumentation.java @@ -47,7 +47,7 @@ public final class TraceAnnotationsInstrumentation extends Instrumenter.Default public TraceAnnotationsInstrumentation() { super("trace", "trace-annotation"); - final String configString = Config.getSettingFromEnvironment(Config.TRACE_ANNOTATIONS, null); + final String configString = Config.get().getTraceAnnotations(); if (configString == null) { additionalTraceAnnotations = Collections.unmodifiableSet(Sets.newHashSet(DEFAULT_ANNOTATIONS)); diff --git a/dd-java-agent/instrumentation/trace-annotation/src/main/java/datadog/trace/instrumentation/trace_annotation/TraceConfigInstrumentation.java b/dd-java-agent/instrumentation/trace-annotation/src/main/java/datadog/trace/instrumentation/trace_annotation/TraceConfigInstrumentation.java index fe2f560816..2365b54782 100644 --- a/dd-java-agent/instrumentation/trace-annotation/src/main/java/datadog/trace/instrumentation/trace_annotation/TraceConfigInstrumentation.java +++ b/dd-java-agent/instrumentation/trace-annotation/src/main/java/datadog/trace/instrumentation/trace_annotation/TraceConfigInstrumentation.java @@ -47,7 +47,7 @@ public class TraceConfigInstrumentation implements Instrumenter { private final Map> classMethodsToTrace; public TraceConfigInstrumentation() { - final String configString = Config.getSettingFromEnvironment(Config.TRACE_METHODS, null); + final String configString = Config.get().getTraceMethods(); if (configString == null || configString.trim().isEmpty()) { classMethodsToTrace = Collections.emptyMap(); diff --git a/dd-trace-api/src/main/java/datadog/trace/api/Config.java b/dd-trace-api/src/main/java/datadog/trace/api/Config.java index 66c9a9d145..80570345c6 100644 --- a/dd-trace-api/src/main/java/datadog/trace/api/Config.java +++ b/dd-trace-api/src/main/java/datadog/trace/api/Config.java @@ -38,6 +38,7 @@ public class Config { public static final String SERVICE_NAME = "service.name"; public static final String SERVICE = "service"; public static final String TRACE_ENABLED = "trace.enabled"; + public static final String INTEGRATIONS_ENABLED = "integrations.enabled"; public static final String WRITER_TYPE = "writer.type"; public static final String AGENT_HOST = "agent.host"; public static final String TRACE_AGENT_PORT = "trace.agent.port"; @@ -51,6 +52,8 @@ public class Config { public static final String JMX_TAGS = "trace.jmx.tags"; public static final String TRACE_ANALYTICS_ENABLED = "trace.analytics.enabled"; public static final String TRACE_ANNOTATIONS = "trace.annotations"; + public static final String TRACE_EXECUTORS_ALL = "trace.executors.all"; + public static final String TRACE_EXECUTORS = "trace.executors"; public static final String TRACE_METHODS = "trace.methods"; public static final String TRACE_CLASSES_EXCLUDE = "trace.classes.exclude"; public static final String TRACE_REPORT_HOSTNAME = "trace.report-hostname"; @@ -84,6 +87,7 @@ public class Config { public static final String DEFAULT_SERVICE_NAME = "unnamed-java-app"; private static final boolean DEFAULT_TRACE_ENABLED = true; + public static final boolean DEFAULT_INTEGRATIONS_ENABLED = true; public static final String DD_AGENT_WRITER_TYPE = "DDAgentWriter"; public static final String LOGGING_WRITER_TYPE = "LoggingWriter"; private static final String DEFAULT_AGENT_WRITER_TYPE = DD_AGENT_WRITER_TYPE; @@ -115,6 +119,12 @@ public class Config { private static final String SPLIT_BY_SPACE_OR_COMMA_REGEX = "[,\\s]+"; private static final boolean DEFAULT_TRACE_REPORT_HOSTNAME = false; + private static final String DEFAULT_TRACE_ANNOTATIONS = null; + private static final boolean DEFAULT_TRACE_EXECUTORS_ALL = false; + private static final String DEFAULT_TRACE_EXECUTORS = ""; + private static final String DEFAULT_TRACE_METHODS = null; + public static final boolean DEFAULT_TRACE_ANALYTICS_ENABLED = false; + public static final float DEFAULT_ANALYTICS_SAMPLE_RATE = 1.0f; public enum PropagationStyle { DATADOG, @@ -132,6 +142,7 @@ public class Config { @Getter private final String serviceName; @Getter private final boolean traceEnabled; + @Getter private final boolean integrationsEnabled; @Getter private final String writerType; @Getter private final String agentHost; @Getter private final int agentPort; @@ -167,6 +178,15 @@ public class Config { @Getter private final boolean reportHostName; + @Getter private final String traceAnnotations; + + @Getter private final String traceMethods; + + @Getter private final boolean traceExecutorsAll; + @Getter private final List traceExecutors; + + @Getter private final boolean traceAnalyticsEnabled; + // Read order: System Properties -> Env Variables, [-> default value] // Visible for testing Config() { @@ -175,6 +195,8 @@ public class Config { serviceName = getSettingFromEnvironment(SERVICE_NAME, DEFAULT_SERVICE_NAME); traceEnabled = getBooleanSettingFromEnvironment(TRACE_ENABLED, DEFAULT_TRACE_ENABLED); + integrationsEnabled = + getBooleanSettingFromEnvironment(INTEGRATIONS_ENABLED, DEFAULT_INTEGRATIONS_ENABLED); writerType = getSettingFromEnvironment(WRITER_TYPE, DEFAULT_AGENT_WRITER_TYPE); agentHost = getSettingFromEnvironment(AGENT_HOST, DEFAULT_AGENT_HOST); agentPort = @@ -254,6 +276,18 @@ public class Config { reportHostName = getBooleanSettingFromEnvironment(TRACE_REPORT_HOSTNAME, DEFAULT_TRACE_REPORT_HOSTNAME); + traceAnnotations = getSettingFromEnvironment(TRACE_ANNOTATIONS, DEFAULT_TRACE_ANNOTATIONS); + + traceMethods = getSettingFromEnvironment(TRACE_METHODS, DEFAULT_TRACE_METHODS); + + traceExecutorsAll = + getBooleanSettingFromEnvironment(TRACE_EXECUTORS_ALL, DEFAULT_TRACE_EXECUTORS_ALL); + + traceExecutors = getListSettingFromEnvironment(TRACE_EXECUTORS, DEFAULT_TRACE_EXECUTORS); + + traceAnalyticsEnabled = + getBooleanSettingFromEnvironment(TRACE_ANALYTICS_ENABLED, DEFAULT_TRACE_ANALYTICS_ENABLED); + log.debug("New instance: {}", this); } @@ -264,6 +298,8 @@ public class Config { serviceName = properties.getProperty(SERVICE_NAME, parent.serviceName); traceEnabled = getPropertyBooleanValue(properties, TRACE_ENABLED, parent.traceEnabled); + integrationsEnabled = + getPropertyBooleanValue(properties, INTEGRATIONS_ENABLED, parent.integrationsEnabled); writerType = properties.getProperty(WRITER_TYPE, parent.writerType); agentHost = properties.getProperty(AGENT_HOST, parent.agentHost); agentPort = @@ -347,6 +383,17 @@ public class Config { reportHostName = getPropertyBooleanValue(properties, TRACE_REPORT_HOSTNAME, parent.reportHostName); + traceAnnotations = properties.getProperty(TRACE_ANNOTATIONS, parent.traceAnnotations); + + traceMethods = properties.getProperty(TRACE_METHODS, parent.traceAnnotations); + + traceExecutorsAll = + getPropertyBooleanValue(properties, TRACE_EXECUTORS_ALL, parent.traceExecutorsAll); + traceExecutors = getPropertyListValue(properties, TRACE_EXECUTORS, parent.traceExecutors); + + traceAnalyticsEnabled = + getPropertyBooleanValue(properties, TRACE_ANALYTICS_ENABLED, parent.traceAnalyticsEnabled); + log.debug("New instance: {}", this); } @@ -388,6 +435,15 @@ public class Config { return Collections.unmodifiableMap(result); } + /** + * Returns the sample rate for the specified instrumentation or {@link + * #DEFAULT_ANALYTICS_SAMPLE_RATE} if none specified. + */ + public float getInstrumentationAnalyticsSampleRate(String instrumentationName) { + return getFloatSettingFromEnvironment( + instrumentationName + ".analytics.sample-rate", DEFAULT_ANALYTICS_SAMPLE_RATE); + } + /** * Return a map of tags required by the datadog backend to link runtime metrics (i.e. jmx) and * traces. @@ -404,7 +460,7 @@ public class Config { return Collections.unmodifiableMap(result); } - public static boolean integrationEnabled( + public boolean isIntegrationEnabled( final SortedSet integrationNames, final boolean defaultEnabled) { // If default is enabled, we want to enable individually, // if default is disabled, we want to disable individually. @@ -421,7 +477,7 @@ public class Config { return anyEnabled; } - public static boolean jmxFetchIntegrationEnabled( + public boolean isJmxFetchIntegrationEnabled( final SortedSet integrationNames, final boolean defaultEnabled) { // If default is enabled, we want to enable individually, // if default is disabled, we want to disable individually. @@ -438,7 +494,7 @@ public class Config { return anyEnabled; } - public static boolean traceAnalyticsIntegrationEnabled( + public boolean isTraceAnalyticsIntegrationEnabled( final SortedSet integrationNames, final boolean defaultEnabled) { // If default is enabled, we want to enable individually, // if default is disabled, we want to disable individually. @@ -463,6 +519,7 @@ public class Config { * @param name * @param defaultValue * @return + * @deprecated This method should only be used internally. Use the explicit getter instead. */ public static String getSettingFromEnvironment(final String name, final String defaultValue) { final String completeName = PREFIX + name; @@ -472,6 +529,7 @@ public class Config { return value == null ? defaultValue : value; } + /** @deprecated This method should only be used internally. Use the explicit getter instead. */ private static Map getMapSettingFromEnvironment( final String name, final String defaultValue) { return parseMap(getSettingFromEnvironment(name, defaultValue), PREFIX + name); @@ -480,6 +538,8 @@ public class Config { /** * Calls {@link #getSettingFromEnvironment(String, String)} and converts the result to a list by * splitting on `,`. + * + * @deprecated This method should only be used internally. Use the explicit getter instead. */ public static List getListSettingFromEnvironment( final String name, final String defaultValue) { @@ -488,6 +548,8 @@ public class Config { /** * Calls {@link #getSettingFromEnvironment(String, String)} and converts the result to a Boolean. + * + * @deprecated This method should only be used internally. Use the explicit getter instead. */ public static Boolean getBooleanSettingFromEnvironment( final String name, final Boolean defaultValue) { @@ -497,6 +559,8 @@ public class Config { /** * Calls {@link #getSettingFromEnvironment(String, String)} and converts the result to a Float. + * + * @deprecated This method should only be used internally. Use the explicit getter instead. */ public static Float getFloatSettingFromEnvironment(final String name, final Float defaultValue) { final String value = getSettingFromEnvironment(name, null); diff --git a/dd-trace-api/src/test/groovy/datadog/trace/api/ConfigTest.groovy b/dd-trace-api/src/test/groovy/datadog/trace/api/ConfigTest.groovy index 5f3c0e754c..069f1a98ed 100644 --- a/dd-trace-api/src/test/groovy/datadog/trace/api/ConfigTest.groovy +++ b/dd-trace-api/src/test/groovy/datadog/trace/api/ConfigTest.groovy @@ -455,7 +455,7 @@ class ConfigTest extends Specification { System.setProperty("dd.integration.disabled-prop.enabled", "false") expect: - Config.integrationEnabled(integrationNames, defaultEnabled) == expected + Config.get().isIntegrationEnabled(integrationNames, defaultEnabled) == expected where: names | defaultEnabled | expected @@ -489,7 +489,7 @@ class ConfigTest extends Specification { System.setProperty("dd.jmxfetch.disabled-prop.enabled", "false") expect: - Config.jmxFetchIntegrationEnabled(integrationNames, defaultEnabled) == expected + Config.get().isJmxFetchIntegrationEnabled(integrationNames, defaultEnabled) == expected where: names | defaultEnabled | expected @@ -523,7 +523,7 @@ class ConfigTest extends Specification { System.setProperty("dd.disabled-prop.analytics.enabled", "false") expect: - Config.traceAnalyticsIntegrationEnabled(integrationNames, defaultEnabled) == expected + Config.get().isTraceAnalyticsIntegrationEnabled(integrationNames, defaultEnabled) == expected where: names | defaultEnabled | expected