From f9db3821a8585418a5109259985258baff647bfb Mon Sep 17 00:00:00 2001 From: beorn7 Date: Sat, 8 Apr 2023 22:19:35 +0200 Subject: [PATCH 1/2] histograms: Small code comment and code formatting improvements Signed-off-by: beorn7 --- prometheus/histogram.go | 56 +++++++++++++++++++++++------------------ 1 file changed, 32 insertions(+), 24 deletions(-) diff --git a/prometheus/histogram.go b/prometheus/histogram.go index eaf4a49..997ea7b 100644 --- a/prometheus/histogram.go +++ b/prometheus/histogram.go @@ -428,12 +428,12 @@ type HistogramOpts struct { // a major version bump. NativeHistogramBucketFactor float64 // All observations with an absolute value of less or equal - // NativeHistogramZeroThreshold are accumulated into a “zero” - // bucket. For best results, this should be close to a bucket - // boundary. This is usually the case if picking a power of two. If + // NativeHistogramZeroThreshold are accumulated into a “zero” bucket. + // For best results, this should be close to a bucket boundary. This is + // usually the case if picking a power of two. If // NativeHistogramZeroThreshold is left at zero, - // DefNativeHistogramZeroThreshold is used as the threshold. To configure - // a zero bucket with an actual threshold of zero (i.e. only + // DefNativeHistogramZeroThreshold is used as the threshold. To + // configure a zero bucket with an actual threshold of zero (i.e. only // observations of precisely zero will go into the zero bucket), set // NativeHistogramZeroThreshold to the NativeHistogramZeroThresholdZero // constant (or any negative float value). @@ -446,23 +446,28 @@ type HistogramOpts struct { // Histogram are sufficiently wide-spread. In particular, this could be // used as a DoS attack vector. Where the observed values depend on // external inputs, it is highly recommended to set a - // NativeHistogramMaxBucketNumber.) Once the set + // NativeHistogramMaxBucketNumber.) Once the set // NativeHistogramMaxBucketNumber is exceeded, the following strategy is - // enacted: First, if the last reset (or the creation) of the histogram - // is at least NativeHistogramMinResetDuration ago, then the whole - // histogram is reset to its initial state (including regular - // buckets). If less time has passed, or if - // NativeHistogramMinResetDuration is zero, no reset is - // performed. Instead, the zero threshold is increased sufficiently to - // reduce the number of buckets to or below - // NativeHistogramMaxBucketNumber, but not to more than - // NativeHistogramMaxZeroThreshold. Thus, if - // NativeHistogramMaxZeroThreshold is already at or below the current - // zero threshold, nothing happens at this step. After that, if the - // number of buckets still exceeds NativeHistogramMaxBucketNumber, the - // resolution of the histogram is reduced by doubling the width of the - // sparse buckets (up to a growth factor between one bucket to the next - // of 2^(2^4) = 65536, see above). + // enacted: + // - First, if the last reset (or the creation) of the histogram is at + // least NativeHistogramMinResetDuration ago, then the whole + // histogram is reset to its initial state (including regular + // buckets). + // - If less time has passed, or if NativeHistogramMinResetDuration is + // zero, no reset is performed. Instead, the zero threshold is + // increased sufficiently to reduce the number of buckets to or below + // NativeHistogramMaxBucketNumber, but not to more than + // NativeHistogramMaxZeroThreshold. Thus, if + // NativeHistogramMaxZeroThreshold is already at or below the current + // zero threshold, nothing happens at this step. + // - After that, if the number of buckets still exceeds + // NativeHistogramMaxBucketNumber, the resolution of the histogram is + // reduced by doubling the width of the sparse buckets (up to a + // growth factor between one bucket to the next of 2^(2^4) = 65536, + // see above). + // - Any increased zero threshold or reduced resolution is reset back + // to their original values once NativeHistogramMinResetDuration has + // passed (since the last reset or the creation of the histogram). NativeHistogramMaxBucketNumber uint32 NativeHistogramMinResetDuration time.Duration NativeHistogramMaxZeroThreshold float64 @@ -854,13 +859,16 @@ func (h *histogram) limitBuckets(counts *histogramCounts, value float64, bucket h.doubleBucketWidth(hotCounts, coldCounts) } -// maybeReset resests the whole histogram if at least h.nativeHistogramMinResetDuration +// maybeReset resets the whole histogram if at least h.nativeHistogramMinResetDuration // has been passed. It returns true if the histogram has been reset. The caller // must have locked h.mtx. -func (h *histogram) maybeReset(hot, cold *histogramCounts, coldIdx uint64, value float64, bucket int) bool { +func (h *histogram) maybeReset( + hot, cold *histogramCounts, coldIdx uint64, value float64, bucket int, +) bool { // We are using the possibly mocked h.now() rather than // time.Since(h.lastResetTime) to enable testing. - if h.nativeHistogramMinResetDuration == 0 || h.now().Sub(h.lastResetTime) < h.nativeHistogramMinResetDuration { + if h.nativeHistogramMinResetDuration == 0 || + h.now().Sub(h.lastResetTime) < h.nativeHistogramMinResetDuration { return false } // Completely reset coldCounts. From 3d82c94432a76bbab75ae4a1d27a6eaaf011196b Mon Sep 17 00:00:00 2001 From: beorn7 Date: Thu, 20 Jul 2023 16:37:00 +0200 Subject: [PATCH 2/2] histogram: Add a no-op span for an otherwise empty histogram Fixes #1127. If a native histogram has no observations and a zero threshold of zero, then it is indistinguishable from a classic histogram. To give scrapers a hint that it is indeed a native histogram, we add a no-op span. This needs follow-up PRs in prometheus/prometheus and prometheus/client_model. Signed-off-by: beorn7 --- prometheus/histogram.go | 10 ++++++++++ prometheus/histogram_test.go | 11 +++++++++++ 2 files changed, 21 insertions(+) diff --git a/prometheus/histogram.go b/prometheus/histogram.go index 997ea7b..a6ec4de 100644 --- a/prometheus/histogram.go +++ b/prometheus/histogram.go @@ -787,6 +787,16 @@ func (h *histogram) Write(out *dto.Metric) error { his.ZeroCount = proto.Uint64(zeroBucket) his.NegativeSpan, his.NegativeDelta = makeBuckets(&coldCounts.nativeHistogramBucketsNegative) his.PositiveSpan, his.PositiveDelta = makeBuckets(&coldCounts.nativeHistogramBucketsPositive) + + // Add a no-op span to a histogram without observations and with + // a zero threshold of zero. Otherwise, a native histogram would + // look like a classic histogram to scrapers. + if *his.ZeroThreshold == 0 && *his.ZeroCount == 0 && len(his.PositiveSpan) == 0 && len(his.NegativeSpan) == 0 { + his.PositiveSpan = []*dto.BucketSpan{{ + Offset: proto.Int32(0), + Length: proto.Uint32(0), + }} + } } addAndResetCounts(hotCounts, coldCounts) return nil diff --git a/prometheus/histogram_test.go b/prometheus/histogram_test.go index 6357c23..69ee883 100644 --- a/prometheus/histogram_test.go +++ b/prometheus/histogram_test.go @@ -485,6 +485,17 @@ func TestNativeHistogram(t *testing.T) { factor: 1, want: `sample_count:3 sample_sum:6 bucket: bucket: bucket: bucket: bucket: bucket: bucket: bucket: bucket: bucket: bucket: `, // Has conventional buckets because there are no sparse buckets. }, + { + name: "no observations", + factor: 1.1, + want: `sample_count:0 sample_sum:0 schema:3 zero_threshold:2.938735877055719e-39 zero_count:0 `, + }, + { + name: "no observations and zero threshold of zero resulting in no-op span", + factor: 1.1, + zeroThreshold: NativeHistogramZeroThresholdZero, + want: `sample_count:0 sample_sum:0 schema:3 zero_threshold:0 zero_count:0 positive_span: `, + }, { name: "factor 1.1 results in schema 3", observations: []float64{0, 1, 2, 3},