From 2bc3707375c86a58d92b35711dfaeaa46b1978fa Mon Sep 17 00:00:00 2001 From: Tommy Ludwig <8924140+shakuzen@users.noreply.github.com> Date: Tue, 16 Mar 2021 16:40:21 +0900 Subject: [PATCH 1/3] Remove outdated comment We switched from long buckets to double buckets, and the code was updated here but the comment refers to the old code. Remove the comment as the code is more self-explanatory with the double `isPositiveInfinity` method. Resolves gh-2507 --- .../core/instrument/distribution/HistogramGauges.java | 2 -- 1 file changed, 2 deletions(-) diff --git a/micrometer-core/src/main/java/io/micrometer/core/instrument/distribution/HistogramGauges.java b/micrometer-core/src/main/java/io/micrometer/core/instrument/distribution/HistogramGauges.java index d757a68419..56abf1a2c9 100644 --- a/micrometer-core/src/main/java/io/micrometer/core/instrument/distribution/HistogramGauges.java +++ b/micrometer-core/src/main/java/io/micrometer/core/instrument/distribution/HistogramGauges.java @@ -52,7 +52,6 @@ private static HistogramGauges getHistogramGauges(HistogramSupport histogramSupp percentile -> Tags.concat(id.getTagsAsIterable(), "phi", DoubleFormat.decimalOrNan(percentile.percentile())), percentile -> percentile.value(baseTimeUnit), bucket -> id.getName() + ".histogram", - // We look for Long.MAX_VALUE to ensure a sensible tag on our +Inf bucket bucket -> Tags.concat(id.getTagsAsIterable(), "le", bucket.isPositiveInf() ? "+Inf" : DoubleFormat.wholeOrDecimal(bucket.bucket(baseTimeUnit)))); } @@ -77,7 +76,6 @@ public static HistogramGauges registerWithCommonFormat(DistributionSummary summa percentile -> Tags.concat(id.getTagsAsIterable(), "phi", DoubleFormat.decimalOrNan(percentile.percentile())), ValueAtPercentile::value, bucket -> id.getName() + ".histogram", - // We look for Long.MAX_VALUE to ensure a sensible tag on our +Inf bucket bucket -> Tags.concat(id.getTagsAsIterable(), "le", bucket.isPositiveInf() ? "+Inf" : DoubleFormat.wholeOrDecimal(bucket.bucket()))); } From c0efa478b42129d815af7b2e858a5f0990c6476d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Fabian=20St=C3=A4ber?= Date: Tue, 16 Mar 2021 08:46:37 +0100 Subject: [PATCH 2/3] Fix duplicate +Inf bucket in Prometheus histograms (#2485) Histograms without an upper bound already have an +Inf bucket. The current implementation added an +Inf bucket regardless, resulting in two +Inf buckets. This fix makes sure that the +Inf bucket is only added if the histogram is configured with a finite upper bound. --- .../prometheus/PrometheusMeterRegistry.java | 12 ++++--- .../PrometheusMeterRegistryTest.java | 31 +++++++++++++++++++ 2 files changed, 38 insertions(+), 5 deletions(-) diff --git a/implementations/micrometer-registry-prometheus/src/main/java/io/micrometer/prometheus/PrometheusMeterRegistry.java b/implementations/micrometer-registry-prometheus/src/main/java/io/micrometer/prometheus/PrometheusMeterRegistry.java index 24180c50fe..8a00ff0434 100644 --- a/implementations/micrometer-registry-prometheus/src/main/java/io/micrometer/prometheus/PrometheusMeterRegistry.java +++ b/implementations/micrometer-registry-prometheus/src/main/java/io/micrometer/prometheus/PrometheusMeterRegistry.java @@ -156,11 +156,13 @@ public DistributionSummary newDistributionSummary(Meter.Id id, DistributionStati sampleName, histogramKeys, histogramValues, c.count())); } - // the +Inf bucket should always equal `count` - final List histogramValues = new LinkedList<>(tagValues); - histogramValues.add("+Inf"); - samples.add(new Collector.MetricFamilySamples.Sample( - sampleName, histogramKeys, histogramValues, count)); + if (Double.isFinite(histogramCounts[histogramCounts.length - 1].bucket())) { + // the +Inf bucket should always equal `count` + final List histogramValues = new LinkedList<>(tagValues); + histogramValues.add("+Inf"); + samples.add(new Collector.MetricFamilySamples.Sample( + sampleName, histogramKeys, histogramValues, count)); + } break; case VictoriaMetrics: histogramKeys.add("vmrange"); diff --git a/implementations/micrometer-registry-prometheus/src/test/java/io/micrometer/prometheus/PrometheusMeterRegistryTest.java b/implementations/micrometer-registry-prometheus/src/test/java/io/micrometer/prometheus/PrometheusMeterRegistryTest.java index 2c4cbd22c6..39b82d7658 100644 --- a/implementations/micrometer-registry-prometheus/src/test/java/io/micrometer/prometheus/PrometheusMeterRegistryTest.java +++ b/implementations/micrometer-registry-prometheus/src/test/java/io/micrometer/prometheus/PrometheusMeterRegistryTest.java @@ -39,9 +39,11 @@ import java.util.concurrent.Future; import java.util.concurrent.TimeUnit; import java.util.concurrent.atomic.AtomicInteger; +import java.util.regex.Pattern; import static io.micrometer.core.instrument.MockClock.clock; import static java.util.Collections.emptyList; +import static java.util.regex.Pattern.DOTALL; import static org.assertj.core.api.Assertions.assertThat; import static org.assertj.core.api.Assertions.offset; import static org.junit.jupiter.api.Assertions.assertThrows; @@ -211,6 +213,35 @@ void percentileHistogramsNeverResetForSummaries() { .contains("s1_bucket{le=\"100.0\",} 1.0"); } + @Test + void percentileHistogramWithUpperBoundContainsExactlyOneInf() { + + DistributionSummary s = DistributionSummary.builder("s") + .publishPercentileHistogram() + .maximumExpectedValue(3.0) + .register(registry); + + s.record(100); + + String inf = Pattern.quote("s_bucket{le=\"+Inf\",} 1.0"); + assertThat(registry.scrape()).matches(Pattern.compile(".*" + inf + ".*", DOTALL)); + assertThat(registry.scrape()).doesNotMatch(Pattern.compile(".*" + inf + ".*" + inf + ".*", DOTALL)); + } + + @Test + void percentileHistogramWithoutUpperBoundContainsExactlyOneInf() { + + DistributionSummary s = DistributionSummary.builder("s") + .publishPercentileHistogram() + .register(registry); + + s.record(100); + + String inf = Pattern.quote("s_bucket{le=\"+Inf\",} 1.0"); + assertThat(registry.scrape()).matches(Pattern.compile(".*" + inf + ".*", DOTALL)); + assertThat(registry.scrape()).doesNotMatch(Pattern.compile(".*" + inf + ".*" + inf + ".*", DOTALL)); + } + @Issue("#247") @Test void distributionPercentileBuckets() { From 3e06732d77d55ea61472ced78ea49acc6b62e708 Mon Sep 17 00:00:00 2001 From: Tommy Ludwig <8924140+shakuzen@users.noreply.github.com> Date: Tue, 16 Mar 2021 16:49:10 +0900 Subject: [PATCH 3/3] Polish PrometheusMeterRegistryTest Simplifies the assertion that the +Inf bucket histogram gauge appears only once in the scrape endpoint. See gh-2485 --- .../prometheus/PrometheusMeterRegistryTest.java | 10 ++-------- 1 file changed, 2 insertions(+), 8 deletions(-) diff --git a/implementations/micrometer-registry-prometheus/src/test/java/io/micrometer/prometheus/PrometheusMeterRegistryTest.java b/implementations/micrometer-registry-prometheus/src/test/java/io/micrometer/prometheus/PrometheusMeterRegistryTest.java index 39b82d7658..830a46a643 100644 --- a/implementations/micrometer-registry-prometheus/src/test/java/io/micrometer/prometheus/PrometheusMeterRegistryTest.java +++ b/implementations/micrometer-registry-prometheus/src/test/java/io/micrometer/prometheus/PrometheusMeterRegistryTest.java @@ -39,11 +39,9 @@ import java.util.concurrent.Future; import java.util.concurrent.TimeUnit; import java.util.concurrent.atomic.AtomicInteger; -import java.util.regex.Pattern; import static io.micrometer.core.instrument.MockClock.clock; import static java.util.Collections.emptyList; -import static java.util.regex.Pattern.DOTALL; import static org.assertj.core.api.Assertions.assertThat; import static org.assertj.core.api.Assertions.offset; import static org.junit.jupiter.api.Assertions.assertThrows; @@ -223,9 +221,7 @@ void percentileHistogramWithUpperBoundContainsExactlyOneInf() { s.record(100); - String inf = Pattern.quote("s_bucket{le=\"+Inf\",} 1.0"); - assertThat(registry.scrape()).matches(Pattern.compile(".*" + inf + ".*", DOTALL)); - assertThat(registry.scrape()).doesNotMatch(Pattern.compile(".*" + inf + ".*" + inf + ".*", DOTALL)); + assertThat(registry.scrape()).containsOnlyOnce("s_bucket{le=\"+Inf\",} 1.0"); } @Test @@ -237,9 +233,7 @@ void percentileHistogramWithoutUpperBoundContainsExactlyOneInf() { s.record(100); - String inf = Pattern.quote("s_bucket{le=\"+Inf\",} 1.0"); - assertThat(registry.scrape()).matches(Pattern.compile(".*" + inf + ".*", DOTALL)); - assertThat(registry.scrape()).doesNotMatch(Pattern.compile(".*" + inf + ".*" + inf + ".*", DOTALL)); + assertThat(registry.scrape()).containsOnlyOnce("s_bucket{le=\"+Inf\",} 1.0"); } @Issue("#247")