Repository: prometheus/prometheus (language: go)

Issue:
TSDB (histograms): Counter reset header might go undetected during ingest
tl;dr: If we cut a new chunk because the schema or the zero threshold has changed in a "compatible" way, we will always set a counter reset header of "no counter reset" in the new chunk although we haven't really checked if there was a counter reset. Luckily, this isn't hurting us right now because we don't trust "no counter reset" in the counter reset header anyway (for other reasons).

More detailed explanation:

First of all: What is a "compatible" change of the schema or the zero threshold? This happens when the instrumented target decides to get rid of some buckets by reducing the resolution and/or grow the zero bucket. The previous histograms (with the higher resolution and/or the smaller zero bucket) can be converted to the new schema and zero bucket width so the histograms are still compatible with each other for PromQL operations.

The "gold standard" for counter reset detection is the [FloatHistogram.DetectReset](https://github.com/prometheus/prometheus/blob/fc3d19725d83e13363ce40d56672c6f97e2955e7/model/histogram/float_histogram.go#L604) method. If it encounters a difference in schema and/or the zero threshold, it will find out if the change is compatible (see above) and will perform the conversion for the earlier histogram first, after which it will run the full reset detection (checking all bucket counts and the total count).

During ingestion, though, we are mostly interested if we have to cut a new chunk at all. One of the reason is a counter reset, but _any_ change in schema requires cutting a new chunk. So if there is a change in schema, the code quickly decides that a new chunk has to be cut, without further counter reset detection. This is fine, in principle, but the problem is that we set a "no counter reset" counter reset header of the new chunk in this case, which might or might not represent the truth. Instead, we either need to run a full counter reset detection or simply set a counter reset header of "unknown counter reset".

Given that we currently do not trust a "no counter reset" header anyway (because the preceding chunk could get removed, which would invalidate the information anyway), this is not breaking anything right now, but we should still fix the behavior. I propose for now to just set "unknown counter reset" in this case because investing all the work in a full counter reset detection is in vain as long as we have no way of trusting a "no counter reset" header (which is something we might do in the future, see https://github.com/prometheus/prometheus/issues/15346 for ideas). Most likely, we should change the [appendable method](https://github.com/prometheus/prometheus/blob/fc3d19725d83e13363ce40d56672c6f97e2955e7/tsdb/chunkenc/histogram.go#L265) to return a `CounterResetHeader` rather than just a bool.

/cc @fionaliao @krajorama 


Discussion / hints:
Could I take this issue? 
Sure. Thanks @fatsheep9146 . I have assigned you to this issue.

Fix patch:
diff --git a/tsdb/chunkenc/histogram.go b/tsdb/chunkenc/histogram.go
index d2eec6b75ae..7f528df8d57 100644
--- 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
 		}


Test patch:
diff --git a/tsdb/chunkenc/histogram_test.go b/tsdb/chunkenc/histogram_test.go
index 7e8807963b9..fb36b232c44 100644
--- 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)
 		})
 	}
 }
diff --git a/tsdb/head_test.go b/tsdb/head_test.go
index 5fff8590211..5c014788d8a 100644
--- a/tsdb/head_test.go
+++ b/tsdb/head_test.go
@@ -5759,7 +5759,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 {