Restore prometheus metric name mapper tests, fix regressions

exporters/prometheus/src/main/java/io/opentelemetry/exporter/prometheus/Otel2PrometheusConverter.java

@@ -183,8 +183,7 @@ private CounterSnapshot convertLongCounter(
       MetricMetadata metadata,
       InstrumentationScopeInfo scope,
       Collection<LongPointData> dataPoints) {
-    List<CounterDataPointSnapshot> data =
-        new ArrayList<CounterDataPointSnapshot>(dataPoints.size());
+    List<CounterDataPointSnapshot> data = new ArrayList<>(dataPoints.size());
     for (LongPointData longData : dataPoints) {
       data.add(
           new CounterDataPointSnapshot(
@@ -449,8 +448,14 @@ private static MetricMetadata convertMetadata(MetricData metricData) {
     String help = metricData.getDescription();
     Unit unit = PrometheusUnitsHelper.convertUnit(metricData.getUnit());
     if (unit != null && !name.endsWith(unit.toString())) {
-      name += "_" + unit;
+      // Need to re-sanitize metric name since unit may contain illegal characters
+      name = sanitizeMetricName(name + "_" + unit);
+    }
+    // Repeated __ are not allowed according to spec, although this is allowed in prometheus
+    while (name.contains("__")) {
+      name = name.replace("__", "_");
     }
+
     return new MetricMetadata(name, help, unit);
   }
 
@@ -538,7 +543,8 @@ private static MetricMetadata mergeMetadata(MetricMetadata a, MetricMetadata b)
           "Conflicting metrics: Multiple metrics with name "
               + name
               + " but different units found. Dropping the one with unit "
-              + b.getUnit());
+              + b.getUnit()
+              + ".");
       return null;
     }
     return new MetricMetadata(name, help, unit);

exporters/prometheus/src/main/java/io/opentelemetry/exporter/prometheus/PrometheusUnitsHelper.java

@@ -69,9 +69,7 @@ private static void initUnit(String otelName, String pluralName, String singular
 
   @Nullable
   static Unit convertUnit(String otelUnit) {
-    if (otelUnit.isEmpty() || otelUnit.equals("1")) {
-      // The spec says "1" should be translated to "ratio", but this is not implemented in the Java
-      // SDK.
+    if (otelUnit.isEmpty()) {
       return null;
     }
     if (otelUnit.contains("{")) {

exporters/prometheus/src/test/java/io/opentelemetry/exporter/prometheus/Otel2PrometheusConverterTest.java

@@ -0,0 +1,224 @@
+/*
+ * Copyright The OpenTelemetry Authors
+ * SPDX-License-Identifier: Apache-2.0
+ */
+
+package io.opentelemetry.exporter.prometheus;
+
+import static org.assertj.core.api.Assertions.assertThat;
+
+import io.opentelemetry.api.common.Attributes;
+import io.opentelemetry.sdk.common.InstrumentationScopeInfo;
+import io.opentelemetry.sdk.metrics.data.AggregationTemporality;
+import io.opentelemetry.sdk.metrics.data.MetricData;
+import io.opentelemetry.sdk.metrics.data.MetricDataType;
+import io.opentelemetry.sdk.metrics.internal.data.ImmutableGaugeData;
+import io.opentelemetry.sdk.metrics.internal.data.ImmutableHistogramData;
+import io.opentelemetry.sdk.metrics.internal.data.ImmutableHistogramPointData;
+import io.opentelemetry.sdk.metrics.internal.data.ImmutableLongPointData;
+import io.opentelemetry.sdk.metrics.internal.data.ImmutableMetricData;
+import io.opentelemetry.sdk.metrics.internal.data.ImmutableSumData;
+import io.opentelemetry.sdk.metrics.internal.data.ImmutableSummaryData;
+import io.opentelemetry.sdk.metrics.internal.data.ImmutableSummaryPointData;
+import io.opentelemetry.sdk.resources.Resource;
+import io.prometheus.metrics.expositionformats.ExpositionFormats;
+import io.prometheus.metrics.model.snapshots.MetricSnapshots;
+import java.io.ByteArrayOutputStream;
+import java.io.IOException;
+import java.nio.charset.StandardCharsets;
+import java.util.Arrays;
+import java.util.Collections;
+import java.util.regex.Matcher;
+import java.util.regex.Pattern;
+import java.util.stream.Stream;
+import org.junit.jupiter.params.ParameterizedTest;
+import org.junit.jupiter.params.provider.Arguments;
+import org.junit.jupiter.params.provider.MethodSource;
+
+class Otel2PrometheusConverterTest {
+
+  private static final Pattern PATTERN =
+      Pattern.compile(
+          "# HELP (?<help>.*)\n# TYPE (?<type>.*)\n(?<metricName>.*)\\{otel_scope_name=\"scope\"}(.|\\n)*");
+
+  private final Otel2PrometheusConverter converter = new Otel2PrometheusConverter(true);
+
+  @ParameterizedTest
+  @MethodSource("metricMetadataArgs")
+  void metricMetadata(
+      MetricData metricData, String expectedType, String expectedHelp, String expectedMetricName)
+      throws IOException {
+    ByteArrayOutputStream baos = new ByteArrayOutputStream();
+    MetricSnapshots snapshots = converter.convert(Collections.singletonList(metricData));
+    ExpositionFormats.init().getPrometheusTextFormatWriter().write(baos, snapshots);
+    String expositionFormat = new String(baos.toByteArray(), StandardCharsets.UTF_8);
+
+    // Uncomment to debug exposition format output
+    // System.out.println(expositionFormat);
+
+    Matcher matcher = PATTERN.matcher(expositionFormat);
+    assertThat(matcher.matches()).isTrue();
+    assertThat(matcher.group("help")).isEqualTo(expectedHelp);
+    assertThat(matcher.group("type")).isEqualTo(expectedType);
+    // Note: Summaries and histograms produce output which matches METRIC_NAME_PATTERN multiple
+    // times. The pattern ends up matching against the first.
+    assertThat(matcher.group("metricName")).isEqualTo(expectedMetricName);
+  }
+
+  private static Stream<Arguments> metricMetadataArgs() {
+    return Stream.of(
+        // the unity unit "1" is translated to "ratio"
+        Arguments.of(
+            createSampleMetricData("sample", "1", MetricDataType.LONG_GAUGE),
+            "sample_ratio gauge",
+            "sample_ratio description",
+            "sample_ratio"),
+        // unit is appended to metric name
+        Arguments.of(
+            createSampleMetricData("sample", "unit", MetricDataType.LONG_GAUGE),
+            "sample_unit gauge",
+            "sample_unit description",
+            "sample_unit"),
+        // units in curly braces are dropped
+        Arguments.of(
+            createSampleMetricData("sample", "1{dropped}", MetricDataType.LONG_GAUGE),
+            "sample_ratio gauge",
+            "sample_ratio description",
+            "sample_ratio"),
+        // monotonic sums always include _total suffix
+        Arguments.of(
+            createSampleMetricData("sample", "unit", MetricDataType.LONG_SUM),
+            "sample_unit_total counter",
+            "sample_unit_total description",
+            "sample_unit_total"),
+        Arguments.of(
+            createSampleMetricData("sample", "1", MetricDataType.LONG_SUM),
+            "sample_ratio_total counter",
+            "sample_ratio_total description",
+            "sample_ratio_total"),
+        // units expressed as numbers other than 1 are retained
+        Arguments.of(
+            createSampleMetricData("sample", "2", MetricDataType.LONG_SUM),
+            "sample_2_total counter",
+            "sample_2_total description",
+            "sample_2_total"),
+        Arguments.of(
+            createSampleMetricData("metric_name", "2", MetricDataType.SUMMARY),
+            "metric_name_2 summary",
+            "metric_name_2 description",
+            "metric_name_2_count"),
+        // unsupported characters are translated to "_", repeated "_" are dropped
+        Arguments.of(
+            createSampleMetricData("s%%ple", "%/min", MetricDataType.SUMMARY),
+            "s_ple_percent_per_minute summary",
+            "s_ple_percent_per_minute description",
+            "s_ple_percent_per_minute_count"),
+        // metric unit is not appended if the name already contains the unit
+        Arguments.of(
+            createSampleMetricData("metric_name_total", "total", MetricDataType.LONG_SUM),
+            "metric_name_total counter",
+            "metric_name_total description",
+            "metric_name_total"),
+        // total suffix is stripped because total is a reserved suffixed for monotonic sums
+        Arguments.of(
+            createSampleMetricData("metric_name_total", "total", MetricDataType.SUMMARY),
+            "metric_name summary",
+            "metric_name description",
+            "metric_name_count"),
+        // if metric name ends with unit the unit is omitted
+        Arguments.of(
+            createSampleMetricData("metric_name_ratio", "1", MetricDataType.LONG_GAUGE),
+            "metric_name_ratio gauge",
+            "metric_name_ratio description",
+            "metric_name_ratio"),
+        Arguments.of(
+            createSampleMetricData("metric_name_ratio", "1", MetricDataType.SUMMARY),
+            "metric_name_ratio summary",
+            "metric_name_ratio description",
+            "metric_name_ratio_count"),
+        Arguments.of(
+            createSampleMetricData("metric_hertz", "hertz", MetricDataType.LONG_GAUGE),
+            "metric_hertz gauge",
+            "metric_hertz description",
+            "metric_hertz"),
+        Arguments.of(
+            createSampleMetricData("metric_hertz", "hertz", MetricDataType.LONG_SUM),
+            "metric_hertz_total counter",
+            "metric_hertz_total description",
+            "metric_hertz_total"),
+        // if metric name ends with unit the unit is omitted - order matters
+        Arguments.of(
+            createSampleMetricData("metric_total_hertz", "hertz_total", MetricDataType.LONG_SUM),
+            "metric_total_hertz_hertz_total counter",
+            "metric_total_hertz_hertz_total description",
+            "metric_total_hertz_hertz_total"),
+        // metric name cannot start with a number
+        Arguments.of(
+            createSampleMetricData("2_metric_name", "By", MetricDataType.SUMMARY),
+            "_metric_name_bytes summary",
+            "_metric_name_bytes description",
+            "_metric_name_bytes_count"));
+  }
+
+  static MetricData createSampleMetricData(
+      String metricName, String metricUnit, MetricDataType metricDataType) {
+    switch (metricDataType) {
+      case SUMMARY:
+        return ImmutableMetricData.createDoubleSummary(
+            Resource.getDefault(),
+            InstrumentationScopeInfo.create("scope"),
+            metricName,
+            "description",
+            metricUnit,
+            ImmutableSummaryData.create(
+                Collections.singletonList(
+                    ImmutableSummaryPointData.create(
+                        0, 1, Attributes.empty(), 1, 1, Collections.emptyList()))));
+      case LONG_SUM:
+        return ImmutableMetricData.createLongSum(
+            Resource.getDefault(),
+            InstrumentationScopeInfo.create("scope"),
+            metricName,
+            "description",
+            metricUnit,
+            ImmutableSumData.create(
+                true,
+                AggregationTemporality.CUMULATIVE,
+                Collections.singletonList(
+                    ImmutableLongPointData.create(0, 1, Attributes.empty(), 1L))));
+      case LONG_GAUGE:
+        return ImmutableMetricData.createLongGauge(
+            Resource.getDefault(),
+            InstrumentationScopeInfo.create("scope"),
+            metricName,
+            "description",
+            metricUnit,
+            ImmutableGaugeData.create(
+                Collections.singletonList(
+                    ImmutableLongPointData.create(0, 1, Attributes.empty(), 1L))));
+      case HISTOGRAM:
+        return ImmutableMetricData.createDoubleHistogram(
+            Resource.getDefault(),
+            InstrumentationScopeInfo.create("scope"),
+            metricName,
+            "description",
+            metricUnit,
+            ImmutableHistogramData.create(
+                AggregationTemporality.CUMULATIVE,
+                Collections.singletonList(
+                    ImmutableHistogramPointData.create(
+                        0,
+                        1,
+                        Attributes.empty(),
+                        1,
+                        false,
+                        -1,
+                        false,
+                        -1,
+                        Collections.singletonList(1.0),
+                        Arrays.asList(0L, 1L)))));
+      default:
+        throw new IllegalArgumentException("Unsupported metric data type: " + metricDataType);
+    }
+  }
+}

exporters/prometheus/src/test/java/io/opentelemetry/exporter/prometheus/PrometheusHttpServerTest.java

@@ -115,12 +115,12 @@ void fetchPrometheus() {
         .isEqualTo("text/plain; version=0.0.4; charset=utf-8");
     assertThat(response.contentUtf8())
         .isEqualTo(
-            "# HELP grpc_name_total long_description\n"
-                + "# TYPE grpc_name_total counter\n"
-                + "grpc_name_total{kp=\"vp\",otel_scope_name=\"grpc\",otel_scope_version=\"version\"} 5.0\n"
-                + "# HELP http_name_total double_description\n"
-                + "# TYPE http_name_total counter\n"
-                + "http_name_total{kp=\"vp\",otel_scope_name=\"http\",otel_scope_version=\"version\"} 3.5\n"
+            "# HELP grpc_name_unit_total long_description\n"
+                + "# TYPE grpc_name_unit_total counter\n"
+                + "grpc_name_unit_total{kp=\"vp\",otel_scope_name=\"grpc\",otel_scope_version=\"version\"} 5.0\n"
+                + "# HELP http_name_unit_total double_description\n"
+                + "# TYPE http_name_unit_total counter\n"
+                + "http_name_unit_total{kp=\"vp\",otel_scope_name=\"http\",otel_scope_version=\"version\"} 3.5\n"
                 + "# TYPE target_info gauge\n"
                 + "target_info{kr=\"vr\"} 1\n");
   }
@@ -142,12 +142,14 @@ void fetchOpenMetrics() {
         .isEqualTo("application/openmetrics-text; version=1.0.0; charset=utf-8");
     assertThat(response.contentUtf8())
         .isEqualTo(
-            "# TYPE grpc_name counter\n"
-                + "# HELP grpc_name long_description\n"
-                + "grpc_name_total{kp=\"vp\",otel_scope_name=\"grpc\",otel_scope_version=\"version\"} 5.0\n"
-                + "# TYPE http_name counter\n"
-                + "# HELP http_name double_description\n"
-                + "http_name_total{kp=\"vp\",otel_scope_name=\"http\",otel_scope_version=\"version\"} 3.5\n"
+            "# TYPE grpc_name_unit counter\n"
+                + "# UNIT grpc_name_unit unit\n"
+                + "# HELP grpc_name_unit long_description\n"
+                + "grpc_name_unit_total{kp=\"vp\",otel_scope_name=\"grpc\",otel_scope_version=\"version\"} 5.0\n"
+                + "# TYPE http_name_unit counter\n"
+                + "# UNIT http_name_unit unit\n"
+                + "# HELP http_name_unit double_description\n"
+                + "http_name_unit_total{kp=\"vp\",otel_scope_name=\"http\",otel_scope_version=\"version\"} 3.5\n"
                 + "# TYPE target info\n"
                 + "target_info{kr=\"vr\"} 1\n"
                 + "# EOF\n");
@@ -157,7 +159,7 @@ void fetchOpenMetrics() {
   void fetchFiltered() {
     AggregatedHttpResponse response =
         client
-            .get("/?name[]=grpc_name_total&name[]=bears_total&name[]=target_info")
+            .get("/?name[]=grpc_name_unit_total&name[]=bears_total&name[]=target_info")
             .aggregate()
             .join();
     assertThat(response.status()).isEqualTo(HttpStatus.OK);
@@ -166,9 +168,9 @@ void fetchFiltered() {
     assertThat(response.contentUtf8())
         .isEqualTo(
             ""
-                + "# HELP grpc_name_total long_description\n"
-                + "# TYPE grpc_name_total counter\n"
-                + "grpc_name_total{kp=\"vp\",otel_scope_name=\"grpc\",otel_scope_version=\"version\"} 5.0\n"
+                + "# HELP grpc_name_unit_total long_description\n"
+                + "# TYPE grpc_name_unit_total counter\n"
+                + "grpc_name_unit_total{kp=\"vp\",otel_scope_name=\"grpc\",otel_scope_version=\"version\"} 5.0\n"
                 + "# TYPE target_info gauge\n"
                 + "target_info{kr=\"vr\"} 1\n");
   }
@@ -189,12 +191,12 @@ void fetchPrometheusCompressed() throws IOException {
     String content = new String(ByteStreams.toByteArray(gis), StandardCharsets.UTF_8);
     assertThat(content)
         .isEqualTo(
-            "# HELP grpc_name_total long_description\n"
-                + "# TYPE grpc_name_total counter\n"
-                + "grpc_name_total{kp=\"vp\",otel_scope_name=\"grpc\",otel_scope_version=\"version\"} 5.0\n"
-                + "# HELP http_name_total double_description\n"
-                + "# TYPE http_name_total counter\n"
-                + "http_name_total{kp=\"vp\",otel_scope_name=\"http\",otel_scope_version=\"version\"} 3.5\n"
+            "# HELP grpc_name_unit_total long_description\n"
+                + "# TYPE grpc_name_unit_total counter\n"
+                + "grpc_name_unit_total{kp=\"vp\",otel_scope_name=\"grpc\",otel_scope_version=\"version\"} 5.0\n"
+                + "# HELP http_name_unit_total double_description\n"
+                + "# TYPE http_name_unit_total counter\n"
+                + "http_name_unit_total{kp=\"vp\",otel_scope_name=\"http\",otel_scope_version=\"version\"} 3.5\n"
                 + "# TYPE target_info gauge\n"
                 + "target_info{kr=\"vr\"} 1\n");
   }
@@ -252,7 +254,7 @@ void fetch_DuplicateMetrics() {
                 InstrumentationScopeInfo.create("scope3"),
                 "foo_unit_total",
                 "unused",
-                "unit",
+                "",
                 ImmutableGaugeData.create(
                     Collections.singletonList(
                         ImmutableLongPointData.create(123, 456, Attributes.empty(), 3))))));
@@ -272,7 +274,7 @@ void fetch_DuplicateMetrics() {
     // Validate conflict warning message
     assertThat(logs.getEvents()).hasSize(1);
     logs.assertContains(
-        "Conflicting metric name foo_unit: Found one metric with type counter and one of type gauge. Dropping the one with type gauge.");
+        "Conflicting metrics: Multiple metrics with name foo_unit but different units found. Dropping the one with unit null.");
   }
 
   @Test
@@ -318,7 +320,7 @@ private static List<MetricData> generateTestData() {
             InstrumentationScopeInfo.builder("grpc").setVersion("version").build(),
             "grpc.name",
             "long_description",
-            "1",
+            "unit",
             ImmutableSumData.create(
                 /* isMonotonic= */ true,
                 AggregationTemporality.CUMULATIVE,
@@ -330,7 +332,7 @@ private static List<MetricData> generateTestData() {
             InstrumentationScopeInfo.builder("http").setVersion("version").build(),
             "http.name",
             "double_description",
-            "1",
+            "unit",
             ImmutableSumData.create(
                 /* isMonotonic= */ true,
                 AggregationTemporality.CUMULATIVE,

exporters/prometheus/src/test/java/io/opentelemetry/exporter/prometheus/PrometheusUnitsHelperTest.java

@@ -70,7 +70,7 @@ private static Stream<Arguments> providePrometheusOTelUnitEquivalentPairs() {
         // Unit not found - Case sensitive
         Arguments.of("S", "S"),
         // Special case - 1
-        Arguments.of("1", null),
+        Arguments.of("1", "ratio"),
         // Special Case - Drop metric units in {}
         Arguments.of("{packets}", null),
         // Special Case - Dropped metric units only in {}