Compact skips prometheus bug, and thanos tool supports fixes for this issue 6723 #6782
Conversation
This file contains 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
Signed-off-by: mickeyzzc <mickey_zzc@163.com>
|
Good stuff! Can you rebase on latest main, we bumped prometheus recently already; that should make this PR a bit smaller probably! |
mickeyzzc
force-pushed
the
compact-skip-twochunks-mint-equal-maxt
branch
from
39dd944
to
16957ef
Compare
mickeyzzc
force-pushed
the
compact-skip-twochunks-mint-equal-maxt
branch
from
8348b11
to
a282729
Compare
sylr
reviewed
Oct 10, 2023
mickeyzzc
force-pushed
the
compact-skip-twochunks-mint-equal-maxt
branch
from
e8f8d19
to
43b17b8
Compare
mickeyzzc
force-pushed
the
compact-skip-twochunks-mint-equal-maxt
branch
from
c22b706
to
e4dfd38
Compare
mickeyzzc
force-pushed
the
compact-skip-twochunks-mint-equal-maxt
branch
from
e4dfd38
to
8d3c09a
Compare
Signed-off-by: mickeyzzc <mickey_zzc@163.com>
|
/review @MichaHoffmann thanks |
Signed-off-by: MickeyZZC <mickey_zzc@163.com>
|
/review @sylr thanks |
Signed-off-by: MickeyZZC <mickey_zzc@163.com>
|
I'm currently testing this on my setup. |
|
It seems to partially work. Here the logs about a block that the verifier failed to completely heal: On my dev thanos env the verifier fixed 75 blocks and failed to fix 61 ones. you can find a dashboard of two runs of the verifier, the first one fixed what it could. I ran the second to see if it could fix more blocks but it didn't (which is expected): https://snapshots.raintank.io/dashboard/snapshot/IGIdVWcQaaKHaIs8WXQkfPFv5t05xQet |
|
Here is the full log of the verifier on a block it can't fix: Click me
|
sylr
reviewed
Oct 28, 2023
yeya24
reviewed
Oct 29, 2023
| @@ -556,9 +639,10 @@ OUTER: | |||
| continue OUTER | |||
| } | |||
| } | |||
|
|
|||
| // TODO: error on duplicate samples | |||
| chk, _ := FixDuplicateSamplesOutsideChunk(last, &chks[i]) | |||
| last = &chks[i] | |||
There was a problem hiding this comment.
Why last is not assigned to chk, the chunk after dedup?
| } | ||
| return ncurr, nil | ||
| } | ||
|
|
||
| // sanitizeChunkSequence ensures order of the input chunks and drops any duplicates. | ||
| // It errors if the sequence contains non-dedupable overlaps. |
There was a problem hiding this comment.
Let's update function comment and mention duplicated samples is also fixed here
yeya24
reviewed
Oct 29, 2023
There was a problem hiding this comment.
I ran this code using blocks from @sylr.
ref 774022, labels: <omitted>, before dedup samples 438, after dedup samples 437
ref 774022 chunk 0, mint 1695254404507 maxt 1695256191479 samples 114
ref 774022 chunk 1, mint 1695256209660 maxt 1695258837856 samples 163
ref 774022 chunk 2, mint 1695258853798 maxt 1695261581203 samples 160
ref 774022 chunk 3, mint 1695261581203 maxt 1695261581203 samples 1
new meta: ref 774022 chunk 0, mint 1695254404507 maxt 1695256191479 samples 114
new meta: ref 774022 chunk 1, mint 1695256209660 maxt 1695258837856 samples 163
new meta: ref 774022 chunk 2, mint 1695258853798 maxt 1695261581203 samples 160
new meta: ref 774022 chunk 3, mint 1695261581203 maxt 1695261581203 samples 0
total samples with rewrite: 437, with merging: 437
The problem is that if a chunk has only one sample and that sample is a duplicate sample, the rewrite code is able to drop it but it doesn't drop the empty chunk.
So after rewrite, chunks are still overlapping.
|
Please let us know if you are willing to continue this pr or not. Thanks |
sorry,i have important things to deal with in November and I will continue to follow up this pr in December |
|
Can someone take over ? |
Can I test these blocks? |
Signed-off-by: mickeyzzc <mickey_zzc@163.com>
Signed-off-by: mickeyzzc <mickey_zzc@163.com>
|
If you are still interested in the blocks @mickeyzzc can you contact me on CNCF's slack workspace ? |
|
@mickeyzzc why did you close this ? @yeya24 can you take over ? |
|
I don't have time to really take it over. |
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.
#6723 issue fix
Changes
Verification