Skip to content

Conversation

tomwilkie
Copy link
Contributor

@tomwilkie tomwilkie commented Aug 26, 2020

Signed-off-by: Tom Wilkie [email protected]

What this PR does:

Makes the ruler evaluation delay configuration per user.

Which issue(s) this PR fixes:
Fixes #

Checklist

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

@tomwilkie
Copy link
Contributor Author

/cc @gotjosh

@tomwilkie tomwilkie requested a review from gouthamve August 26, 2020 18:28
@tomwilkie
Copy link
Contributor Author

What did we say about compatibility and changing the config structs?

@gotjosh
Copy link
Contributor

gotjosh commented Aug 26, 2020

What did we say about compatibility and changing the config structs?

I think we need to keep the old flag around for 2 more minor versions but others might know best.

@gotjosh
Copy link
Contributor

gotjosh commented Aug 26, 2020

Nice @tomwilkie ! FWIW I'd be in favour of this change as I think the delay is certainly something tenants might not agree on.

- Put the config back in the struct, but mark as deprecated.
- Decouple ruler from limits package.
- Put comment back where its more relevant.

Signed-off-by: Tom Wilkie <[email protected]>
Copy link
Contributor

@gouthamve gouthamve left a comment

Choose a reason for hiding this comment

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

LGTM with nits.

# have been pushed to cortex.
# CLI flag: -ruler.evaluation-delay-duration
# Deprecated. Please use -ruler.evaluation-delay-duration instead.
# CLI flag: -ruler.evaluation-delay-duration-deprecated
Copy link
Contributor

Choose a reason for hiding this comment

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

Hrm, this deprecated notice is not super useful for the YAML config. Hrm. Not sure how to fix it though.

Copy link
Contributor

Choose a reason for hiding this comment

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

This flag only exists to have YAML field with "deprecated" documentation. Ideally we would hide CLI flag, but our tooling is not there yet. Given that this will be removed in Cortex 1.6, let's not waste too much time on it.

Signed-off-by: Peter Štibraný <[email protected]>
@pstibrany pstibrany merged commit 4c96879 into master Sep 10, 2020
@tomwilkie tomwilkie deleted the per-user-ruler-evaluation-delay branch April 27, 2021 11:26
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.

5 participants