Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Don't pass configuration to SDK autoconfigure through system props #3866

Merged
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

This file was deleted.

Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@
import static java.util.Objects.requireNonNull;

import com.google.auto.value.AutoValue;
import java.time.Duration;
import java.util.Collections;
import java.util.List;
import java.util.Map;
Expand All @@ -17,6 +18,17 @@
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;

/**
* Represents the global agent configuration consisting of system properties, environment variables,
* contents of the agent configuration file and properties defined by the {@code
* ConfigPropertySource} SPI (see {@code ConfigInitializer} and {@link ConfigBuilder}).
*
* <p>In case any {@code get*()} method variant gets called for the same property more than once
* (e.g. each time an advice class executes) it is suggested to cache the result instead of
* repeatedly calling {@link Config}. Agent configuration does not change during the runtime so
* retrieving the property once and storing its result in e.g. static final field allows JIT to do
* its magic and remove some code branches.
*/
@AutoValue
public abstract class Config {
private static final Logger logger = LoggerFactory.getLogger(Config.class);
Expand All @@ -34,6 +46,9 @@ static Config create(Map<String, String> allProperties) {
return new AutoValue_Config(allProperties);
}

// package protected constructor to make extending this class impossible
Config() {}

/**
* Sets the agent configuration singleton. This method is only supposed to be called once, from
* the agent classloader just before the first instrumentation is loaded (and before {@link
Expand All @@ -47,6 +62,7 @@ public static void internalInitializeConfig(Config config) {
instance = requireNonNull(config);
}

/** Returns the global agent configuration. */
public static Config get() {
if (instance == null) {
// this should only happen in library instrumentation
Expand All @@ -61,98 +77,186 @@ public static Config get() {
public abstract Map<String, String> getAllProperties();

/**
* Returns a string property value or null if a property with name {@code name} did not exist.
*
* @see #getProperty(String, String)
* Returns a string-valued configuration property or {@code null} if a property with name {@code
* name} has not been configured.
*/
@Nullable
public String getProperty(String name) {
return getProperty(name, null);
return getRawProperty(name, null);
}

/**
* Retrieves a property value from the agent configuration consisting of system properties,
* environment variables and contents of the agent configuration file (see {@code
* ConfigInitializer} and {@code ConfigBuilder}).
*
* <p>In case any {@code get*Property()} method variant gets called for the same property more
* than once (e.g. each time an advice executes) it is suggested to cache the result instead of
* repeatedly calling {@link Config}. Agent configuration does not change during the runtime so
* retrieving the property once and storing its result in e.g. static final field allows JIT to do
* its magic and remove some code branches.
*
* @return A string property value or {@code defaultValue} if a property with name {@code name}
* did not exist.
* Returns a string-valued configuration property or {@code defaultValue} if a property with name
* {@code name} has not been configured.
*/
public String getProperty(String name, String defaultValue) {
return getAllProperties().getOrDefault(NamingConvention.DOT.normalize(name), defaultValue);
return getRawProperty(name, defaultValue);
}

/**
* Returns a boolean property value or {@code defaultValue} if a property with name {@code name}
* did not exist.
*
* @see #getProperty(String, String)
* Returns a boolean-valued configuration property or {@code null} if a property with name {@code
* name} has not been configured.
*/
@Nullable
public Boolean getBoolean(String name) {
return getTypedProperty(name, Boolean::parseBoolean, null);
}

/**
* Returns a boolean-valued configuration property or {@code defaultValue} if a property with name
* {@code name} has not been configured.
*/
public boolean getBooleanProperty(String name, boolean defaultValue) {
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'm planning to rename all get*Property() methods to get*() in the next PR - those shorter names seem a bit nicer, and they're consistent with the SDK ConfigProperties interface too.

Copy link
Contributor

Choose a reason for hiding this comment

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

is the plan to have this implement ConfigProperties then?

Copy link
Member Author

Choose a reason for hiding this comment

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

Unfortunately that won't be possible with the current shape of this repo: instrumentation-api only uses OTel API, not the SDK, so we can't directly implement the ConfigProperties interface here.

Copy link
Contributor

Choose a reason for hiding this comment

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

ok then. makes sense!

return getTypedProperty(name, Boolean::parseBoolean, defaultValue);
}

/**
* Returns a long property value or {@code defaultValue} if a property with name {@code name} did
* not exist.
* Returns a integer-valued configuration property or {@code null} if a property with name {@code
* name} has not been configured.
*/
@Nullable
public Integer getInt(String name) {
return getTypedProperty(name, Integer::parseInt, null);
}

/**
* Returns a integer-valued configuration property or {@code defaultValue} if a property with name
* {@code name} has not been configured.
*/
public int getInt(String name, int defaultValue) {
return getTypedProperty(name, Integer::parseInt, defaultValue);
}

/**
* Returns a long-valued configuration property or {@code null} if a property with name {@code
* name} has not been configured.
*/
@Nullable
public Long getLong(String name) {
return getTypedProperty(name, Long::parseLong, null);
}

/**
* Returns a long-valued configuration property or {@code defaultValue} if a property with name
* {@code name} has not been configured.
*/
public long getLong(String name, long defaultValue) {
return getTypedProperty(name, Long::parseLong, defaultValue);
}

/**
* Returns a double-valued configuration property or {@code null} if a property with name {@code
* name} has not been configured.
*/
@Nullable
public Double getDouble(String name) {
return getTypedProperty(name, Double::parseDouble, null);
}

/**
* Returns a double-valued configuration property or {@code defaultValue} if a property with name
* {@code name} has not been configured.
*/
public double getDouble(String name, double defaultValue) {
return getTypedProperty(name, Double::parseDouble, defaultValue);
}

/**
* Returns a duration-valued configuration property or {@code null} if a property with name {@code
* name} has not been configured.
*
* <p>This property may be used by vendor distributions to get numerical values.
* <p>Durations can be of the form "{number}{unit}", where unit is one of:
*
* @see #getProperty(String, String)
* <ul>
* <li>ms
* <li>s
* <li>m
* <li>h
* <li>d
* </ul>
*
* <p>If no unit is specified, milliseconds is the assumed duration unit.
*/
public long getLongProperty(String name, long defaultValue) {
return getTypedProperty(name, Long::parseLong, defaultValue);
@Nullable
public Duration getDuration(String name) {
return getTypedProperty(name, ConfigValueParsers::parseDuration, null);
}

/**
* Returns a list-of-strings property value or empty list if a property with name {@code name} did
* not exist.
* Returns a duration-valued configuration property or {@code defaultValue} if a property with
* name {@code name} has not been configured.
*
* <p>Durations can be of the form "{number}{unit}", where unit is one of:
*
* @see #getProperty(String, String)
* <ul>
* <li>ms
* <li>s
* <li>m
* <li>h
* <li>d
* </ul>
*
* <p>If no unit is specified, milliseconds is the assumed duration unit.
*/
public Duration getDuration(String name, Duration defaultValue) {
return getTypedProperty(name, ConfigValueParsers::parseDuration, defaultValue);
}

/**
* Returns a list-valued configuration property or an empty list if a property with name {@code
* name} has not been configured. The format of the original value must be comma-separated, e.g.
* {@code one,two,three}.
*/
public List<String> getListProperty(String name) {
return getListProperty(name, Collections.emptyList());
}

/**
* Returns a list-of-strings property value or {@code defaultValue} if a property with name {@code
* name} did not exist.
*
* @see #getProperty(String, String)
* Returns a list-valued configuration property or {@code defaultValue} if a property with name
* {@code name} has not been configured. The format of the original value must be comma-separated,
* e.g. {@code one,two,three}.
*/
public List<String> getListProperty(String name, List<String> defaultValue) {
return getTypedProperty(name, CollectionParsers::parseList, defaultValue);
return getTypedProperty(name, ConfigValueParsers::parseList, defaultValue);
}

/**
* Returns a map-of-strings property value or empty map if a property with name {@code name} did
* not exist.
*
* @see #getProperty(String, String)
* Returns a map-valued configuration property or an empty map if a property with name {@code
* name} has not been configured. The format of the original value must be comma-separated for
* each key, with an '=' separating the key and value, e.g. {@code
* key=value,anotherKey=anotherValue}.
*/
public Map<String, String> getMapProperty(String name) {
return getTypedProperty(name, CollectionParsers::parseMap, Collections.emptyMap());
return getMapProperty(name, Collections.emptyMap());
}

/**
* Returns a map-valued configuration property or {@code defaultValue} if a property with name
* {@code name} has not been configured. The format of the original value must be comma-separated
* for each key, with an '=' separating the key and value, e.g. {@code
* key=value,anotherKey=anotherValue}.
*/
public Map<String, String> getMapProperty(String name, Map<String, String> defaultValue) {
return getTypedProperty(name, ConfigValueParsers::parseMap, defaultValue);
}

private <T> T getTypedProperty(String name, Function<String, T> parser, T defaultValue) {
String value = getProperty(name);
String value = getRawProperty(name, null);
if (value == null || value.trim().isEmpty()) {
return defaultValue;
}
try {
return parser.apply(value);
} catch (Throwable t) {
} catch (RuntimeException t) {
logger.debug("Cannot parse {}", value, t);
return defaultValue;
}
}

private String getRawProperty(String name, String defaultValue) {
return getAllProperties().getOrDefault(NamingConvention.DOT.normalize(name), defaultValue);
}

public boolean isInstrumentationEnabled(
Iterable<String> instrumentationNames, boolean defaultEnabled) {
return isInstrumentationPropertyEnabled(instrumentationNames, "enabled", defaultEnabled);
Expand All @@ -176,6 +280,10 @@ public boolean isInstrumentationPropertyEnabled(
return anyEnabled;
}

public boolean isAgentDebugEnabled() {
return getBooleanProperty("otel.javaagent.debug", false);
}

/**
* Converts this config instance to Java {@link Properties}.
*
Expand All @@ -187,8 +295,4 @@ public Properties asJavaProperties() {
properties.putAll(getAllProperties());
return properties;
}

public boolean isAgentDebugEnabled() {
return getBooleanProperty("otel.javaagent.debug", false);
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,11 @@ public final class ConfigBuilder {

private final Map<String, String> allProperties = new HashMap<>();

public ConfigBuilder addProperty(String name, String value) {
allProperties.put(NamingConvention.DOT.normalize(name), value);
return this;
}

public ConfigBuilder readProperties(Properties properties) {
for (String name : properties.stringPropertyNames()) {
allProperties.put(NamingConvention.DOT.normalize(name), properties.getProperty(name));
Expand Down
Loading