Merge pull request #16682 from prometheus/krajo-fix-histogram-count-bug-main

fix(promql): histogram_count and histogram_sum inconsistent

promql/engine_test.go

@@ -37,6 +37,7 @@ import (
 	"github.com/prometheus/prometheus/promql/parser/posrange"
 	"github.com/prometheus/prometheus/promql/promqltest"
 	"github.com/prometheus/prometheus/storage"
+	"github.com/prometheus/prometheus/tsdb"
 	"github.com/prometheus/prometheus/tsdb/chunkenc"
 	"github.com/prometheus/prometheus/util/annotations"
 	"github.com/prometheus/prometheus/util/logging"
@@ -3744,3 +3745,98 @@ eval instant at 10m max_over_time(metric_total{env="1"}[10m])
 	{env="1"} 120
 `, engine)
 }
+
+// TestInconsistentHistogramCount is testing for
+// https://github.com/prometheus/prometheus/issues/16681 which needs mixed
+// integer and float histograms. The promql test framework only uses float
+// histograms, so we cannot move this to promql tests.
+func TestInconsistentHistogramCount(t *testing.T) {
+	dir := t.TempDir()
+
+	opts := tsdb.DefaultHeadOptions()
+	opts.EnableNativeHistograms.Store(true)
+	opts.ChunkDirRoot = dir
+	// We use TSDB head only. By using full TSDB DB, and appending samples to it, closing it would cause unnecessary HEAD compaction, which slows down the test.
+	head, err := tsdb.NewHead(nil, nil, nil, nil, opts, nil)
+	require.NoError(t, err)
+	t.Cleanup(func() {
+		_ = head.Close()
+	})
+
+	app := head.Appender(context.Background())
+
+	l := labels.FromStrings("__name__", "series_1")
+
+	mint := time.Now().Add(-time.Hour).UnixMilli()
+	maxt := mint
+	dT := int64(15 * 1000) // 15 seconds in milliseconds.
+	inHs := []*histogram.Histogram{
+		{
+			Count: 2,
+			Sum:   100,
+			PositiveSpans: []histogram.Span{
+				{Offset: 0, Length: 1},
+			},
+			PositiveBuckets: []int64{2},
+		},
+		{
+			Count: 4,
+			Sum:   200,
+			PositiveSpans: []histogram.Span{
+				{Offset: 0, Length: 1},
+			},
+			PositiveBuckets: []int64{4},
+		},
+		{}, // Empty histogram to trigger counter reset.
+	}
+
+	for i, h := range inHs {
+		maxt = mint + dT*int64(i)
+		_, err = app.AppendHistogram(0, l, maxt, h, nil)
+		require.NoError(t, err)
+	}
+
+	require.NoError(t, app.Commit())
+	queryable := storage.QueryableFunc(func(mint, maxt int64) (storage.Querier, error) {
+		return tsdb.NewBlockQuerier(head, mint, maxt)
+	})
+
+	engine := promql.NewEngine(promql.EngineOpts{
+		MaxSamples:    1000000,
+		Timeout:       10 * time.Second,
+		LookbackDelta: 5 * time.Minute,
+	})
+
+	var (
+		query       promql.Query
+		queryResult *promql.Result
+		v           promql.Vector
+	)
+
+	query, err = engine.NewInstantQuery(context.Background(), queryable, nil, "(rate(series_1[1m]))", time.UnixMilli(maxt))
+	require.NoError(t, err)
+	queryResult = query.Exec(context.Background())
+	require.NoError(t, queryResult.Err)
+	require.NotNil(t, queryResult)
+	v, err = queryResult.Vector()
+	require.NoError(t, err)
+	require.Len(t, v, 1)
+	require.NotNil(t, v[0].H)
+	require.Greater(t, v[0].H.Count, float64(0))
+	query.Close()
+	countFromHistogram := v[0].H.Count
+
+	query, err = engine.NewInstantQuery(context.Background(), queryable, nil, "histogram_count((rate(series_1[1m])))", time.UnixMilli(maxt))
+	require.NoError(t, err)
+	queryResult = query.Exec(context.Background())
+	require.NoError(t, queryResult.Err)
+	require.NotNil(t, queryResult)
+	v, err = queryResult.Vector()
+	require.NoError(t, err)
+	require.Len(t, v, 1)
+	require.Nil(t, v[0].H)
+	query.Close()
+	countFromFunction := float64(v[0].F)
+
+	require.Equal(t, countFromHistogram, countFromFunction, "histogram_count function should return the same count as the histogram itself")
+}

promql/histogram_stats_iterator.go

@@ -105,6 +105,7 @@ func (f *histogramStatsIterator) AtFloatHistogram(fh *histogram.FloatHistogram)
 }
 
 func (f *histogramStatsIterator) setLastH(h *histogram.Histogram) {
+	f.lastFH = nil
 	if f.lastH == nil {
 		f.lastH = h.Copy()
 	} else {
@@ -113,6 +114,7 @@ func (f *histogramStatsIterator) setLastH(h *histogram.Histogram) {
 }
 
 func (f *histogramStatsIterator) setLastFH(fh *histogram.FloatHistogram) {
+	f.lastH = nil
 	if f.lastFH == nil {
 		f.lastFH = fh.Copy()
 	} else {
@@ -124,11 +126,15 @@ func (f *histogramStatsIterator) getFloatResetHint(hint histogram.CounterResetHi
 	if hint != histogram.UnknownCounterReset {
 		return hint
 	}
-	if f.lastFH == nil {
+	prevFH := f.lastFH
-		return histogram.NotCounterReset
+	if prevFH == nil {
+		if f.lastH == nil {
+			// We don't know if there's a counter reset.
+			return histogram.UnknownCounterReset
 		}
-
+		prevFH = f.lastH.ToFloat(nil)
-	if f.currentFH.DetectReset(f.lastFH) {
+	}
+	if f.currentFH.DetectReset(prevFH) {
 		return histogram.CounterReset
 	}
 	return histogram.NotCounterReset
@@ -138,11 +144,17 @@ func (f *histogramStatsIterator) getResetHint(h *histogram.Histogram) histogram.
 	if h.CounterResetHint != histogram.UnknownCounterReset {
 		return h.CounterResetHint
 	}
+	var prevFH *histogram.FloatHistogram
 	if f.lastH == nil {
-		return histogram.NotCounterReset
+		if f.lastFH == nil {
+			// We don't know if there's a counter reset.
+			return histogram.UnknownCounterReset
 		}
-
+		prevFH = f.lastFH
-	fh, prevFH := h.ToFloat(nil), f.lastH.ToFloat(nil)
+	} else {
+		prevFH = f.lastH.ToFloat(nil)
+	}
+	fh := h.ToFloat(nil)
 	if fh.DetectReset(prevFH) {
 		return histogram.CounterReset
 	}

promql/histogram_stats_iterator_test.go

@@ -68,7 +68,7 @@ func TestHistogramStatsDecoding(t *testing.T) {
 				tsdbutil.GenerateTestHistogramWithHint(1, histogram.UnknownCounterReset),
 			},
 			expectedHints: []histogram.CounterResetHint{
-				histogram.NotCounterReset,
+				histogram.UnknownCounterReset,
 			},
 		},
 		{
@@ -78,7 +78,7 @@ func TestHistogramStatsDecoding(t *testing.T) {
 				tsdbutil.GenerateTestHistogramWithHint(1, histogram.UnknownCounterReset),
 			},
 			expectedHints: []histogram.CounterResetHint{
-				histogram.NotCounterReset,
+				histogram.UnknownCounterReset,
 				histogram.CounterReset,
 			},
 		},
@@ -90,7 +90,7 @@ func TestHistogramStatsDecoding(t *testing.T) {
 				tsdbutil.GenerateTestHistogramWithHint(1, histogram.UnknownCounterReset),
 			},
 			expectedHints: []histogram.CounterResetHint{
-				histogram.NotCounterReset,
+				histogram.UnknownCounterReset,
 				histogram.UnknownCounterReset,
 				histogram.CounterReset,
 			},
@@ -141,6 +141,41 @@ func TestHistogramStatsDecoding(t *testing.T) {
 	}
 }
 
+func TestHistogramStatsMixedUse(t *testing.T) {
+	histograms := []*histogram.Histogram{
+		tsdbutil.GenerateTestHistogramWithHint(2, histogram.UnknownCounterReset),
+		tsdbutil.GenerateTestHistogramWithHint(4, histogram.UnknownCounterReset),
+		tsdbutil.GenerateTestHistogramWithHint(0, histogram.UnknownCounterReset),
+	}
+
+	series := newHistogramSeries(histograms)
+	it := series.Iterator(nil)
+
+	statsIterator := NewHistogramStatsIterator(it)
+
+	expectedHints := []histogram.CounterResetHint{
+		histogram.UnknownCounterReset,
+		histogram.NotCounterReset,
+		histogram.CounterReset,
+	}
+	actualHints := make([]histogram.CounterResetHint, 3)
+	typ := statsIterator.Next()
+	require.Equal(t, chunkenc.ValHistogram, typ)
+	_, h := statsIterator.AtHistogram(nil)
+	actualHints[0] = h.CounterResetHint
+	typ = statsIterator.Next()
+	require.Equal(t, chunkenc.ValHistogram, typ)
+	_, h = statsIterator.AtHistogram(nil)
+	actualHints[1] = h.CounterResetHint
+	typ = statsIterator.Next()
+	require.Equal(t, chunkenc.ValHistogram, typ)
+	_, fh := statsIterator.AtFloatHistogram(nil)
+	actualHints[2] = fh.CounterResetHint
+
+	require.Equal(t, chunkenc.ValNone, statsIterator.Next())
+	require.Equal(t, expectedHints, actualHints)
+}
+
 type histogramSeries struct {
 	histograms []*histogram.Histogram
 }