Skip to content

Commit a43cad3

Browse files
authored
Merge pull request #38 from jantman/issue-12-monitoring-alerting
Fixes #12: Implement monitoring + alerting and system-health metrics
2 parents 1d79931 + 1268191 commit a43cad3

11 files changed

Lines changed: 1393 additions & 40 deletions

File tree

‎_bmad-output/implementation-artifacts/tech-spec-monitoring-and-alerting.md‎

Lines changed: 692 additions & 0 deletions
Large diffs are not rendered by default.

‎docker-compose.yml‎

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -9,6 +9,11 @@ services:
99
env_file: .env
1010
environment:
1111
- SLACK_SOCKET_MODE_CONNECT=true
12+
# Force unbuffered stdout/stderr so log lines reach journald/Loki
13+
# immediately rather than sitting in Python's block buffer when stdout
14+
# is not a TTY. Without this, app-side log-based alerting (Loki) lags
15+
# multi-second behind the actual events.
16+
- PYTHONUNBUFFERED=1
1217
volumes:
1318
- ./uploads:/app/uploads
1419
healthcheck:

‎docs/administrators.md‎

Lines changed: 122 additions & 32 deletions
Original file line numberDiff line numberDiff line change
@@ -279,6 +279,127 @@ docker compose logs app | grep -i "new.relic\|newrelic"
279279

280280
To disable New Relic, remove or comment out `NEW_RELIC_LICENSE_KEY` in your `.env` file and restart the services. When the license key is not set, no New Relic code is loaded and there is zero performance impact.
281281

282+
## Monitoring and Alerting
283+
284+
### Overview
285+
286+
ESB exposes Prometheus metrics on `/metrics` (unauthenticated; trusted-network deployment). Both the `app` and `worker` containers run with `PYTHONUNBUFFERED=1` so log lines reach Loki/Promtail without buffering latency. The metrics are designed for direct Grafana panel use. This section is complementary to the optional New Relic integration above; it gives recommended *signals*, not a turnkey configuration.
287+
288+
### Prometheus Metrics
289+
290+
Example scrape config:
291+
292+
```yaml
293+
scrape_configs:
294+
- job_name: esb
295+
metrics_path: /metrics
296+
static_configs:
297+
- targets: ['esb.example.com:5000']
298+
```
299+
300+
| Metric | Type | Description | Emission |
301+
|--------|------|-------------|----------|
302+
| `esb_pending_notifications_count` | gauge | Number of rows in `pending_notifications` with `status='pending'` | Always |
303+
| `esb_oldest_pending_notification_timestamp_seconds` | gauge | Unix epoch seconds of the oldest pending row's `created_at` | Omitted when queue empty (alert with `absent()`) |
304+
| `esb_worker_last_iteration_timestamp_seconds` | gauge | Unix epoch seconds of the worker's last successful poll cycle (read from `AppConfig.value`) | Omitted when worker has never run, or when the `AppConfig` query fails (alert with `absent()`, **`for: 5m` minimum**) |
305+
| `esb_socket_mode_enabled` | gauge | `1` if `init_slack` entered the Socket Mode setup block (tokens set, not `TESTING`, opt-in flag true); `0` otherwise | Always |
306+
| `esb_socket_mode_connected` | gauge | `1` if a Bolt SocketModeHandler is currently bound; `0` otherwise. Transitions 1→0 at process shutdown. | Always |
307+
308+
Example alert rules:
309+
310+
```yaml
311+
- alert: ESBNotificationQueueStuck
312+
expr: time() - esb_oldest_pending_notification_timestamp_seconds > 300
313+
for: 1m
314+
annotations:
315+
summary: "ESB notification worker is not draining the queue"
316+
```
317+
318+
```yaml
319+
- alert: ESBWorkerStalled
320+
expr: time() - esb_worker_last_iteration_timestamp_seconds > 120
321+
for: 1m
322+
annotations:
323+
summary: "ESB notification worker has not iterated in 2+ minutes"
324+
```
325+
326+
```yaml
327+
- alert: ESBWorkerNeverRan
328+
expr: absent(esb_worker_last_iteration_timestamp_seconds)
329+
for: 5m
330+
annotations:
331+
summary: "ESB worker has not produced a heartbeat row since deploy (or DB reset / transient query failure)"
332+
```
333+
334+
```yaml
335+
- alert: ESBSocketModeFailedAtBoot
336+
expr: (esb_socket_mode_enabled == 1 and esb_socket_mode_connected == 0) unless on(instance) up == 0
337+
for: 5m
338+
annotations:
339+
summary: "ESB intended to run Slack Socket Mode but the handler failed at boot"
340+
```
341+
342+
**`ESBWorkerStalled` and `ESBWorkerNeverRan` are complementary and should both be loaded.** `ESBWorkerStalled` detects "worker was alive recently but stopped iterating" — fires on a normal stall but doesn't fire when the metric is missing entirely. `ESBWorkerNeverRan` detects the metric-missing case — fires on cold-deploy time-to-first-poll AND on transient `AppConfig` query failures. Together they cover the full failure space.
343+
344+
!!! note "Clock skew"
345+
`time() - <gauge>` rules mix Prometheus's clock with the worker container's clock. Run NTP on every node and pick the threshold ≥ 4× `poll_interval` (so 120s for the default 30s). Note the failure asymmetry: if the worker's clock runs *behind* Prometheus's, the rule fires aggressively; if the worker's clock runs *ahead*, `time() - gauge` goes negative and the rule is **silent forever**. NTP is mandatory, not optional.
346+
347+
!!! note "Single-worker assumption"
348+
These metrics assume the current single-gunicorn-worker deployment (`--workers 1`). Scaling app-side gunicorn workers makes the Socket Mode metrics non-deterministic across scrapes.
349+
350+
!!! note "Information disclosure"
351+
The Socket Mode gauges let any unauthenticated reader of `/metrics` distinguish "Slack not configured" from "Slack configured but failed at boot" from "Slack working." Acceptable on a trusted network; something to be aware of if `/metrics` is ever exposed more broadly.
352+
353+
!!! note "`ESBSocketModeFailedAtBoot` shutdown safety"
354+
The rule includes `unless on(instance) up == 0` to suppress the alert during a full app outage (where `up == 0` is already the dominant signal) and uses `for: 5m` to absorb gunicorn worker reloads (`--max-requests` recycling) where `_shutdown_socket()` briefly leaves state at `(1, 0)`. The `on(instance)` clause assumes targets share an `instance` label; if your relabeling adds richer labels (e.g. `cluster`, `env`) you may need `unless on(job, instance) up == 0` to keep the join correct.
355+
356+
!!! note "`/metrics` endpoint resilience"
357+
The endpoint returns HTTP 200 even when the `app_config` table is missing (e.g., on a fresh deployment that hasn't yet run `flask db upgrade`). The worker-timestamp metric is simply omitted; alert via `ESBWorkerNeverRan`. The first per-process query failure logs a full stack trace; subsequent failures log a one-line warning to avoid log flooding.
358+
359+
### Container and Process Liveness
360+
361+
`up{job="esb"} == 0` for ≥ 1 minute indicates the app is not responding to scrapes — covers OOM kills, gunicorn wedges, and network partitions. cAdvisor's `container_last_seen` (or its restart-count rate) catches container restart loops where the app keeps crashing fast enough that `up{}` may briefly recover between scrapes.
362+
363+
### Log-Based Alerting (Loki)
364+
365+
ESB writes logs to stdout/stderr; both the `app` and `worker` containers run with `PYTHONUNBUFFERED=1` so lines reach Promtail/Loki without Python's default block-buffering latency.
366+
367+
| What to detect | Source | Log substring |
368+
|----------------|--------|---------------|
369+
| Worker poll-cycle failure (any exception in the loop body) | `notification_service.py` (worker outer-try) | `Error in worker polling loop` |
370+
| Slack delivery exception (per notification, app-log line) | `notification_service.py` (per-notification failure log) | `delivery failed:` (trailing colon required — uniquely matches the failure-line format string `'Notification %d delivery failed: %s'`; does NOT match the success log or the `NotImplementedError` log even though all three share the `Notification %d ...` prefix; also does not match the JSON mutation log) |
371+
| Worker heartbeat write failure | `notification_service.py` (`_write_heartbeat`) | `Failed to update worker heartbeat at` |
372+
| Worker last-iteration write failure | `notification_service.py` (`_record_iteration_timestamp`) | `Failed to update worker last-iteration timestamp` |
373+
| Buggy iteration-timestamp helper | `notification_service.py` (worker-loop call site) | `BUG: _record_iteration_timestamp raised unexpectedly` |
374+
| Slack Socket Mode setup failure (import / instantiation / connect) | `esb/slack/__init__.py` (unified setup `try`) | `Failed to set up Slack Socket Mode` |
375+
| `/metrics` AppConfig query failure (first per process) | `esb/services/metrics_service.py` | `Failed to query worker_last_iteration_at from AppConfig` |
376+
| Generic ERROR-level traffic | any | `ERROR` (level) and/or `Traceback` |
377+
378+
**Permanent-fail signal lives in the structured JSON mutation log.** When a notification is permanently failed after `MAX_RETRIES`, the mutation logger writes a single-line JSON record to logger `esb.mutations` containing `event: notification.permanently_failed`. Two equivalent alerting options:
379+
380+
- **Substring match** on `notification.permanently_failed` — simplest; works regardless of JSON-whitespace variation.
381+
- **Promtail JSON-stage parsing** — extract `event` as a structured Loki label. Use a Promtail `match` stage to apply the JSON parser only to lines starting with `{`, since the regular Python logger and the mutation logger share the same stdout stream.
382+
383+
Operators write their own LogQL queries and alert rules; this guide intentionally lists signals, not queries. Note that the substrings in the table above are stable today but unanchored — a future log message containing the same text would also match. For a future-resistant query, anchor with the `Notification N` prefix (regex `Notification \d+ delivery failed:` for the per-notification case), or filter by log level.
384+
385+
### What to Alert On
386+
387+
- **App down** — `up{job="esb"} == 0` for ≥ 1 m
388+
- **Worker stalled** — the `ESBWorkerStalled` rule
389+
- **Worker never ran since deploy / DB reset** — the `ESBWorkerNeverRan` rule
390+
- **Notification queue stuck** — the existing `ESBNotificationQueueStuck` rule
391+
- **Slack Socket Mode failed at boot** — the `ESBSocketModeFailedAtBoot` rule (covers import, instantiation, and connect failures; suppressed during full app outage)
392+
- **Elevated rate of Slack delivery failures** — Loki on `delivery failed:` substring exceeding a per-minute threshold
393+
- **Container flapping** — cAdvisor restart-count rate
394+
395+
### Grafana Dashboards
396+
397+
The ESB metrics are designed for direct panel use — gauge panels for the Socket Mode and worker-timestamp metrics, time-series panels for the queue gauges, and log panels backed by the Loki substrings above. ESB does not ship a dashboard JSON; operators build the panels they actually want to watch.
398+
399+
### Relationship to New Relic
400+
401+
New Relic (above) and the Prometheus/Loki/Grafana stack here observe different layers and are complementary. New Relic provides per-transaction APM and end-user browser monitoring; Prometheus provides system-health gauges and log-based alerting suitable for on-call paging. They can run together with no additional configuration.
402+
282403
## Ongoing Maintenance
283404

284405
### Viewing Logs
@@ -343,38 +464,7 @@ docker inspect --format '{{.State.Health.Status}}' equipment-status-board-worker
343464

344465
If the worker is reported as `unhealthy`, the autoheal sidecar will restart it automatically (typically within a minute).
345466

346-
### Prometheus Metrics
347-
348-
The application exposes a Prometheus exposition endpoint at `/metrics` (unauthenticated). Two gauges are published from the `pending_notifications` table:
349-
350-
| Metric | Description |
351-
|--------|-------------|
352-
| `esb_pending_notifications_count` | Number of rows with `status='pending'`. Always present. |
353-
| `esb_oldest_pending_notification_timestamp_seconds` | Unix epoch (seconds) of the oldest pending row's `created_at`. **Omitted entirely** when the queue is empty — alert rules should rely on `absent()` rather than checking for a sentinel value. |
354-
355-
These two metrics together catch a stuck worker, a bad Slack token, and a Slack outage — the queue grows and the oldest pending row ages.
356-
357-
Example Prometheus scrape config:
358-
359-
```yaml
360-
scrape_configs:
361-
- job_name: esb
362-
metrics_path: /metrics
363-
static_configs:
364-
- targets: ['esb.example.com:5000']
365-
```
366-
367-
Example alert rule (oldest pending row stuck for more than 5 minutes):
368-
369-
```yaml
370-
- alert: ESBNotificationQueueStuck
371-
expr: time() - esb_oldest_pending_notification_timestamp_seconds > 300
372-
for: 1m
373-
annotations:
374-
summary: "ESB notification worker is not draining the queue"
375-
```
376-
377-
The alert evaluates `time() - X` against a fresh timestamp on every scrape, which is more robust than a worker-computed "age" gauge that would be stale between scrapes.
467+
For metrics, log-based alerting, and recommended dashboards, see the [Monitoring and Alerting](#monitoring-and-alerting) section above. (External links to `#prometheus-metrics` continue to resolve — the new `### Prometheus Metrics` subsection auto-generates the same anchor.)
378468

379469
### Upload Storage
380470

‎esb/services/metrics_service.py‎

Lines changed: 116 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,28 +1,45 @@
11
"""Prometheus metrics for the notification queue.
22
3-
Exposes two gauges that, together, catch a stuck worker, a bad Slack token,
3+
Exposes gauges that, together, catch a stuck worker, a bad Slack token,
44
or a Slack outage:
55
66
- ``esb_pending_notifications_count`` — count of rows with status='pending'.
77
- ``esb_oldest_pending_notification_timestamp_seconds`` — Unix epoch seconds
88
of the oldest pending row's ``created_at``. **Omitted entirely** when the
99
table has no pending rows; Prometheus alert rules should use ``absent()``
1010
rather than a sentinel value.
11+
- ``esb_worker_last_iteration_timestamp_seconds`` — Unix epoch seconds of the
12+
worker's last successful poll cycle (read from ``AppConfig``). Omitted when
13+
the worker has never run or the underlying query fails.
14+
- ``esb_socket_mode_enabled`` / ``esb_socket_mode_connected`` — Slack Socket
15+
Mode intent and binding state. Always emitted.
1116
1217
Alert rules belong in Prometheus, not here. Example::
1318
1419
time() - esb_oldest_pending_notification_timestamp_seconds > 300
1520
"""
1621

17-
from datetime import UTC
22+
import logging
23+
from datetime import UTC, datetime
1824

1925
from prometheus_client import CollectorRegistry, generate_latest
2026
from prometheus_client.core import GaugeMetricFamily
2127
from prometheus_client.exposition import CONTENT_TYPE_LATEST
28+
from sqlalchemy import select
29+
from sqlalchemy.exc import SQLAlchemyError
2230

2331
from esb.extensions import db
32+
from esb.models.app_config import AppConfig
2433
from esb.models.pending_notification import PendingNotification
2534

35+
logger = logging.getLogger(__name__)
36+
37+
# Per-process latch: the first AppConfig query failure logs a full traceback
38+
# (diagnostically useful), subsequent failures log a one-line warning. Prevents
39+
# log flooding on pre-migration deployments where /metrics is scraped every
40+
# 15-30s and the app_config table doesn't yet exist.
41+
_app_config_query_failed_once: bool = False
42+
2643

2744
def _query_pending_stats() -> tuple[int, float | None]:
2845
"""Return (pending_count, oldest_created_at_unix_seconds_or_None).
@@ -81,12 +98,109 @@ def collect(self):
8198
)
8299

83100

101+
class _WorkerStatusCollector:
102+
"""Custom collector for worker-liveness and Slack Socket Mode state.
103+
104+
Emits up to three gauges per scrape:
105+
- ``esb_worker_last_iteration_timestamp_seconds`` (omitted if absent or
106+
the AppConfig query fails)
107+
- ``esb_socket_mode_enabled`` (always emitted)
108+
- ``esb_socket_mode_connected`` (always emitted)
109+
110+
Failures of the AppConfig query never propagate out of ``collect()`` — the
111+
worker-timestamp gauge is simply omitted. The Socket Mode gauges still
112+
emit so the actionable failure mode (enabled=1, connected=0) remains
113+
observable even when the DB is unavailable.
114+
"""
115+
116+
def collect(self):
117+
global _app_config_query_failed_once
118+
row = None
119+
try:
120+
row = db.session.execute(
121+
select(AppConfig).where(AppConfig.key == 'worker_last_iteration_at')
122+
).scalar_one_or_none()
123+
except SQLAlchemyError as e:
124+
# CRITICAL: rollback the session before continuing. SQLAlchemy 2.x
125+
# marks the session as failed after a query error; subsequent
126+
# db.session.execute() calls in the same request (e.g. by the
127+
# _PendingNotificationsCollector that runs in the same scrape)
128+
# would otherwise raise PendingRollbackError.
129+
db.session.rollback()
130+
# Rate-limit: full traceback only on the first failure per
131+
# process; subsequent failures get a one-line warning so /metrics
132+
# scrapes (every 15-30s) on a pre-migration deployment do not
133+
# flood logs.
134+
if not _app_config_query_failed_once:
135+
logger.warning(
136+
'Failed to query worker_last_iteration_at from AppConfig; '
137+
'omitting metric',
138+
exc_info=True,
139+
)
140+
_app_config_query_failed_once = True
141+
else:
142+
logger.warning(
143+
'Failed to query worker_last_iteration_at from AppConfig: %s: %s',
144+
type(e).__name__, e,
145+
)
146+
if row is not None:
147+
try:
148+
# datetime.fromisoformat accepts only str — None or wrong type
149+
# raises TypeError, malformed string raises ValueError. Both
150+
# are non-fatal here; omit the metric and log.
151+
ts = datetime.fromisoformat(row.value)
152+
yield GaugeMetricFamily(
153+
'esb_worker_last_iteration_timestamp_seconds',
154+
"Unix timestamp (seconds) of the worker's last successful "
155+
'poll. Omitted if the worker has never run or the AppConfig '
156+
'query failed.',
157+
value=ts.timestamp(),
158+
)
159+
except (ValueError, TypeError):
160+
logger.warning(
161+
'Failed to parse worker last-iteration timestamp value=%r',
162+
row.value, exc_info=True,
163+
)
164+
165+
# Lazy + dotted import inside collect(). Two reasons (NOT circular-
166+
# import avoidance — there is no circular-import risk):
167+
# 1. Defers Flask app-context dependencies that some test fixtures
168+
# lazily install during early test setup.
169+
# 2. Preserves the monkeypatch surface: tests do
170+
# monkeypatch.setattr('esb.slack.is_socket_mode_connected', ...)
171+
# which only takes effect via attribute lookup at call time. A
172+
# top-of-module `from esb.slack import is_socket_mode_connected`
173+
# would bind the symbol locally and silently miss the patch.
174+
# If you "clean this up" by moving the import to module top, you will
175+
# break the test design. Don't.
176+
import esb.slack as _slack
177+
yield GaugeMetricFamily(
178+
'esb_socket_mode_enabled',
179+
'1 if init_slack entered the Socket Mode setup block (tokens set, '
180+
'not TESTING, opt-in flag true). 0 on any of the four early-return '
181+
'paths or if the setup block was never reached.',
182+
value=1.0 if _slack.is_socket_mode_enabled() else 0.0,
183+
)
184+
yield GaugeMetricFamily(
185+
'esb_socket_mode_connected',
186+
'1 if a Bolt SocketModeHandler is currently bound; 0 if never '
187+
'bound or released. Reflects handler-binding state, NOT live '
188+
'WebSocket connection state — Slack Bolt does not expose mid-life '
189+
'connection callbacks. Transitions 1->0 at process shutdown.',
190+
value=1.0 if _slack.is_socket_mode_connected() else 0.0,
191+
)
192+
193+
84194
def render_metrics() -> tuple[bytes, str]:
85195
"""Render the Prometheus exposition payload and content-type.
86196
87197
Returns:
88198
Tuple of (body, content_type) suitable for a Flask response.
89199
"""
90200
registry = CollectorRegistry()
201+
# Order is defensive: with the explicit db.session.rollback() in
202+
# _WorkerStatusCollector, registration order is no longer load-bearing
203+
# for correctness, but pinning it makes intent unambiguous.
91204
registry.register(_PendingNotificationsCollector())
205+
registry.register(_WorkerStatusCollector())
92206
return generate_latest(registry), CONTENT_TYPE_LATEST

0 commit comments

Comments
 (0)