dedup: don't inject a counter adjustment before any sample is emitted - #9041
Open
DeviousCardi wants to merge 2 commits into
Open
DeviousCardi wants to merge 2 commits into
DeviousCardi wants to merge 2 commits into
Conversation
newDedupSeriesIterator pre-seeds lastIter to replica "a" with useA=true before anything has been emitted, while lastT is still math.MinInt64. The first Next() therefore captures lastUseA=true and reads lastFloatVal from replica a's first sample. If replica b owns the earliest sample of the window, useA flips to false and this looks like a replica switch, so counterErrAdjustSeriesIterator.adjustAtValue sees lastFloatValue > v and records a permanent errAdjust equal to the inter-replica scrape offset. Replica b's counter is then flattened at the start of the window and rate() under-reports for the whole range. Query-range splitting is the trigger rather than the defect: each split gets a fresh iterator over a different first sample, so results alternate between correct and wrong depending on which replica owns the first sample of each split. No sample has been emitted at that point, so there is no previous value to adjust against. Guard the adjustment on lastT != math.MinInt64, the same "nothing emitted yet" sentinel the penalty logic already uses in this function. A genuine mid-stream replica switch still adjusts, so the mitigation for thanos-io#2401 is unaffected. Fixes thanos-io#9034 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Aarav <aaravsjadav@gmail.com>
The PR template asks for `- [#<PR-id>](<PR-URL>) Thanos <Component> ...` with the pull URL. The entry was written before the PR existed, so it referenced the issue. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Aarav <aaravsjadav@gmail.com>
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #9041 +/- ##
=======================================
Coverage 64.55% 64.56%
=======================================
Files 289 289
Lines 37412 37413 +1
=======================================
+ Hits 24153 24157 +4
+ Misses 11158 11156 -2
+ Partials 2101 2100 -1 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Changes
newDedupSeriesIteratorpre-seedslastIter: a, useA: truebefore any sample has been emitted, whilelastTis stillmath.MinInt64.Next()captures that aslastUseAand, if it then picks replica b, fires the counter-reset adjustment against replica a's first sample — a value that was never emitted.adjustAtValuesees a decrease, concludes "counter reset", and installs a permanenterrAdjustequal to the inter-replica scrape offset.The result is a counter flattened at the start of the window, so
rate()under-reports. Query-frontend range splitting makes it visible in practice: each split gets a fresh iterator over a different first sample, and split starts are aligned to absolute interval multiples, so the dip lands at the same wall-clock boundaries every time.This guards the adjustment on
emittedAny := it.lastT != math.MinInt64, reusing the sentinelNext()already uses twice for the initial-penalty decision. Genuine mid-stream replica switches still adjust, so the #2401 mitigation is untouched.Closes #9034
Verification
Synthetic counter increasing exactly 1/s across two replicas at 30s scrape with a 15s offset, so the true
rate()is exactly1.0:The corrupted sample is directly visible —
t=1755000 v=1770.000where it should be1755.000, giving delta 30 over 45s.Controls isolate it to this code path: a single replica,
--deduplication.func=chain, andirateall return1.000000before and after.New table case
Regression test against 9034inTestDedupSeriesSet, with replica b holding the earliest sample. Reverting onlyiter.gomakes it fail with+3on every sample — exactly103 - 100, the inter-replica offset.TestDedupSeriesSet/Regression_test_against_2401still passes.go test ./pkg/dedup/... -count=1and-raceboth pass;gofmtclean; no newgo vetwarnings.