From ca8a119e01a2e56c9094d23577838b6e9fccda00 Mon Sep 17 00:00:00 2001 From: Mateusz Rzeszutek Date: Fri, 20 Aug 2021 01:56:12 +0200 Subject: [PATCH] Don't pass configuration to SDK autoconfigure through system props (#3866) * Don't pass configuration to SDK autoconfigure through system props * suppress CanonicalDuration * checkstyle --- .../api/config/CollectionParsers.java | 44 ---- .../instrumentation/api/config/Config.java | 196 ++++++++++++++---- .../api/config/ConfigBuilder.java | 5 + .../api/config/ConfigValueParsers.java | 87 ++++++++ ...nfigTest.groovy => ConfigSpockTest.groovy} | 61 +----- .../api/config/ConfigTest.java | 166 +++++++++++++++ .../tooling/OpenTelemetryInstaller.java | 29 +-- .../config/ConfigPropertiesAdapter.java | 67 ++++++ .../tooling/OpenTelemetryInstallerTest.groovy | 2 +- 9 files changed, 481 insertions(+), 176 deletions(-) delete mode 100644 instrumentation-api/src/main/java/io/opentelemetry/instrumentation/api/config/CollectionParsers.java create mode 100644 instrumentation-api/src/main/java/io/opentelemetry/instrumentation/api/config/ConfigValueParsers.java rename instrumentation-api/src/test/groovy/io/opentelemetry/instrumentation/api/config/{ConfigTest.groovy => ConfigSpockTest.groovy} (54%) create mode 100644 instrumentation-api/src/test/java/io/opentelemetry/instrumentation/api/config/ConfigTest.java create mode 100644 javaagent-tooling/src/main/java/io/opentelemetry/javaagent/tooling/config/ConfigPropertiesAdapter.java diff --git a/instrumentation-api/src/main/java/io/opentelemetry/instrumentation/api/config/CollectionParsers.java b/instrumentation-api/src/main/java/io/opentelemetry/instrumentation/api/config/CollectionParsers.java deleted file mode 100644 index 90f16ba0b1e5..000000000000 --- a/instrumentation-api/src/main/java/io/opentelemetry/instrumentation/api/config/CollectionParsers.java +++ /dev/null @@ -1,44 +0,0 @@ -/* - * Copyright The OpenTelemetry Authors - * SPDX-License-Identifier: Apache-2.0 - */ - -package io.opentelemetry.instrumentation.api.config; - -import java.util.Arrays; -import java.util.Collections; -import java.util.LinkedHashMap; -import java.util.List; -import java.util.Map; -import org.slf4j.Logger; -import org.slf4j.LoggerFactory; - -final class CollectionParsers { - private static final Logger logger = LoggerFactory.getLogger(CollectionParsers.class); - - static List parseList(String value) { - String[] tokens = value.split(",", -1); - // Remove whitespace from each item. - for (int i = 0; i < tokens.length; i++) { - tokens[i] = tokens[i].trim(); - } - return Collections.unmodifiableList(Arrays.asList(tokens)); - } - - static Map parseMap(String value) { - Map result = new LinkedHashMap<>(); - for (String token : value.split(",", -1)) { - token = token.trim(); - String[] parts = token.split("=", -1); - if (parts.length != 2) { - logger.warn( - "Invalid map config part, should be formatted key1=value1,key2=value2: {}", value); - return Collections.emptyMap(); - } - result.put(parts[0], parts[1]); - } - return Collections.unmodifiableMap(result); - } - - private CollectionParsers() {} -} diff --git a/instrumentation-api/src/main/java/io/opentelemetry/instrumentation/api/config/Config.java b/instrumentation-api/src/main/java/io/opentelemetry/instrumentation/api/config/Config.java index 01e628b86c99..5c3fb18690ec 100644 --- a/instrumentation-api/src/main/java/io/opentelemetry/instrumentation/api/config/Config.java +++ b/instrumentation-api/src/main/java/io/opentelemetry/instrumentation/api/config/Config.java @@ -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; @@ -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}). + * + *

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); @@ -34,6 +46,9 @@ static Config create(Map 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 @@ -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 @@ -61,98 +77,186 @@ public static Config get() { public abstract Map 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}). - * - *

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) { 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. * - *

This property may be used by vendor distributions to get numerical values. + *

Durations can be of the form "{number}{unit}", where unit is one of: * - * @see #getProperty(String, String) + *

    + *
  • ms + *
  • s + *
  • m + *
  • h + *
  • d + *
+ * + *

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. + * + *

Durations can be of the form "{number}{unit}", where unit is one of: * - * @see #getProperty(String, String) + *

    + *
  • ms + *
  • s + *
  • m + *
  • h + *
  • d + *
+ * + *

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 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 getListProperty(String name, List 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 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 getMapProperty(String name, Map defaultValue) { + return getTypedProperty(name, ConfigValueParsers::parseMap, defaultValue); } private T getTypedProperty(String name, Function 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 instrumentationNames, boolean defaultEnabled) { return isInstrumentationPropertyEnabled(instrumentationNames, "enabled", defaultEnabled); @@ -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}. * @@ -187,8 +295,4 @@ public Properties asJavaProperties() { properties.putAll(getAllProperties()); return properties; } - - public boolean isAgentDebugEnabled() { - return getBooleanProperty("otel.javaagent.debug", false); - } } diff --git a/instrumentation-api/src/main/java/io/opentelemetry/instrumentation/api/config/ConfigBuilder.java b/instrumentation-api/src/main/java/io/opentelemetry/instrumentation/api/config/ConfigBuilder.java index c44cd32bf867..04bc1bffbca3 100644 --- a/instrumentation-api/src/main/java/io/opentelemetry/instrumentation/api/config/ConfigBuilder.java +++ b/instrumentation-api/src/main/java/io/opentelemetry/instrumentation/api/config/ConfigBuilder.java @@ -14,6 +14,11 @@ public final class ConfigBuilder { private final Map 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)); diff --git a/instrumentation-api/src/main/java/io/opentelemetry/instrumentation/api/config/ConfigValueParsers.java b/instrumentation-api/src/main/java/io/opentelemetry/instrumentation/api/config/ConfigValueParsers.java new file mode 100644 index 000000000000..2ec4d9d84e89 --- /dev/null +++ b/instrumentation-api/src/main/java/io/opentelemetry/instrumentation/api/config/ConfigValueParsers.java @@ -0,0 +1,87 @@ +/* + * Copyright The OpenTelemetry Authors + * SPDX-License-Identifier: Apache-2.0 + */ + +package io.opentelemetry.instrumentation.api.config; + +import java.time.Duration; +import java.util.Arrays; +import java.util.Collections; +import java.util.LinkedHashMap; +import java.util.List; +import java.util.Map; +import java.util.concurrent.TimeUnit; + +final class ConfigValueParsers { + + static List parseList(String value) { + String[] tokens = value.split(",", -1); + // Remove whitespace from each item. + for (int i = 0; i < tokens.length; i++) { + tokens[i] = tokens[i].trim(); + } + return Collections.unmodifiableList(Arrays.asList(tokens)); + } + + static Map parseMap(String value) { + Map result = new LinkedHashMap<>(); + for (String token : value.split(",", -1)) { + token = token.trim(); + String[] parts = token.split("=", -1); + if (parts.length != 2) { + throw new IllegalArgumentException( + "Invalid map config part, should be formatted key1=value1,key2=value2: " + value); + } + result.put(parts[0], parts[1]); + } + return Collections.unmodifiableMap(result); + } + + static Duration parseDuration(String value) { + String unitString = getUnitString(value); + String numberString = value.substring(0, value.length() - unitString.length()); + long rawNumber = Long.parseLong(numberString.trim()); + TimeUnit unit = getDurationUnit(unitString.trim()); + return Duration.ofMillis(TimeUnit.MILLISECONDS.convert(rawNumber, unit)); + } + + /** Returns the TimeUnit associated with a unit string. Defaults to milliseconds. */ + private static TimeUnit getDurationUnit(String unitString) { + switch (unitString) { + case "": // Fallthrough expected + case "ms": + return TimeUnit.MILLISECONDS; + case "s": + return TimeUnit.SECONDS; + case "m": + return TimeUnit.MINUTES; + case "h": + return TimeUnit.HOURS; + case "d": + return TimeUnit.DAYS; + default: + throw new IllegalArgumentException("Invalid duration string, found: " + unitString); + } + } + + /** + * Fragments the 'units' portion of a config value from the 'value' portion. + * + *

E.g. "1ms" would return the string "ms". + */ + private static String getUnitString(String rawValue) { + int lastDigitIndex = rawValue.length() - 1; + while (lastDigitIndex >= 0) { + char c = rawValue.charAt(lastDigitIndex); + if (Character.isDigit(c)) { + break; + } + lastDigitIndex -= 1; + } + // Pull everything after the last digit. + return rawValue.substring(lastDigitIndex + 1); + } + + private ConfigValueParsers() {} +} diff --git a/instrumentation-api/src/test/groovy/io/opentelemetry/instrumentation/api/config/ConfigTest.groovy b/instrumentation-api/src/test/groovy/io/opentelemetry/instrumentation/api/config/ConfigSpockTest.groovy similarity index 54% rename from instrumentation-api/src/test/groovy/io/opentelemetry/instrumentation/api/config/ConfigTest.groovy rename to instrumentation-api/src/test/groovy/io/opentelemetry/instrumentation/api/config/ConfigSpockTest.groovy index 374d93b9b5da..331cdf7c8b10 100644 --- a/instrumentation-api/src/test/groovy/io/opentelemetry/instrumentation/api/config/ConfigTest.groovy +++ b/instrumentation-api/src/test/groovy/io/opentelemetry/instrumentation/api/config/ConfigSpockTest.groovy @@ -7,7 +7,8 @@ package io.opentelemetry.instrumentation.api.config import spock.lang.Specification -class ConfigTest extends Specification { +// TODO: rewrite to Java & JUnit +class ConfigSpockTest extends Specification { def "verify instrumentation config"() { setup: def config = new ConfigBuilder().readProperties([ @@ -42,64 +43,6 @@ class ConfigTest extends Specification { instrumentationNames = new TreeSet(names) } - def "should get string property"() { - given: - def config = new ConfigBuilder().readProperties([ - "property.string": "whatever" - ]).build() - - expect: - config.getProperty("property.string") == "whatever" - config.getProperty("property.string", "default") == "whatever" - config.getProperty("does-not-exist") == null - config.getProperty("does-not-exist", "default") == "default" - } - - def "should get boolean property"() { - given: - def config = new ConfigBuilder().readProperties([ - "property.bool": "false" - ]).build() - - expect: - !config.getBooleanProperty("property.bool", true) - config.getBooleanProperty("does-not-exist", true) - } - - def "should get list property"() { - given: - def config = new ConfigBuilder().readProperties([ - "property.list": "one, two, three" - ]).build() - - expect: - config.getListProperty("property.list") == ["one", "two", "three"] - config.getListProperty("property.list", ["four"]) == ["one", "two", "three"] - config.getListProperty("does-not-exist") == [] - config.getListProperty("does-not-exist", ["four"]) == ["four"] - } - - def "should get map property"() { - given: - def config = new ConfigBuilder().readProperties([ - "property.map": "one=1, two=2" - ]).build() - - expect: - config.getMapProperty("property.map") == ["one": "1", "two": "2"] - config.getMapProperty("does-not-exist").isEmpty() - } - - def "should return empty map when map property value is invalid"() { - given: - def config = new ConfigBuilder().readProperties([ - "property.map": "one=1, broken!" - ]).build() - - expect: - config.getMapProperty("property.map").isEmpty() - } - def "should expose if agent debug is enabled"() { given: def config = new ConfigBuilder().readProperties([ diff --git a/instrumentation-api/src/test/java/io/opentelemetry/instrumentation/api/config/ConfigTest.java b/instrumentation-api/src/test/java/io/opentelemetry/instrumentation/api/config/ConfigTest.java new file mode 100644 index 000000000000..adbd29d0829f --- /dev/null +++ b/instrumentation-api/src/test/java/io/opentelemetry/instrumentation/api/config/ConfigTest.java @@ -0,0 +1,166 @@ +/* + * Copyright The OpenTelemetry Authors + * SPDX-License-Identifier: Apache-2.0 + */ + +package io.opentelemetry.instrumentation.api.config; + +import static java.util.Arrays.asList; +import static java.util.Collections.singletonList; +import static java.util.Collections.singletonMap; +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertFalse; +import static org.junit.jupiter.api.Assertions.assertNull; +import static org.junit.jupiter.api.Assertions.assertTrue; + +import java.time.Duration; +import java.util.HashMap; +import java.util.Map; +import org.junit.jupiter.api.Test; + +// suppress duration unit check, e.g. ofMillis(5000) -> ofSeconds(5) +@SuppressWarnings("CanonicalDuration") +class ConfigTest { + @Test + void shouldGetString() { + Config config = Config.newBuilder().addProperty("prop.string", "some text").build(); + + assertEquals("some text", config.getProperty("prop.string")); + assertEquals("some text", config.getProperty("prop.string", "default")); + assertNull(config.getProperty("prop.missing")); + assertEquals("default", config.getProperty("prop.missing", "default")); + } + + @Test + void shouldGetBoolean() { + Config config = Config.newBuilder().addProperty("prop.boolean", "true").build(); + + assertTrue(config.getBoolean("prop.boolean")); + assertTrue(config.getBooleanProperty("prop.boolean", false)); + assertNull(config.getBoolean("prop.missing")); + assertFalse(config.getBooleanProperty("prop.missing", false)); + } + + @Test + void shouldGetInt() { + Config config = + Config.newBuilder() + .addProperty("prop.int", "12") + .addProperty("prop.wrong", "twelve") + .build(); + + assertEquals(12, config.getInt("prop.int")); + assertEquals(12, config.getInt("prop.int", 1000)); + assertNull(config.getInt("prop.wrong")); + assertEquals(1000, config.getInt("prop.wrong", 1000)); + assertNull(config.getInt("prop.missing")); + assertEquals(1000, config.getInt("prop.missing", 1000)); + } + + @Test + void shouldGetLong() { + Config config = + Config.newBuilder() + .addProperty("prop.long", "12") + .addProperty("prop.wrong", "twelve") + .build(); + + assertEquals(12, config.getLong("prop.long")); + assertEquals(12, config.getLong("prop.long", 1000)); + assertNull(config.getLong("prop.wrong")); + assertEquals(1000, config.getLong("prop.wrong", 1000)); + assertNull(config.getLong("prop.missing")); + assertEquals(1000, config.getLong("prop.missing", 1000)); + } + + @Test + void shouldGetDouble() { + Config config = + Config.newBuilder() + .addProperty("prop.double", "12.345") + .addProperty("prop.wrong", "twelve point something") + .build(); + + assertEquals(12.345, config.getDouble("prop.double")); + assertEquals(12.345, config.getDouble("prop.double", 99.99)); + assertNull(config.getDouble("prop.wrong")); + assertEquals(99.99, config.getDouble("prop.wrong", 99.99)); + assertNull(config.getDouble("prop.missing")); + assertEquals(99.99, config.getDouble("prop.missing", 99.99)); + } + + @Test + void shouldGetDuration_defaultUnit() { + Config config = + Config.newBuilder() + .addProperty("prop.duration", "5000") + .addProperty("prop.wrong", "hundred days") + .build(); + + assertEquals(Duration.ofMillis(5000), config.getDuration("prop.duration")); + assertEquals(Duration.ofMillis(5000), config.getDuration("prop.duration", Duration.ZERO)); + assertNull(config.getDuration("prop.wrong")); + assertEquals(Duration.ZERO, config.getDuration("prop.wrong", Duration.ZERO)); + assertNull(config.getDuration("prop.missing")); + assertEquals(Duration.ZERO, config.getDuration("prop.missing", Duration.ZERO)); + } + + @Test + void shouldGetDuration_variousUnits() { + Config config = Config.newBuilder().addProperty("prop.duration", "100ms").build(); + assertEquals(Duration.ofMillis(100), config.getDuration("prop.duration")); + + config = Config.newBuilder().addProperty("prop.duration", "100s").build(); + assertEquals(Duration.ofSeconds(100), config.getDuration("prop.duration")); + + config = Config.newBuilder().addProperty("prop.duration", "100m").build(); + assertEquals(Duration.ofMinutes(100), config.getDuration("prop.duration")); + + config = Config.newBuilder().addProperty("prop.duration", "100h").build(); + assertEquals(Duration.ofHours(100), config.getDuration("prop.duration")); + + config = Config.newBuilder().addProperty("prop.duration", "100d").build(); + assertEquals(Duration.ofDays(100), config.getDuration("prop.duration")); + } + + @Test + void shouldGetList() { + Config config = Config.newBuilder().addProperty("prop.list", "one, two ,three").build(); + + assertEquals(asList("one", "two", "three"), config.getListProperty("prop.list")); + assertEquals( + asList("one", "two", "three"), + config.getListProperty("prop.list", singletonList("default"))); + assertTrue(config.getListProperty("prop.missing").isEmpty()); + assertEquals( + singletonList("default"), config.getListProperty("prop.missing", singletonList("default"))); + } + + @Test + void shouldGetMap() { + Config config = + Config.newBuilder() + .addProperty("prop.map", "one=1, two=2") + .addProperty("prop.wrong", "one=1, but not two!") + .build(); + + assertEquals(map("one", "1", "two", "2"), config.getMapProperty("prop.map")); + assertEquals( + map("one", "1", "two", "2"), config.getMapProperty("prop.map", singletonMap("three", "3"))); + assertTrue(config.getMapProperty("prop.wrong").isEmpty()); + assertEquals( + singletonMap("three", "3"), + config.getMapProperty("prop.wrong", singletonMap("three", "3"))); + assertTrue(config.getMapProperty("prop.missing").isEmpty()); + assertEquals( + singletonMap("three", "3"), + config.getMapProperty("prop.missing", singletonMap("three", "3"))); + } + + public static Map map(String k1, String v1, String k2, String v2) { + Map map = new HashMap<>(); + map.put(k1, v1); + map.put(k2, v2); + return map; + } +} diff --git a/javaagent-tooling/src/main/java/io/opentelemetry/javaagent/tooling/OpenTelemetryInstaller.java b/javaagent-tooling/src/main/java/io/opentelemetry/javaagent/tooling/OpenTelemetryInstaller.java index 193e960cd47d..4db8425216be 100644 --- a/javaagent-tooling/src/main/java/io/opentelemetry/javaagent/tooling/OpenTelemetryInstaller.java +++ b/javaagent-tooling/src/main/java/io/opentelemetry/javaagent/tooling/OpenTelemetryInstaller.java @@ -11,6 +11,7 @@ import io.opentelemetry.instrumentation.api.config.Config; import io.opentelemetry.javaagent.extension.AgentListener; import io.opentelemetry.javaagent.instrumentation.api.OpenTelemetrySdkAccess; +import io.opentelemetry.javaagent.tooling.config.ConfigPropertiesAdapter; import io.opentelemetry.sdk.OpenTelemetrySdk; import io.opentelemetry.sdk.autoconfigure.OpenTelemetrySdkAutoConfiguration; import io.opentelemetry.sdk.autoconfigure.spi.SdkMeterProviderConfigurer; @@ -23,7 +24,6 @@ import io.opentelemetry.sdk.metrics.view.InstrumentSelector; import io.opentelemetry.sdk.metrics.view.View; import java.util.Arrays; -import java.util.Map; import org.slf4j.Logger; import org.slf4j.LoggerFactory; @@ -48,14 +48,13 @@ public void beforeAgent(Config config) { public static synchronized void installAgentTracer(Config config) { if (config.getBooleanProperty(JAVAAGENT_ENABLED_CONFIG, true)) { - copySystemProperties(config); - if (config.getBooleanProperty(JAVAAGENT_NOOP_CONFIG, false)) { GlobalOpenTelemetry.set(NoopOpenTelemetry.getInstance()); } else { System.setProperty("io.opentelemetry.context.contextStorageProvider", "default"); - OpenTelemetrySdk sdk = OpenTelemetrySdkAutoConfiguration.initialize(); + OpenTelemetrySdk sdk = + OpenTelemetrySdkAutoConfiguration.initialize(true, new ConfigPropertiesAdapter(config)); OpenTelemetrySdkAccess.internalSetForceFlush( (timeout, unit) -> { CompletableResultCode traceResult = sdk.getSdkTracerProvider().forceFlush(); @@ -70,28 +69,6 @@ public static synchronized void installAgentTracer(Config config) { } } - // OpenTelemetrySdkAutoConfiguration currently only supports configuration from environment. We - // massage any properties we have that aren't in the environment to system properties. - // TODO(anuraaga): Make this less hacky - private static void copySystemProperties(Config config) { - Map allProperties = config.getAllProperties(); - Map environmentProperties = - Config.newBuilder() - .readEnvironmentVariables() - .readSystemProperties() - .build() - .getAllProperties(); - - allProperties.forEach( - (key, value) -> { - if (!environmentProperties.containsKey(key) - && key.startsWith("otel.") - && !key.startsWith("otel.instrumentation")) { - System.setProperty(key, value); - } - }); - } - // Configure histogram metrics similarly to how the SDK will default in 1.5.0 for early feedback. @AutoService(SdkMeterProviderConfigurer.class) public static final class OpenTelemetryMetricsConfigurer implements SdkMeterProviderConfigurer { diff --git a/javaagent-tooling/src/main/java/io/opentelemetry/javaagent/tooling/config/ConfigPropertiesAdapter.java b/javaagent-tooling/src/main/java/io/opentelemetry/javaagent/tooling/config/ConfigPropertiesAdapter.java new file mode 100644 index 000000000000..216098fe1a32 --- /dev/null +++ b/javaagent-tooling/src/main/java/io/opentelemetry/javaagent/tooling/config/ConfigPropertiesAdapter.java @@ -0,0 +1,67 @@ +/* + * Copyright The OpenTelemetry Authors + * SPDX-License-Identifier: Apache-2.0 + */ + +package io.opentelemetry.javaagent.tooling.config; + +import io.opentelemetry.instrumentation.api.config.Config; +import io.opentelemetry.sdk.autoconfigure.ConfigProperties; +import java.time.Duration; +import java.util.List; +import java.util.Map; +import org.checkerframework.checker.nullness.qual.Nullable; + +public final class ConfigPropertiesAdapter implements ConfigProperties { + private final Config config; + + public ConfigPropertiesAdapter(Config config) { + this.config = config; + } + + @Nullable + @Override + public String getString(String name) { + return config.getProperty(name); + } + + @Nullable + @Override + public Boolean getBoolean(String name) { + return config.getBoolean(name); + } + + @Nullable + @Override + public Integer getInt(String name) { + return config.getInt(name); + } + + @Nullable + @Override + public Long getLong(String name) { + return config.getLong(name); + } + + @Nullable + @Override + public Double getDouble(String name) { + return config.getDouble(name); + } + + @Nullable + @Override + public Duration getDuration(String name) { + return config.getDuration(name); + } + + @Override + public List getCommaSeparatedValues(String name) { + return config.getListProperty(name); + } + + @Override + public Map getCommaSeparatedMap(String name) { + return config.getMapProperty(name); + } +} diff --git a/javaagent-tooling/src/test/groovy/io/opentelemetry/javaagent/tooling/OpenTelemetryInstallerTest.groovy b/javaagent-tooling/src/test/groovy/io/opentelemetry/javaagent/tooling/OpenTelemetryInstallerTest.groovy index a1060e7f6fab..9416ec2e702d 100755 --- a/javaagent-tooling/src/test/groovy/io/opentelemetry/javaagent/tooling/OpenTelemetryInstallerTest.groovy +++ b/javaagent-tooling/src/test/groovy/io/opentelemetry/javaagent/tooling/OpenTelemetryInstallerTest.groovy @@ -6,8 +6,8 @@ package io.opentelemetry.javaagent.tooling import io.opentelemetry.api.GlobalOpenTelemetry +import io.opentelemetry.extension.noopapi.NoopOpenTelemetry import io.opentelemetry.instrumentation.api.config.Config -import io.opentelemetry.extension.noopapi.NoopOpenTelemetry; import spock.lang.Specification class OpenTelemetryInstallerTest extends Specification {