Make Config behave exactly as SDK DefaultConfigProperties (#4035)

* Make Config behave exactly as SDK DefaultConfigProperties

* errorprone

* errorprone part 2

* errorprone part 3

* fix failing tests

Author: Mateusz Rzeszutek

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

@@ -5,15 +5,15 @@
 
 package io.opentelemetry.instrumentation.api.config;
 
+import static java.util.Collections.emptyList;
+import static java.util.Collections.emptyMap;
 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;
 import java.util.Properties;
-import java.util.function.Function;
 import org.checkerframework.checker.nullness.qual.Nullable;
 import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
@@ -99,66 +99,75 @@ public String getString(String name, String defaultValue) {
    */
   @Nullable
   public Boolean getBoolean(String name) {
-    return getTypedProperty(name, Boolean::parseBoolean, null);
+    return getTypedProperty(name, ConfigValueParsers::parseBoolean);
   }
 
   /**
    * Returns a boolean-valued configuration property or {@code defaultValue} if a property with name
    * {@code name} has not been configured.
    */
   public boolean getBoolean(String name, boolean defaultValue) {
-    return getTypedProperty(name, Boolean::parseBoolean, defaultValue);
+    return safeGetTypedProperty(name, ConfigValueParsers::parseBoolean, defaultValue);
   }
 
   /**
    * Returns a integer-valued configuration property or {@code null} if a property with name {@code
    * name} has not been configured.
+   *
+   * @throws ConfigParsingException if the property is not a valid integer.
    */
   @Nullable
   public Integer getInt(String name) {
-    return getTypedProperty(name, Integer::parseInt, null);
+    return getTypedProperty(name, ConfigValueParsers::parseInt);
   }
 
   /**
    * Returns a integer-valued configuration property or {@code defaultValue} if a property with name
-   * {@code name} has not been configured.
+   * {@code name} has not been configured or when parsing has failed. This is the safe variant of
+   * {@link #getInt(String)}.
    */
   public int getInt(String name, int defaultValue) {
-    return getTypedProperty(name, Integer::parseInt, defaultValue);
+    return safeGetTypedProperty(name, ConfigValueParsers::parseInt, defaultValue);
   }
 
   /**
    * Returns a long-valued configuration property or {@code null} if a property with name {@code
    * name} has not been configured.
+   *
+   * @throws ConfigParsingException if the property is not a valid long.
    */
   @Nullable
   public Long getLong(String name) {
-    return getTypedProperty(name, Long::parseLong, null);
+    return getTypedProperty(name, ConfigValueParsers::parseLong);
   }
 
   /**
    * Returns a long-valued configuration property or {@code defaultValue} if a property with name
-   * {@code name} has not been configured.
+   * {@code name} has not been configured or when parsing has failed. This is the safe variant of
+   * {@link #getLong(String)}.
    */
   public long getLong(String name, long defaultValue) {
-    return getTypedProperty(name, Long::parseLong, defaultValue);
+    return safeGetTypedProperty(name, ConfigValueParsers::parseLong, defaultValue);
   }
 
   /**
    * Returns a double-valued configuration property or {@code null} if a property with name {@code
    * name} has not been configured.
+   *
+   * @throws ConfigParsingException if the property is not a valid long.
    */
   @Nullable
   public Double getDouble(String name) {
-    return getTypedProperty(name, Double::parseDouble, null);
+    return getTypedProperty(name, ConfigValueParsers::parseDouble);
   }
 
   /**
    * Returns a double-valued configuration property or {@code defaultValue} if a property with name
-   * {@code name} has not been configured.
+   * {@code name} has not been configured or when parsing has failed. This is the safe variant of
+   * {@link #getDouble(String)}.
    */
   public double getDouble(String name, double defaultValue) {
-    return getTypedProperty(name, Double::parseDouble, defaultValue);
+    return safeGetTypedProperty(name, ConfigValueParsers::parseDouble, defaultValue);
   }
 
   /**
@@ -176,15 +185,18 @@ public double getDouble(String name, double defaultValue) {
    * </ul>
    *
    * <p>If no unit is specified, milliseconds is the assumed duration unit.
+   *
+   * @throws ConfigParsingException if the property is not a valid long.
    */
   @Nullable
   public Duration getDuration(String name) {
-    return getTypedProperty(name, ConfigValueParsers::parseDuration, null);
+    return getTypedProperty(name, ConfigValueParsers::parseDuration);
   }
 
   /**
    * Returns a duration-valued configuration property or {@code defaultValue} if a property with
-   * name {@code name} has not been configured.
+   * name {@code name} has not been configured or when parsing has failed. This is the safe variant
+   * of {@link #getDuration(String)}.
    *
    * <p>Durations can be of the form "{number}{unit}", where unit is one of:
    *
@@ -199,7 +211,7 @@ public Duration getDuration(String name) {
    * <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);
+    return safeGetTypedProperty(name, ConfigValueParsers::parseDuration, defaultValue);
   }
 
   /**
@@ -208,7 +220,8 @@ public Duration getDuration(String name, Duration defaultValue) {
    * {@code one,two,three}.
    */
   public List<String> getList(String name) {
-    return getList(name, Collections.emptyList());
+    List<String> list = getTypedProperty(name, ConfigValueParsers::parseList);
+    return list == null ? emptyList() : list;
   }
 
   /**
@@ -217,42 +230,51 @@ public List<String> getList(String name) {
    * e.g. {@code one,two,three}.
    */
   public List<String> getList(String name, List<String> defaultValue) {
-    return getTypedProperty(name, ConfigValueParsers::parseList, defaultValue);
+    return safeGetTypedProperty(name, ConfigValueParsers::parseList, defaultValue);
   }
 
   /**
    * 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}.
+   *
+   * @throws ConfigParsingException if the property is not a valid long.
    */
   public Map<String, String> getMap(String name) {
-    return getMap(name, Collections.emptyMap());
+    Map<String, String> map = getTypedProperty(name, ConfigValueParsers::parseMap);
+    return map == null ? emptyMap() : map;
   }
 
   /**
    * 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}.
+   * {@code name} has not been configured or when parsing has failed. This is the safe variant of
+   * {@link #getMap(String)}. 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> getMap(String name, Map<String, String> defaultValue) {
-    return getTypedProperty(name, ConfigValueParsers::parseMap, defaultValue);
+    return safeGetTypedProperty(name, ConfigValueParsers::parseMap, defaultValue);
   }
 
-  private <T> T getTypedProperty(String name, Function<String, T> parser, T defaultValue) {
-    String value = getRawProperty(name, null);
-    if (value == null || value.trim().isEmpty()) {
-      return defaultValue;
-    }
+  private <T> T safeGetTypedProperty(String name, ConfigValueParser<T> parser, T defaultValue) {
     try {
-      return parser.apply(value);
+      T value = getTypedProperty(name, parser);
+      return value == null ? defaultValue : value;
     } catch (RuntimeException t) {
-      logger.debug("Cannot parse {}", value, t);
+      logger.debug("Error occurred during parsing: {}", t.getMessage(), t);
       return defaultValue;
     }
   }
 
+  @Nullable
+  private <T> T getTypedProperty(String name, ConfigValueParser<T> parser) {
+    String value = getRawProperty(name, null);
+    if (value == null || value.trim().isEmpty()) {
+      return null;
+    }
+    return parser.parse(name, value);
+  }
+
   private String getRawProperty(String name, String defaultValue) {
     return getAllProperties().getOrDefault(NamingConvention.DOT.normalize(name), defaultValue);
   }

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

@@ -0,0 +1,16 @@
+/*
+ * Copyright The OpenTelemetry Authors
+ * SPDX-License-Identifier: Apache-2.0
+ */
+
+package io.opentelemetry.instrumentation.api.config;
+
+public class ConfigParsingException extends RuntimeException {
+  public ConfigParsingException(String message) {
+    super(message);
+  }
+
+  public ConfigParsingException(String message, Throwable cause) {
+    super(message, cause);
+  }
+}

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

@@ -0,0 +1,11 @@
+/*
+ * Copyright The OpenTelemetry Authors
+ * SPDX-License-Identifier: Apache-2.0
+ */
+
+package io.opentelemetry.instrumentation.api.config;
+
+@FunctionalInterface
+interface ConfigValueParser<T> {
+  T parse(String propertyName, String rawValue);
+}

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

@@ -15,20 +15,57 @@
 import java.util.concurrent.TimeUnit;
 import java.util.stream.Collectors;
 
+// most of the parsing code copied from
+// https://github.com/open-telemetry/opentelemetry-java/blob/main/sdk-extensions/autoconfigure/src/main/java/io/opentelemetry/sdk/autoconfigure/DefaultConfigProperties.java
+@SuppressWarnings("UnusedException")
 final class ConfigValueParsers {
 
-  static List<String> parseList(String value) {
+  static boolean parseBoolean(@SuppressWarnings("unused") String propertyName, String value) {
+    return Boolean.parseBoolean(value);
+  }
+
+  static int parseInt(String propertyName, String value) {
+    try {
+      return Integer.parseInt(value);
+    } catch (NumberFormatException e) {
+      throw newInvalidPropertyException(propertyName, value, "integer");
+    }
+  }
+
+  static long parseLong(String propertyName, String value) {
+    try {
+      return Long.parseLong(value);
+    } catch (NumberFormatException e) {
+      throw newInvalidPropertyException(propertyName, value, "long");
+    }
+  }
+
+  static double parseDouble(String propertyName, String value) {
+    try {
+      return Double.parseDouble(value);
+    } catch (NumberFormatException e) {
+      throw newInvalidPropertyException(propertyName, value, "double");
+    }
+  }
+
+  private static ConfigParsingException newInvalidPropertyException(
+      String name, String value, String type) {
+    throw new ConfigParsingException(
+        "Invalid value for property " + name + "=" + value + ". Must be a " + type + ".");
+  }
+
+  static List<String> parseList(@SuppressWarnings("unused") String propertyName, String value) {
     return Collections.unmodifiableList(filterBlanksAndNulls(value.split(",")));
   }
 
-  static Map<String, String> parseMap(String value) {
-    return parseList(value).stream()
+  static Map<String, String> parseMap(String propertyName, String value) {
+    return parseList(propertyName, value).stream()
         .map(keyValuePair -> filterBlanksAndNulls(keyValuePair.split("=", 2)))
         .map(
             splitKeyValuePairs -> {
               if (splitKeyValuePairs.size() != 2) {
-                throw new IllegalArgumentException(
-                    "Invalid map property, should be formatted key1=value1,key2=value2: " + value);
+                throw new ConfigParsingException(
+                    "Invalid map property: " + propertyName + "=" + value);
               }
               return new AbstractMap.SimpleImmutableEntry<>(
                   splitKeyValuePairs.get(0), splitKeyValuePairs.get(1));
@@ -47,12 +84,25 @@ private static List<String> filterBlanksAndNulls(String[] values) {
         .collect(Collectors.toList());
   }
 
-  static Duration parseDuration(String value) {
+  static Duration parseDuration(String propertyName, 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));
+    try {
+      long rawNumber = Long.parseLong(numberString.trim());
+      TimeUnit unit = getDurationUnit(unitString.trim());
+      return Duration.ofMillis(TimeUnit.MILLISECONDS.convert(rawNumber, unit));
+    } catch (NumberFormatException e) {
+      throw new ConfigParsingException(
+          "Invalid duration property "
+              + propertyName
+              + "="
+              + value
+              + ". Expected number, found: "
+              + numberString);
+    } catch (ConfigParsingException ex) {
+      throw new ConfigParsingException(
+          "Invalid duration property " + propertyName + "=" + value + ". " + ex.getMessage());
+    }
   }
 
   /** Returns the TimeUnit associated with a unit string. Defaults to milliseconds. */
@@ -70,7 +120,7 @@ private static TimeUnit getDurationUnit(String unitString) {
       case "d":
         return TimeUnit.DAYS;
       default:
-        throw new IllegalArgumentException("Invalid duration string, found: " + unitString);
+        throw new ConfigParsingException("Invalid duration string, found: " + unitString);
     }
   }