Avoid calling naming convention on scrape

The naming convention was being called on each scrape for each metric to create the MetricMetadata. This is unnecessary because we already have the computed convention name specifically to avoid needing to call the convention again.
shakuzen · Jul 8, 2024 · 6cbd194 · 6cbd194
1 parent 08bea96
commit 6cbd194
Show file tree

Hide file tree

Showing 3 changed files with 133 additions and 39 deletions.
diff --git a/...heus-simpleclient/src/test/java/io/micrometer/prometheus/PrometheusMeterRegistryTest.java b/...heus-simpleclient/src/test/java/io/micrometer/prometheus/PrometheusMeterRegistryTest.java
@@ -15,6 +15,7 @@
  */
 package io.micrometer.prometheus;
 
+import io.micrometer.common.lang.Nullable;
 import io.micrometer.core.Issue;
 import io.micrometer.core.instrument.Timer;
 import io.micrometer.core.instrument.*;
@@ -717,6 +718,50 @@ void noExemplarsIfNoSampler() {
         assertThat(scraped).endsWith("# EOF\n");
     }
 
+    @Test
+    @Issue("#5229")
+    void doesNotCallConventionOnScrape() {
+        CountingPrometheusNamingConvention convention = new CountingPrometheusNamingConvention();
+        registry.config().namingConvention(convention);
+
+        Timer.builder("timer").tag("k1", "v1").description("my timer").register(registry);
+        Counter.builder("counter").tag("k1", "v1").description("my counter").register(registry);
+        DistributionSummary.builder("summary").tag("k1", "v1").description("my summary").register(registry);
+        Gauge.builder("gauge", new AtomicInteger(), AtomicInteger::doubleValue)
+            .tag("k1", "v1")
+            .description("my gauge")
+            .register(registry);
+        LongTaskTimer.builder("long.task.timer").tag("k1", "v1").description("my long task timer").register(registry);
+
+        int expectedNameCount = convention.nameCount.get();
+        int expectedTagKeyCount = convention.tagKeyCount.get();
+
+        registry.scrape();
+
+        assertThat(convention.nameCount.get()).isEqualTo(expectedNameCount);
+        assertThat(convention.tagKeyCount.get()).isEqualTo(expectedTagKeyCount);
+    }
+
+    private static class CountingPrometheusNamingConvention extends PrometheusNamingConvention {
+
+        AtomicInteger nameCount = new AtomicInteger();
+
+        AtomicInteger tagKeyCount = new AtomicInteger();
+
+        @Override
+        public String name(String name, Meter.Type type, @Nullable String baseUnit) {
+            nameCount.incrementAndGet();
+            return super.name(name, type, baseUnit);
+        }
+
+        @Override
+        public String tagKey(String key) {
+            tagKeyCount.incrementAndGet();
+            return super.tagKey(key);
+        }
+
+    }
+
     static class TestSpanContextSupplier implements SpanContextSupplier {
 
         private final AtomicLong count = new AtomicLong();

diff --git a/...try-prometheus/src/main/java/io/micrometer/prometheusmetrics/PrometheusMeterRegistry.java b/...try-prometheus/src/main/java/io/micrometer/prometheusmetrics/PrometheusMeterRegistry.java
@@ -213,8 +213,8 @@ public Counter newCounter(Meter.Id id) {
                     (conventionName,
                             tagKeys) -> Stream.of(new MicrometerCollector.Family<>(conventionName,
                                     family -> new CounterSnapshot(family.metadata, family.dataPointSnapshots),
-                                    getMetadata(id), new CounterDataPointSnapshot(counter.count(),
-                                            Labels.of(tagKeys, tagValues), counter.exemplar(), 0))));
+                                    getMetadata(conventionName, id.getDescription()), new CounterDataPointSnapshot(
+                                            counter.count(), Labels.of(tagKeys, tagValues), counter.exemplar(), 0))));
         });
         return counter;
     }
@@ -246,9 +246,9 @@ public DistributionSummary newDistributionSummary(Meter.Id id,
 
                     Exemplars exemplars = summary.exemplars();
                     families.add(new MicrometerCollector.Family<>(conventionName,
-                            family -> new SummarySnapshot(family.metadata, family.dataPointSnapshots), getMetadata(id),
-                            new SummaryDataPointSnapshot(count, sum, quantiles, Labels.of(tagKeys, tagValues),
-                                    exemplars, 0)));
+                            family -> new SummarySnapshot(family.metadata, family.dataPointSnapshots),
+                            getMetadata(conventionName, id.getDescription()), new SummaryDataPointSnapshot(count, sum,
+                                    quantiles, Labels.of(tagKeys, tagValues), exemplars, 0)));
                 }
                 else {
                     List<Double> buckets = new ArrayList<>();
@@ -277,8 +277,9 @@ public DistributionSummary newDistributionSummary(Meter.Id id,
                     families.add(new MicrometerCollector.Family<>(conventionName,
                             family -> new io.prometheus.metrics.model.snapshots.HistogramSnapshot(family.metadata,
                                     family.dataPointSnapshots),
-                            getMetadata(id), new HistogramDataPointSnapshot(ClassicHistogramBuckets.of(buckets, counts),
-                                    sum, Labels.of(tagKeys, tagValues), exemplars, 0)));
+                            getMetadata(conventionName, id.getDescription()),
+                            new HistogramDataPointSnapshot(ClassicHistogramBuckets.of(buckets, counts), sum,
+                                    Labels.of(tagKeys, tagValues), exemplars, 0)));
 
                     // TODO: Add support back for VictoriaMetrics
                     // Previously we had low-level control so a histogram was just
@@ -292,7 +293,7 @@ public DistributionSummary newDistributionSummary(Meter.Id id,
 
                 families.add(new MicrometerCollector.Family<>(conventionName + "_max",
                         family -> new GaugeSnapshot(family.metadata, family.dataPointSnapshots),
-                        getMetadata(id, "_max"),
+                        getMetadata(conventionName + "_max", id.getDescription()),
                         new GaugeDataPointSnapshot(summary.max(), Labels.of(tagKeys, tagValues), null)));
 
                 return families.build();
@@ -319,17 +320,18 @@ protected <T> io.micrometer.core.instrument.Gauge newGauge(Meter.Id id, @Nullabl
         applyToCollector(id, (collector) -> {
             List<String> tagValues = tagValues(id);
             if (id.getName().endsWith(".info")) {
-                collector
-                    .add(tagValues,
-                            (conventionName, tagKeys) -> Stream.of(new MicrometerCollector.Family<>(conventionName,
-                                    family -> new InfoSnapshot(family.metadata, family.dataPointSnapshots),
-                                    getMetadata(id), new InfoDataPointSnapshot(Labels.of(tagKeys, tagValues)))));
+                collector.add(tagValues,
+                        (conventionName,
+                                tagKeys) -> Stream.of(new MicrometerCollector.Family<>(conventionName,
+                                        family -> new InfoSnapshot(family.metadata, family.dataPointSnapshots),
+                                        getMetadata(conventionName, id.getDescription()),
+                                        new InfoDataPointSnapshot(Labels.of(tagKeys, tagValues)))));
             }
             else {
                 collector.add(tagValues,
                         (conventionName, tagKeys) -> Stream.of(new MicrometerCollector.Family<>(conventionName,
                                 family -> new GaugeSnapshot(family.metadata, family.dataPointSnapshots),
-                                getMetadata(id),
+                                getMetadata(conventionName, id.getDescription()),
                                 new GaugeDataPointSnapshot(gauge.value(), Labels.of(tagKeys, tagValues), null))));
             }
         });
@@ -352,10 +354,12 @@ protected <T> FunctionTimer newFunctionTimer(Meter.Id id, T obj, ToLongFunction<
         applyToCollector(id, (collector) -> {
             List<String> tagValues = tagValues(id);
             collector.add(tagValues,
-                    (conventionName, tagKeys) -> Stream.of(new MicrometerCollector.Family<>(conventionName,
-                            family -> new SummarySnapshot(family.metadata, family.dataPointSnapshots), getMetadata(id),
-                            new SummaryDataPointSnapshot((long) ft.count(), ft.totalTime(getBaseTimeUnit()),
-                                    Quantiles.EMPTY, Labels.of(tagKeys, tagValues), null, 0))));
+                    (conventionName,
+                            tagKeys) -> Stream.of(new MicrometerCollector.Family<>(conventionName,
+                                    family -> new SummarySnapshot(family.metadata, family.dataPointSnapshots),
+                                    getMetadata(conventionName, id.getDescription()),
+                                    new SummaryDataPointSnapshot((long) ft.count(), ft.totalTime(getBaseTimeUnit()),
+                                            Quantiles.EMPTY, Labels.of(tagKeys, tagValues), null, 0))));
         });
         return ft;
     }
@@ -365,10 +369,12 @@ protected <T> FunctionCounter newFunctionCounter(Meter.Id id, T obj, ToDoubleFun
         FunctionCounter fc = new CumulativeFunctionCounter<>(id, obj, countFunction);
         applyToCollector(id, (collector) -> {
             List<String> tagValues = tagValues(id);
-            collector.add(tagValues,
-                    (conventionName, tagKeys) -> Stream.of(new MicrometerCollector.Family<>(conventionName,
-                            family -> new CounterSnapshot(family.metadata, family.dataPointSnapshots), getMetadata(id),
-                            new CounterDataPointSnapshot(fc.count(), Labels.of(tagKeys, tagValues), null, 0))));
+            collector
+                .add(tagValues,
+                        (conventionName, tagKeys) -> Stream.of(new MicrometerCollector.Family<>(conventionName,
+                                family -> new CounterSnapshot(family.metadata, family.dataPointSnapshots),
+                                getMetadata(conventionName, id.getDescription()),
+                                new CounterDataPointSnapshot(fc.count(), Labels.of(tagKeys, tagValues), null, 0))));
         });
         return fc;
     }
@@ -423,14 +429,16 @@ protected Meter newMeter(Meter.Id id, Meter.Type type, Iterable<Measurement> mea
     private MicrometerCollector.Family<CounterDataPointSnapshot> customCounterFamily(Meter.Id id, String conventionName,
             String suffix, Labels labels, double value) {
         return new MicrometerCollector.Family<>(conventionName + suffix,
-                family -> new CounterSnapshot(family.metadata, family.dataPointSnapshots), getMetadata(id, suffix),
+                family -> new CounterSnapshot(family.metadata, family.dataPointSnapshots),
+                getMetadata(conventionName + suffix, id.getDescription()),
                 new CounterDataPointSnapshot(value, labels, null, 0));
     }
 
     private MicrometerCollector.Family<GaugeDataPointSnapshot> customGaugeFamily(Meter.Id id, String conventionName,
             String suffix, Labels labels, double value) {
         return new MicrometerCollector.Family<>(conventionName + suffix,
-                family -> new GaugeSnapshot(family.metadata, family.dataPointSnapshots), getMetadata(id, suffix),
+                family -> new GaugeSnapshot(family.metadata, family.dataPointSnapshots),
+                getMetadata(conventionName + suffix, id.getDescription()),
                 new GaugeDataPointSnapshot(value, labels, null));
     }
 
@@ -470,9 +478,9 @@ private void addDistributionStatisticSamples(Meter.Id id, MicrometerCollector co
 
                 Exemplars exemplars = createExemplarsWithScaledValues(exemplarsSupplier.get());
                 families.add(new MicrometerCollector.Family<>(conventionName,
-                        family -> new SummarySnapshot(family.metadata, family.dataPointSnapshots), getMetadata(id),
-                        new SummaryDataPointSnapshot(count, sum, quantiles, Labels.of(tagKeys, tagValues), exemplars,
-                                0)));
+                        family -> new SummarySnapshot(family.metadata, family.dataPointSnapshots),
+                        getMetadata(conventionName, id.getDescription()), new SummaryDataPointSnapshot(count, sum,
+                                quantiles, Labels.of(tagKeys, tagValues), exemplars, 0)));
             }
             else {
                 List<Double> buckets = new ArrayList<>();
@@ -501,8 +509,9 @@ private void addDistributionStatisticSamples(Meter.Id id, MicrometerCollector co
                 families.add(new MicrometerCollector.Family<>(conventionName,
                         family -> new io.prometheus.metrics.model.snapshots.HistogramSnapshot(forLongTaskTimer,
                                 family.metadata, family.dataPointSnapshots),
-                        getMetadata(id), new HistogramDataPointSnapshot(ClassicHistogramBuckets.of(buckets, counts),
-                                sum, Labels.of(tagKeys, tagValues), exemplars, 0)));
+                        getMetadata(conventionName, id.getDescription()),
+                        new HistogramDataPointSnapshot(ClassicHistogramBuckets.of(buckets, counts), sum,
+                                Labels.of(tagKeys, tagValues), exemplars, 0)));
 
                 // TODO: Add support back for VictoriaMetrics
                 // Previously we had low-level control so a histogram was just
@@ -515,9 +524,9 @@ private void addDistributionStatisticSamples(Meter.Id id, MicrometerCollector co
             }
 
             families.add(new MicrometerCollector.Family<>(conventionName + "_max",
-                    family -> new GaugeSnapshot(family.metadata, family.dataPointSnapshots), getMetadata(id, "_max"),
-                    new GaugeDataPointSnapshot(histogramSnapshot.max(getBaseTimeUnit()), Labels.of(tagKeys, tagValues),
-                            null)));
+                    family -> new GaugeSnapshot(family.metadata, family.dataPointSnapshots),
+                    getMetadata(conventionName + "_max", id.getDescription()), new GaugeDataPointSnapshot(
+                            histogramSnapshot.max(getBaseTimeUnit()), Labels.of(tagKeys, tagValues), null)));
 
             return families.build();
         });
@@ -549,13 +558,8 @@ private void onMeterRemoved(Meter meter) {
         }
     }
 
-    private MetricMetadata getMetadata(Meter.Id id) {
-        return getMetadata(id, "");
-    }
-
-    private MetricMetadata getMetadata(Meter.Id id, String suffix) {
-        String name = config().namingConvention().name(id.getName(), id.getType(), id.getBaseUnit()) + suffix;
-        String help = prometheusConfig.descriptions() ? Optional.ofNullable(id.getDescription()).orElse(" ") : " ";
+    private MetricMetadata getMetadata(String name, @Nullable String description) {
+        String help = prometheusConfig.descriptions() ? Optional.ofNullable(description).orElse(" ") : " ";
         // Unit is intentionally not set, see:
         // https://github.com/OpenObservability/OpenMetrics/blob/1386544931307dff279688f332890c31b6c5de36/specification/OpenMetrics.md#unit
         return new MetricMetadata(name, help, null);

diff --git a/...prometheus/src/test/java/io/micrometer/prometheusmetrics/PrometheusMeterRegistryTest.java b/...prometheus/src/test/java/io/micrometer/prometheusmetrics/PrometheusMeterRegistryTest.java
@@ -15,6 +15,7 @@
  */
 package io.micrometer.prometheusmetrics;
 
+import io.micrometer.common.lang.Nullable;
 import io.micrometer.core.Issue;
 import io.micrometer.core.instrument.LongTaskTimer.Sample;
 import io.micrometer.core.instrument.Timer;
@@ -991,6 +992,50 @@ void noExemplarsIfNoSampler() {
         assertThat(scraped).endsWith("# EOF\n");
     }
 
+    @Test
+    @Issue("#5229")
+    void doesNotCallConventionOnScrape() {
+        CountingPrometheusNamingConvention convention = new CountingPrometheusNamingConvention();
+        registry.config().namingConvention(convention);
+
+        Timer.builder("timer").tag("k1", "v1").description("my timer").register(registry);
+        Counter.builder("counter").tag("k1", "v1").description("my counter").register(registry);
+        DistributionSummary.builder("summary").tag("k1", "v1").description("my summary").register(registry);
+        Gauge.builder("gauge", new AtomicInteger(), AtomicInteger::doubleValue)
+            .tag("k1", "v1")
+            .description("my gauge")
+            .register(registry);
+        LongTaskTimer.builder("long.task.timer").tag("k1", "v1").description("my long task timer").register(registry);
+
+        int expectedNameCount = convention.nameCount.get();
+        int expectedTagKeyCount = convention.tagKeyCount.get();
+
+        registry.scrape();
+
+        assertThat(convention.nameCount.get()).isEqualTo(expectedNameCount);
+        assertThat(convention.tagKeyCount.get()).isEqualTo(expectedTagKeyCount);
+    }
+
+    private static class CountingPrometheusNamingConvention extends PrometheusNamingConvention {
+
+        AtomicInteger nameCount = new AtomicInteger();
+
+        AtomicInteger tagKeyCount = new AtomicInteger();
+
+        @Override
+        public String name(String name, Meter.Type type, @Nullable String baseUnit) {
+            nameCount.incrementAndGet();
+            return super.name(name, type, baseUnit);
+        }
+
+        @Override
+        public String tagKey(String key) {
+            tagKeyCount.incrementAndGet();
+            return super.tagKey(key);
+        }
+
+    }
+
     private PrometheusMeterRegistry createPrometheusMeterRegistryWithProperties(Properties properties) {
         PrometheusConfig prometheusConfig = new PrometheusConfig() {
             @Override