feat(limit): drop resolved alerts without an admitted firing counterpart#5381
feat(limit): drop resolved alerts without an admitted firing counterpart#5381siavashs wants to merge 1 commit into
Conversation
Rework the per-alertname alert limit so resolved notifications no longer consume limit slots, and change how they are handled. - limit: add Bucket.Remove to free a tracked slot by value/fingerprint. - store: apply the per-alert limit only to firing alerts. A resolved notification is forwarded only if its firing counterpart was previously admitted (its fingerprint is still tracked); removing it frees the slot the firing alert was holding. Resolves with no admitted firing are dropped as noise, since nothing downstream ever received a firing for them from us. - provider/mem: add a `state` label (firing/resolved) to alertmanager_alerts_limited_total so dropped firing alerts and dropped resolved notifications can be distinguished. - docs: document the new resolved handling and metric label in alertmanager.md and configuration.md. - tests: add Bucket.Remove coverage (presence return, slot freeing, double remove, heap eviction-order integrity, concurrency) and store coverage for the drop-if-unseen resolved semantics. Signed-off-by: Siavash Safi <siavash@cloudflare.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (10)
📝 WalkthroughWalkthroughThe change adds removal support to limit buckets, forwards resolved alerts only when their firing alerts were admitted, labels limited-alert metrics by state, migrates provider and store APIs to ChangesAlert limits and type migration
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant AlertInput
participant Store
participant LimitBucket
participant Metrics
AlertInput->>Store: Submit firing or resolved alert
Store->>LimitBucket: Admit firing or remove resolved fingerprint
LimitBucket-->>Store: Admission result
Store->>Metrics: Record dropped alert state
Store-->>AlertInput: Store or reject notification
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
| labels = append(labels, alert.Name()) | ||
| } | ||
| state := "firing" | ||
| if alert.Resolved() { |
There was a problem hiding this comment.
nit: this state may theoretically change between when we checked in a.alerts.Set and now... Maybe we need to put the state of the ErrLimited in the error itself?
There was a problem hiding this comment.
The status should not change, for firing alerts Prometheus sets EndsAt to few minutes in future by default, so this will work fine unless someone has a very weirdly short-lived alerts configured on prometheus.
For resolved alerts they are already resolved when they hit Alertmanager.
Maybe we need to put the state of the ErrLimited in the error itself?
Do you mean a special error for the case of resolved vs. firing?
|
|
||
| // Apply per alert limits if necessary | ||
| // Apply per alert limits if necessary. | ||
| if a.perAlertLimit > 0 { |
There was a problem hiding this comment.
On the "firing" side it might be that if a firing alert is rate-limited, it still shows as resolved in a.alerts[fp] until GC, if it resolved before being subsequently rate-limited. Do we care?
There was a problem hiding this comment.
If a firing alert is dropped Alertmanager does not track it and it will not be in the bucket or the store.
| // If the bucket is full, oldest expired item is evicted based on priority and the new value is added. | ||
| // Otherwise the new value is ignored and the method returns false. | ||
| func (b *Bucket[V]) Upsert(value V, priority time.Time) (ok bool) { | ||
| if b.capacity < 1 { |
There was a problem hiding this comment.
Do we need this check? Would there be a point for a capacity 0 bucket? Should we fail at NewBucket instead in that case?
There was a problem hiding this comment.
The bucket limiter was added as a generic implementation, zero here means drop all, we can change it in future to allow all.
2 participants
Rework the per-alertname alert limit so resolved notifications no longer consume limit slots, and change how they are handled.
statelabel (firing/resolved) to alertmanager_alerts_limited_total so dropped firing alerts and dropped resolved notifications can be distinguished.Pull Request Checklist
Please check all the applicable boxes.
benchstatto compare benchmarksWhich user-facing changes does this PR introduce?