diff --git a/model/histogram/float_histogram.go b/model/histogram/float_histogram.go index 256679a8c..cd73083bb 100644 --- a/model/histogram/float_histogram.go +++ b/model/histogram/float_histogram.go @@ -192,6 +192,30 @@ func (h *FloatHistogram) Scale(factor float64) *FloatHistogram { // // This method returns a pointer to the receiving histogram for convenience. func (h *FloatHistogram) Add(other *FloatHistogram) *FloatHistogram { + switch { + case other.CounterResetHint == h.CounterResetHint: + // Adding apples to apples, all good. No need to change anything. + case h.CounterResetHint == GaugeType: + // Adding something else to a gauge. That's probably OK. Outcome is a gauge. + // Nothing to do since the receiver is already marked as gauge. + case other.CounterResetHint == GaugeType: + // Similar to before, but this time the receiver is "something else" and we have to change it to gauge. + h.CounterResetHint = GaugeType + case h.CounterResetHint == UnknownCounterReset: + // With the receiver's CounterResetHint being "unknown", this could still be legitimate + // if the caller knows what they are doing. Outcome is then again "unknown". + // No need to do anything since the receiver's CounterResetHint is already "unknown". + case other.CounterResetHint == UnknownCounterReset: + // Similar to before, but now we have to set the receiver's CounterResetHint to "unknown". + h.CounterResetHint = UnknownCounterReset + default: + // All other cases shouldn't actually happen. + // They are a direct collision of CounterReset and NotCounterReset. + // Conservatively set the CounterResetHint to "unknown" and isse a warning. + h.CounterResetHint = UnknownCounterReset + // TODO(trevorwhitney): Actually issue the warning as soon as the plumbing for it is in place + } + otherZeroCount := h.reconcileZeroBuckets(other) h.ZeroCount += otherZeroCount h.Count += other.Count @@ -414,6 +438,10 @@ func (h *FloatHistogram) Compact(maxEmptyBuckets int) *FloatHistogram { // of observations, but NOT the sum of observations) is smaller in the receiving // histogram compared to the previous histogram. Otherwise, it returns false. // +// This method will shortcut to true if a CounterReset is detected, and shortcut +// to false if NotCounterReset is detected. Otherwise it will do the work to detect +// a reset. +// // Special behavior in case the Schema or the ZeroThreshold are not the same in // both histograms: // @@ -432,12 +460,23 @@ func (h *FloatHistogram) Compact(maxEmptyBuckets int) *FloatHistogram { // - Upon a decrease of the Schema, the buckets of the previous histogram are // merged so that they match the new, lower-resolution schema (again without // mutating the provided previous histogram). -// -// Note that this kind of reset detection is quite expensive. Ideally, resets -// are detected at ingest time and stored in the TSDB, so that the reset -// information can be read directly from there rather than be detected each time -// again. func (h *FloatHistogram) DetectReset(previous *FloatHistogram) bool { + if h.CounterResetHint == CounterReset { + return true + } + if h.CounterResetHint == NotCounterReset { + return false + } + // In all other cases of CounterResetHint (UnknownCounterReset and GaugeType), + // we go on as we would otherwise, for reasons explained below. + // + // If the CounterResetHint is UnknownCounterReset, we do not know yet if this histogram comes + // with a counter reset. Therefore, we have to do all the detailed work to find out if there + // is a counter reset or not. + // We do the same if the CounterResetHint is GaugeType, which should not happen, but PromQL still + // allows the user to apply functions to gauge histograms that are only meant for counter histograms. + // In this case, we treat the gauge histograms as a counter histograms + // (and we plan to return a warning about it to the user). if h.Count < previous.Count { return true } diff --git a/promql/engine_test.go b/promql/engine_test.go index e2e209849..d1c4570ea 100644 --- a/promql/engine_test.go +++ b/promql/engine_test.go @@ -3155,8 +3155,7 @@ func TestNativeHistogramRate(t *testing.T) { require.Len(t, vector, 1) actualHistogram := vector[0].H expectedHistogram := &histogram.FloatHistogram{ - // TODO(beorn7): This should be GaugeType. Change it once supported by PromQL. - CounterResetHint: histogram.NotCounterReset, + CounterResetHint: histogram.GaugeType, Schema: 1, ZeroThreshold: 0.001, ZeroCount: 1. / 15., @@ -3200,8 +3199,7 @@ func TestNativeFloatHistogramRate(t *testing.T) { require.Len(t, vector, 1) actualHistogram := vector[0].H expectedHistogram := &histogram.FloatHistogram{ - // TODO(beorn7): This should be GaugeType. Change it once supported by PromQL. - CounterResetHint: histogram.NotCounterReset, + CounterResetHint: histogram.GaugeType, Schema: 1, ZeroThreshold: 0.001, ZeroCount: 1. / 15., diff --git a/promql/functions.go b/promql/functions.go index c5922002b..3da38ea0f 100644 --- a/promql/functions.go +++ b/promql/functions.go @@ -187,6 +187,7 @@ func histogramRate(points []Point, isCounter bool) *histogram.FloatHistogram { if curr == nil { return nil // Range contains a mix of histograms and floats. } + // TODO(trevorwhitney): Check if isCounter is consistent with curr.CounterResetHint. if !isCounter { continue } @@ -208,6 +209,8 @@ func histogramRate(points []Point, isCounter bool) *histogram.FloatHistogram { prev = curr } } + + h.CounterResetHint = histogram.GaugeType return h.Compact(0) } diff --git a/rules/manager.go b/rules/manager.go index 6f6ce2cfe..f8dcf081f 100644 --- a/rules/manager.go +++ b/rules/manager.go @@ -31,7 +31,6 @@ import ( "go.opentelemetry.io/otel/attribute" "go.opentelemetry.io/otel/codes" - "github.com/prometheus/prometheus/model/histogram" "github.com/prometheus/prometheus/model/labels" "github.com/prometheus/prometheus/model/rulefmt" "github.com/prometheus/prometheus/model/timestamp" @@ -671,9 +670,6 @@ func (g *Group) Eval(ctx context.Context, ts time.Time) { for _, s := range vector { if s.H != nil { - // We assume that all native histogram results are gauge histograms. - // TODO(codesome): once PromQL can give the counter reset info, remove this assumption. - s.H.CounterResetHint = histogram.GaugeType _, err = app.AppendHistogram(0, s.Metric, s.T, nil, s.H) } else { _, err = app.Append(0, s.Metric, s.T, s.V) diff --git a/rules/manager_test.go b/rules/manager_test.go index d287c25ce..aed289c5b 100644 --- a/rules/manager_test.go +++ b/rules/manager_test.go @@ -30,7 +30,6 @@ import ( "go.uber.org/goleak" "gopkg.in/yaml.v2" - "github.com/prometheus/prometheus/model/histogram" "github.com/prometheus/prometheus/model/labels" "github.com/prometheus/prometheus/model/rulefmt" "github.com/prometheus/prometheus/model/timestamp" @@ -1393,7 +1392,6 @@ func TestNativeHistogramsInRecordingRules(t *testing.T) { for _, h := range hists[1:] { expHist = expHist.Add(h.ToFloat()) } - expHist.CounterResetHint = histogram.GaugeType it := s.Iterator(nil) require.Equal(t, chunkenc.ValFloatHistogram, it.Next())