From 6fc69ba64e9a239ed8eee3837b6b68c113e975d7 Mon Sep 17 00:00:00 2001 From: Luca Abbati Date: Mon, 3 Jun 2019 14:26:05 +0200 Subject: [PATCH 01/10] Config to fallback to properties file to load settings --- .../main/java/datadog/trace/api/Config.java | 97 +++++++++++++++++-- 1 file changed, 89 insertions(+), 8 deletions(-) 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 fafb539372..6a1dc9c142 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 @@ -1,5 +1,9 @@ package datadog.trace.api; +import java.io.File; +import java.io.FileNotFoundException; +import java.io.FileReader; +import java.io.IOException; import java.net.InetAddress; import java.net.UnknownHostException; import java.util.Arrays; @@ -35,6 +39,7 @@ public class Config { private static final Pattern ENV_REPLACEMENT = Pattern.compile("[^a-zA-Z0-9_]"); + public static final String CONFIGURATION_FILE = "trace.config"; public static final String SERVICE_NAME = "service.name"; public static final String SERVICE = "service"; public static final String TRACE_ENABLED = "trace.enabled"; @@ -186,6 +191,7 @@ public class Config { @Getter private final List traceExecutors; @Getter private final boolean traceAnalyticsEnabled; + private static final Properties propertiesFromConfigFile = loadConfigurationFile(); // Read order: System Properties -> Env Variables, [-> default value] // Visible for testing @@ -555,7 +561,8 @@ public class Config { /** * Helper method that takes the name, adds a "dd." prefix then checks for System Properties of * that name. If none found, the name is converted to an Environment Variable and used to check - * the env. If setting not configured in either location, defaultValue is returned. + * the env. If none of the above returns a value, then an optional properties file if checked. If + * setting is not configured in either location, defaultValue is returned. * * @param name * @param defaultValue @@ -563,11 +570,27 @@ public class Config { * @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; - final String value = - System.getProperties() - .getProperty(completeName, System.getenv(propertyToEnvironmentName(completeName))); - return value == null ? defaultValue : value; + String value; + + // System properties and properties provided from command line have the highest precedence + value = System.getProperties().getProperty(propertyNameToSystemPropertyName(name)); + if (null != value) { + return value; + } + + // If value not provided from system properties, looking at env variables + value = System.getenv(propertyNameToEnvironmentVariableName(name)); + if (null != value) { + return value; + } + + // If value is not defined yet, we look at properties optionally defined in a properties file + value = propertiesFromConfigFile.getProperty(propertyNameToSystemPropertyName(name)); + if (null != value) { + return value; + } + + return defaultValue; } /** @deprecated This method should only be used internally. Use the explicit getter instead. */ @@ -663,8 +686,28 @@ public class Config { } } - private static String propertyToEnvironmentName(final String name) { - return ENV_REPLACEMENT.matcher(name.toUpperCase()).replaceAll("_"); + /** + * Converts the property name, e.g. 'service.name' into a public environment variable name, e.g. + * `DD_SERVICE_NAME`. + * + * @param setting The setting name, e.g. `service.name` + * @return The public facing environment variable name + */ + private static String propertyNameToEnvironmentVariableName(final String setting) { + return ENV_REPLACEMENT + .matcher(propertyNameToSystemPropertyName(setting).toUpperCase()) + .replaceAll("_"); + } + + /** + * Converts the property name, e.g. 'service.name' into a public system property name, e.g. + * `dd.service.name`. + * + * @param setting The setting name, e.g. `service.name` + * @return The public facing system property name + */ + private static String propertyNameToSystemPropertyName(String setting) { + return PREFIX + setting; } private static Map getPropertyMapValue( @@ -819,6 +862,44 @@ public class Config { return Collections.unmodifiableSet(result); } + /** + * Loads the optional configuration properties file into the global {@link Properties} object. + * + * @return The {@link Properties} object. the returned instance might be empty of file does not + * exist or if it is in a wrong format. + */ + private static Properties loadConfigurationFile() { + Properties properties = new Properties(); + + String configurationFilePath = + System.getProperty(propertyNameToSystemPropertyName(CONFIGURATION_FILE)); + if (null == configurationFilePath) { + configurationFilePath = + System.getenv(propertyNameToEnvironmentVariableName(CONFIGURATION_FILE)); + } + if (null == configurationFilePath) { + return properties; + } + + // Configuration properties file is optional + File configurationFile = new File(configurationFilePath); + if (!configurationFile.exists()) { + return properties; + } + + try { + FileReader fileReader = new FileReader(configurationFile); + properties.load(fileReader); + } catch (FileNotFoundException fnf) { + log.error("Configuration file '{}' not found.", configurationFilePath); + } catch (IOException e) { + log.error( + "Configuration file '{}' cannot be accessed or correctly parsed.", configurationFilePath); + } + + return properties; + } + /** * Returns the detected hostname. This operation is time consuming so if the usage changes and * this method will be called several times then we should implement some sort of caching. From 1d57a5873b2db4c63748e899351b22242e79b923 Mon Sep 17 00:00:00 2001 From: Luca Abbati Date: Mon, 3 Jun 2019 15:13:15 +0200 Subject: [PATCH 02/10] Tests for functionality: Config fallback to properties files --- .../main/java/datadog/trace/api/Config.java | 10 ++-- .../datadog/trace/api/ConfigTest.groovy | 48 +++++++++++++++++++ .../test/resources/dd-java-tracer.properties | 1 + 3 files changed, 56 insertions(+), 3 deletions(-) create mode 100644 dd-trace-api/src/test/resources/dd-java-tracer.properties 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 6a1dc9c142..5cb59c9ce3 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 @@ -191,11 +191,15 @@ public class Config { @Getter private final List traceExecutors; @Getter private final boolean traceAnalyticsEnabled; - private static final Properties propertiesFromConfigFile = loadConfigurationFile(); + + // Values from an optionally provided properties file + private static Properties propertiesFromConfigFile; // Read order: System Properties -> Env Variables, [-> default value] // Visible for testing Config() { + propertiesFromConfigFile = loadConfigurationFile(); + runtimeId = UUID.randomUUID().toString(); serviceName = getSettingFromEnvironment(SERVICE_NAME, DEFAULT_SERVICE_NAME); @@ -868,7 +872,7 @@ public class Config { * @return The {@link Properties} object. the returned instance might be empty of file does not * exist or if it is in a wrong format. */ - private static Properties loadConfigurationFile() { + private static synchronized Properties loadConfigurationFile() { Properties properties = new Properties(); String configurationFilePath = @@ -892,7 +896,7 @@ public class Config { properties.load(fileReader); } catch (FileNotFoundException fnf) { log.error("Configuration file '{}' not found.", configurationFilePath); - } catch (IOException e) { + } catch (IOException ioe) { log.error( "Configuration file '{}' cannot be accessed or correctly parsed.", configurationFilePath); } 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 069f1a98ed..3d26081cef 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 @@ -8,7 +8,9 @@ import spock.lang.Specification import static datadog.trace.api.Config.AGENT_HOST import static datadog.trace.api.Config.AGENT_PORT_LEGACY import static datadog.trace.api.Config.AGENT_UNIX_DOMAIN_SOCKET +import static datadog.trace.api.Config.CONFIGURATION_FILE import static datadog.trace.api.Config.DEFAULT_JMX_FETCH_STATSD_PORT +import static datadog.trace.api.Config.DEFAULT_SERVICE_NAME import static datadog.trace.api.Config.GLOBAL_TAGS import static datadog.trace.api.Config.HEADER_TAGS import static datadog.trace.api.Config.HTTP_CLIENT_ERROR_STATUSES @@ -715,4 +717,50 @@ class ConfigTest extends Specification { then: config.localRootSpanTags.get('_dd.hostname') == InetAddress.localHost.hostName } + + def "verify fallback to properties file"() { + setup: + System.setProperty(PREFIX + CONFIGURATION_FILE, "src/test/resources/dd-java-tracer.properties") + + when: + def config = new Config() + + then: + config.serviceName == "set-in-properties" + } + + def "verify fallback to properties file has lower priority then system property"() { + setup: + System.setProperty(PREFIX + CONFIGURATION_FILE, "src/test/resources/dd-java-tracer.properties") + System.setProperty(PREFIX + SERVICE_NAME, "set-in-system") + + when: + def config = new Config() + + then: + config.serviceName == "set-in-system" + } + + def "verify fallback to properties file has lower priority then env var"() { + setup: + System.setProperty(PREFIX + CONFIGURATION_FILE, "src/test/resources/dd-java-tracer.properties") + environmentVariables.set("DD_SERVICE_NAME", "set-in-env") + + when: + def config = new Config() + + then: + config.serviceName == "set-in-env" + } + + def "verify fallback to properties file that does not exist does not crash app"() { + setup: + System.setProperty(PREFIX + CONFIGURATION_FILE, "src/test/resources/do-not-exist.properties") + + when: + def config = new Config() + + then: + config.serviceName == 'unnamed-java-app' + } } diff --git a/dd-trace-api/src/test/resources/dd-java-tracer.properties b/dd-trace-api/src/test/resources/dd-java-tracer.properties new file mode 100644 index 0000000000..1b4c322c6d --- /dev/null +++ b/dd-trace-api/src/test/resources/dd-java-tracer.properties @@ -0,0 +1 @@ +dd.service.name=set-in-properties From 4dd6708852bcf316ca8d45292821fb21dababcb5 Mon Sep 17 00:00:00 2001 From: Luca Abbati Date: Mon, 3 Jun 2019 15:16:41 +0200 Subject: [PATCH 03/10] Update comment on Config class to show new functionality --- dd-trace-api/src/main/java/datadog/trace/api/Config.java | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) 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 5cb59c9ce3..ec52fff4b2 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 @@ -23,8 +23,8 @@ import lombok.ToString; import lombok.extern.slf4j.Slf4j; /** - * Config gives priority to system properties and falls back to environment variables. It also - * includes default values to ensure a valid config. + * Config reads values with the following priority: 1) system properties, 2) environment variables, + * 3) optional configuration file. It also includes default values to ensure a valid config. * *

* From 076c2b1b4e6754081e57249416bd8f95775d9da8 Mon Sep 17 00:00:00 2001 From: Luca Abbati Date: Mon, 3 Jun 2019 15:28:26 +0200 Subject: [PATCH 04/10] Removing synchronized as the load properties method no longer set the static istance --- dd-trace-api/src/main/java/datadog/trace/api/Config.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) 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 ec52fff4b2..47c3d32bed 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 @@ -872,7 +872,7 @@ public class Config { * @return The {@link Properties} object. the returned instance might be empty of file does not * exist or if it is in a wrong format. */ - private static synchronized Properties loadConfigurationFile() { + private static Properties loadConfigurationFile() { Properties properties = new Properties(); String configurationFilePath = From bfd9680d75372137464bd5786da541541f4405d0 Mon Sep 17 00:00:00 2001 From: Luca Abbati Date: Mon, 3 Jun 2019 15:33:45 +0200 Subject: [PATCH 05/10] Fix typos --- .../src/test/groovy/datadog/trace/api/ConfigTest.groovy | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) 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 3d26081cef..fe02d61cb3 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 @@ -729,7 +729,7 @@ class ConfigTest extends Specification { config.serviceName == "set-in-properties" } - def "verify fallback to properties file has lower priority then system property"() { + def "verify fallback to properties file has lower priority than system property"() { setup: System.setProperty(PREFIX + CONFIGURATION_FILE, "src/test/resources/dd-java-tracer.properties") System.setProperty(PREFIX + SERVICE_NAME, "set-in-system") @@ -741,7 +741,7 @@ class ConfigTest extends Specification { config.serviceName == "set-in-system" } - def "verify fallback to properties file has lower priority then env var"() { + def "verify fallback to properties file has lower priority than env var"() { setup: System.setProperty(PREFIX + CONFIGURATION_FILE, "src/test/resources/dd-java-tracer.properties") environmentVariables.set("DD_SERVICE_NAME", "set-in-env") From da0577289782f7b426cdcfcf583e9e8e568f058f Mon Sep 17 00:00:00 2001 From: Luca Abbati Date: Mon, 3 Jun 2019 15:44:22 +0200 Subject: [PATCH 06/10] Add properties file to a comment listing all the source of configuration --- dd-trace-api/src/main/java/datadog/trace/api/Config.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) 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 47c3d32bed..dd4398ae23 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 @@ -195,7 +195,7 @@ public class Config { // Values from an optionally provided properties file private static Properties propertiesFromConfigFile; - // Read order: System Properties -> Env Variables, [-> default value] + // Read order: System Properties -> Env Variables, [-> properties file], [-> default value] // Visible for testing Config() { propertiesFromConfigFile = loadConfigurationFile(); From 12dc7c4fd8fdcb3bc868803545ac0943838eaa43 Mon Sep 17 00:00:00 2001 From: Luca Abbati Date: Thu, 6 Jun 2019 11:05:43 +0200 Subject: [PATCH 07/10] Remove unused imports --- dd-trace-api/src/test/groovy/datadog/trace/api/ConfigTest.groovy | 1 - 1 file changed, 1 deletion(-) 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 fe02d61cb3..54ad44b635 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 @@ -10,7 +10,6 @@ import static datadog.trace.api.Config.AGENT_PORT_LEGACY import static datadog.trace.api.Config.AGENT_UNIX_DOMAIN_SOCKET import static datadog.trace.api.Config.CONFIGURATION_FILE import static datadog.trace.api.Config.DEFAULT_JMX_FETCH_STATSD_PORT -import static datadog.trace.api.Config.DEFAULT_SERVICE_NAME import static datadog.trace.api.Config.GLOBAL_TAGS import static datadog.trace.api.Config.HEADER_TAGS import static datadog.trace.api.Config.HTTP_CLIENT_ERROR_STATUSES From f578c9ccf25060e695d016dcb6fb9c17d4a2d385 Mon Sep 17 00:00:00 2001 From: Luca Abbati Date: Fri, 14 Jun 2019 11:50:11 +0200 Subject: [PATCH 08/10] Remove system properties and envs left-over in Config tests --- .../src/main/java/datadog/trace/api/Config.java | 3 ++- .../groovy/datadog/trace/api/ConfigTest.groovy | 15 +++++++++++++++ 2 files changed, 17 insertions(+), 1 deletion(-) 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 dd4398ae23..b03e62ea4c 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 @@ -600,7 +600,8 @@ public class Config { /** @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); + return parseMap( + getSettingFromEnvironment(name, defaultValue), propertyNameToSystemPropertyName(name)); } /** 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 54ad44b635..6937268c91 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 @@ -726,6 +726,9 @@ class ConfigTest extends Specification { then: config.serviceName == "set-in-properties" + + cleanup: + System.clearProperty(PREFIX + CONFIGURATION_FILE) } def "verify fallback to properties file has lower priority than system property"() { @@ -738,6 +741,10 @@ class ConfigTest extends Specification { then: config.serviceName == "set-in-system" + + cleanup: + System.clearProperty(PREFIX + CONFIGURATION_FILE) + System.clearProperty(PREFIX + SERVICE_NAME) } def "verify fallback to properties file has lower priority than env var"() { @@ -750,6 +757,11 @@ class ConfigTest extends Specification { then: config.serviceName == "set-in-env" + + cleanup: + System.clearProperty(PREFIX + CONFIGURATION_FILE) + System.clearProperty(PREFIX + SERVICE_NAME) + environmentVariables.clear("DD_SERVICE_NAME") } def "verify fallback to properties file that does not exist does not crash app"() { @@ -761,5 +773,8 @@ class ConfigTest extends Specification { then: config.serviceName == 'unnamed-java-app' + + cleanup: + System.clearProperty(PREFIX + CONFIGURATION_FILE) } } From d0b32147a864153ee851271b4d7d3ae14260426f Mon Sep 17 00:00:00 2001 From: Luca Abbati Date: Mon, 17 Jun 2019 08:10:50 -0400 Subject: [PATCH 09/10] Be more friendly for config file to unix users using tilde for home --- dd-trace-api/src/main/java/datadog/trace/api/Config.java | 5 +++++ 1 file changed, 5 insertions(+) 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 b03e62ea4c..b334728ee1 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 @@ -876,6 +876,7 @@ public class Config { private static Properties loadConfigurationFile() { Properties properties = new Properties(); + // Reading from system property first and from env after String configurationFilePath = System.getProperty(propertyNameToSystemPropertyName(CONFIGURATION_FILE)); if (null == configurationFilePath) { @@ -886,9 +887,13 @@ public class Config { return properties; } + // Normalizing tilde (~) paths for unix systems + configurationFilePath = configurationFilePath.replaceFirst("^~", System.getProperty("user.home")); + // Configuration properties file is optional File configurationFile = new File(configurationFilePath); if (!configurationFile.exists()) { + log.error("Configuration file '{}' not found.", configurationFilePath); return properties; } From d865a1a24920962b9210c8bedb328b50a4379c4e Mon Sep 17 00:00:00 2001 From: Luca Abbati Date: Mon, 17 Jun 2019 08:22:44 -0400 Subject: [PATCH 10/10] Format code --- dd-trace-api/src/main/java/datadog/trace/api/Config.java | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) 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 7ea7e7cec3..37ca73aa57 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 @@ -899,7 +899,8 @@ public class Config { } // Normalizing tilde (~) paths for unix systems - configurationFilePath = configurationFilePath.replaceFirst("^~", System.getProperty("user.home")); + configurationFilePath = + configurationFilePath.replaceFirst("^~", System.getProperty("user.home")); // Configuration properties file is optional File configurationFile = new File(configurationFilePath);