From 9ef5f90a767e6e4b42ffb2583014e07958519290 Mon Sep 17 00:00:00 2001 From: beorn7 Date: Tue, 20 Jul 2021 19:01:13 +0200 Subject: [PATCH] Allow a zero threshold of zero Signed-off-by: beorn7 --- prometheus/examples_test.go | 2 -- prometheus/histogram.go | 49 ++++++++++++++++++++---------------- prometheus/histogram_test.go | 2 +- 3 files changed, 29 insertions(+), 24 deletions(-) diff --git a/prometheus/examples_test.go b/prometheus/examples_test.go index bdcdfb4..a73ed18 100644 --- a/prometheus/examples_test.go +++ b/prometheus/examples_test.go @@ -538,8 +538,6 @@ func ExampleHistogram() { // cumulative_count: 816 // upper_bound: 40 // > - // sb_schema: 0 - // sb_zero_threshold: 0 // > } diff --git a/prometheus/histogram.go b/prometheus/histogram.go index 7dda036..37d3bb0 100644 --- a/prometheus/histogram.go +++ b/prometheus/histogram.go @@ -369,8 +369,13 @@ type HistogramOpts struct { // SparseBucketsZeroThreshold 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 - // SparseBucketsZeroThreshold is left at zero (or set to a negative - // value), DefSparseBucketsZeroThreshold is used as the threshold. + // SparseBucketsZeroThreshold is left at zero, + // DefSparseBucketsZeroThreshold is used as the threshold. If it is set + // to a negative value, a threshold of zero is used, i.e. only + // observations of precisely zero will go into the zero + // bucket. (TODO(beorn7): That's obviously weird and just a consequence + // of making the zero value of HistogramOpts meaningful. Has to be + // solved more elegantly in the final version.) SparseBucketsZeroThreshold float64 // TODO(beorn7): Need a setting to limit total bucket count and to // configure a strategy to enforce the limit, e.g. if minimum duration @@ -413,22 +418,24 @@ func newHistogram(desc *Desc, opts HistogramOpts, labelValues ...string) Histogr } h := &histogram{ - desc: desc, - upperBounds: opts.Buckets, - sparseThreshold: opts.SparseBucketsZeroThreshold, - labelPairs: MakeLabelPairs(desc, labelValues), - counts: [2]*histogramCounts{{}, {}}, - now: time.Now, + desc: desc, + upperBounds: opts.Buckets, + labelPairs: MakeLabelPairs(desc, labelValues), + counts: [2]*histogramCounts{{}, {}}, + now: time.Now, } if len(h.upperBounds) == 0 && opts.SparseBucketsFactor <= 1 { h.upperBounds = DefBuckets } - if h.sparseThreshold <= 0 { - h.sparseThreshold = DefSparseBucketsZeroThreshold - } if opts.SparseBucketsFactor <= 1 { - h.sparseThreshold = 0 // To mark that there are no sparse buckets. + h.sparseSchema = math.MinInt32 // To mark that there are no sparse buckets. } else { + switch { + case opts.SparseBucketsZeroThreshold > 0: + h.sparseThreshold = opts.SparseBucketsZeroThreshold + case opts.SparseBucketsZeroThreshold == 0: + h.sparseThreshold = DefSparseBucketsZeroThreshold + } // Leave h.sparseThreshold at 0 otherwise. h.sparseSchema = pickSparseSchema(opts.SparseBucketsFactor) } for i, upperBound := range h.upperBounds { @@ -559,8 +566,8 @@ type histogram struct { upperBounds []float64 labelPairs []*dto.LabelPair exemplars []atomic.Value // One more than buckets (to include +Inf), each a *dto.Exemplar. - sparseSchema int32 - sparseThreshold float64 // This is zero iff no sparse buckets are used. + sparseSchema int32 // Set to math.MinInt32 if no sparse buckets are used. + sparseThreshold float64 now func() time.Time // To mock out time.Now() for testing. } @@ -604,11 +611,9 @@ func (h *histogram) Write(out *dto.Metric) error { } his := &dto.Histogram{ - Bucket: make([]*dto.Bucket, len(h.upperBounds)), - SampleCount: proto.Uint64(count), - SampleSum: proto.Float64(math.Float64frombits(atomic.LoadUint64(&coldCounts.sumBits))), - SbSchema: &h.sparseSchema, - SbZeroThreshold: &h.sparseThreshold, + Bucket: make([]*dto.Bucket, len(h.upperBounds)), + SampleCount: proto.Uint64(count), + SampleSum: proto.Float64(math.Float64frombits(atomic.LoadUint64(&coldCounts.sumBits))), } out.Histogram = his out.Label = h.labelPairs @@ -648,7 +653,9 @@ func (h *histogram) Write(out *dto.Metric) error { atomic.AddUint64(&hotCounts.buckets[i], atomic.LoadUint64(&coldCounts.buckets[i])) atomic.StoreUint64(&coldCounts.buckets[i], 0) } - if h.sparseThreshold != 0 { + if h.sparseSchema > math.MinInt32 { + his.SbZeroThreshold = &h.sparseThreshold + his.SbSchema = &h.sparseSchema zeroBucket := atomic.LoadUint64(&coldCounts.sparseZeroBucket) defer func() { @@ -749,7 +756,7 @@ func (h *histogram) findBucket(v float64) int { // observe is the implementation for Observe without the findBucket part. func (h *histogram) observe(v float64, bucket int) { // Do not add to sparse buckets for NaN observations. - doSparse := h.sparseThreshold != 0 && !math.IsNaN(v) + doSparse := h.sparseSchema > math.MinInt32 && !math.IsNaN(v) var whichSparse, sparseKey int if doSparse { switch { diff --git a/prometheus/histogram_test.go b/prometheus/histogram_test.go index 61d8047..08e5e9e 100644 --- a/prometheus/histogram_test.go +++ b/prometheus/histogram_test.go @@ -470,7 +470,7 @@ func TestSparseHistogram(t *testing.T) { name: "no sparse buckets", observations: []float64{1, 2, 3}, factor: 1, - want: `sample_count:3 sample_sum:6 bucket: bucket: bucket: bucket: bucket: bucket: bucket: bucket: bucket: bucket: bucket: sb_schema:0 sb_zero_threshold:0 `, // Has conventional buckets because there are no sparse buckets. + 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: "factor 1.1 results in schema 3",