querier: Adjust deduplication for counters when querying for PromQL rates. #2548
Conversation
|
Works perfectly now ❤️
|
|
Let's not merge before #2528 merges. |
|
Let's merge once #2528 get green mark. |
pkg/query/iter.go
Outdated
| func (it *counterDedupAdjustSeriesIterator) Next() bool { | ||
| if it.SeriesIterator.Next() { | ||
| _, v := it.SeriesIterator.At() | ||
| if it.lastV > v { |
There was a problem hiding this comment.
Hmm, I was thinking but maybe a stupid question: shouldn't we set it.adjust back to 0 if we will jump back to a replica with a higher counter value again at some point during the iteration? Wouldn't that lead to higher counter values than usual without such a reset?
There was a problem hiding this comment.
Hmm, you might be right. Adjust has to be cumulative, but per replica not globally, otherwise, we are adjusting true value... This might mean we need to
- Actually hook inside the dedup iterator and adjust per replica (fine)
- Or reset adjust as you propose (easier, but we lost the info that replica was behind).
Also if we do 1 it's a bit broken anyway as we know about WHEN & WHAT to adjust only if we are lucky that replica changes (: Still it's better than what we had before, plus better than any solution in the world for this (e.g dedup on ingest Cortex have)
Will think about it a bit more, thanks for spotting this!
There was a problem hiding this comment.
I fixed this problem, but changed code a little bit, please review @GiedriusS and thanks for spotting this! I added test cases for this
bwplotka
force-pushed
the
issue2147-fix-actual
branch
from
e903228 to
8d5b0a2
Compare
bwplotka
force-pushed
the
issue2147-fix-actual
branch
from
caac262 to
34859f1
Compare
|
PTAL @GiedriusS @brancz |
|
Let's merge on Monday. I plan to add benchmarks for this, plus consider to rewrite to heap approach, plus few other fixes, but let's do on later PR maybe (: |
…ates. Signed-off-by: Bartlomiej Plotka <[email protected]>
Signed-off-by: Bartlomiej Plotka <[email protected]>
Signed-off-by: Bartlomiej Plotka <[email protected]>
Signed-off-by: Bartlomiej Plotka <[email protected]>
bwplotka
force-pushed
the
issue2147-fix-actual
branch
from
53e69bd to
4aa201c
Compare
|
Let's merge without squash (: |
|
And of course I squashed ;p |
cc @SuperQ
Chained over: #2528
Fixes: #2401
Signed-off-by: Bartlomiej Plotka [email protected]