Skip to content

Commit

Permalink
Avoid calling naming convention on scrape (#5288)
Browse files Browse the repository at this point in the history 
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
  • Loading branch information
@shakuzen
shakuzen committed Jul 9, 2024
1 parent faeebde commit 083f12e
Show file tree
Hide file tree
Showing 3 changed files with 133 additions and 39 deletions.
45 changes: 45 additions & 0 deletions ...heus-simpleclient/src/test/java/io/micrometer/prometheus/PrometheusMeterRegistryTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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.*;
Expand Down Expand Up @@ -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();
Expand Down
82 changes: 43 additions & 39 deletions ...try-prometheus/src/main/java/io/micrometer/prometheusmetrics/PrometheusMeterRegistry.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}
Expand Down Expand Up @@ -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<>();
Expand Down Expand Up @@ -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
Expand All @@ -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();
Expand All @@ -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))));
}
});
Expand All @@ -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;
}
Expand All @@ -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;
}
Expand Down Expand Up @@ -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));
}

Expand Down Expand Up @@ -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<>();
Expand Down Expand Up @@ -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
Expand All @@ -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();
});
Expand Down Expand Up @@ -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);
Expand Down
45 changes: 45 additions & 0 deletions ...prometheus/src/test/java/io/micrometer/prometheusmetrics/PrometheusMeterRegistryTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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
Expand Down

0 comments on commit 083f12e

Please sign in to comment.