From b0ad1b65b6f953b72c8d4802ca853e92f4594b0f Mon Sep 17 00:00:00 2001 From: Mateusz Rzeszutek Date: Wed, 16 Jun 2021 12:58:29 +0200 Subject: [PATCH] Hide Config#create() method and use builder everywhere --- .../instrumentation/api/config/Config.java | 24 ++++++++++++++++--- .../api/config/ConfigBuilder.java | 5 ++-- .../internal/SupportabilityMetricsTest.java | 15 +++++++----- .../tooling/OpenTelemetryInstaller.java | 8 +++---- .../tooling/config/ConfigInitializer.java | 3 +-- .../UserExcludedClassesConfigurerTest.java | 6 ++--- 6 files changed, 40 insertions(+), 21 deletions(-) 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 e144f7a221ce..8f6dceb78f71 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 @@ -25,7 +25,12 @@ public abstract class Config { // read system properties @Nullable private static volatile Config instance = null; - public static Config create(Map allProperties) { + /** Start building a new {@link Config} instance. */ + public static ConfigBuilder newBuilder() { + return new ConfigBuilder(); + } + + static Config create(Map allProperties) { return new AutoValue_Config(allProperties); } @@ -47,12 +52,13 @@ public static Config get() { // this should only happen in library instrumentation // // no need to synchronize because worst case is creating INSTANCE more than once - instance = new ConfigBuilder().readEnvironmentVariables().readSystemProperties().build(); + instance = newBuilder().readEnvironmentVariables().readSystemProperties().build(); } return instance; } - abstract Map getAllProperties(); + /** Returns all properties stored in this instance. The returned map is unmodifiable. */ + public abstract Map getAllProperties(); /** * Returns a string property value or null if a property with name {@code name} did not exist. @@ -92,6 +98,18 @@ 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. + * + *

This property may be used by vendor distributions to get numerical values. + * + * @see #getProperty(String, String) + */ + public long getLongProperty(String name, long defaultValue) { + return getTypedProperty(name, Long::parseLong, defaultValue); + } + /** * Returns a list-of-strings property value or empty list if a property with name {@code name} did * not exist. 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 f39cd51bb175..c44cd32bf867 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 @@ -5,11 +5,12 @@ package io.opentelemetry.instrumentation.api.config; +import java.util.Collections; import java.util.HashMap; import java.util.Map; import java.util.Properties; -public class ConfigBuilder { +public final class ConfigBuilder { private final Map allProperties = new HashMap<>(); @@ -43,6 +44,6 @@ private ConfigBuilder fromConfigMap( } public Config build() { - return Config.create(allProperties); + return Config.create(Collections.unmodifiableMap(new HashMap<>(allProperties))); } } diff --git a/instrumentation-api/src/test/java/io/opentelemetry/instrumentation/api/internal/SupportabilityMetricsTest.java b/instrumentation-api/src/test/java/io/opentelemetry/instrumentation/api/internal/SupportabilityMetricsTest.java index 62dadc314768..2c597c9e1972 100644 --- a/instrumentation-api/src/test/java/io/opentelemetry/instrumentation/api/internal/SupportabilityMetricsTest.java +++ b/instrumentation-api/src/test/java/io/opentelemetry/instrumentation/api/internal/SupportabilityMetricsTest.java @@ -19,8 +19,7 @@ class SupportabilityMetricsTest { void disabled() { List reports = new ArrayList<>(); SupportabilityMetrics metrics = - new SupportabilityMetrics( - Config.create(Collections.singletonMap("otel.javaagent.debug", "false")), reports::add); + new SupportabilityMetrics(configWithJavaagentDebug(false), reports::add); metrics.recordSuppressedSpan(SpanKind.CLIENT, "favoriteInstrumentation"); metrics.recordSuppressedSpan(SpanKind.SERVER, "favoriteInstrumentation"); @@ -39,8 +38,7 @@ void disabled() { void reportsMetrics() { List reports = new ArrayList<>(); SupportabilityMetrics metrics = - new SupportabilityMetrics( - Config.create(Collections.singletonMap("otel.javaagent.debug", "true")), reports::add); + new SupportabilityMetrics(configWithJavaagentDebug(true), reports::add); metrics.recordSuppressedSpan(SpanKind.CLIENT, "favoriteInstrumentation"); metrics.recordSuppressedSpan(SpanKind.SERVER, "favoriteInstrumentation"); @@ -65,8 +63,7 @@ void reportsMetrics() { void resetsCountsEachReport() { List reports = new ArrayList<>(); SupportabilityMetrics metrics = - new SupportabilityMetrics( - Config.create(Collections.singletonMap("otel.javaagent.debug", "true")), reports::add); + new SupportabilityMetrics(configWithJavaagentDebug(true), reports::add); metrics.recordSuppressedSpan(SpanKind.CLIENT, "favoriteInstrumentation"); metrics.incrementCounter("some counter"); @@ -79,4 +76,10 @@ void resetsCountsEachReport() { "Suppressed Spans by 'favoriteInstrumentation' (CLIENT) : 1", "Counter 'some counter' : 1"); } + + private static Config configWithJavaagentDebug(boolean enabled) { + return Config.newBuilder() + .readProperties(Collections.singletonMap("otel.javaagent.debug", Boolean.toString(enabled))) + .build(); + } } 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 7411416d2604..c999658048c6 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 @@ -7,11 +7,11 @@ import com.google.auto.service.AutoService; import io.opentelemetry.instrumentation.api.config.Config; -import io.opentelemetry.instrumentation.api.config.ConfigBuilder; import io.opentelemetry.javaagent.extension.AgentListener; import io.opentelemetry.javaagent.instrumentation.api.OpenTelemetrySdkAccess; import io.opentelemetry.sdk.OpenTelemetrySdk; import io.opentelemetry.sdk.autoconfigure.OpenTelemetrySdkAutoConfiguration; +import java.util.Map; import java.util.Properties; import org.slf4j.Logger; import org.slf4j.LoggerFactory; @@ -50,12 +50,12 @@ public static synchronized void installAgentTracer(Config config) { // TODO(anuraaga): Make this less hacky private static void copySystemProperties(Config config) { Properties allProperties = config.asJavaProperties(); - Properties environmentProperties = - new ConfigBuilder() + Map environmentProperties = + Config.newBuilder() .readEnvironmentVariables() .readSystemProperties() .build() - .asJavaProperties(); + .getAllProperties(); allProperties.forEach( (key, value) -> { diff --git a/javaagent-tooling/src/main/java/io/opentelemetry/javaagent/tooling/config/ConfigInitializer.java b/javaagent-tooling/src/main/java/io/opentelemetry/javaagent/tooling/config/ConfigInitializer.java index 35abb217c2e0..06529e4d4ac7 100644 --- a/javaagent-tooling/src/main/java/io/opentelemetry/javaagent/tooling/config/ConfigInitializer.java +++ b/javaagent-tooling/src/main/java/io/opentelemetry/javaagent/tooling/config/ConfigInitializer.java @@ -6,7 +6,6 @@ package io.opentelemetry.javaagent.tooling.config; import io.opentelemetry.instrumentation.api.config.Config; -import io.opentelemetry.instrumentation.api.config.ConfigBuilder; import io.opentelemetry.javaagent.spi.config.PropertySource; import io.opentelemetry.javaagent.tooling.SafeServiceLoader; import java.io.File; @@ -31,7 +30,7 @@ public static void initialize() { // visible for testing static Config create(Properties spiConfiguration, Properties configurationFile) { - return new ConfigBuilder() + return Config.newBuilder() .readProperties(spiConfiguration) .readProperties(configurationFile) .readEnvironmentVariables() diff --git a/javaagent-tooling/src/test/java/io/opentelemetry/javaagent/tooling/ignore/UserExcludedClassesConfigurerTest.java b/javaagent-tooling/src/test/java/io/opentelemetry/javaagent/tooling/ignore/UserExcludedClassesConfigurerTest.java index f652a370d503..965e7c08da3d 100644 --- a/javaagent-tooling/src/test/java/io/opentelemetry/javaagent/tooling/ignore/UserExcludedClassesConfigurerTest.java +++ b/javaagent-tooling/src/test/java/io/opentelemetry/javaagent/tooling/ignore/UserExcludedClassesConfigurerTest.java @@ -6,13 +6,11 @@ package io.opentelemetry.javaagent.tooling.ignore; import static io.opentelemetry.javaagent.tooling.ignore.UserExcludedClassesConfigurer.EXCLUDED_CLASSES_CONFIG; -import static java.util.Collections.emptyMap; import static java.util.Collections.singletonMap; import static org.mockito.Mockito.verify; import static org.mockito.Mockito.verifyNoInteractions; import io.opentelemetry.instrumentation.api.config.Config; -import io.opentelemetry.instrumentation.api.config.ConfigBuilder; import io.opentelemetry.javaagent.extension.ignore.IgnoredTypesBuilder; import io.opentelemetry.javaagent.extension.ignore.IgnoredTypesConfigurer; import org.junit.jupiter.api.Test; @@ -29,7 +27,7 @@ class UserExcludedClassesConfigurerTest { @Test void shouldAddNothingToBuilderWhenPropertyIsEmpty() { // when - underTest.configure(Config.create(emptyMap()), builder); + underTest.configure(Config.newBuilder().build(), builder); // then verifyNoInteractions(builder); @@ -39,7 +37,7 @@ void shouldAddNothingToBuilderWhenPropertyIsEmpty() { void shouldIgnoreClassesAndPackages() { // given Config config = - new ConfigBuilder() + Config.newBuilder() .readProperties( singletonMap( EXCLUDED_CLASSES_CONFIG,