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

Hide Config#create() method and use builder everywhere #3338

Merged
merged 2 commits into from
Jun 17, 2021
Merged
Show file tree
Hide file tree
Changes from 1 commit
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
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,12 @@ public abstract class Config {
// read system properties
@Nullable private static volatile Config instance = null;

public static Config create(Map<String, String> allProperties) {
/** Start building a new {@link Config} instance. */
public static ConfigBuilder newBuilder() {
return new ConfigBuilder();
}

static Config create(Map<String, String> allProperties) {
return new AutoValue_Config(allProperties);
}

Expand All @@ -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<String, String> getAllProperties();
/** Returns all properties stored in this instance. The returned map is unmodifiable. */
public abstract Map<String, String> getAllProperties();

/**
* Returns a string property value or null if a property with name {@code name} did not exist.
Expand Down Expand Up @@ -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.
*
* <p>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.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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<String, String> allProperties = new HashMap<>();

Expand Down Expand Up @@ -43,6 +44,6 @@ private ConfigBuilder fromConfigMap(
}

public Config build() {
return Config.create(allProperties);
return Config.create(Collections.unmodifiableMap(new HashMap<>(allProperties)));
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -19,8 +19,7 @@ class SupportabilityMetricsTest {
void disabled() {
List<String> 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");
Expand All @@ -39,8 +38,7 @@ void disabled() {
void reportsMetrics() {
List<String> 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");
Expand All @@ -65,8 +63,7 @@ void reportsMetrics() {
void resetsCountsEachReport() {
List<String> 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");
Expand All @@ -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();
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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<String, String> environmentProperties =
Config.newBuilder()
.readEnvironmentVariables()
.readSystemProperties()
.build()
.asJavaProperties();
.getAllProperties();

allProperties.forEach(
(key, value) -> {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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()
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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);
Expand All @@ -39,7 +37,7 @@ void shouldAddNothingToBuilderWhenPropertyIsEmpty() {
void shouldIgnoreClassesAndPackages() {
// given
Config config =
new ConfigBuilder()
Config.newBuilder()
.readProperties(
singletonMap(
EXCLUDED_CLASSES_CONFIG,
Expand Down