histograms: Doc comment and naming improvements

Signed-off-by: beorn7 <beorn@grafana.com>

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
+// positive buckets in descending order (starting at the highest bucket and going
-// down upto zero bucket).
+// 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
+// negative buckets in ascending order (starting at the lowest bucket and going up
-// upto zero bucket).
+// towards the zero bucket).
 func (h *FloatHistogram) NegativeReverseBucketIterator() FloatBucketIterator {
 	return newReverseFloatBucketIterator(h, false)
 }
 
-// AllFloatBucketIterator returns a FloatBucketIterator to iterate over all
+// AllBucketIterator returns a FloatBucketIterator to iterate over all negative,
-// negative, zero, and positive buckets in ascending order (starting at the
+// zero, and positive buckets in ascending order (starting at the lowest bucket
-// lowest bucket and going up).
+// and going up).
-func (h *FloatHistogram) AllFloatBucketIterator() FloatBucketIterator {
+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

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())
 			}

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 testing, the 3rd case of both positive and
-			// when the storage was shared even with good time gap between all histograms.
+			// negative buckets did not get any histograms in the
-			// It is possible that the recode is failing. It was fine between first 2 cases where there is
+			// query engine when the storage was shared, even with a
-			// a change of bucket layout.
+			// 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)

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

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
+// The quantile value is interpolated assuming a linear distribution within a
-// lowest bucket is greater 0. In that case, interpolation in the lowest bucket
+// bucket.
-// happens linearly between 0 and the upper bound of the lowest bucket.
+// TODO(beorn7): Find an interpolation method that is a better fit for
-// However, if the lowest bucket has an upper bound less or equal 0, this upper
+// exponential buckets (and think about configurable interpolation).
-// bound is returned if the quantile falls into the lowest bucket.
+//
+// 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
+// If q>1, +Inf is returned.
-// 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.
 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.
+		// Zero bucket has the result and it happens to be the first
-		// So we consider 0 to be the lower bound.
+		// bucket of this histogram.  So we consider 0 to be the lower
+		// bound.
 		bucket.Lower = 0
 	}