Ensure counter reset hints are corrected when merging batches #9909

fionaliao · 2024-11-14T18:44:24Z

What this PR does

When merging samples from different iterators, histogram counter reset hints need to be recalculated. The mergeIterator currently does not do so. This PR fixes that by remembering the iterator for a sample in a batch, so when the next sample comes along, the iterators for the two samples can be compared and an unknown counter reset hint is set for the next sample if the iterators don't match. See prometheus/prometheus#15346 for related discussion.

Which issue(s) this PR fixes or relates to

Fixes #

Checklist

Tests updated.
Documentation added.
CHANGELOG.md updated - the order of entries should be [CHANGE], [FEATURE], [ENHANCEMENT], [BUGFIX].
about-versioning.md updated with experimental features.

pkg/querier/batch/stream.go

fionaliao · 2024-11-26T21:50:26Z

Implementation and testing is done (pending CI), but I want to extract the mimir-prometheus update into a separate PR before removing the "WIP"

Co-authored-by: George Krajcsovits <[email protected]>

Signed-off-by: György Krajcsovits <[email protected]>

fionaliao · 2024-11-27T17:55:20Z

mimir-prometheus update was done as part of #10033

fionaliao · 2024-11-27T18:47:42Z

pkg/querier/batch/merge_test.go

+					name: "overlapping and interleaved samples",
+					chunks: func() []GenericChunk {
+						chk1, err := chunk.NewForEncoding(enc)
+						require.NoError(t, err)
+						addFunc(chk1, 0, 0)
+						addFunc(chk1, 3, 1)
+						addFunc(chk1, 4, 2)
+						addFunc(chk1, 5, 3)
+						addFunc(chk1, 7, 4)
+						addFunc(chk1, 8, 5)
+						chk2, err := chunk.NewForEncoding(enc)
+						require.NoError(t, err)
+						addFunc(chk2, 1, 1)
+						addFunc(chk2, 2, 7)
+						addFunc(chk2, 5, 8)
+						addFunc(chk2, 6, 9)
+						addFunc(chk2, 7, 10)
+						addFunc(chk2, 9, 11)
+						return []GenericChunk{
+							NewGenericChunk(0, 8, chunk.NewChunk(labels.FromStrings(model.MetricNameLabel, "foo"), chk1, 0, 8).Data.NewIterator),
+							NewGenericChunk(1, 9, chunk.NewChunk(labels.FromStrings(model.MetricNameLabel, "foo"), chk2, 1, 9).Data.NewIterator),
+						}
+					}(),
+					expectedSamples: []checkHintTestSample{
+						{t: 0, v: 0, hint: histogram.UnknownCounterReset}, // c1
+						{t: 1, v: 1, hint: histogram.UnknownCounterReset}, // c2
+						{t: 2, v: 7, hint: histogram.NotCounterReset},     // c2
+						{t: 3, v: 1, hint: histogram.UnknownCounterReset}, // c1
+						{t: 4, v: 2, hint: histogram.NotCounterReset},     // c1
+						{t: 5, v: 8, hint: histogram.UnknownCounterReset}, // c2
+						// Next sample is from c2. This is consecutive, but this ends up as the first sample in a merged
+						// batch, and the c1 sample at ts 7 and 8 is added to the batch stream before this c2 sample, so
+						// the prevIteratorID is set to c1 rather than c2 when we append this sample.
+						{t: 6, v: 9, hint: histogram.UnknownCounterReset},
+						{t: 7, v: 4, hint: histogram.UnknownCounterReset},  // c1
+						{t: 8, v: 5, hint: histogram.NotCounterReset},      // c1
+						{t: 9, v: 11, hint: histogram.UnknownCounterReset}, // c2
+					},


This is an interesting test case - you can have as two consecutive samples from the same iterator but the second one isn't always detected as NotCounterReset, as you can see with ts 6. This is a case where keeping track of the last timestamp per iterator rather than just the last iterator would have detected the NotCounterReset properly. However, I think the current algorithm is good enough for now, I don't want to complicate it further without checking how it works in practice.

Agreed, we aim to fail safe.

krajorama

looking good a few nits added first round

pkg/querier/batch/stream.go

krajorama · 2024-11-28T08:27:53Z

pkg/querier/batch/merge_test.go

+					name: "overlapping and interleaved samples",
+					chunks: func() []GenericChunk {
+						chk1, err := chunk.NewForEncoding(enc)
+						require.NoError(t, err)
+						addFunc(chk1, 0, 0)
+						addFunc(chk1, 3, 1)
+						addFunc(chk1, 4, 2)
+						addFunc(chk1, 5, 3)
+						addFunc(chk1, 7, 4)
+						addFunc(chk1, 8, 5)
+						chk2, err := chunk.NewForEncoding(enc)
+						require.NoError(t, err)
+						addFunc(chk2, 1, 1)
+						addFunc(chk2, 2, 7)
+						addFunc(chk2, 5, 8)
+						addFunc(chk2, 6, 9)
+						addFunc(chk2, 7, 10)
+						addFunc(chk2, 9, 11)
+						return []GenericChunk{
+							NewGenericChunk(0, 8, chunk.NewChunk(labels.FromStrings(model.MetricNameLabel, "foo"), chk1, 0, 8).Data.NewIterator),
+							NewGenericChunk(1, 9, chunk.NewChunk(labels.FromStrings(model.MetricNameLabel, "foo"), chk2, 1, 9).Data.NewIterator),
+						}
+					}(),
+					expectedSamples: []checkHintTestSample{
+						{t: 0, v: 0, hint: histogram.UnknownCounterReset}, // c1
+						{t: 1, v: 1, hint: histogram.UnknownCounterReset}, // c2
+						{t: 2, v: 7, hint: histogram.NotCounterReset},     // c2
+						{t: 3, v: 1, hint: histogram.UnknownCounterReset}, // c1
+						{t: 4, v: 2, hint: histogram.NotCounterReset},     // c1
+						{t: 5, v: 8, hint: histogram.UnknownCounterReset}, // c2
+						// Next sample is from c2. This is consecutive, but this ends up as the first sample in a merged
+						// batch, and the c1 sample at ts 7 and 8 is added to the batch stream before this c2 sample, so
+						// the prevIteratorID is set to c1 rather than c2 when we append this sample.
+						{t: 6, v: 9, hint: histogram.UnknownCounterReset},
+						{t: 7, v: 4, hint: histogram.UnknownCounterReset},  // c1
+						{t: 8, v: 5, hint: histogram.NotCounterReset},      // c1
+						{t: 9, v: 11, hint: histogram.UnknownCounterReset}, // c2
+					},


Agreed, we aim to fail safe.

krajorama · 2024-11-28T09:01:43Z

CHANGELOG.md

@@ -85,6 +85,7 @@
 * [BUGFIX] Fix pooling buffer reuse logic when `-distributor.max-request-pool-buffer-size` is set. #9666
 * [BUGFIX] Fix issue when using the experimental `-ruler.max-independent-rule-evaluation-concurrency` feature, where the ruler could panic as it updates a running ruleset or shutdowns. #9726
 * [BUGFIX] Always return unknown hint for first sample in non-gauge native histograms chunk to avoid incorrect counter reset hints when merging chunks from different sources. #10033
+* [BUGFIX] Ensure counter reset hints are corrected when merging batches. #9909


Suggested change

* [BUGFIX] Ensure counter reset hints are corrected when merging batches. #9909

* [BUGFIX] Ensure native histograms counter reset hints are corrected when merging results from different sources. #9909

krajorama · 2024-11-28T09:57:41Z

pkg/storage/chunk/prometheus_chunk.go

@@ -119,6 +119,8 @@ func (p *prometheusHistogramChunk) AddFloatHistogram(_ int64, _ *histogram.Float
 // AddHistogram adds another histogram to the chunk. While AddHistogram works, it is only implemented to make tests
 // work, and should not be used in production. In particular, it appends all histograms to single chunk, and uses new
 // Appender for each invocation.
+// AddHistogram adds another histogram to the chunk. While AddHistogram works, it is only implemented to make tests


nit: I think we meant to replace the doc string not add to it

krajorama reviewed Nov 15, 2024

View reviewed changes

pkg/querier/batch/stream.go Outdated Show resolved Hide resolved

pkg/querier/batch/stream.go Outdated Show resolved Hide resolved

fionaliao force-pushed the fl/counter-resets-ooonh branch from 18d9cf2 to d9c34a7 Compare November 18, 2024 15:48

fionaliao marked this pull request as ready for review November 26, 2024 21:49

fionaliao requested review from stevesg, grafanabot and a team as code owners November 26, 2024 21:49

fionaliao and others added 23 commits November 27, 2024 17:49

Add krajo's test (currently failing)

9c1cf59

Add test showing single ingester return

ed75cb5

add failing test

dfeabaa

typo

a807173

Fix but overdetecting unknowns

d48c564

Link batches from same iterator

981617f

clarify comment

7fb9087

Update pkg/querier/batch/stream.go

5b0026a

Co-authored-by: George Krajcsovits <[email protected]>

Update pkg/querier/batch/stream.go

a6d624f

Co-authored-by: George Krajcsovits <[email protected]>

Fix stream test

b5c1eca

Fix nh counter resets when merging overlapping chunks

fa510a0

Signed-off-by: György Krajcsovits <[email protected]>

Set UCR when NCR for some tests

d3fcd03

Pass id as constructor arg

d8fb5c4

Uncomment

b0246e7

Remove unused function

0c8ee53

Add tests for checking hints

ef1cafb

More complex reset tests

7e2148e

Remove unused test func

9cc1b84

Remove TODO

62e7235

Add interleaving test

732ec71

Clean up ingester test

aff826f

Consistent ID

89cd0ed

Update ID again

3594ee6

fionaliao added 6 commits November 27, 2024 17:49

Refactor ingester querying tests and add streaming case

15643c5

Add store-gw tests

53594d6

Add float test

e9ccf39

Explain reuse of values

00b0e96

Add comment

d780a01

Fix comment typo

0f4f294

fionaliao force-pushed the fl/counter-resets-ooonh branch from 7b27917 to 0f4f294 Compare November 27, 2024 17:50

fionaliao added 2 commits November 27, 2024 17:52

Fix more comments

deb2847

Update CHANGELOG

9932a4f

fionaliao changed the title ~~WIP: fix counter resets when merging batches~~ Ensure counter reset hints are corrected when merging batches Nov 27, 2024

fionaliao added 2 commits November 27, 2024 18:02

Lint

6844b1e

More lint

24bea3c

fionaliao force-pushed the fl/counter-resets-ooonh branch from e7d5fb6 to 24bea3c Compare November 27, 2024 18:17

Fix bad rename

3a2310c

fionaliao commented Nov 27, 2024

View reviewed changes

krajorama reviewed Nov 28, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Ensure counter reset hints are corrected when merging batches #9909

Ensure counter reset hints are corrected when merging batches #9909

fionaliao commented Nov 14, 2024 •

edited

Loading

fionaliao commented Nov 26, 2024

fionaliao commented Nov 27, 2024

fionaliao Nov 27, 2024

krajorama Nov 28, 2024

krajorama left a comment

krajorama Nov 28, 2024

krajorama Nov 28, 2024

krajorama Nov 28, 2024

	* [BUGFIX] Ensure counter reset hints are corrected when merging batches. #9909
	* [BUGFIX] Ensure native histograms counter reset hints are corrected when merging results from different sources. #9909

Ensure counter reset hints are corrected when merging batches #9909

Are you sure you want to change the base?

Ensure counter reset hints are corrected when merging batches #9909

Conversation

fionaliao commented Nov 14, 2024 • edited Loading

What this PR does

Which issue(s) this PR fixes or relates to

Checklist

fionaliao commented Nov 26, 2024

fionaliao commented Nov 27, 2024

fionaliao Nov 27, 2024

Choose a reason for hiding this comment

krajorama Nov 28, 2024

Choose a reason for hiding this comment

krajorama left a comment

Choose a reason for hiding this comment

krajorama Nov 28, 2024

Choose a reason for hiding this comment

krajorama Nov 28, 2024

Choose a reason for hiding this comment

krajorama Nov 28, 2024

Choose a reason for hiding this comment

fionaliao commented Nov 14, 2024 •

edited

Loading