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

Add per-tenant block retention #508

Merged
merged 4 commits into from
Feb 8, 2021

Conversation

mdisibio
Copy link
Contributor

@mdisibio mdisibio commented Feb 5, 2021

What this PR does:
Adds per-tenant block retention. Uses existing overrides.yaml system, value is optional, defaults to global retention. The callback design is done so that it continues to work with the overrides.yaml polling and use new limits without restarting.

This introduces a not-great situation where it is now possible to specify block retention in a third location: tempo.yaml -> overrides.block_retention, which are the default limits, and would technically override the original value in tempo.yaml -> compactor.compaction.block_retention. This isn't harmful and unlikely to ever be used, but wanted to mention it.

Which issue(s) this PR fixes:
Fixes #77

Checklist

  • Tests updated
  • Documentation added
  • CHANGELOG.md updated - the order of entries should be [CHANGE], [FEATURE], [ENHANCEMENT], [BUGFIX]

Copy link
Member

@joe-elliott joe-elliott left a comment

Choose a reason for hiding this comment

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

One nit. I also agree with leaving the config in the compactor. Nearly all Tempo users outside of us will be running in single tenant mode and it doesn't make sense to force them to use overrides to set retention.

@joe-elliott joe-elliott merged commit da5e397 into grafana:master Feb 8, 2021
@mdisibio mdisibio deleted the 77-per-tentant-retention branch May 27, 2021 13:23
@bputt-e
Copy link

bputt-e commented Jun 15, 2022

@joe-elliott is this a part of tempo 1.4.1? The docs state "Currenly only ingestion limits can be overridden." which makes me think this was backed out or its undocumented

@joe-elliott
Copy link
Member

Yup, this feature has been in Tempo since 0.6.0. Thanks for the heads up. Pushing a fix to the docs now.

@joe-elliott joe-elliott mentioned this pull request Jun 16, 2022
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add per tenant retention
3 participants