fix according to code review

Signed-off-by: Jeanette Tan <jeanette.tan@grafana.com>

model/histogram/float_histogram.go

@@ -30,7 +30,7 @@ import (
 type FloatHistogram struct {
 	// Counter reset information.
 	CounterResetHint CounterResetHint
-	// Currently valid schema numbers are -4 <= n <= 8 for exponential buckets,
+	// Currently valid schema numbers are -4 <= n <= 8 for exponential buckets.
 	// They are all for base-2 bucket schemas, where 1 is a bucket boundary in
 	// each case, and then each power of two is divided into 2^n logarithmic buckets.
 	// Or in other words, each bucket boundary is the previous boundary times
@@ -54,7 +54,7 @@ type FloatHistogram struct {
 	// This slice is interned, to be treated as immutable and copied by reference.
 	// These numbers should be strictly increasing. This field is only used when the
 	// schema is for custom buckets, and the ZeroThreshold, ZeroCount, NegativeSpans
-	// and NegativeBuckets fields are not used.
+	// and NegativeBuckets fields are not used in that case.
 	CustomValues []float64
 }
 
@@ -141,7 +141,8 @@ func (h *FloatHistogram) CopyTo(to *FloatHistogram) {
 
 // CopyToSchema works like Copy, but the returned deep copy has the provided
 // target schema, which must be ≤ the original schema (i.e. it must have a lower
-// resolution).
+// resolution). This method panics if a custom buckets schema is used in the
+// receiving FloatHistogram or as the provided targetSchema.
 func (h *FloatHistogram) CopyToSchema(targetSchema int32) *FloatHistogram {
 	if targetSchema == h.Schema {
 		// Fast path.
@@ -253,7 +254,7 @@ func (h *FloatHistogram) TestExpression() string {
 	return "{{" + strings.Join(res, " ") + "}}"
 }
 
-// ZeroBucket returns the zero bucket.
+// ZeroBucket returns the zero bucket. This method panics if the schema is for custom buckets.
 func (h *FloatHistogram) ZeroBucket() Bucket[float64] {
 	if h.UsesCustomBuckets() {
 		panic("histograms with custom buckets have no zero bucket")
@@ -922,7 +923,8 @@ func (h *FloatHistogram) reconcileZeroBuckets(other *FloatHistogram) float64 {
 //
 // targetSchema must be ≤ the schema of FloatHistogram (and of course within the
 // legal values for schemas in general). The buckets are merged to match the
-// targetSchema prior to iterating (without mutating FloatHistogram).
+// targetSchema prior to iterating (without mutating FloatHistogram), but custom buckets
+// schemas cannot be merged with other schemas.
 func (h *FloatHistogram) floatBucketIterator(
 	positive bool, absoluteStartValue float64, targetSchema int32,
 ) floatBucketIterator {
@@ -1317,6 +1319,8 @@ func FloatBucketsMatch(b1, b2 []float64) bool {
 
 // ReduceResolution reduces the float histogram's spans, buckets into target schema.
 // The target schema must be smaller than the current float histogram's schema.
+// This will panic if the histogram has custom buckets or if the target schema is
+// a custom buckets schema.
 func (h *FloatHistogram) ReduceResolution(targetSchema int32) *FloatHistogram {
 	if h.UsesCustomBuckets() {
 		panic("cannot reduce resolution when there are custom buckets")

model/histogram/generic.go

@@ -777,3 +777,10 @@ func reduceResolution[IBC InternalBucketCount](
 
 	return targetSpans, targetBuckets
 }
+
+func clearIfNotNil[T any](items []T) []T {
+	if items == nil {
+		return nil
+	}
+	return items[:0]
+}

model/histogram/histogram.go

@@ -74,7 +74,7 @@ type Histogram struct {
 	// This slice is interned, to be treated as immutable and copied by reference.
 	// These numbers should be strictly increasing. This field is only used when the
 	// schema is for custom buckets, and the ZeroThreshold, ZeroCount, NegativeSpans
-	// and NegativeBuckets fields are not used.
+	// and NegativeBuckets fields are not used in that case.
 	CustomValues []float64
 }
 
@@ -199,7 +199,7 @@ func (h *Histogram) String() string {
 	return sb.String()
 }
 
-// ZeroBucket returns the zero bucket.
+// ZeroBucket returns the zero bucket. This method panics if the schema is for custom buckets.
 func (h *Histogram) ZeroBucket() Bucket[uint64] {
 	if h.UsesCustomBuckets() {
 		panic("histograms with custom buckets have no zero bucket")
@@ -417,13 +417,6 @@ func resize[T any](items []T, n int) []T {
 	return items[:n]
 }
 
-func clearIfNotNil[T any](items []T) []T {
-	if items == nil {
-		return nil
-	}
-	return items[:0]
-}
-
 // Validate validates consistency between span and bucket slices. Also, buckets are checked
 // against negative values. We check to make sure there are no unexpected fields or field values
 // based on the exponential / custom buckets schema.
@@ -615,6 +608,8 @@ func (c *cumulativeBucketIterator) At() Bucket[uint64] {
 
 // ReduceResolution reduces the histogram's spans, buckets into target schema.
 // The target schema must be smaller than the current histogram's schema.
+// This will panic if the histogram has custom buckets or if the target schema is
+// a custom buckets schema.
 func (h *Histogram) ReduceResolution(targetSchema int32) *Histogram {
 	if h.UsesCustomBuckets() {
 		panic("cannot reduce resolution when there are custom buckets")

promql/promqltest/test.go

@@ -208,14 +208,14 @@ func parseLoad(lines []string, i int) (int, *loadCmd, error) {
 	}
 	parts := patLoad.FindStringSubmatch(lines[i])
 	var (
-		withNhcb = parts[1] == "with_nhcb"
+		withNHCB = parts[1] == "with_nhcb"
 		step     = parts[2]
 	)
 	gap, err := model.ParseDuration(step)
 	if err != nil {
 		return i, nil, raise(i, "invalid step definition %q: %s", step, err)
 	}
-	cmd := newLoadCmd(time.Duration(gap), withNhcb)
+	cmd := newLoadCmd(time.Duration(gap), withNHCB)
 	for i+1 < len(lines) {
 		i++
 		defLine := lines[i]
@@ -253,12 +253,12 @@ func (t *test) parseEval(lines []string, i int) (int, *evalCmd, error) {
 
 	isInstant := instantParts != nil
 
-	var withNhcb bool
+	var withNHCB bool
 	var mod string
 	var expr string
 
 	if isInstant {
-		withNhcb = instantParts[1] == "with_nhcb"
+		withNHCB = instantParts[1] == "with_nhcb"
 		mod = instantParts[2]
 		expr = instantParts[4]
 	} else {
@@ -332,7 +332,7 @@ func (t *test) parseEval(lines []string, i int) (int, *evalCmd, error) {
 	case "warn":
 		cmd.warn = true
 	}
-	cmd.withNhcb = withNhcb
+	cmd.withNHCB = withNHCB
 
 	for j := 1; i+1 < len(lines); j++ {
 		i++
@@ -419,16 +419,16 @@ type loadCmd struct {
 	metrics   map[uint64]labels.Labels
 	defs      map[uint64][]promql.Sample
 	exemplars map[uint64][]exemplar.Exemplar
-	withNhcb  bool
+	withNHCB  bool
 }
 
-func newLoadCmd(gap time.Duration, withNhcb bool) *loadCmd {
+func newLoadCmd(gap time.Duration, withNHCB bool) *loadCmd {
 	return &loadCmd{
 		gap:       gap,
 		metrics:   map[uint64]labels.Labels{},
 		defs:      map[uint64][]promql.Sample{},
 		exemplars: map[uint64][]exemplar.Exemplar{},
-		withNhcb:  withNhcb,
+		withNHCB:  withNHCB,
 	}
 }
 
@@ -467,7 +467,7 @@ func (cmd *loadCmd) append(a storage.Appender) error {
 			}
 		}
 	}
-	if cmd.withNhcb {
+	if cmd.withNHCB {
 		return cmd.appendCustomHistogram(a)
 	}
 	return nil
@@ -535,11 +535,11 @@ func processClassicHistogramSeries(m labels.Labels, suffix string, histMap map[u
 func processUpperBoundsAndCreateBaseHistogram(upperBounds0 []float64) ([]float64, *histogram.FloatHistogram) {
 	sort.Float64s(upperBounds0)
 	upperBounds := make([]float64, 0, len(upperBounds0))
-	prevLe := math.Inf(-1)
+	prevLE := math.Inf(-1)
 	for _, le := range upperBounds0 {
-		if le != prevLe { // deduplicate
+		if le != prevLE { // deduplicate
 			upperBounds = append(upperBounds, le)
-			prevLe = le
+			prevLE = le
 		}
 	}
 	var customBounds []float64
@@ -655,7 +655,7 @@ type evalCmd struct {
 
 	isRange             bool // if false, instant query
 	fail, warn, ordered bool
-	withNhcb            bool
+	withNHCB            bool
 
 	metrics  map[uint64]labels.Labels
 	expected map[uint64]entry
@@ -1028,7 +1028,7 @@ func (t *test) execInstantEval(cmd *evalCmd, engine promql.QueryEngine) error {
 		if err := t.runInstantQuery(iq, cmd, engine); err != nil {
 			return err
 		}
-		if cmd.withNhcb {
+		if cmd.withNHCB {
 			if !strings.Contains(iq.expr, "_bucket") {
 				return fmt.Errorf("expected '_bucket' in the expression %q", iq.expr)
 			}

rules/manager_test.go

@@ -1400,7 +1400,8 @@ func TestNativeHistogramsInRecordingRules(t *testing.T) {
 
 	expHist := hists[0].ToFloat(nil)
 	for _, h := range hists[1:] {
-		expHist, _ = expHist.Add(h.ToFloat(nil))
+		expHist, err = expHist.Add(h.ToFloat(nil))
+		require.NoError(t, err)
 	}
 
 	it := s.Iterator(nil)

scrape/target.go

@@ -365,16 +365,22 @@ type bucketLimitAppender struct {
 
 func (app *bucketLimitAppender) AppendHistogram(ref storage.SeriesRef, lset labels.Labels, t int64, h *histogram.Histogram, fh *histogram.FloatHistogram) (storage.SeriesRef, error) {
 	if h != nil {
+		if len(h.PositiveBuckets)+len(h.NegativeBuckets) > app.limit && h.Schema > histogram.ExponentialSchemaMax {
+			return 0, errBucketLimit
+		}
 		for len(h.PositiveBuckets)+len(h.NegativeBuckets) > app.limit {
-			if h.Schema <= histogram.ExponentialSchemaMin || h.Schema > histogram.ExponentialSchemaMax {
+			if h.Schema <= histogram.ExponentialSchemaMin {
 				return 0, errBucketLimit
 			}
 			h = h.ReduceResolution(h.Schema - 1)
 		}
 	}
 	if fh != nil {
+		if len(fh.PositiveBuckets)+len(fh.NegativeBuckets) > app.limit && fh.Schema > histogram.ExponentialSchemaMax {
+			return 0, errBucketLimit
+		}
 		for len(fh.PositiveBuckets)+len(fh.NegativeBuckets) > app.limit {
-			if fh.Schema <= histogram.ExponentialSchemaMin || fh.Schema > histogram.ExponentialSchemaMax {
+			if fh.Schema <= histogram.ExponentialSchemaMin {
 				return 0, errBucketLimit
 			}
 			fh = fh.ReduceResolution(fh.Schema - 1)

tsdb/chunkenc/histogram_meta.go

@@ -199,7 +199,7 @@ func isWholeWhenMultiplied(in float64) bool {
 // we can convert the value to a float64 by subtracting 1 and dividing by 1000.
 func putCustomBound(b *bstream, f float64) {
 	tf := f * 1000
-	// 33554431-1 comes from the maximum that can be stored in a varint in 4
+	// 33554431-1 comes from the maximum that can be stored in a varbit in 4
 	// bytes, other values are stored in 8 bytes anyway.
 	if tf < 0 || tf > 33554430 || !isWholeWhenMultiplied(f) {
 		b.writeBit(zero)