feat: prometheus alerting rules - #129
Conversation
📝 WalkthroughWalkthroughThe monitoring stack now loads Prometheus alert rules, detects feed and watchdog failures plus prediction-quality issues, and routes alerts through a new Alertmanager service to an internal webhook. ChangesPrometheus alerting pipeline
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant Prometheus
participant Alertmanager
participant InternalWebhook
Prometheus->>Prometheus: Evaluate configured alert rules
Prometheus->>Alertmanager: Send firing or resolved alerts
Alertmanager->>InternalWebhook: Send grouped webhook notification
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
|
coverage: 51.371%. remained the same — feat/prometheus-alerting-rules into main |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docker-compose.yml`:
- Around line 33-34: Update the Alertmanager service volume configuration
alongside the existing alertmanager.yml bind mount to add a named volume for
Alertmanager’s data directory, and declare that named volume in the compose
configuration so silences and notification history survive container
replacement.
- Around line 35-36: Restrict the Alertmanager management endpoint in the
service ports configuration by binding port 9093 to host loopback instead of all
interfaces. Preserve container port 9093 while preventing direct access from
untrusted hosts, and ensure any reverse-proxy or firewall policy allows only
trusted operators.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 07cba630-8211-4f89-8809-5f8b5d102eca
📒 Files selected for processing (6)
docker-compose.ymlprometheus/alertmanager.ymlprometheus/prometheus.ymlprometheus/rules/feed_staleness.ymlprometheus/rules/prediction_quality.ymlprometheus/rules/watchdog_meta.yml
| volumes: | ||
| - ./prometheus/alertmanager.yml:/etc/alertmanager/alertmanager.yml |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Persist Alertmanager state in a named volume.
This mount supplies only configuration. The service has no explicit data mount, so a container replacement can lose silences and notification-log state. Alertmanager persists both state types; losing them can restore muted alerts and cause duplicate notifications. (prometheus.io)
Proposed fix
alertmanager:
volumes:
- ./prometheus/alertmanager.yml:/etc/alertmanager/alertmanager.yml
+ - alertmanager-data:/alertmanager
+
+volumes:
+ alertmanager-data:📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| volumes: | |
| - ./prometheus/alertmanager.yml:/etc/alertmanager/alertmanager.yml | |
| volumes: | |
| - ./prometheus/alertmanager.yml:/etc/alertmanager/alertmanager.yml | |
| - alertmanager-data:/alertmanager | |
| volumes: | |
| alertmanager-data: |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docker-compose.yml` around lines 33 - 34, Update the Alertmanager service
volume configuration alongside the existing alertmanager.yml bind mount to add a
named volume for Alertmanager’s data directory, and declare that named volume in
the compose configuration so silences and notification history survive container
replacement.
| ports: | ||
| - "9093:9093" |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf 'Files:\n'
git ls-files | rg '(^|/)docker-compose\.ya?ml$|alertmanager|promtail|prometheus' || true
printf '\nRelevant compose snippets:\n'
if [ -f docker-compose.yml ]; then
nl -ba docker-compose.yml | sed -n '1,120p'
fi
printf '\nSearch for Alertmanager auth/reverse proxy/firewall config:\n'
rg -n 'alertsmanager|Alertmanager|alertmanager|web\.authorization|basic_auth|password_file|forwardauth|auth_request|allow.*9093|block.*9093|port.*9093|bindaddr|listen:' -S . || trueRepository: OneBusAway/watchdog
Length of output: 464
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf 'docker-compose.yml:\n'
cat -n docker-compose.yml | sed -n '1,120p'
printf '\nprometheus/alertmanager.yml:\n'
cat -n prometheus/alertmanager.yml | sed -n '1,220p'
printf '\nSearch for Alertmanager-related auth, proxy, or network controls:\n'
rg -n -S 'alertmanager|Alertmanager|9093|authorization|basic_auth|password|forwardauth|auth_request|allow|block|bindaddr|listen|web\.http' . || true
printf '\nSearch for firewall/network policy mentions:\n'
rg -n -S 'firewall|iptables|ufw|nginx|nginx-proxy|traefik|haproxy|caddy|cloudflare|access|allow|block|trusted|operator' . || trueRepository: OneBusAway/watchdog
Length of output: 11276
Broken Authentication (CWE-306): Missing Authentication for Critical Function
Reachability: External
Restrict the Alertmanager management endpoint.
"9093:9093" publishes Alertmanager on the Docker host without authentication or documented access control. Clients that reach this port can create a broad silence and suppress webhook notifications. Bind the port to loopback, or use authenticated ingress; verify that host firewall and reverse-proxy policy restrict port 9093 to trusted operators.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docker-compose.yml` around lines 35 - 36, Restrict the Alertmanager
management endpoint in the service ports configuration by binding port 9093 to
host loopback instead of all interfaces. Preserve container port 9093 while
preventing direct access from untrusted hosts, and ensure any reverse-proxy or
firewall policy allows only trusted operators.
Code reviewNo issues found. Checked for bugs and CLAUDE.md compliance. 🤖 Generated with Claude Code |
aaronbrethorst
left a comment
aaronbrethorst
left a comment
There was a problem hiding this comment.
This is clean. I checked every metric name and label in the rules against internal/metrics/metrics.go on main — oba_time_since_last_update_seconds, oba_realtime_trip_match_ratio, and oba_stop_match_ratio all exist and all carry the server label your annotations interpolate, and up{job="watchdog"} matches the existing scrape job name. The rule glob, the bind mount, and the alertmanager:9093 target line up, and the whole thing validates under promtool check rules, promtool check config, amtool check-config, and docker compose config. The absent() arm on WatchdogDown plus keep_firing_for is a nice detail.
Merging as-is. A few follow-ups worth picking up separately, none of which should hold this:
- The webhook receiver points at
http://host.internal/alerts, which nothing in the repo serves and which won't resolve on plain Linux Docker Engine. Worth a comment inalertmanager.ymlmarking it as a placeholder. - All three OBA metrics are
(server, agency)-labeled but the annotations only interpolateserver, so a multi-agency server produces alerts with byte-identical summaries — andgroup_by: ["alertname", "severity"]groups them into one notification. Adding{{ $labels.agency }}fixes both. - Alertmanager has no data volume, so silences and the notification log are lost on container recreate.
FeedStalecan't catch a fully unreachable OBA server:oba_time_since_last_update_secondsis only set when the poll succeeds, so the gauge freezes at its last good value rather than climbing past 900. Anoba_api_status == 0rule would close that gap.- Heads up that #131 removes the
serverlabel from all three of these metrics in favor ofagency_id. These rules are correct today; they'll need updating when that lands.
|
Thanks for merging, @aaronbrethorst! And no worries, I’ll take care of the points raised. |
3 participants
Summary by CodeRabbit
New Features
Operations