From 083f12e2a09e081567807f1be6273acf4da39dc5 Mon Sep 17 00:00:00 2001 From: Tommy Ludwig <8924140+shakuzen@users.noreply.github.com> Date: Tue, 9 Jul 2024 11:40:33 +0900 Subject: [PATCH] Avoid calling naming convention on scrape (#5288) 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. See gh-5229 --- .../PrometheusMeterRegistryTest.java | 45 ++++++++++ .../PrometheusMeterRegistry.java | 82 ++++++++++--------- .../PrometheusMeterRegistryTest.java | 45 ++++++++++ 3 files changed, 133 insertions(+), 39 deletions(-) diff --git a/implementations/micrometer-registry-prometheus-simpleclient/src/test/java/io/micrometer/prometheus/PrometheusMeterRegistryTest.java b/implementations/micrometer-registry-prometheus-simpleclient/src/test/java/io/micrometer/prometheus/PrometheusMeterRegistryTest.java index 0bc53ffcec..cf4c0da227 100644 --- a/implementations/micrometer-registry-prometheus-simpleclient/src/test/java/io/micrometer/prometheus/PrometheusMeterRegistryTest.java +++ b/implementations/micrometer-registry-prometheus-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/implementations/micrometer-registry-prometheus/src/main/java/io/micrometer/prometheusmetrics/PrometheusMeterRegistry.java b/implementations/micrometer-registry-prometheus/src/main/java/io/micrometer/prometheusmetrics/PrometheusMeterRegistry.java index 641acb4fb4..748efa48df 100644 --- a/implementations/micrometer-registry-prometheus/src/main/java/io/micrometer/prometheusmetrics/PrometheusMeterRegistry.java +++ b/implementations/micrometer-registry-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 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 io.micrometer.core.instrument.Gauge newGauge(Meter.Id id, @Nullabl applyToCollector(id, (collector) -> { List 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 FunctionTimer newFunctionTimer(Meter.Id id, T obj, ToLongFunction< applyToCollector(id, (collector) -> { List 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 FunctionCounter newFunctionCounter(Meter.Id id, T obj, ToDoubleFun FunctionCounter fc = new CumulativeFunctionCounter<>(id, obj, countFunction); applyToCollector(id, (collector) -> { List 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 mea private MicrometerCollector.Family 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 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 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/implementations/micrometer-registry-prometheus/src/test/java/io/micrometer/prometheusmetrics/PrometheusMeterRegistryTest.java b/implementations/micrometer-registry-prometheus/src/test/java/io/micrometer/prometheusmetrics/PrometheusMeterRegistryTest.java index 40e0d1be33..5d41d34302 100644 --- a/implementations/micrometer-registry-prometheus/src/test/java/io/micrometer/prometheusmetrics/PrometheusMeterRegistryTest.java +++ b/implementations/micrometer-registry-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