From 93056ac79ff89b370e71b4c601aa14ff66e9bd37 Mon Sep 17 00:00:00 2001 From: Jonatan Ivanov Date: Wed, 20 Mar 2024 12:25:58 -0700 Subject: [PATCH 1/2] Sanitize metric names for the Prometheus client We need to sanitize metric names for the Prometheus client otherwise it fails with something like this: java.lang.IllegalArgumentException: 'jvm_info': Illegal metric name. The metric name must not include the '_info' suffix. This can be reproduced with: Gauge.builder("test.info", () -> 1).register(registry); The name of this Gauge will be test_info in Prometheus that is invalid according to the new client, see PrometheusNaming: https://github.com/prometheus/client_java/blob/8bb75446c6dca2a4e2c074e39fe34c1b061b028a/prometheus-metrics-model/src/main/java/io/prometheus/metrics/model/snapshots/PrometheusNaming.java#L37-L40 We have built-in instrumentation that fails this validation too, see JvmInfoMetrics: https://github.com/micrometer-metrics/micrometer/blob/966f5fe953a69c315b44bf4f63adf101a060bbfb/micrometer-core/src/main/java/io/micrometer/core/instrument/binder/jvm/JvmInfoMetrics.java#L32 If we don't want to break this, we need to sanitize the invalid suffixes. Please check the tests because the behavior can be unexpected sometimes: - If you create a Micrometer Gauge named test.info, the name of the Prometheus gauge will be test_info (since the registry removes but the Prometheus client adds back the `_info` suffix). - If you create a Micrometer Counter named test.total, the name of the Prometheus counter will be test_total (since the registry removes but the Prometheus client adds back the _total suffix). - If you create a Micrometer Counter named test.info, the name of the Prometheus counter will be test_total (since the registry removes the _info suffix and the Prometheus client adds the _total suffix for counters). - If you create a Micrometer Gauge named test.total, the name of the Prometheus counter will be test (since the registry removes the _total suffix and the Prometheus client does not add _info suffix for gauges). - Similarly, _created and _bucket will be removed otherwise the the Prometheus client will fail. --- .../PrometheusMeterRegistry.java | 33 ++++++++++++--- .../PrometheusMeterRegistryTest.java | 40 +++++++++++++++++-- 2 files changed, 64 insertions(+), 9 deletions(-) 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 aac4e9103b..28b109abea 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 @@ -15,6 +15,7 @@ */ package io.micrometer.prometheusmetrics; +import io.micrometer.common.lang.NonNull; import io.micrometer.common.lang.Nullable; import io.micrometer.core.instrument.*; import io.micrometer.core.instrument.cumulative.CumulativeFunctionCounter; @@ -32,6 +33,7 @@ import io.prometheus.metrics.model.snapshots.CounterSnapshot.CounterDataPointSnapshot; import io.prometheus.metrics.model.snapshots.GaugeSnapshot.GaugeDataPointSnapshot; import io.prometheus.metrics.model.snapshots.HistogramSnapshot.HistogramDataPointSnapshot; +import io.prometheus.metrics.model.snapshots.InfoSnapshot.InfoDataPointSnapshot; import io.prometheus.metrics.model.snapshots.SummarySnapshot.SummaryDataPointSnapshot; import java.io.ByteArrayOutputStream; @@ -310,10 +312,20 @@ protected io.micrometer.core.instrument.Gauge newGauge(Meter.Id id, @Nullabl Gauge gauge = new DefaultGauge<>(id, obj, valueFunction); applyToCollector(id, (collector) -> { List tagValues = tagValues(id); - collector.add(tagValues, - (conventionName, tagKeys) -> Stream.of(new MicrometerCollector.Family<>(conventionName, - family -> new GaugeSnapshot(family.metadata, family.dataPointSnapshots), getMetadata(id), - new GaugeDataPointSnapshot(gauge.value(), Labels.of(tagKeys, tagValues), null)))); + 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))))); + } + else { + collector.add(tagValues, + (conventionName, tagKeys) -> Stream.of(new MicrometerCollector.Family<>(conventionName, + family -> new GaugeSnapshot(family.metadata, family.dataPointSnapshots), + getMetadata(id), + new GaugeDataPointSnapshot(gauge.value(), Labels.of(tagKeys, tagValues), null)))); + } }); return gauge; } @@ -524,9 +536,20 @@ private MetricMetadata getMetadata(Meter.Id id) { private MetricMetadata getMetadata(Meter.Id id, String suffix) { String name = config().namingConvention().name(id.getName(), id.getType(), id.getBaseUnit()) + suffix; + String sanitizedName = sanitize(name); String help = prometheusConfig.descriptions() ? Optional.ofNullable(id.getDescription()).orElse(" ") : " "; Unit unit = id.getBaseUnit() != null ? new Unit(id.getBaseUnit()) : null; - return new MetricMetadata(name, help, unit); + return new MetricMetadata(sanitizedName, help, unit); + } + + private String sanitize(@NonNull String name) { + if (name.endsWith("_info") || name.endsWith("_total") || name.endsWith("_created") + || name.endsWith("_bucket")) { + return name.substring(0, name.lastIndexOf('_')); + } + else { + return name; + } } private void applyToCollector(Meter.Id id, Consumer consumer) { 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 2d0d8fd40d..f3b5e06257 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 @@ -16,14 +16,12 @@ package io.micrometer.prometheusmetrics; import io.micrometer.core.Issue; -import io.micrometer.core.instrument.Timer; import io.micrometer.core.instrument.*; import io.micrometer.core.instrument.binder.BaseUnits; +import io.micrometer.core.instrument.binder.jvm.JvmInfoMetrics; import io.micrometer.core.instrument.composite.CompositeMeterRegistry; import io.micrometer.core.instrument.distribution.DistributionStatisticConfig; import io.micrometer.core.instrument.distribution.HistogramSnapshot; -import io.micrometer.prometheusmetrics.PrometheusConfig; -import io.micrometer.prometheusmetrics.PrometheusMeterRegistry; import io.prometheus.metrics.model.registry.PrometheusRegistry; import io.prometheus.metrics.model.snapshots.*; import io.prometheus.metrics.tracer.common.SpanContext; @@ -32,7 +30,10 @@ import org.junit.jupiter.api.Test; import java.time.Duration; -import java.util.*; +import java.util.Arrays; +import java.util.Collections; +import java.util.HashSet; +import java.util.List; import java.util.concurrent.*; import java.util.concurrent.atomic.AtomicInteger; import java.util.concurrent.atomic.AtomicLong; @@ -103,6 +104,37 @@ void quantiles() { assertThat(prometheusRegistry.scrape()).has(withNameAndQuantile("ds")); } + @Test + void invalidMeterNameSuffixesShouldBeRemovedForMetadata() { + new JvmInfoMetrics().bindTo(registry); + Gauge.builder("test1.info", () -> 1).register(registry); + Counter.builder("test2.total").register(registry).increment(2); + Gauge.builder("test3.created", () -> 3).register(registry); + Counter.builder("test4.bucket").register(registry).increment(4); + + Counter.builder("test5.info").register(registry).increment(5); + Gauge.builder("test6.total", () -> 6).register(registry); + + String scrapeResult = registry.scrape(); + + assertThat(scrapeResult).contains("# HELP test1_info") + .contains("# TYPE test1_info gauge") + .contains("test1_info 1"); + assertThat(scrapeResult).contains("# HELP test2_total") + .contains("# TYPE test2_total counter") + .contains("test2_total 2.0"); + assertThat(scrapeResult).contains("# HELP jvm_info").contains("# TYPE jvm_info gauge").contains("jvm_info{"); + assertThat(scrapeResult).contains("# HELP test3").contains("# TYPE test3 gauge").contains("test3 3"); + assertThat(scrapeResult).contains("# HELP test4_total") + .contains("# TYPE test4_total counter") + .contains("test4_total 4.0"); + + assertThat(scrapeResult).contains("# HELP test5_total") + .contains("# TYPE test5_total counter") + .contains("test5_total 5.0"); + assertThat(scrapeResult).contains("# HELP test6").contains("# TYPE test6 gauge").contains("test6 6"); + } + @Issue("#27") @DisplayName("custom distribution summaries respect varying tags") @Test From fda1f187906c04aae58f2ff31d424865fcf97e14 Mon Sep 17 00:00:00 2001 From: Jonatan Ivanov Date: Thu, 21 Mar 2024 15:27:54 -0700 Subject: [PATCH 2/2] Use PrometheusNaming in PrometheusNamingConvention --- .../PrometheusMeterRegistry.java | 14 +---------- .../PrometheusNamingConvention.java | 24 +++---------------- .../PrometheusNamingConventionTest.java | 4 ++-- 3 files changed, 6 insertions(+), 36 deletions(-) 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 28b109abea..2f47eb1167 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 @@ -15,7 +15,6 @@ */ package io.micrometer.prometheusmetrics; -import io.micrometer.common.lang.NonNull; import io.micrometer.common.lang.Nullable; import io.micrometer.core.instrument.*; import io.micrometer.core.instrument.cumulative.CumulativeFunctionCounter; @@ -536,20 +535,9 @@ private MetricMetadata getMetadata(Meter.Id id) { private MetricMetadata getMetadata(Meter.Id id, String suffix) { String name = config().namingConvention().name(id.getName(), id.getType(), id.getBaseUnit()) + suffix; - String sanitizedName = sanitize(name); String help = prometheusConfig.descriptions() ? Optional.ofNullable(id.getDescription()).orElse(" ") : " "; Unit unit = id.getBaseUnit() != null ? new Unit(id.getBaseUnit()) : null; - return new MetricMetadata(sanitizedName, help, unit); - } - - private String sanitize(@NonNull String name) { - if (name.endsWith("_info") || name.endsWith("_total") || name.endsWith("_created") - || name.endsWith("_bucket")) { - return name.substring(0, name.lastIndexOf('_')); - } - else { - return name; - } + return new MetricMetadata(name, help, unit); } private void applyToCollector(Meter.Id id, Consumer consumer) { diff --git a/implementations/micrometer-registry-prometheus/src/main/java/io/micrometer/prometheusmetrics/PrometheusNamingConvention.java b/implementations/micrometer-registry-prometheus/src/main/java/io/micrometer/prometheusmetrics/PrometheusNamingConvention.java index 1f96185800..9460c8b84b 100644 --- a/implementations/micrometer-registry-prometheus/src/main/java/io/micrometer/prometheusmetrics/PrometheusNamingConvention.java +++ b/implementations/micrometer-registry-prometheus/src/main/java/io/micrometer/prometheusmetrics/PrometheusNamingConvention.java @@ -18,8 +18,7 @@ import io.micrometer.common.lang.Nullable; import io.micrometer.core.instrument.Meter; import io.micrometer.core.instrument.config.NamingConvention; - -import java.util.regex.Pattern; +import io.prometheus.metrics.model.snapshots.PrometheusNaming; /** * See https://prometheus.io/docs/concepts/data_model/#metric-names-and-labels for a @@ -29,10 +28,6 @@ */ public class PrometheusNamingConvention implements NamingConvention { - private static final Pattern nameChars = Pattern.compile("[^a-zA-Z0-9_:]"); - - private static final Pattern tagKeyChars = Pattern.compile("[^a-zA-Z0-9_]"); - private final String timerSuffix; public PrometheusNamingConvention() { @@ -45,9 +40,6 @@ public PrometheusNamingConvention(String timerSuffix) { /** * Names are snake-cased. They contain a base unit suffix when applicable. - *

- * Names may contain ASCII letters and digits, as well as underscores and colons. They - * must match the regex [a-zA-Z_:][a-zA-Z0-9_:]* */ @Override public String name(String name, Meter.Type type, @Nullable String baseUnit) { @@ -81,11 +73,7 @@ else if (!conventionName.endsWith("_seconds")) { break; } - String sanitized = nameChars.matcher(conventionName).replaceAll("_"); - if (!Character.isLetter(sanitized.charAt(0))) { - sanitized = "m_" + sanitized; - } - return sanitized; + return PrometheusNaming.sanitizeMetricName(conventionName); } /** @@ -95,13 +83,7 @@ else if (!conventionName.endsWith("_seconds")) { */ @Override public String tagKey(String key) { - String conventionKey = NamingConvention.snakeCase.tagKey(key); - - String sanitized = tagKeyChars.matcher(conventionKey).replaceAll("_"); - if (!Character.isLetter(sanitized.charAt(0))) { - sanitized = "m_" + sanitized; - } - return sanitized; + return PrometheusNaming.sanitizeLabelName(key); } } diff --git a/implementations/micrometer-registry-prometheus/src/test/java/io/micrometer/prometheusmetrics/PrometheusNamingConventionTest.java b/implementations/micrometer-registry-prometheus/src/test/java/io/micrometer/prometheusmetrics/PrometheusNamingConventionTest.java index 8ba28bee9e..34934b8949 100644 --- a/implementations/micrometer-registry-prometheus/src/test/java/io/micrometer/prometheusmetrics/PrometheusNamingConventionTest.java +++ b/implementations/micrometer-registry-prometheus/src/test/java/io/micrometer/prometheusmetrics/PrometheusNamingConventionTest.java @@ -30,12 +30,12 @@ class PrometheusNamingConventionTest { @Test void formatName() { - assertThat(convention.name("123abc/{:id}水", Meter.Type.GAUGE)).startsWith("m_123abc__:id__"); + assertThat(convention.name("123abc/{:id}水", Meter.Type.GAUGE)).startsWith("_23abc__:id__"); } @Test void formatTagKey() { - assertThat(convention.tagKey("123abc/{:id}水")).startsWith("m_123abc___id__"); + assertThat(convention.tagKey("123abc/{:id}水")).startsWith("_23abc___id__"); } @Test