Implement "Prometheus mode" for better micrometer->OTel->Prometheus support (#4274)

* Implement "Prometheus mode" for better micrometer->OTel->Prometheus support

* code review comments

micrometer1-shim/src/main/java/io/opentelemetry/micrometer1shim/OpenTelemetryDistributionSummary.java

@@ -11,7 +11,6 @@ import static io.opentelemetry.micrometer1shim.Bridging.tagsAsAttributes;
 
 import io.micrometer.core.instrument.AbstractDistributionSummary;
 import io.micrometer.core.instrument.Clock;
-import io.micrometer.core.instrument.DistributionSummary;
 import io.micrometer.core.instrument.Measurement;
 import io.micrometer.core.instrument.config.NamingConvention;
 import io.micrometer.core.instrument.distribution.DistributionStatisticConfig;
@@ -26,7 +25,7 @@ import java.util.concurrent.atomic.DoubleAdder;
 import java.util.concurrent.atomic.LongAdder;
 
 final class OpenTelemetryDistributionSummary extends AbstractDistributionSummary
-    implements DistributionSummary, RemovableMeter {
+    implements RemovableMeter {
 
   private final Measurements measurements;
   private final TimeWindowMax max;
@@ -77,7 +76,7 @@ final class OpenTelemetryDistributionSummary extends AbstractDistributionSummary
 
   @Override
   protected void recordNonNegative(double amount) {
-    if (amount >= 0 && !removed) {
+    if (!removed) {
       otelHistogram.record(amount, attributes);
       measurements.record(amount);
       max.record(amount);

micrometer1-shim/src/main/java/io/opentelemetry/micrometer1shim/OpenTelemetryMeterRegistry.java

@@ -53,13 +53,16 @@ public final class OpenTelemetryMeterRegistry extends MeterRegistry {
   private final io.opentelemetry.api.metrics.Meter otelMeter;
 
   OpenTelemetryMeterRegistry(
-      Clock clock, TimeUnit baseTimeUnit, io.opentelemetry.api.metrics.Meter otelMeter) {
+      Clock clock,
+      TimeUnit baseTimeUnit,
+      NamingConvention namingConvention,
+      io.opentelemetry.api.metrics.Meter otelMeter) {
     super(clock);
     this.baseTimeUnit = baseTimeUnit;
     this.otelMeter = otelMeter;
 
     this.config()
-        .namingConvention(NamingConvention.identity)
+        .namingConvention(namingConvention)
         .onMeterRemoved(OpenTelemetryMeterRegistry::onMeterRemoved);
   }
 

micrometer1-shim/src/main/java/io/opentelemetry/micrometer1shim/OpenTelemetryMeterRegistryBuilder.java

@@ -7,6 +7,7 @@ package io.opentelemetry.micrometer1shim;
 
 import io.micrometer.core.instrument.Clock;
 import io.micrometer.core.instrument.MeterRegistry;
+import io.micrometer.core.instrument.config.NamingConvention;
 import io.opentelemetry.api.OpenTelemetry;
 import java.util.concurrent.TimeUnit;
 
@@ -19,6 +20,7 @@ public final class OpenTelemetryMeterRegistryBuilder {
   private final OpenTelemetry openTelemetry;
   private Clock clock = Clock.SYSTEM;
   private TimeUnit baseTimeUnit = TimeUnit.MILLISECONDS;
+  private boolean prometheusMode = false;
 
   OpenTelemetryMeterRegistryBuilder(OpenTelemetry openTelemetry) {
     this.openTelemetry = openTelemetry;
@@ -36,12 +38,32 @@ public final class OpenTelemetryMeterRegistryBuilder {
     return this;
   }
 
+  /**
+   * Enables the "Prometheus mode" - this will simulate the behavior of Micrometer's {@code
+   * PrometheusMeterRegistry}. The instruments will be renamed to match Micrometer instrument
+   * naming, and the base time unit will be set to seconds.
+   *
+   * <p>Set this to {@code true} if you are using the Prometheus metrics exporter.
+   */
+  public OpenTelemetryMeterRegistryBuilder setPrometheusMode(boolean prometheusMode) {
+    this.prometheusMode = prometheusMode;
+    return this;
+  }
+
   /**
    * Returns a new {@link OpenTelemetryMeterRegistry} with the settings of this {@link
    * OpenTelemetryMeterRegistryBuilder}.
    */
   public MeterRegistry build() {
+    // prometheus mode overrides any unit settings with SECONDS
+    TimeUnit baseTimeUnit = prometheusMode ? TimeUnit.SECONDS : this.baseTimeUnit;
+    NamingConvention namingConvention =
+        prometheusMode ? PrometheusModeNamingConvention.INSTANCE : NamingConvention.identity;
+
     return new OpenTelemetryMeterRegistry(
-        clock, baseTimeUnit, openTelemetry.getMeterProvider().get(INSTRUMENTATION_NAME));
+        clock,
+        baseTimeUnit,
+        namingConvention,
+        openTelemetry.getMeterProvider().get(INSTRUMENTATION_NAME));
   }
 }

micrometer1-shim/src/main/java/io/opentelemetry/micrometer1shim/OpenTelemetryTimer.java

@@ -79,7 +79,7 @@ final class OpenTelemetryTimer extends AbstractTimer implements RemovableMeter {
 
   @Override
   protected void recordNonNegative(long amount, TimeUnit unit) {
-    if (amount >= 0 && !removed) {
+    if (!removed) {
       long nanos = unit.toNanos(amount);
       double time = TimeUtils.nanosToUnit(nanos, baseTimeUnit);
       otelHistogram.record(time, attributes);

micrometer1-shim/src/main/java/io/opentelemetry/micrometer1shim/PrometheusModeNamingConvention.java

@@ -0,0 +1,36 @@
+/*
+ * Copyright The OpenTelemetry Authors
+ * SPDX-License-Identifier: Apache-2.0
+ */
+
+package io.opentelemetry.micrometer1shim;
+
+import io.micrometer.core.instrument.Meter;
+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;
+
+  @Override
+  public String name(String name, Meter.Type type, @Nullable String baseUnit) {
+    if (type == Meter.Type.COUNTER
+        || type == Meter.Type.DISTRIBUTION_SUMMARY
+        || type == Meter.Type.GAUGE) {
+      if (baseUnit != null && !name.endsWith("." + baseUnit)) {
+        name = name + "." + baseUnit;
+      }
+    }
+
+    if (type == Meter.Type.LONG_TASK_TIMER || type == Meter.Type.TIMER) {
+      if (!name.endsWith(".seconds")) {
+        name = name + ".seconds";
+      }
+    }
+
+    return name;
+  }
+}

micrometer1-shim/src/test/java/io/opentelemetry/micrometer1shim/PrometheusModeTest.java

@@ -0,0 +1,341 @@
+/*
+ * Copyright The OpenTelemetry Authors
+ * SPDX-License-Identifier: Apache-2.0
+ */
+
+package io.opentelemetry.micrometer1shim;
+
+import static io.opentelemetry.micrometer1shim.OpenTelemetryMeterRegistryBuilder.INSTRUMENTATION_NAME;
+import static io.opentelemetry.sdk.testing.assertj.MetricAssertions.assertThat;
+import static io.opentelemetry.sdk.testing.assertj.OpenTelemetryAssertions.attributeEntry;
+
+import io.micrometer.core.instrument.Counter;
+import io.micrometer.core.instrument.DistributionSummary;
+import io.micrometer.core.instrument.FunctionTimer;
+import io.micrometer.core.instrument.Gauge;
+import io.micrometer.core.instrument.LongTaskTimer;
+import io.micrometer.core.instrument.Metrics;
+import io.micrometer.core.instrument.Timer;
+import io.opentelemetry.sdk.common.InstrumentationScopeInfo;
+import java.util.concurrent.TimeUnit;
+import org.junit.jupiter.api.BeforeEach;
+import org.junit.jupiter.api.Test;
+import org.junit.jupiter.api.extension.RegisterExtension;
+
+@SuppressWarnings("PreferJavaTimeOverload")
+class PrometheusModeTest {
+
+  @RegisterExtension
+  static final MicrometerTestingExtension testing =
+      new MicrometerTestingExtension() {
+
+        @Override
+        OpenTelemetryMeterRegistryBuilder configureOtelRegistry(
+            OpenTelemetryMeterRegistryBuilder registry) {
+          return registry.setPrometheusMode(true);
+        }
+      };
+
+  final TestTimer timerObj = new TestTimer();
+
+  @BeforeEach
+  void cleanupMeters() {
+    Metrics.globalRegistry.forEachMeter(Metrics.globalRegistry::remove);
+  }
+
+  @Test
+  void testCounter() {
+    // given
+    Counter counter =
+        Counter.builder("testPrometheusCounter")
+            .description("This is a test counter")
+            .tags("tag", "value")
+            .baseUnit("items")
+            .register(Metrics.globalRegistry);
+
+    // when
+    counter.increment(12);
+
+    // then
+    assertThat(testing.collectAllMetrics())
+        .satisfiesExactlyInAnyOrder(
+            metric ->
+                assertThat(metric)
+                    .hasName("testPrometheusCounter.items")
+                    .hasInstrumentationScope(
+                        InstrumentationScopeInfo.create(INSTRUMENTATION_NAME, null, null))
+                    .hasDescription("This is a test counter")
+                    .hasUnit("items")
+                    .hasDoubleSum()
+                    .isMonotonic()
+                    .points()
+                    .satisfiesExactly(
+                        point ->
+                            assertThat(point)
+                                .hasValue(12)
+                                .attributes()
+                                .containsOnly(attributeEntry("tag", "value"))));
+  }
+
+  @Test
+  void testDistributionSummary() {
+    // given
+    DistributionSummary summary =
+        DistributionSummary.builder("testPrometheusSummary")
+            .description("This is a test summary")
+            .baseUnit("items")
+            .tag("tag", "value")
+            .register(Metrics.globalRegistry);
+
+    // when
+    summary.record(12);
+    summary.record(42);
+
+    // then
+    assertThat(testing.collectAllMetrics())
+        .satisfiesExactlyInAnyOrder(
+            metric ->
+                assertThat(metric)
+                    .hasName("testPrometheusSummary.items")
+                    .hasInstrumentationScope(
+                        InstrumentationScopeInfo.create(INSTRUMENTATION_NAME, null, null))
+                    .hasDescription("This is a test summary")
+                    .hasUnit("items")
+                    .hasDoubleHistogram()
+                    .points()
+                    .satisfiesExactly(
+                        point ->
+                            assertThat(point)
+                                .hasSum(54)
+                                .hasCount(2)
+                                .attributes()
+                                .containsOnly(attributeEntry("tag", "value"))),
+            metric ->
+                assertThat(metric)
+                    .hasName("testPrometheusSummary.items.max")
+                    .hasInstrumentationScope(
+                        InstrumentationScopeInfo.create(INSTRUMENTATION_NAME, null, null))
+                    .hasDescription("This is a test summary")
+                    .hasUnit("items")
+                    .hasDoubleGauge()
+                    .points()
+                    .satisfiesExactly(
+                        point ->
+                            assertThat(point)
+                                .hasValue(42)
+                                .attributes()
+                                .containsOnly(attributeEntry("tag", "value"))));
+  }
+
+  @Test
+  void testFunctionTimer() {
+    // given
+    FunctionTimer.builder(
+            "testPrometheusFunctionTimer",
+            timerObj,
+            TestTimer::getCount,
+            TestTimer::getTotalTimeNanos,
+            TimeUnit.NANOSECONDS)
+        .description("This is a test function timer")
+        .tags("tag", "value")
+        .register(Metrics.globalRegistry);
+
+    // when
+    timerObj.add(42, TimeUnit.SECONDS);
+
+    // then
+    assertThat(testing.collectAllMetrics())
+        .satisfiesExactlyInAnyOrder(
+            metric ->
+                assertThat(metric)
+                    .hasName("testPrometheusFunctionTimer.seconds.count")
+                    .hasInstrumentationScope(
+                        InstrumentationScopeInfo.create(INSTRUMENTATION_NAME, null, null))
+                    .hasDescription("This is a test function timer")
+                    .hasUnit("1")
+                    .hasLongSum()
+                    .isMonotonic()
+                    .points()
+                    .satisfiesExactly(
+                        point ->
+                            assertThat(point)
+                                .hasValue(1)
+                                .attributes()
+                                .containsOnly(attributeEntry("tag", "value"))),
+            metric ->
+                assertThat(metric)
+                    .hasName("testPrometheusFunctionTimer.seconds.sum")
+                    .hasInstrumentationScope(
+                        InstrumentationScopeInfo.create(INSTRUMENTATION_NAME, null, null))
+                    .hasDescription("This is a test function timer")
+                    .hasUnit("s")
+                    .hasDoubleSum()
+                    .points()
+                    .satisfiesExactly(
+                        point ->
+                            assertThat(point)
+                                .hasValue(42)
+                                .attributes()
+                                .containsOnly(attributeEntry("tag", "value"))));
+  }
+
+  @Test
+  void testGauge() {
+    // when
+    Gauge.builder("testPrometheusGauge", () -> 42)
+        .description("This is a test gauge")
+        .tags("tag", "value")
+        .baseUnit("items")
+        .register(Metrics.globalRegistry);
+
+    // then
+    assertThat(testing.collectAllMetrics())
+        .satisfiesExactlyInAnyOrder(
+            metric ->
+                assertThat(metric)
+                    .hasName("testPrometheusGauge.items")
+                    .hasInstrumentationScope(
+                        InstrumentationScopeInfo.create(INSTRUMENTATION_NAME, null, null))
+                    .hasDescription("This is a test gauge")
+                    .hasUnit("items")
+                    .hasDoubleGauge()
+                    .points()
+                    .satisfiesExactly(
+                        point ->
+                            assertThat(point)
+                                .hasValue(42)
+                                .attributes()
+                                .containsOnly(attributeEntry("tag", "value"))));
+  }
+
+  @Test
+  void testLongTaskTimer() throws InterruptedException {
+    // given
+    LongTaskTimer timer =
+        LongTaskTimer.builder("testPrometheusLongTaskTimer")
+            .description("This is a test long task timer")
+            .tags("tag", "value")
+            .register(Metrics.globalRegistry);
+
+    // when
+    LongTaskTimer.Sample sample = timer.start();
+
+    // then
+    assertThat(testing.collectAllMetrics())
+        .satisfiesExactlyInAnyOrder(
+            metric ->
+                assertThat(metric)
+                    .hasName("testPrometheusLongTaskTimer.seconds.active")
+                    .hasInstrumentationScope(
+                        InstrumentationScopeInfo.create(INSTRUMENTATION_NAME, null, null))
+                    .hasDescription("This is a test long task timer")
+                    .hasUnit("tasks")
+                    .hasLongSum()
+                    .isNotMonotonic()
+                    .points()
+                    .satisfiesExactly(
+                        point ->
+                            assertThat(point)
+                                .hasValue(1)
+                                .attributes()
+                                .containsOnly(attributeEntry("tag", "value"))),
+            metric ->
+                assertThat(metric)
+                    .hasName("testPrometheusLongTaskTimer.seconds.duration")
+                    .hasInstrumentationScope(
+                        InstrumentationScopeInfo.create(INSTRUMENTATION_NAME, null, null))
+                    .hasDescription("This is a test long task timer")
+                    .hasUnit("s")
+                    .hasDoubleSum()
+                    .isNotMonotonic()
+                    .points()
+                    .satisfiesExactly(
+                        point -> {
+                          assertThat(point)
+                              .attributes()
+                              .containsOnly(attributeEntry("tag", "value"));
+                          // any value >0 - duration of currently running tasks
+                          assertThat(point.getValue()).isPositive();
+                        }));
+
+    // when
+    TimeUnit.MILLISECONDS.sleep(100);
+    sample.stop();
+
+    // then
+    assertThat(testing.collectAllMetrics())
+        .satisfiesExactlyInAnyOrder(
+            metric ->
+                assertThat(metric)
+                    .hasName("testPrometheusLongTaskTimer.seconds.active")
+                    .hasLongSum()
+                    .points()
+                    .satisfiesExactly(
+                        point ->
+                            assertThat(point)
+                                .hasValue(0)
+                                .attributes()
+                                .containsOnly(attributeEntry("tag", "value"))),
+            metric ->
+                assertThat(metric)
+                    .hasName("testPrometheusLongTaskTimer.seconds.duration")
+                    .hasDoubleSum()
+                    .points()
+                    .satisfiesExactly(
+                        point ->
+                            assertThat(point)
+                                .hasValue(0)
+                                .attributes()
+                                .containsOnly(attributeEntry("tag", "value"))));
+  }
+
+  @Test
+  void testTimer() {
+    // given
+    Timer timer =
+        Timer.builder("testPrometheusTimer")
+            .description("This is a test timer")
+            .tags("tag", "value")
+            .register(Metrics.globalRegistry);
+
+    // when
+    timer.record(1, TimeUnit.SECONDS);
+    timer.record(5, TimeUnit.SECONDS);
+    timer.record(10_789, TimeUnit.MILLISECONDS);
+
+    // then
+    assertThat(testing.collectAllMetrics())
+        .satisfiesExactlyInAnyOrder(
+            metric ->
+                assertThat(metric)
+                    .hasName("testPrometheusTimer.seconds")
+                    .hasInstrumentationScope(
+                        InstrumentationScopeInfo.create(INSTRUMENTATION_NAME, null, null))
+                    .hasDescription("This is a test timer")
+                    .hasUnit("s")
+                    .hasDoubleHistogram()
+                    .points()
+                    .satisfiesExactly(
+                        point ->
+                            assertThat(point)
+                                .hasSum(16.789)
+                                .hasCount(3)
+                                .attributes()
+                                .containsOnly(attributeEntry("tag", "value"))),
+            metric ->
+                assertThat(metric)
+                    .hasName("testPrometheusTimer.seconds.max")
+                    .hasInstrumentationScope(
+                        InstrumentationScopeInfo.create(INSTRUMENTATION_NAME, null, null))
+                    .hasDescription("This is a test timer")
+                    .hasUnit("s")
+                    .hasDoubleGauge()
+                    .points()
+                    .satisfiesExactly(
+                        point ->
+                            assertThat(point)
+                                .hasValue(10.789)
+                                .attributes()
+                                .containsOnly(attributeEntry("tag", "value"))));
+  }
+}