-
Notifications
You must be signed in to change notification settings - Fork 529
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
Support limits for silences #8241
Support limits for silences #8241
Conversation
7e78842
to
f2ae89c
Compare
3126d2d
to
7d7a12c
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM with one non-blocking comment
Metrics: am.registry, | ||
Limits: silence.Limits{ | ||
MaxSilences: cfg.Limits.AlertmanagerMaxSilencesCount(cfg.UserID), | ||
MaxPerSilenceBytes: cfg.Limits.AlertmanagerMaxSilenceSizeBytes(cfg.UserID), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
max-per-silence-bytes
makes this limit a lot easier to understand (compared to max-silence-size-bytes
). WDYT about using max per silence bytes everywhere? OK too if you'd like to keep it as-is for consistency with other limits.
EDIT: lol, I see @alexweav suggested the opposite. Ignore me!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would like to keep it consistent with other limits 🙂
This reverts commit 2ead4da.
What this PR does
This pull request adds support for silence limits prometheus/alertmanager#3852.
Which issue(s) this PR fixes or relates to
Fixes #
Checklist
CHANGELOG.md
updated - the order of entries should be[CHANGE]
,[FEATURE]
,[ENHANCEMENT]
,[BUGFIX]
.about-versioning.md
updated with experimental features.