Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
116 changes: 104 additions & 12 deletions dd-trace-api/src/main/java/datadog/trace/api/Config.java
Original file line number Diff line number Diff line change
@@ -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;
Expand All @@ -19,8 +23,8 @@
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.
*
* <p>
*
Expand All @@ -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";
Expand Down Expand Up @@ -190,9 +195,14 @@ public enum PropagationStyle {

@Getter private final boolean traceAnalyticsEnabled;

// Read order: System Properties -> Env Variables, [-> default value]
// Values from an optionally provided properties file
private static Properties propertiesFromConfigFile;

// Read order: System Properties -> Env Variables, [-> properties file], [-> default value]
// Visible for testing
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When I commented about updating comment I meant this one :)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

opps :)

Config() {
propertiesFromConfigFile = loadConfigurationFile();
Copy link
Member Author

@labbati labbati Jun 3, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A comment here. I set a static variable in a constructor which is clearly a smell. The motivation behind this was testability and this is not my favorite approach.

What I would prefer to do is to change the class quite a few moving from imperative static loading of properties to a more instance centric approach, where I define inner providers [systemPropertyProvider, environmentVariableProvider, configurationFileProvider, defaultValuesProvider] instantiated in constructor which are tested in sequence until I find a non null.

But the approach above (that I would be more than happy to implement) requires quite a few changes and I will do only if we all agree.

@mar-kolya , @tylerbenson

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

First of all: if you simply remove static from that field nothing changes much :).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think one possible approach here is to implement a separate way of loading fully fledged config file from all possible sources separately and then also implement merging config objects. There is sort of a very primitive implementation of this already in a form of second constructor for Config - but it is not flexible enough at the moment.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I don't follow. Are you suggesting to make the various getSettingFromEnvironment and the likes non-static and to pass along the loaded properties? In this case I would not clearly understand why we are treating properties from properties file differently from system properties and environment variables

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, right, getSettingFromEnvironment that is static... And not only that - it is also used outside of Config... in this particular case I think that initializing that in constructor is outright dangerous and confusing and I would suggest funding way of not doing that.

I think first step there might be refactoring all places that use getSettingFromEnvironment directly to use actual config instance. And then this can be made non static and proper config inheriting/overwriting scheme could be implemented.


runtimeId = UUID.randomUUID().toString();

serviceName = getSettingFromEnvironment(SERVICE_NAME, DEFAULT_SERVICE_NAME);
Expand Down Expand Up @@ -566,25 +576,43 @@ public static boolean traceAnalyticsIntegrationEnabled(
/**
* 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, <code>defaultValue</code> is returned.
*
* @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;
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;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is a comment on top of Config default constructor - you may want to update it to reflect the new reality.

}

/** @deprecated This method should only be used internally. Use the explicit getter instead. */
private static Map<String, String> getMapSettingFromEnvironment(
final String name, final String defaultValue) {
return parseMap(getSettingFromEnvironment(name, defaultValue), PREFIX + name);
return parseMap(
getSettingFromEnvironment(name, defaultValue), propertyNameToSystemPropertyName(name));
}

/**
Expand Down Expand Up @@ -674,8 +702,28 @@ private Set<Integer> getIntegerRangeSettingFromEnvironment(
}
}

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;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍
Should getMapSettingFromEnvironment use this method too?

}

private static Map<String, String> getPropertyMapValue(
Expand Down Expand Up @@ -830,6 +878,50 @@ private static <V extends Enum<V>> Set<V> convertStringSetToEnumSet(
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();

// Reading from system property first and from env after
String configurationFilePath =
System.getProperty(propertyNameToSystemPropertyName(CONFIGURATION_FILE));
if (null == configurationFilePath) {
configurationFilePath =
System.getenv(propertyNameToEnvironmentVariableName(CONFIGURATION_FILE));
}
if (null == configurationFilePath) {
return properties;
}

// Normalizing tilde (~) paths for unix systems
configurationFilePath =
configurationFilePath.replaceFirst("^~", System.getProperty("user.home"));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm surprised... I would think java would handle this automatically.


// Configuration properties file is optional
File configurationFile = new File(configurationFilePath);
if (!configurationFile.exists()) {
log.error("Configuration file '{}' not found.", configurationFilePath);
return properties;
}

try {
FileReader fileReader = new FileReader(configurationFile);
properties.load(fileReader);
} catch (FileNotFoundException fnf) {
log.error("Configuration file '{}' not found.", configurationFilePath);
} catch (IOException ioe) {
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.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ 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.DB_CLIENT_HOST_SPLIT_BY_INSTANCE
import static datadog.trace.api.Config.DEFAULT_JMX_FETCH_STATSD_PORT
import static datadog.trace.api.Config.GLOBAL_TAGS
Expand Down Expand Up @@ -725,4 +726,65 @@ 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"

cleanup:
System.clearProperty(PREFIX + CONFIGURATION_FILE)
}

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")

when:
def config = new Config()

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"() {
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"

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"() {
setup:
System.setProperty(PREFIX + CONFIGURATION_FILE, "src/test/resources/do-not-exist.properties")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You may want to clear this property in cleanup.


when:
def config = new Config()

then:
config.serviceName == 'unnamed-java-app'

cleanup:
System.clearProperty(PREFIX + CONFIGURATION_FILE)
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
dd.service.name=set-in-properties