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

Setting ruler.evaluation-delay-duration to be deprecated. #6149

Merged

Conversation

danielblando
Copy link
Contributor

@danielblando danielblando commented Aug 9, 2024

What this PR does:
Follow up from #6085

Deprecating -ruler.evaluation-delay-duration
Should use new flag ruler.query-offset

Checklist

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

@danielblando danielblando force-pushed the deprecateEvaluationDelayDuration branch from 798228f to f30dc87 Compare August 9, 2024 20:30
@danielblando danielblando marked this pull request as ready for review August 9, 2024 20:47
Copy link
Contributor

@yeya24 yeya24 left a comment

Choose a reason for hiding this comment

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

Change looks great!
Integration test for ruler failed on TestRulerEvaluationDelay. Let's just remove that test.

@danielblando danielblando force-pushed the deprecateEvaluationDelayDuration branch from ef6df4c to f84944f Compare August 9, 2024 21:37
@danielblando danielblando changed the title Deprecate -ruler.evaluation-delay-duration Deprecate -ruler.evaluation-delay-duration Aug 9, 2024
@danielblando danielblando changed the title Deprecate -ruler.evaluation-delay-duration Deprecate ruler.evaluation-delay-duration Aug 9, 2024
@rapphil
Copy link
Contributor

rapphil commented Aug 9, 2024

Was this flag experimental? If no, do we need to give a grace period before removing altogether and have just the warning message for now if this flag is being used (this is what I would understand as deprecating something).

@yeya24
Copy link
Contributor

yeya24 commented Aug 9, 2024

https://cortexmetrics.io/docs/configuration/v1guarantees/#flags-config-and-minor-version-upgrades

I am trying to understand this process better.

Upgrading cortex from one minor version to the next should “just work”; that being said, we don’t want to bump the major version every time we remove a flag, so we will will keep deprecated flags around for 2 minor release. There is a metric (deprecated_flags_inuse_total) you can alert on to find out if you’re using a deprecated flag.

just work means we need to keep the same functionality or there shouldn't be any issue to run Cortex with the existing config?

If the latter this PR should be ok.

@danielblando
Copy link
Contributor Author

I think the biggest issue here is that now we have two flags doing the same thing. Looking at the code it looks like the delay would be the sum of two values. This seems more like a bug than a flag deprecate. We can go back to remove the new RuleQueryOffset flag and maintain RulerEvaluationDelay, but now as the query-offset time for Prometheus.

Signed-off-by: Daniel Deluiggi <[email protected]>
Signed-off-by: Daniel Deluiggi <[email protected]>
@danielblando danielblando force-pushed the deprecateEvaluationDelayDuration branch from 8143601 to ecd5fda Compare August 12, 2024 23:10
Signed-off-by: Daniel Deluiggi <[email protected]>
@danielblando danielblando changed the title Deprecate ruler.evaluation-delay-duration Setting ruler.evaluation-delay-duration to be deprecated. Aug 12, 2024
Signed-off-by: Daniel Deluiggi <[email protected]>
Copy link
Contributor

@yeya24 yeya24 left a comment

Choose a reason for hiding this comment

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

Looks good. Just a small nit.

CHANGELOG.md Outdated Show resolved Hide resolved
Signed-off-by: Daniel Deluiggi <[email protected]>
CHANGELOG.md Outdated Show resolved Hide resolved
Signed-off-by: Daniel Deluiggi <[email protected]>
@yeya24 yeya24 merged commit 24edba0 into cortexproject:master Aug 13, 2024
14 checks passed
@danielblando danielblando deleted the deprecateEvaluationDelayDuration branch August 13, 2024 17:49
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