diff --git a/model/histogram/float_histogram.go b/model/histogram/float_histogram.go index 24e0689e7..0493984a1 100644 --- a/model/histogram/float_histogram.go +++ b/model/histogram/float_histogram.go @@ -419,23 +419,23 @@ func (h *FloatHistogram) NegativeBucketIterator() FloatBucketIterator { } // PositiveReverseBucketIterator returns a FloatBucketIterator to iterate over all -// positive buckets in decending order (starting at the highest bucket and going -// down upto zero bucket). +// positive buckets in descending order (starting at the highest bucket and going +// down towards the zero bucket). func (h *FloatHistogram) PositiveReverseBucketIterator() FloatBucketIterator { return newReverseFloatBucketIterator(h, true) } // NegativeReverseBucketIterator returns a FloatBucketIterator to iterate over all -// negative buckets in ascending order (starting at the lowest bucket and doing up -// upto zero bucket). +// negative buckets in ascending order (starting at the lowest bucket and going up +// towards the zero bucket). func (h *FloatHistogram) NegativeReverseBucketIterator() FloatBucketIterator { return newReverseFloatBucketIterator(h, false) } -// AllFloatBucketIterator returns a FloatBucketIterator to iterate over all -// negative, zero, and positive buckets in ascending order (starting at the -// lowest bucket and going up). -func (h *FloatHistogram) AllFloatBucketIterator() FloatBucketIterator { +// AllBucketIterator returns a FloatBucketIterator to iterate over all negative, +// zero, and positive buckets in ascending order (starting at the lowest bucket +// and going up). +func (h *FloatHistogram) AllBucketIterator() FloatBucketIterator { return newAllFloatBucketIterator(h) } @@ -657,7 +657,7 @@ type allFloatBucketIterator struct { h *FloatHistogram negIter, posIter FloatBucketIterator // -1 means we are iterating negative buckets. - // 0 means it is time for zero bucket. + // 0 means it is time for the zero bucket. // 1 means we are iterating positive buckets. // Anything else means iteration is over. state int8 diff --git a/model/histogram/float_histogram_test.go b/model/histogram/float_histogram_test.go index 7fe438562..5c8bce248 100644 --- a/model/histogram/float_histogram_test.go +++ b/model/histogram/float_histogram_test.go @@ -983,7 +983,7 @@ func TestAllFloatBucketIterator(t *testing.T) { } } - it := c.h.AllFloatBucketIterator() + it := c.h.AllBucketIterator() for it.Next() { actBuckets = append(actBuckets, it.At()) } diff --git a/promql/engine_test.go b/promql/engine_test.go index 4f6876901..3e07b4f4c 100644 --- a/promql/engine_test.go +++ b/promql/engine_test.go @@ -2860,10 +2860,13 @@ func TestSparseHistogram_HistogramQuantile(t *testing.T) { for i, c := range cases { t.Run(c.text, func(t *testing.T) { // TODO(codesome): Check if TSDB is handling these histograms properly. - // When testing, the 3rd case of both pos neg was getting no histograms in the query engine - // when the storage was shared even with good time gap between all histograms. - // It is possible that the recode is failing. It was fine between first 2 cases where there is - // a change of bucket layout. + // When testing, the 3rd case of both positive and + // negative buckets did not get any histograms in the + // query engine when the storage was shared, even with a + // good time gap between all histograms. It is possible + // that the recode is failing. It was fine between the + // first two cases where there is a change of bucket + // layout. test, err := NewTest(t, "") require.NoError(t, err) diff --git a/promql/functions.go b/promql/functions.go index 4a65659b1..8a1c890c1 100644 --- a/promql/functions.go +++ b/promql/functions.go @@ -883,7 +883,9 @@ func funcHistogramQuantile(vals []parser.Value, args parser.Expressions, enh *Ev if el.H != nil { // It's a histogram type. _, ok := enh.signatureToMetricWithBuckets[l] if ok { - // This signature exists for both conventional and new histograms which is not supported. + // This signature exists for both conventional + // and new histograms, which is not supported. + // TODO(beorn7): Issue a warning somehow. delete(enh.signatureToMetricWithBuckets, l) delete(enh.signatureToMetricWithHistograms, l) ignoreSignature[l] = true @@ -913,7 +915,9 @@ func funcHistogramQuantile(vals []parser.Value, args parser.Expressions, enh *Ev _, ok := enh.signatureToMetricWithHistograms[l] if ok { - // This signature exists for both conventional and new histograms which is not supported. + // This signature exists for both conventional and new + // histograms, which is not supported. + // TODO(beorn7): Issue a warning somehow. delete(enh.signatureToMetricWithBuckets, l) delete(enh.signatureToMetricWithHistograms, l) ignoreSignature[l] = true diff --git a/promql/quantile.go b/promql/quantile.go index 1b24a83f4..cb9783ced 100644 --- a/promql/quantile.go +++ b/promql/quantile.go @@ -121,27 +121,23 @@ func bucketQuantile(q float64, buckets buckets) float64 { } // histogramQuantile calculates the quantile 'q' based on the given histogram. -// The quantile value is interpolated assuming a linear distribution within a bucket. -// A natural lower bound of 0 is assumed if the upper bound of the -// lowest bucket is greater 0. In that case, interpolation in the lowest bucket -// happens linearly between 0 and the upper bound of the lowest bucket. -// However, if the lowest bucket has an upper bound less or equal 0, this upper -// bound is returned if the quantile falls into the lowest bucket. +// +// The quantile value is interpolated assuming a linear distribution within a +// bucket. +// TODO(beorn7): Find an interpolation method that is a better fit for +// exponential buckets (and think about configurable interpolation). +// +// A natural lower bound of 0 is assumed if the histogram has no negative buckets. // // There are a number of special cases (once we have a way to report errors // happening during evaluations of AST functions, we should report those // explicitly): // -// If 'buckets' has 0 observations, NaN is returned. +// If the histogram has 0 observations, NaN is returned. // // If q<0, -Inf is returned. // -// The following special cases are ignored from conventional histograms because -// we don't have a +Inf bucket in new histograms: -// If the highest bucket is not +Inf, NaN is returned. -// If 'buckets' has fewer than 2 elements, NaN is returned. -// -// TODO(codesome): Support negative buckets. +// If q>1, +Inf is returned. func histogramQuantile(q float64, h *histogram.FloatHistogram) float64 { if q < 0 { return math.Inf(-1) @@ -157,7 +153,7 @@ func histogramQuantile(q float64, h *histogram.FloatHistogram) float64 { var ( bucket *histogram.FloatBucket count float64 - it = h.AllFloatBucketIterator() + it = h.AllBucketIterator() rank = q * h.Count idx = -1 ) @@ -176,8 +172,9 @@ func histogramQuantile(q float64, h *histogram.FloatHistogram) float64 { } if idx == 0 && bucket.Lower < 0 && bucket.Upper > 0 { - // Zero bucket has the result and it happens to be the first bucket of this histogram. - // So we consider 0 to be the lower bound. + // Zero bucket has the result and it happens to be the first + // bucket of this histogram. So we consider 0 to be the lower + // bound. bucket.Lower = 0 }