Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Avoid calling naming convention on scrape #5288

Merged
merged 1 commit into from
Jul 9, 2024
Merged

Avoid calling naming convention on scrape #5288

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
45 changes: 45 additions & 0 deletions ...heus-simpleclient/src/test/java/io/micrometer/prometheus/PrometheusMeterRegistryTest.java
View file
Open in desktop
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
Open in desktop
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(" ") : " ";
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was a copy-paste form the 0.x client code but eliminating the Optional might also have some positive impact on performance.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed and it stood out to me too when making these changes, but I wanted to keep the changes here focused to one issue. Let's take it up in a separate change.

// 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
Open in desktop
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