code review comments

open-telemetry · Mar 18, 2022 · 0bfe5bc · 0bfe5bc
1 parent 0c393df
commit 0bfe5bc
Show file tree

Hide file tree

Showing 2 changed files with 19 additions and 15 deletions.
diff --git a/...a/io/opentelemetry/instrumentation/micrometer/v1_5/OpenTelemetryMeterRegistryBuilder.java b/...a/io/opentelemetry/instrumentation/micrometer/v1_5/OpenTelemetryMeterRegistryBuilder.java
@@ -10,6 +10,7 @@
 import io.opentelemetry.api.OpenTelemetry;
 import io.opentelemetry.instrumentation.api.config.Config;
 import java.util.concurrent.TimeUnit;
+import javax.annotation.Nullable;
 
 /** A builder of {@link OpenTelemetryMeterRegistry}. */
 public final class OpenTelemetryMeterRegistryBuilder {
@@ -18,23 +19,12 @@ public final class OpenTelemetryMeterRegistryBuilder {
 
   private final OpenTelemetry openTelemetry;
   private Clock clock = Clock.SYSTEM;
-  private TimeUnit baseTimeUnit;
-  private boolean prometheusMode;
+  @Nullable private TimeUnit baseTimeUnit = null;
+  private boolean prometheusMode =
+      Config.get().getBoolean("otel.instrumentation.micrometer.prometheus-mode.enabled", false);
 
   OpenTelemetryMeterRegistryBuilder(OpenTelemetry openTelemetry) {
     this.openTelemetry = openTelemetry;
-
-    Config config = Config.get();
-
-    prometheusMode =
-        config.getBoolean("otel.instrumentation.micrometer.prometheus-mode.enabled", false);
-
-    // use seconds as the default unit if prometheus mode is enabled
-    TimeUnit defaultBaseTimeUnit = prometheusMode ? TimeUnit.SECONDS : TimeUnit.MILLISECONDS;
-    baseTimeUnit =
-        TimeUnitHelper.parseConfigValue(
-            config.getString("otel.instrumentation.micrometer.base-time-unit"),
-            defaultBaseTimeUnit);
   }
 
   /** Sets a custom {@link Clock}. Useful for testing. */
@@ -58,14 +48,25 @@ public OpenTelemetryMeterRegistryBuilder setBaseTimeUnit(TimeUnit baseTimeUnit)
    */
   public OpenTelemetryMeterRegistryBuilder setPrometheusMode(boolean prometheusMode) {
     this.prometheusMode = prometheusMode;
-    return setBaseTimeUnit(TimeUnit.SECONDS);
+    return this;
   }
 
   /**
    * Returns a new {@link OpenTelemetryMeterRegistry} with the settings of this {@link
    * OpenTelemetryMeterRegistryBuilder}.
    */
   public MeterRegistry build() {
+    if (prometheusMode) {
+      // prometheus mode overrides any unit settings with SECONDS
+      setBaseTimeUnit(TimeUnit.SECONDS);
+    } else if (baseTimeUnit == null) {
+      // if the unit was not manually set, try to initialize it using config
+      setBaseTimeUnit(
+          TimeUnitHelper.parseConfigValue(
+              Config.get().getString("otel.instrumentation.micrometer.base-time-unit"),
+              TimeUnit.MILLISECONDS));
+    }
+
     return new OpenTelemetryMeterRegistry(
         clock,
         baseTimeUnit,

diff --git a/...java/io/opentelemetry/instrumentation/micrometer/v1_5/PrometheusModeNamingConvention.java b/...java/io/opentelemetry/instrumentation/micrometer/v1_5/PrometheusModeNamingConvention.java
@@ -9,6 +9,9 @@
 import io.micrometer.core.instrument.config.NamingConvention;
 import javax.annotation.Nullable;
 
+// This naming strategy does not replace '.' with '_', and it does not append '_total' to counter
+// names - the reason behind it is that this is already done by the Prometheus exporter; see the
+// io.opentelemetry.exporter.prometheus.MetricAdapter class
 enum PrometheusModeNamingConvention implements NamingConvention {
   INSTANCE;