feat: alert on Raven when hourly backup success rate drops below 97% - #7382
feat: alert on Raven when hourly backup success rate drops below 97%#7382regdocs wants to merge 1 commit into
Conversation
Site backup failures were only visible per-site (failure emails to the user, daily audit). Nothing flagged a fleet-wide dip in the backup success rate. Add an hourly check that compares settled Site Backups (Success/Failure) created in the last hour against a 97% success-rate threshold, and posts to the server alerts Raven channel when the rate falls below it. The message lists the worst-offending sites, following the format used by the public server pool health alert. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Confidence Score: 4/5Not safe to merge until delayed backup outcomes are included in the alert calculation. The creation-time filter can permanently omit failures from long-running backups, defeating the alert during a realistic degraded state. Files Needing Attention: press/press/doctype/site_backup/site_backup.py; press/press/doctype/site_backup/test_site_backup.py Prompt To Fix All With AI### Issue 1
press/press/doctype/site_backup/site_backup.py:960-962
**Delayed outcomes are omitted**
Backups receive their final status asynchronously, but this query filters them by creation time. A backup that takes more than an hour to settle is never counted, so delayed failures can be silently omitted from every alert.
### Issue 2
press/press/doctype/site_backup/test_site_backup.py:420-426
**Threshold boundary remains untested**
This test uses 34/35, which is 97.14%, rather than the exact 97% threshold. It would not catch a comparison regression at the boundary; use 97 successes and 3 failures instead.
```suggestion
def test_no_alert_when_success_rate_is_at_threshold(self, mock_send_raven_message):
# 97 out of 100 backups succeeded, i.e. 97%
self._create_backups(successes=97, failures=3)
alert_if_backup_success_rate_is_low()
mock_send_raven_message.assert_not_called()
```
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.Reviews (1): Last reviewed commit: "feat: alert on Raven when hourly backup ..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| completed_backups = frappe.db.count( | ||
| "Site Backup", {"status": ("in", ["Success", "Failure"]), "creation": (">=", since)} | ||
| ) |
There was a problem hiding this comment.
Backups receive their final status asynchronously, but this query filters them by creation time. A backup that takes more than an hour to settle is never counted, so delayed failures can be silently omitted from every alert.
Knowledge Base Used: Database operations and backups
Prompt To Fix With AI
This is a comment left during a code review.
Path: press/press/doctype/site_backup/site_backup.py
Line: 960-962
Comment:
**Delayed outcomes are omitted**
Backups receive their final status asynchronously, but this query filters them by creation time. A backup that takes more than an hour to settle is never counted, so delayed failures can be silently omitted from every alert.
**Knowledge Base Used:** [Database operations and backups](https://app.greptile.com/frappe/-/custom-context/knowledge-base/frappe/press/-/docs/database-operations-and-backups.md)
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
| def test_no_alert_when_success_rate_is_at_threshold(self, mock_send_raven_message): | ||
| # 34 out of 35 backups succeeded, i.e. 97.14% | ||
| self._create_backups(successes=34, failures=1) | ||
|
|
||
| alert_if_backup_success_rate_is_low() | ||
|
|
||
| mock_send_raven_message.assert_not_called() |
There was a problem hiding this comment.
This test uses 34/35, which is 97.14%, rather than the exact 97% threshold. It would not catch a comparison regression at the boundary; use 97 successes and 3 failures instead.
| def test_no_alert_when_success_rate_is_at_threshold(self, mock_send_raven_message): | |
| # 34 out of 35 backups succeeded, i.e. 97.14% | |
| self._create_backups(successes=34, failures=1) | |
| alert_if_backup_success_rate_is_low() | |
| mock_send_raven_message.assert_not_called() | |
| def test_no_alert_when_success_rate_is_at_threshold(self, mock_send_raven_message): | |
| # 97 out of 100 backups succeeded, i.e. 97% | |
| self._create_backups(successes=97, failures=3) | |
| alert_if_backup_success_rate_is_low() | |
| mock_send_raven_message.assert_not_called() |
Prompt To Fix With AI
This is a comment left during a code review.
Path: press/press/doctype/site_backup/test_site_backup.py
Line: 420-426
Comment:
**Threshold boundary remains untested**
This test uses 34/35, which is 97.14%, rather than the exact 97% threshold. It would not catch a comparison regression at the boundary; use 97 successes and 3 failures instead.
```suggestion
def test_no_alert_when_success_rate_is_at_threshold(self, mock_send_raven_message):
# 97 out of 100 backups succeeded, i.e. 97%
self._create_backups(successes=97, failures=3)
alert_if_backup_success_rate_is_low()
mock_send_raven_message.assert_not_called()
```
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #7382 +/- ##
============================================
- Coverage 85.83% 61.05% -24.79%
============================================
Files 137 1059 +922
Lines 26237 100134 +73897
Branches 1643 1643
============================================
+ Hits 22521 61138 +38617
- Misses 3675 38956 +35281
+ Partials 41 40 -1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
Tick the box to add this pull request to the merge queue (same as
|
2 participants
Problem
Site backup failures are only visible one site at a time — a failure email to the affected team, plus the daily
check_backup_recordsaudit. Nothing surfaces a fleet-wide dip in the backup success rate while it's happening.Solution
Add
alert_if_backup_success_rate_is_low(), registered underhourlyinhooks.py:Success/Failure).PendingandRunningare excluded, since they haven't resolved yet._send_public_server_pool_health_alertinserver_monitoring.py: actual rate in the header, the threshold, totals, and a markdown table of the worst-offending sites (capped at 20, with aN more failures on other sitesrow when truncated).It's two
COUNT(*)queries in the common case, so it sits inhourlyrather thanhourly_long.Notes for reviewers
Two calls worth a second opinion:
RAVEN_SERVER_ALERTS_CHANNEL(frappe-cloud-server-alerts). Press Settings also has araven_incidents_channelfield if backup alerts belong there instead.Tests
TestBackupSuccessRateAlertcovers below-threshold, exactly-at-threshold, an empty window, and unfinished backups not being counted.🤖 Generated with Claude Code