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

…3866)

* Don't pass configuration to SDK autoconfigure through system props

* suppress CanonicalDuration

* checkstyle

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}).
+ *
+ * <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);
@@ -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
@@ -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<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) {
     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);
@@ -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);
-  }
 }

instrumentation-api/src/main/java/io/opentelemetry/instrumentation/api/config/ConfigBuilder.java

@@ -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));