Alertmanager alerts limits

[pstibrany]

What this PR does: This PR adds new limits in Alertmanager:

• total number of alerts that a user can have in Alertmanager's memory (-alertmanager.max-alerts-count)
• total size of alerts that user can have in Alertmanager's memory (-alertmanager.max-alerts-size-bytes)

These are overrideable per tenant. When limits are reached, Alertmanager will reject more alerts, produce a log message and increment cortex_alertmanager_insert_alert_failures_total metric. Additional metric to track behaviour of alerts limiter are: cortex_alertmanager_alerts_limiter_current_alerts_count and cortex_alertmanager_alerts_limiter_current_alerts_size_bytes.

This PR builds on top of #4237, and will be rebased once #4237 is merged.

Checklist

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

[pstibrany]

Marking as draft until #4237 is merged.

[gotjosh]

Can you assign this to me? I'll give you a first pass on it.

[pstibrany]

Can you assign this to me? I'll give you a first pass on it.

Unfortunately assigning only cortex-project members :( But don't let that stop you. Thank you!

[pstibrany]

Rebased on top of master now, it's ready for review.

[stevesg]

Will let Josh do a first pass and come back to this one.

[gotjosh]

🎖️ Fantastic job @pstibrany ! Most of my comments are nits and questions (for the benefit of my understanding).

[gotjosh]

An alert is considered unique based on the hash of its labels (fingerprint), technically if an alert comes in with a size that's OK, then its annotations change and its size is no longer acceptable we would bypass it because we would consider its size to be OK because it was already there.

I believe we need to check that if it does exist, its new size would not tip us over the limit. Unless you want the semantic to be: If we had the alert before, we'll always accept it regardless of whenever it tips us over the limit

If this is the case, can we add a comment?

[pstibrany]

If we had the alert before, we'll always accept it regardless of whenever it tips us over the limit

That was indeed my intention, mostly to make sure that we don't accidentally reject "resolved" alerts. But if the size has changed, it may make sense to reject it if it's over limit. I don't have strong opinion on this.

[pstibrany]

Also ... when new alert arrives and "merges" with existing one, it cannot update labels (fingerprint would change) or annotations (right now, at least).

See the merging code:

cortex/vendor/github.com/prometheus/alertmanager/types/types.go

Line 366 in 5e496f4

func (a *Alert) Merge(o *Alert) *Alert {

[gotjosh]

or annotations (right now, at least)

I see that is true for the labels, which is expected but is it also for the annotations? I can't seem to see it in the code - am I missing something?

[pstibrany]

I see that is true for the labels, which is expected but is it also for the annotations? I can't seem to see it in the code - am I missing something?

No, you’re right. I misread the code I linked… line 372 is copying everything over, including annotations.

[gotjosh]

Makes sense. In this case, I think rejection would be the best course of action - don't you think?

The alert that existed before should have a short enough endsAt that it would resolve itself eventually so there's little to no risk of just leaving the existing one to resolve by itself. On the contrary, accepting the alert that might tip us over the limit risks our availability.

[pstibrany]

I think you're right, and I will make this change before merging.

[pstibrany]

I've changed the logic such that existing alert that grows and doesn't fit the limit anymore is rejected.

[pracucci]

Good job! I left a couple of comments I would be glad if you could take a look 🙏 🙇

[pstibrany]

Thank you for reviews, I've addressed all your comments. Please take a look again.

[pracucci]

LGTM (modulo a nit). Thanks a lot for addressing my feedback 🙏