feat(monitoring): Give each server its own storage alert rule - #7444
feat(monitoring): Give each server its own storage alert rule#7444huzan-kazi wants to merge 1 commit into
Conversation
Storage thresholds were split into one rule per distinct threshold,
generated in memory by the shared rule. Servers sharing a threshold
shared a rule, and none of it was visible in Desk to inspect or disable
for a single server.
Follow the convention the autoscale triggers already set: a real
Prometheus Alert Rule document per overriding server, named
`<base rule> - <server>`, inheriting severity, timings, labels and press
job type from the shared rule. Reactions still resolve, because
react_for_instance looks the firing alertname up as a rule document.
The shared rule keeps `{{ instances }}` and now renders only the
exclusion, so an overriding server never alerts twice. Syncing also runs
on the shared rule's on_update, so editing its expression propagates
instead of leaving the per-server rules stale.
Follow-up to #7405.
 |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
|
|
||
| frappe.flags.syncing_storage_alert_rules = True | ||
| try: | ||
| for server, threshold in overrides.items(): |
There was a problem hiding this comment.
Cleanup runs inside the override loop. When the last server returns to the default threshold, the loop does not run, so its obsolete per-server rule remains enabled and can produce duplicate alerts at the old threshold. Run stale-rule cleanup after the loop.
Knowledge Base Used: Monitoring and telemetry
Prompt To Fix With AI
This is a comment left during a code review.
Path: press/press/doctype/prometheus_alert_rule/prometheus_alert_rule.py
Line: 291
Comment:
**Cleanup Skips Empty Overrides**
Cleanup runs inside the override loop. When the last server returns to the default threshold, the loop does not run, so its obsolete per-server rule remains enabled and can produce duplicate alerts at the old threshold. Run stale-rule cleanup after the loop.
**Knowledge Base Used:** [Monitoring and telemetry](https://app.greptile.com/frappe/-/custom-context/knowledge-base/frappe/press/-/docs/monitoring-and-telemetry.md)
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.|
|
||
| overrides = servers_by_storage_alert_threshold() | ||
| overridden_servers = [server for servers in overrides.values() for server in servers] | ||
|
|
||
| rules = [self.get_rule_for_threshold(DEFAULT_STORAGE_ALERT_THRESHOLD, overridden_servers, True)] | ||
| rules.extend( | ||
| self.get_rule_for_threshold(threshold, servers, False) for threshold, servers in overrides.items() | ||
| ) | ||
| return rules | ||
| overriding_servers = list(servers_by_storage_alert_threshold()) | ||
| return [ |
There was a problem hiding this comment.
Existing servers with custom thresholds have no generated rule documents immediately after deployment. Any alert-rule save can then publish the shared rule with those servers excluded, leaving them without storage alerts until the base rule or a server threshold is changed. Add an idempotent backfill before publishing these exclusions.
Knowledge Base Used: Monitoring and telemetry
Prompt To Fix With AI
This is a comment left during a code review.
Path: press/press/doctype/prometheus_alert_rule/prometheus_alert_rule.py
Line: 103-105
Comment:
**Existing Overrides Lose Alerts**
Existing servers with custom thresholds have no generated rule documents immediately after deployment. Any alert-rule save can then publish the shared rule with those servers excluded, leaving them without storage alerts until the base rule or a server threshold is changed. Add an idempotent backfill before publishing these exclusions.
**Knowledge Base Used:** [Monitoring and telemetry](https://app.greptile.com/frappe/-/custom-context/knowledge-base/frappe/press/-/docs/monitoring-and-telemetry.md)
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.| prefix = storage_alert_rule_name(base_rule.name, "") | ||
| for name in frappe.get_all("Prometheus Alert Rule", {"name": ("like", f"{prefix}%")}, pluck="name"): | ||
| if name[len(prefix) :] not in overriding_servers: | ||
| frappe.delete_doc("Prometheus Alert Rule", name) |
There was a problem hiding this comment.
Alert-rule names are user-defined, but cleanup deletes every rule whose name begins with <base> - when its suffix is not an overriding server. An unrelated rule using that prefix can therefore be deleted, removing its monitoring coverage. Mark generated children explicitly and restrict cleanup to those records.
Knowledge Base Used: Operations and reliability
Prompt To Fix With AI
This is a comment left during a code review.
Path: press/press/doctype/prometheus_alert_rule/prometheus_alert_rule.py
Line: 327-330
Comment:
**Cleanup Deletes Unrelated Rules**
Alert-rule names are user-defined, but cleanup deletes every rule whose name begins with `<base> - ` when its suffix is not an overriding server. An unrelated rule using that prefix can therefore be deleted, removing its monitoring coverage. Mark generated children explicitly and restrict cleanup to those records.
**Knowledge Base Used:** [Operations and reliability](https://app.greptile.com/frappe/-/custom-context/knowledge-base/frappe/press/-/docs/operations-and-reliability.md)
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.1 participant
Why
#7405 gave teams a storage alert threshold, and split the disk alert into one Prometheus rule per distinct threshold — generated in memory by the shared rule. Servers that picked the same threshold shared a rule, and none of the split was visible anywhere: nothing in Desk to inspect, disable, or reason about for a single server.
Press already has a convention for this. The autoscale triggers create a real
Prometheus Alert Ruledocument per server, namedAuto Scale Up Trigger - f929-mumbai.frappe.cloud. Storage alerts should look the same.What
A rule document per overriding server, named
<base rule> - <server>:Each inherits severity,
for, group timings, labels, annotations andpress_job_typefrom the shared rule, with its own threshold baked into the expression. Only servers that override the default get one; everyone else stays on the shared rule, which keeps{{ instances }}and now renders only the exclusion — so an overriding server never alerts twice.How
sync_storage_alert_rules()upserts the documents and deletes the ones no longer needed. It runs from theServer/Database Serverdoc event as before, and additionally from the shared rule's ownon_update, so editing the shared expression propagates instead of leaving the per-server rules stale.Reactions still resolve:
react_for_instancelooks the firing alertname up as aPrometheus Alert Ruledocument, and each per-server document carries the inheritedpress_job_type. There's a test pinning that, since it's the part that quietly breaks if the naming drifts.A
frappe.flags.syncing_storage_alert_rulesguard stops each child save from firing its own push to the monitor server; the shared rule pushes once for the whole sync.Review notes
Disk Space Lowrule is saved after deploy, viaon_update. No patch, but nothing happens until someone saves that rule.servers_by_storage_alert_threshold()filters onstatus = "Active", so a stale rule is cleaned up on the next sync — but archiving doesn't itself trigger one, since the threshold didn't change. The leftover rule matches an instance Prometheus no longer scrapes, so it never fires. Left alone rather than adding a second doc event; happy to hook status if reviewers prefer.🤖 Generated with Claude Code