Skip to content

ruler: Add flag to configure the queue for pending Alertmanager notif… #8234

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

roth-wine
Copy link
Contributor

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

Changes

Add flag to configure the queue for pending Alertmanager notifications
In the prometheus package we are already able to configure the capacity size. The fixed value could lead to issues sending alerts when evaluating a huge amount of alerts in one ruler instance.

@roth-wine roth-wine force-pushed the ruler_alert_capacity_configurable branch 4 times, most recently from 8666ef4 to e64d2eb Compare April 30, 2025 11:19
@roth-wine
Copy link
Contributor Author

Failing test seems unrelated to my changes and might be flacky.

Copy link
Member

@GiedriusS GiedriusS left a comment

Choose a reason for hiding this comment

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

How would the user or anyone know what should be a "good" value? Could we provide some guidance in the README?

@roth-wine
Copy link
Contributor Author

Unfortunately, I don't know what a “good” value is or should be. Both the implementation in Thanos and the implementation in Prometheus don't mention anything about this. That's why I would just leave the default and make it configurable as needed.

We can also set the flag to hidden if you prefer.

@GiedriusS
Copy link
Member

I think let's then make it hidden 👍

@roth-wine roth-wine force-pushed the ruler_alert_capacity_configurable branch 2 times, most recently from 4f63513 to ff39a64 Compare July 2, 2025 09:05
@roth-wine
Copy link
Contributor Author

I have made the flag hidden 😄

@roth-wine roth-wine force-pushed the ruler_alert_capacity_configurable branch from ff39a64 to b78b796 Compare July 2, 2025 09:13
@roth-wine
Copy link
Contributor Author

Failing test seems again unrelated to my changes and might be flacky.

@roth-wine
Copy link
Contributor Author

@GiedriusS can this get merged?

@roth-wine roth-wine force-pushed the ruler_alert_capacity_configurable branch from b78b796 to fc532f8 Compare July 29, 2025 06:22
…ications

In the [prometheus package](https://github.com/prometheus/prometheus/blob/7789ef
27c838a567edd65b5fb767d4a35cab91ac/cmd/prometheus/main.go#L532) we are already a
ble to configure the capacity size. The fixed value could lead to issues
sending alerts when evaluating a huge amount of alerts in one ruler
instance.

Signed-off-by: roth-wine <[email protected]>
@roth-wine roth-wine force-pushed the ruler_alert_capacity_configurable branch from fc532f8 to f99767d Compare July 29, 2025 06:24
@roth-wine
Copy link
Contributor Author

@GiedriusS i have rebased the branch to get this thing merged.
Failing tests are flacky and not related to my changes.

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.

2 participants