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

mixin: make range interval configurable in alerts #7591

Conversation

jmichalek132
Copy link
Contributor

@jmichalek132 jmichalek132 commented Mar 11, 2024

What this PR does

Which issue(s) this PR fixes or relates to

Fixes #7546

Checklist

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

@jmichalek132 jmichalek132 changed the title Jm chore mixin make range interval configurable in alerts mixin: make range interval configurable in alerts Mar 11, 2024
Copy link
Contributor

@dimitarvdimitrov dimitarvdimitrov left a comment

Choose a reason for hiding this comment

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

the approach looks good 👍

@jmichalek132 jmichalek132 marked this pull request as ready for review March 12, 2024 14:47
@jmichalek132 jmichalek132 requested a review from a team as a code owner March 12, 2024 14:47
@jmichalek132
Copy link
Contributor Author

As discussed, the range selector is now dynamic only for alerts with the default value under 10m.

Copy link
Contributor

@dimitarvdimitrov dimitarvdimitrov left a comment

Choose a reason for hiding this comment

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

thanks for the change. There is one more query I could find in autoscaling.libsonnet which uses a range of 5m. Can you take care of that too?

label_replace(rate(keda_scaler_errors[5m]), "namespace", "$1", "exported_namespace", "(.*)")

@jmichalek132
Copy link
Contributor Author

thanks for the change. There is one more query I could find in autoscaling.libsonnet which uses a range of 5m. Can you take care of that too?

label_replace(rate(keda_scaler_errors[5m]), "namespace", "$1", "exported_namespace", "(.*)")

fixed.

@jmichalek132
Copy link
Contributor Author

I also noticed that a lot of recording rules have range interval hardcoded and don't use recording_rules_range_interval variable, some of them having 1m hardcoded https://github.com/grafana/mimir/blob/main/operations/mimir-mixin/recording_rules.libsonnet#L338. I will open a separate PR to address those.

@jmichalek132
Copy link
Contributor Author

Test failure seems to be unrelated to my changes to the mixin.

@dimitarvdimitrov
Copy link
Contributor

Test failure seems to be unrelated to my changes to the mixin.

yep, there's already a filed issue for that test #5471

Copy link
Contributor

@dimitarvdimitrov dimitarvdimitrov left a comment

Choose a reason for hiding this comment

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

LGTM, thank you :)

@dimitarvdimitrov dimitarvdimitrov enabled auto-merge (squash) March 14, 2024 13:56
@dimitarvdimitrov
Copy link
Contributor

would actually be nice to add a changelog entry. My suggestion was something like

...
### Mixin 
...
* [ENHANCEMENT] Alerts: allow configuring alerts range interval via `_config.base_alerts_range_interval_minutes`. #7591

@jmichalek132 jmichalek132 force-pushed the jm-chore-mixin-make-range-interval-configurable-in-alerts branch from 724036d to f688191 Compare March 14, 2024 14:46
Copy link
Contributor

@dimitarvdimitrov dimitarvdimitrov left a comment

Choose a reason for hiding this comment

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

lovely, thank you!

@dimitarvdimitrov dimitarvdimitrov enabled auto-merge (squash) March 14, 2024 14:50
auto-merge was automatically disabled March 14, 2024 15:42

Head branch was pushed to by a user without write access

@dimitarvdimitrov dimitarvdimitrov merged commit be893e7 into grafana:main Mar 14, 2024
29 checks passed
@jmichalek132 jmichalek132 deleted the jm-chore-mixin-make-range-interval-configurable-in-alerts branch March 15, 2024 08:01
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.

Mimir mixin: alerts make range interval configurable
2 participants