Let HistogramAppender.appendable return CounterResetHeader instead of… (#16195)

Let HistogramAppender.appendable return CounterResetHeader instead of boolean Signed-off-by: Ziqi Zhao <zhaoziqi9146@gmail.com> Signed-off-by: Björn Rabenstein <github@rabenste.in> --------- Signed-off-by: Ziqi Zhao <zhaoziqi9146@gmail.com> Signed-off-by: Björn Rabenstein <github@rabenste.in> Co-authored-by: Björn Rabenstein <github@rabenste.in>
2025-08-06 22:27:17 +02:00 · 2025-03-19 00:40:27 +08:00 · 2025-03-19 00:40:27 +08:00 · f6903bcc22
commit f6903bcc22
parent d77d2a2d31
3 changed files with 68 additions and 30 deletions
--- a/tsdb/chunkenc/histogram.go
+++ b/tsdb/chunkenc/histogram.go
@ -262,17 +262,23 @@ func (a *HistogramAppender) Append(int64, float64) {
 // The method returns an additional boolean set to true if it is not appendable
 // because of a counter reset. If the given sample is stale, it is always ok to
 // append. If counterReset is true, okToAppend is always false.
+//
+// The method returns an additional CounterResetHeader value that indicates the
+// status of the counter reset detection. But it returns UnknownCounterReset
+// when schema or zero threshold changed, because we don't do a full counter
+// reset detection.
 func (a *HistogramAppender) appendable(h *histogram.Histogram) (
 	positiveInserts, negativeInserts []Insert,
 	backwardPositiveInserts, backwardNegativeInserts []Insert,
-	okToAppend, counterReset bool,
+	okToAppend bool, counterResetHint CounterResetHeader,
 ) {
+	counterResetHint = NotCounterReset
 	if a.NumSamples() > 0 && a.GetCounterResetHeader() == GaugeType {
 		return
 	}
 	if h.CounterResetHint == histogram.CounterReset {
 		// Always honor the explicit counter reset hint.
-		counterReset = true
+		counterResetHint = CounterReset
 		return
 	}
 	if value.IsStaleNaN(h.Sum) {
@ -283,39 +289,45 @@ func (a *HistogramAppender) appendable(h *histogram.Histogram) (
 	if value.IsStaleNaN(a.sum) {
 		// If the last sample was stale, then we can only accept stale
 		// samples in this chunk.
+		counterResetHint = UnknownCounterReset
 		return
 	}

 	if h.Count < a.cnt {
 		// There has been a counter reset.
-		counterReset = true
+		counterResetHint = CounterReset
 		return
 	}

 	if h.Schema != a.schema || h.ZeroThreshold != a.zThreshold {
+		// This case might or might not go along with a counter reset and
+		// we do not want to invest the work of a full counter reset detection
+		// as long as https://github.com/prometheus/prometheus/issues/15346 is still open.
+		// TODO: consider adding the counter reset detection here once #15346 is fixed.
+		counterResetHint = UnknownCounterReset
 		return
 	}

 	if histogram.IsCustomBucketsSchema(h.Schema) && !histogram.FloatBucketsMatch(h.CustomValues, a.customValues) {
-		counterReset = true
+		counterResetHint = CounterReset
 		return
 	}

 	if h.ZeroCount < a.zCnt {
 		// There has been a counter reset since ZeroThreshold didn't change.
-		counterReset = true
+		counterResetHint = CounterReset
 		return
 	}

 	var ok bool
 	positiveInserts, backwardPositiveInserts, ok = expandIntSpansAndBuckets(a.pSpans, h.PositiveSpans, a.pBuckets, h.PositiveBuckets)
 	if !ok {
-		counterReset = true
+		counterResetHint = CounterReset
 		return
 	}
 	negativeInserts, backwardNegativeInserts, ok = expandIntSpansAndBuckets(a.nSpans, h.NegativeSpans, a.nBuckets, h.NegativeBuckets)
 	if !ok {
-		counterReset = true
+		counterResetHint = CounterReset
 		return
 	}

@ -781,21 +793,17 @@ func (a *HistogramAppender) AppendHistogram(prev *HistogramAppender, t int64, h
 		case prev != nil:
 			// This is a new chunk, but continued from a previous one. We need to calculate the reset header unless already set.
 			_, _, _, _, _, counterReset := prev.appendable(h)
-			if counterReset {
-				a.setCounterResetHeader(CounterReset)
-			} else {
-				a.setCounterResetHeader(NotCounterReset)
-			}
+			a.setCounterResetHeader(counterReset)
 		}
 		return nil, false, a, nil
 	}

 	// Adding counter-like histogram.
 	if h.CounterResetHint != histogram.GaugeType {
-		pForwardInserts, nForwardInserts, pBackwardInserts, nBackwardInserts, okToAppend, counterReset := a.appendable(h)
-		if !okToAppend || counterReset {
+		pForwardInserts, nForwardInserts, pBackwardInserts, nBackwardInserts, okToAppend, counterResetHint := a.appendable(h)
+		if !okToAppend || counterResetHint != NotCounterReset {
 			if appendOnly {
-				if counterReset {
+				if counterResetHint == CounterReset {
 					return nil, false, a, errors.New("histogram counter reset")
 				}
 				return nil, false, a, errors.New("histogram schema change")
@ -806,9 +814,7 @@ func (a *HistogramAppender) AppendHistogram(prev *HistogramAppender, t int64, h
 				panic(err) // This should never happen for an empty histogram chunk.
 			}
 			happ := app.(*HistogramAppender)
-			if counterReset {
-				happ.setCounterResetHeader(CounterReset)
-			}
+			happ.setCounterResetHeader(counterResetHint)
 			happ.appendHistogram(t, h)
 			return newChunk, false, app, nil
 		}
--- a/tsdb/chunkenc/histogram_test.go
+++ b/tsdb/chunkenc/histogram_test.go
@ -268,7 +268,7 @@ func TestHistogramChunkBucketChanges(t *testing.T) {
 	require.Empty(t, backwardPositiveInserts)
 	require.Empty(t, backwardNegativeInserts)
 	require.True(t, ok) // Only new buckets came in.
-	require.False(t, cr)
+	require.Equal(t, NotCounterReset, cr)
 	c, app = hApp.recode(posInterjections, negInterjections, h2.PositiveSpans, h2.NegativeSpans)
 	chk, _, _, err = app.AppendHistogram(nil, ts2, h2, false)
 	require.NoError(t, err)
@ -394,7 +394,7 @@ func TestHistogramChunkAppendable(t *testing.T) {
 		require.Empty(t, backwardPositiveInserts)
 		require.Empty(t, backwardNegativeInserts)
 		require.True(t, ok) // Only new buckets came in.
-		require.False(t, cr)
+		require.Equal(t, NotCounterReset, cr)

 		assertRecodedHistogramChunkOnAppend(t, c, hApp, ts+1, h2, UnknownCounterReset)
 	}
@ -417,7 +417,7 @@ func TestHistogramChunkAppendable(t *testing.T) {
 		require.Empty(t, backwardPositiveInserts)
 		require.Empty(t, backwardNegativeInserts)
 		require.False(t, ok) // Need to cut a new chunk.
-		require.True(t, cr)
+		require.Equal(t, CounterReset, cr)

 		assertNewHistogramChunkOnAppend(t, c, hApp, ts+1, h2, CounterReset, histogram.UnknownCounterReset)
 	}
@ -443,7 +443,7 @@ func TestHistogramChunkAppendable(t *testing.T) {
 		require.NotEmpty(t, backwardPositiveInserts)
 		require.Empty(t, backwardNegativeInserts)
 		require.True(t, ok)
-		require.False(t, cr)
+		require.Equal(t, NotCounterReset, cr)

 		assertNoNewHistogramChunkOnAppend(t, c, hApp, ts+1, h2, UnknownCounterReset)

@ -474,7 +474,7 @@ func TestHistogramChunkAppendable(t *testing.T) {
 		require.NotEmpty(t, backwardPositiveInserts)
 		require.Empty(t, backwardNegativeInserts)
 		require.True(t, ok)
-		require.False(t, cr)
+		require.Equal(t, NotCounterReset, cr)

 		assertRecodedHistogramChunkOnAppend(t, c, hApp, ts+1, h2, UnknownCounterReset)

@ -502,7 +502,7 @@ func TestHistogramChunkAppendable(t *testing.T) {
 		require.Empty(t, backwardPositiveInserts)
 		require.Empty(t, backwardNegativeInserts)
 		require.False(t, ok) // Need to cut a new chunk.
-		require.True(t, cr)
+		require.Equal(t, CounterReset, cr)

 		assertNewHistogramChunkOnAppend(t, c, hApp, ts+1, h2, CounterReset, histogram.UnknownCounterReset)
 	}
@ -528,7 +528,7 @@ func TestHistogramChunkAppendable(t *testing.T) {
 		require.Empty(t, backwardPositiveInserts)
 		require.Empty(t, backwardNegativeInserts)
 		require.False(t, ok) // Need to cut a new chunk.
-		require.True(t, cr)
+		require.Equal(t, CounterReset, cr)

 		assertNewHistogramChunkOnAppend(t, c, hApp, ts+1, h2, CounterReset, histogram.UnknownCounterReset)
 	}
@ -560,7 +560,7 @@ func TestHistogramChunkAppendable(t *testing.T) {
 		require.Empty(t, backwardPositiveInserts)
 		require.Empty(t, backwardNegativeInserts)
 		require.False(t, ok) // Need to cut a new chunk.
-		require.True(t, cr)
+		require.Equal(t, CounterReset, cr)

 		assertNewHistogramChunkOnAppend(t, c, hApp, ts+1, h2, CounterReset, histogram.UnknownCounterReset)
 	}
@ -667,7 +667,7 @@ func TestHistogramChunkAppendable(t *testing.T) {
 		require.NotEmpty(t, backwardPositiveInserts)
 		require.Empty(t, backwardNegativeInserts)
 		require.True(t, ok)
-		require.False(t, cr)
+		require.Equal(t, NotCounterReset, cr)

 		assertNoNewHistogramChunkOnAppend(t, c, hApp, ts+1, h2, UnknownCounterReset)
 	}
@ -735,10 +735,42 @@ func TestHistogramChunkAppendable(t *testing.T) {
 		require.Empty(t, backwardPositiveInserts)
 		require.Empty(t, backwardNegativeInserts)
 		require.True(t, ok) // Only new buckets came in.
-		require.False(t, cr)
+		require.Equal(t, NotCounterReset, cr)

 		assertRecodedHistogramChunkOnAppend(t, c, hApp, ts+1, h2, UnknownCounterReset)
 	}
+
+	{ // New histogram with a different schema.
+		c, hApp, ts, h1 := setup(eh)
+		h2 := h1.Copy()
+		h2.Schema = 2
+
+		posInterjections, negInterjections, backwardPositiveInserts, backwardNegativeInserts, ok, cr := hApp.appendable(h2)
+		require.Empty(t, posInterjections)
+		require.Empty(t, negInterjections)
+		require.Empty(t, backwardPositiveInserts)
+		require.Empty(t, backwardNegativeInserts)
+		require.False(t, ok) // Need to cut a new chunk.
+		require.Equal(t, UnknownCounterReset, cr)
+
+		assertNewHistogramChunkOnAppend(t, c, hApp, ts+1, h2, UnknownCounterReset, histogram.UnknownCounterReset)
+	}
+
+	{ // New histogram with a different schema.
+		c, hApp, ts, h1 := setup(eh)
+		h2 := h1.Copy()
+		h2.ZeroThreshold = 1e-120
+
+		posInterjections, negInterjections, backwardPositiveInserts, backwardNegativeInserts, ok, cr := hApp.appendable(h2)
+		require.Empty(t, posInterjections)
+		require.Empty(t, negInterjections)
+		require.Empty(t, backwardPositiveInserts)
+		require.Empty(t, backwardNegativeInserts)
+		require.False(t, ok) // Need to cut a new chunk.
+		require.Equal(t, UnknownCounterReset, cr)
+
+		assertNewHistogramChunkOnAppend(t, c, hApp, ts+1, h2, UnknownCounterReset, histogram.UnknownCounterReset)
+	}
 }

 func assertNewHistogramChunkOnAppend(t *testing.T, oldChunk Chunk, hApp *HistogramAppender, ts int64, h *histogram.Histogram, expectHeader CounterResetHeader, expectHint histogram.CounterResetHint) {
@ -1007,7 +1039,7 @@ func TestHistogramChunkAppendableWithEmptySpan(t *testing.T) {
 			require.Empty(t, bpI)
 			require.Empty(t, bnI)
 			require.True(t, okToAppend)
-			require.False(t, counterReset)
+			require.Equal(t, NotCounterReset, counterReset)
 		})
 	}
 }
--- a/tsdb/head_test.go
+++ b/tsdb/head_test.go
@ -5768,7 +5768,7 @@ func TestGaugeHistogramWALAndChunkHeader(t *testing.T) {
 		expHeaders := []chunkenc.CounterResetHeader{
 			chunkenc.UnknownCounterReset,
 			chunkenc.GaugeType,
-			chunkenc.UnknownCounterReset,
+			chunkenc.NotCounterReset,
 			chunkenc.GaugeType,
 		}
 		for i, mmapChunk := range ms.mmappedChunks {