Fixes for @nullable

Converters stored in a single flat map
Exception thrown when converter not found

Author: robsunday

instrumentation/jmx-metrics/library/src/main/java/io/opentelemetry/instrumentation/jmx/engine/MetricInfo.java

@@ -28,7 +28,7 @@ public enum Type {
   private final String metricName; // used as Instrument name
   @Nullable private final String description;
   @Nullable private final String sourceUnit;
-  @Nullable private final String unit;
+  private final String unit;
   private final Type type;
 
   /**
@@ -44,7 +44,7 @@ public MetricInfo(
       String metricName,
       @Nullable String description,
       @Nullable String sourceUnit,
-      @Nullable String unit,
+      String unit,
       @Nullable Type type) {
     this.metricName = metricName;
     this.description = description;
@@ -67,7 +67,6 @@ public String getSourceUnit() {
     return sourceUnit;
   }
 
-  @Nullable
   public String getUnit() {
     return unit;
   }

instrumentation/jmx-metrics/library/src/main/java/io/opentelemetry/instrumentation/jmx/engine/MetricRegistrar.java

@@ -87,7 +87,7 @@ void enrollExtractor(
           // CHECKSTYLE:ON
           LongCounterBuilder builder = meter.counterBuilder(metricName);
           Optional.ofNullable(description).ifPresent(builder::setDescription);
-          Optional.ofNullable(unit).ifPresent(builder::setUnit);
+          builder.setUnit(unit);
 
           if (recordDoubleValue) {
             builder.ofDoubles().buildWithCallback(doubleTypeCallback(extractor, unitConverter));
@@ -104,7 +104,7 @@ void enrollExtractor(
           // CHECKSTYLE:ON
           LongUpDownCounterBuilder builder = meter.upDownCounterBuilder(metricName);
           Optional.ofNullable(description).ifPresent(builder::setDescription);
-          Optional.ofNullable(unit).ifPresent(builder::setUnit);
+          builder.setUnit(unit);
 
           if (recordDoubleValue) {
             builder.ofDoubles().buildWithCallback(doubleTypeCallback(extractor, unitConverter));
@@ -121,7 +121,7 @@ void enrollExtractor(
           // CHECKSTYLE:ON
           DoubleGaugeBuilder builder = meter.gaugeBuilder(metricName);
           Optional.ofNullable(description).ifPresent(builder::setDescription);
-          Optional.ofNullable(unit).ifPresent(builder::setUnit);
+          builder.setUnit(unit);
 
           if (recordDoubleValue) {
             builder.buildWithCallback(doubleTypeCallback(extractor, unitConverter));

instrumentation/jmx-metrics/library/src/main/java/io/opentelemetry/instrumentation/jmx/engine/unit/UnitConverterFactory.java

@@ -12,8 +12,7 @@
 import javax.annotation.Nullable;
 
 public class UnitConverterFactory {
-
-  private static final Map<String, Map<String, UnitConverter>> conversionMappings = new HashMap<>();
+  private static final Map<String, UnitConverter> conversionMappings = new HashMap<>();
 
   static {
     registerConverter("ms", "s", value -> value.doubleValue() / TimeUnit.SECONDS.toMillis(1), true);
@@ -22,31 +21,49 @@ public class UnitConverterFactory {
 
   private UnitConverterFactory() {}
 
-  public static UnitConverter getConverter(@Nullable String fromUnit, @Nullable String toUnit) {
-    if (fromUnit == null || toUnit == null) {
-      return null;
+  @Nullable
+  public static UnitConverter getConverter(@Nullable String sourceUnit, String targetUnit) {
+    if (targetUnit.isEmpty()) {
+      throw new IllegalArgumentException("Non empty targetUnit must be provided");
     }
 
-    Map<String, UnitConverter> converters = conversionMappings.get(fromUnit);
-    if (converters == null) {
+    if (sourceUnit == null || sourceUnit.isEmpty()) {
+      // No conversion is needed
       return null;
     }
 
-    return converters.get(toUnit);
+    String converterKey = getConverterKey(sourceUnit, targetUnit);
+    UnitConverter converter = conversionMappings.get(converterKey);
+    if (converter == null) {
+      throw new IllegalStateException(
+          "No [" + sourceUnit + "] to [" + targetUnit + "] unit converter");
+    }
+
+    return converter;
   }
 
   public static void registerConverter(
       String sourceUnit,
       String targetUnit,
       Function<Number, Number> convertingFunction,
       boolean convertToDouble) {
-    Map<String, UnitConverter> converters =
-        conversionMappings.computeIfAbsent(sourceUnit, k -> new HashMap<>());
+    if (sourceUnit.isEmpty()) {
+      throw new IllegalArgumentException("Non empty sourceUnit must be provided");
+    }
+    if (targetUnit.isEmpty()) {
+      throw new IllegalArgumentException("Non empty targetUnit must be provided");
+    }
+
+    String converterKey = getConverterKey(sourceUnit, targetUnit);
 
-    if (converters.containsKey(targetUnit)) {
+    if (conversionMappings.containsKey(converterKey)) {
       throw new IllegalArgumentException(
-          "Converter from " + sourceUnit + " to " + targetUnit + " already registered");
+          "Converter from [" + sourceUnit + "] to [" + targetUnit + "] already registered");
     }
-    converters.put(targetUnit, new UnitConverter(convertingFunction, convertToDouble));
+    conversionMappings.put(converterKey, new UnitConverter(convertingFunction, convertToDouble));
+  }
+
+  private static String getConverterKey(String sourceUnit, String targetUnit) {
+    return sourceUnit + "->" + targetUnit;
   }
 }

instrumentation/jmx-metrics/library/src/test/java/io/opentelemetry/instrumentation/jmx/engine/unit/UnitConverterFactoryTest.java

@@ -5,17 +5,16 @@
 
 package io.opentelemetry.instrumentation.jmx.engine.unit;
 
+import static org.assertj.core.api.Assertions.assertThat;
 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.assertThrows;
 import static org.junit.jupiter.api.Assertions.assertTrue;
 
-import java.util.stream.Stream;
 import org.junit.jupiter.api.Test;
 import org.junit.jupiter.params.ParameterizedTest;
-import org.junit.jupiter.params.provider.Arguments;
 import org.junit.jupiter.params.provider.CsvSource;
-import org.junit.jupiter.params.provider.MethodSource;
 
 class UnitConverterFactoryTest {
 
@@ -62,18 +61,51 @@ void shouldSupportCustomConverter() {
   }
 
   @ParameterizedTest
-  @MethodSource("provideUnitsForMissingConverter")
-  void shouldHandleMissingConverter(String sourceUnit, String targetUnit) {
+  @CsvSource({
+    "--, --",
+    "ms, non-existing",
+    "non-existing, s",
+  })
+  void shouldHandleNonExistingConverter(String sourceUnit, String targetUnit) {
+    IllegalStateException exception =
+        assertThrows(
+            IllegalStateException.class,
+            () -> UnitConverterFactory.getConverter(sourceUnit, targetUnit));
+    assertEquals(
+        "No [" + sourceUnit + "] to [" + targetUnit + "] unit converter", exception.getMessage());
+  }
+
+  @ParameterizedTest
+  @CsvSource({
+    ", s", // null -> "s"
+    "'', s", // "" -> "s"
+  })
+  void shouldSkipConversionWhenSourceUnitNotSpecified(String sourceUnit, String targetUnit) {
     UnitConverter converter = UnitConverterFactory.getConverter(sourceUnit, targetUnit);
     assertNull(converter);
   }
 
-  private static Stream<Arguments> provideUnitsForMissingConverter() {
-    return Stream.of(
-        Arguments.of(null, null),
-        Arguments.of("ms", null),
-        Arguments.of("ms", ""),
-        Arguments.of("ms", "--"),
-        Arguments.of("--", "--"));
+  @ParameterizedTest
+  @CsvSource({
+    "'', By", "By, ''",
+  })
+  void shouldThrowExceptionWhenRegisteringConverterWithAnyUnitEmpty(
+      String sourceUnit, String targetUnit) {
+    IllegalArgumentException exception =
+        assertThrows(
+            IllegalArgumentException.class,
+            () ->
+                UnitConverterFactory.registerConverter(
+                    sourceUnit, targetUnit, (value) -> 0, false));
+    assertThat(exception.getMessage()).matches("Non empty .+Unit must be provided");
+  }
+
+  @Test
+  void shouldNotAllowRegisteringAgainAlreadyExistingConverter() {
+    IllegalArgumentException exception =
+        assertThrows(
+            IllegalArgumentException.class,
+            () -> UnitConverterFactory.registerConverter("ms", "s", (v) -> 0, false));
+    assertEquals("Converter from [ms] to [s] already registered", exception.getMessage());
   }
 }