From 6cd1b779f655e9aae2a16b1c49cbc1df1e57daef Mon Sep 17 00:00:00 2001
From: Filip Petkovski <filip.petkovsky@gmail.com>
Date: Fri, 26 Jan 2024 09:27:03 +0100
Subject: [PATCH] Preserve reset hint during histogram deduplication

Penalty based deduplication can over-skip samples because when switching
between replicas, because it uses a penalty of 2 times the estimated scrape interval.
This works okay for float counters because the reset is detected based on the value of the counter.
With native histograms, detecting a reset is much more expensive which is why there is a
hint stored in the first sample of a chunk, which indicates whether the chunk has been created
because of a reset.

It can easily happen that the dedup iterator switches replicas, and skips the first sample of a chunk
because of the added penalty. In this case we will not read the hint and will assume that no
histogram reset happened.

This commit fixes the issue by explicitly calling DetectReset if a replica stream has been switched.
The result is then stored and set in the histogram returned by AtHistogram and AtFloatHistogram.

Signed-off-by: Filip Petkovski <filip.petkovsky@gmail.com>
---
 pkg/query/iter.go | 66 ++++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 65 insertions(+), 1 deletion(-)

diff --git a/pkg/query/iter.go b/pkg/query/iter.go
index b9f40999956..04034e05660 100644
--- a/pkg/query/iter.go
+++ b/pkg/query/iter.go
@@ -168,7 +168,7 @@ func getFirstIterator(cs ...*storepb.Chunk) chunkenc.Iterator {
 		if err != nil {
 			return errSeriesIterator{err}
 		}
-		return chk.Iterator(nil)
+		return newHistogramResetDetector(chk.Iterator(nil))
 	}
 	return errSeriesIterator{errors.New("no valid chunk found")}
 }
@@ -304,3 +304,67 @@ func (c *lazySeriesSet) Warnings() annotations.Annotations {
 	}
 	return nil
 }
+
+// histogramResetDetector sets the CounterResetHint to UnknownCounterReset for the first histogram read from the Iterator.
+//
+// The reset hint is always present in the first sample of the chunk, and all
+// consecutive samples will have a NotCounterReset hint. During deduplication the
+// first sample could be skipped, which means the reset will not be properly
+// detected. This iterator will make sure that in that case the hint for the
+// first read sample is set to UnknownCounterReset so that the PromQL engine will
+// do the reset detection manually.
+type histogramResetDetector struct {
+	chunkenc.Iterator
+
+	lastT       int64
+	lastValType chunkenc.ValueType
+
+	i             int16
+	histogramRead bool
+}
+
+func newHistogramResetDetector(iterator chunkenc.Iterator) *histogramResetDetector {
+	return &histogramResetDetector{
+		Iterator: iterator,
+		i:        -1,
+	}
+}
+
+func (it *histogramResetDetector) Seek(t int64) chunkenc.ValueType {
+	for {
+		if it.lastT >= t {
+			return it.lastValType
+		}
+		if it.lastValType = it.Next(); it.lastValType == chunkenc.ValNone {
+			return chunkenc.ValNone
+		}
+	}
+}
+
+func (it *histogramResetDetector) Next() chunkenc.ValueType {
+	it.i++
+	it.lastValType = it.Iterator.Next()
+	if it.lastValType == chunkenc.ValNone {
+		return chunkenc.ValNone
+	}
+	it.lastT = it.Iterator.AtT()
+	return it.lastValType
+}
+
+func (it *histogramResetDetector) AtHistogram() (int64, *histogram.Histogram) {
+	t, h := it.Iterator.AtHistogram()
+	if !it.histogramRead && it.i != 0 {
+		h.CounterResetHint = histogram.UnknownCounterReset
+	}
+	it.histogramRead = true
+	return t, h
+}
+
+func (it *histogramResetDetector) AtFloatHistogram() (int64, *histogram.FloatHistogram) {
+	t, fh := it.Iterator.AtFloatHistogram()
+	if !it.histogramRead && it.i != 0 {
+		fh.CounterResetHint = histogram.UnknownCounterReset
+	}
+	it.histogramRead = true
+	return t, fh
+}