Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Compactor: Adding minimum retention flag validation for downsampling retention #5059

Conversation

moadz
Copy link
Contributor

@moadz moadz commented Jan 11, 2022

  • I added CHANGELOG entry for this change.
  • Change is not relevant to the end user.

Changes

  • Added 40h minimum raw resolution retention if downsampling enabled and --retention.resolution-raw > 0
  • Added 10d minimum 5m resolution retention if downsampling enabled and --retention.resolution-5m > 0
  • Added 10d minimum 1h resolution retention if downsampling enabled and --retention.resolution-1h > 0
  • Added clarifying docs around why downsampling time delay

Verification

  • Ran locally with e2e compactor tests with flags enabled below set retention to test flag validation

Changes based on: #813

tl;dr: For downsampling to happen, blocks must be pass a certain age for the preceding resolution. This was always the case in Compactor but not transparently to users. As such, it's very easy to set up retention in such a way that disrupts downsampling or leads to accidental data loss.

…he mapping between the downsample ranges and their respective resolutions

Signed-off-by: mzardab <mzardab@redhat.com>
...if the following conditions are met:
* Downsampling is enabled, raw retention must be > 10 days
* Downsampling is enabled, 5m resolution retention must be at least 40h
* Downsampling is enabled, 1h resolution must be at least 10d

This is to avoid unexpected deletions of downsampled/raw data as compactor will only downsample raw blocks after there is enough data to form 2 whole blocks, it will not downsample immediately.

Signed-off-by: mzardab <mzardab@redhat.com>
Signed-off-by: mzardab <mzardab@redhat.com>

* Creating 5m downsampling for blocks larger than **40 hours** (2d, 2w)
* Creating 1h downsampling for blocks larger than **10 days** (2w)
Compactor is also responsible for downsampling of data. There is a time delay before downsampling at a given resolution is possible. This is necessary because downsampled chunks will have fewer samples in them, and as chunks are fixed size, data spanning more time will be required to fill them.
Copy link
Contributor Author

@moadz moadz Jan 11, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm inferring this from the code, 2hr @ 15s sampling to 20hrs @ 5min sampling (~factor of 10). It mentions that we want to produce at least 2 chunks for each downsampling pass, unsure of any other reason for why this would be the case.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍🏽

cmd/thanos/compact.go Outdated Show resolved Hide resolved
cmd/thanos/compact.go Outdated Show resolved Hide resolved
cmd/thanos/compact.go Outdated Show resolved Hide resolved
pkg/compact/downsample/downsample.go Show resolved Hide resolved
test/e2e/e2ethanos/services.go Outdated Show resolved Hide resolved
@moadz moadz requested a review from yeya24 January 18, 2022 10:09
Copy link
Collaborator

@matej-g matej-g left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think is going to be a really nice improvement 👏 However I'm still seeing some extra commits in the PR, shall we remove those? Happy to re-review afterwards again!

@moadz
Copy link
Contributor Author

moadz commented Jan 19, 2022

I think is going to be a really nice improvement 👏 However I'm still seeing some extra commits in the PR, shall we remove those? Happy to re-review afterwards again!

@matej-g just trying to fix some flakey docs tests by rebasing on master. All current feedback has been addressed pending re-review by @yeya24

Signed-off-by: mzardab <mzardab@redhat.com>
Signed-off-by: mzardab <mzardab@redhat.com>
Signed-off-by: mzardab <mzardab@redhat.com>
@moadz moadz force-pushed the thanos-compactor-retention-resolution-flag-validation branch from a174bb1 to 26d20d9 Compare January 20, 2022 15:14
Copy link
Member

@bwplotka bwplotka left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, LGTM!


* Creating 5m downsampling for blocks larger than **40 hours** (2d, 2w)
* Creating 1h downsampling for blocks larger than **10 days** (2w)
Compactor is also responsible for downsampling of data. There is a time delay before downsampling at a given resolution is possible. This is necessary because downsampled chunks will have fewer samples in them, and as chunks are fixed size, data spanning more time will be required to fill them.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍🏽

@bwplotka bwplotka merged commit ec73ed7 into thanos-io:main Jan 21, 2022
Nicholaswang pushed a commit to Nicholaswang/thanos that referenced this pull request Mar 6, 2022
…retention (thanos-io#5059)

* Renaming `DownsampleRange` -> `ResLevel1DownsampleRange` to specify the mapping between the downsample ranges and their respective resolutions

Signed-off-by: mzardab <mzardab@redhat.com>

* Adding compactor downsample retention flag validation...

...if the following conditions are met:
* Downsampling is enabled, raw retention must be > 10 days
* Downsampling is enabled, 5m resolution retention must be at least 40h
* Downsampling is enabled, 1h resolution must be at least 10d

This is to avoid unexpected deletions of downsampled/raw data as compactor will only downsample raw blocks after there is enough data to form 2 whole blocks, it will not downsample immediately.

Signed-off-by: mzardab <mzardab@redhat.com>

* Added more clarifying docs on impact of retention on downsampling

Signed-off-by: mzardab <mzardab@redhat.com>

* Resolving conflicts

Signed-off-by: mzardab <mzardab@redhat.com>

* Updated CHANGELOG.md

Signed-off-by: mzardab <mzardab@redhat.com>

* Addressing PR comments

Signed-off-by: mzardab <mzardab@redhat.com>
Signed-off-by: Nicholaswang <wzhever@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants