[Tim020]

Adds docker-compose.monitoring.yaml overlay (Prometheus v3.15.1, Grafana 13.2.4), alert rules, provisioned dashboard, and a Monitoring section in documentation/deployment.md. Grafana password required via GF_SECURITY_ADMIN_PASSWORD; ports bound to 127.0.0.1.

🤖 Generated with Claude Code

[github-actions]

Client Test Results

102 tests   102 ✅  0s ⏱️
  9 suites    0 💤
  1 files      0 ❌

Results for commit 9f55fbd.

♻️ This comment has been updated with latest results.

[github-actions]

Client V3 Test Results

227 tests   227 ✅  1s ⏱️
 20 suites    0 💤
  1 files      0 ❌

Results for commit 9f55fbd.

♻️ This comment has been updated with latest results.

[github-actions]

Python Test Results

    1 files  ±0      1 suites  ±0   1m 21s ⏱️ -1s
1 241 tests ±0  1 241 ✅ ±0  0 💤 ±0  0 ❌ ±0 
1 272 runs  ±0  1 272 ✅ ±0  0 💤 ±0  0 ❌ ±0 

Results for commit 9f55fbd. ± Comparison against base commit d106e36.

♻️ This comment has been updated with latest results.

[github-actions]

Playwright E2E Results (firefox)

227 tests   227 ✅  2m 23s ⏱️
 14 suites    0 💤
  1 files      0 ❌

Results for commit 9f55fbd.

♻️ This comment has been updated with latest results.

[github-actions]

Playwright E2E Results (chromium)

227 tests  ±0   227 ✅ ±0   2m 26s ⏱️ +12s
 14 suites ±0     0 💤 ±0 
  1 files   ±0     0 ❌ ±0 

Results for commit 9f55fbd. ± Comparison against base commit d106e36.

♻️ This comment has been updated with latest results.

[Tim020]

Review round 1

Head SHA reviewed: 4151830

Verdict: Changes needed — the overlay itself (compose wiring, provisioning paths, datasource UID, security posture, health-check docs) is solid and matches the base branch exactly, but two of the five alert rules have real bugs that undermine the point of adding alerting in the first place.

Previous round follow-up: N/A (round 1).

SonarQube: Quality gate passed — 0 new issues, 0 security hotspots, 0% new coverage/duplication (config/docs-only PR, no code to cover). Source: GitHub check-run output on the head commit (SonarCloud Code Analysis), cross-checked against the sonarqubecloud[bot] PR comment.

New findings: 2 Important, 2 Suggestion.

• Important: DigiScriptHigh5xxRate alert (and the matching dashboard panel) will fire on ordinary WebSocket traffic, not real errors — the Tornado Prometheus mixin buckets the WS handshake's 101 status as "5xx".
• Important: DigiScriptUnhandledErrors alert (and its dashboard panel) will typically miss the first unhandled error for a given source after process start, which is the case it most needs to catch — a Prometheus counter series created on first increment can't be measured by increase() in that same window.
• Suggestion: the new "Metrics reference" note in documentation/deployment.md says tornado_http_* currently has no samples, but the fix for that already ships on this PR's own base branch.
• Suggestion: the dashboard's label_replace on the status-class panel is redundant (the label is already a pre-bucketed class string, not numeric) and worth simplifying while the 5xx issue is fixed.

All four are inline below with suggested fixes. Both Important issues ultimately need a code change upstream (in server/digi_server/app_server.py and server/digi_server/logger.py on the feature/obs-app-metrics base), not just a tweak to these YAML/JSON files — I've suggested a local mitigation in the alert expressions too, in case the upstream fix lands later.

Strengths:

• Compose overlay wiring is correct: service name, port, and volume mount paths all line up with the base docker-compose.yaml and with each other (scrape target, rule_files path, Grafana provisioning/dashboards paths, datasource UID referenced consistently across every panel).
• No committed secrets; GF_SECURITY_ADMIN_PASSWORD has no default and compose refuses to start without it; ports default to 127.0.0.1; images are pinned to exact tags.
• The unauthenticated /debug/metrics exposure is called out explicitly and accurately in the docs.
• Health-check documentation (timeouts, retry counts, response shape, log levels on failure) matches server/controllers/api/v1/health.py and the Dockerfile exactly.
• Every other metric name in the dashboard/alerts matches what's actually exported on the base branch (digiscript_auth_events_total is the one exception, correctly hedged by the "added incrementally" docs note).

🤖 Generated with Claude Code

[Tim020]

Review round 1 addressed in 3bb8cad.

• 5xx alert fires on WS upgrades: fixed. Verified in tornado_prometheus.classify_status_code (anything outside 200-499, including 101, is "5xx"). The root-cause fix is in Fix missing HTTP request metrics; demote success access logs to DEBUG #1464. As defence in depth the alert and the dashboard rate panel now exclude handler="WebSocketController", and alerts.yml carries a comment on the dependency.
• Unhandled-errors alert misses the first error per source: fixed. The expression is now increase(x[5m]) > 0 or (x > 0 unless x offset 5m), which also catches a series that first appeared in the window. The dashboard panel uses the same logic. Pre-registering at 0 (Capture asyncio/alembic/warnings logs and install crash hooks #1466) remains the root-cause fix.
• Stale "no samples" note in deployment.md: fixed. It now only says older releases lack some metrics. The stale panel description was also replaced.
• Redundant label_replace: fixed. The panel is now sum by (status) (rate(...)).

Validated: promtool check rules and config pass, docker compose config passes, and the dashboard JSON parses. The /health section was left untouched pending the final contract.

[Tim020]

Docs update 3bb8cad: the Health check section of deployment.md now matches the final /api/v1/health contract from #1463 (sqlite_master probe, single-flight 3s timeout, storage paths, 503 and unknown-state behaviour, Docker HEALTHCHECK port constraint with a compose healthcheck override example).

[Tim020]

Correction: the health docs update is commit 523de87 (the previous comment quoted the wrong sha).

[Tim020]

Review round 2

Head SHA reviewed: fc51b30

Verdict: Changes needed, but close — one new Important finding and a couple of minor documentation nits. All four round-1 findings are now fully and correctly fixed, with defense-in-depth beyond what was asked for.

Previous round follow-up: 4 fixed, 0 still open, 0 incomplete, 0 needs human decision.

• WS-101-as-5xx alert bug — verified fixed server-side plus a defence-in-depth handler!="WebSocketController" exclusion in the alert itself.
• Missed-first-error increase() bug — verified fixed via pre-registered label series plus a second unless-based alert branch.
• Stale tornado_http_* docs note — verified fixed, note no longer contradicts the base branch.
• Redundant label_replace — verified removed, panel simplified.

All four threads replied to and resolved.

SonarQube: Quality gate still passing on the current head — 0 new issues, 0 security hotspots, 0%/0% new coverage/duplication (still a config/docs-only diff). Source: GitHub check-run output plus the sonarqubecloud[bot] PR comment at 2026-10-22T22:33:20Z.

New findings: 1 Important, 1 Suggestion (both in the newly-expanded Health check docs section — everything else in this round's diff is the round-1 fixes, verified above).

• Important: the new Docker HEALTHCHECK override example in documentation/deployment.md points at port 9090, which this same document assigns to Prometheus a few lines above.
• Suggestion: the "unexpected internal error" health-check behaviour is now documented twice (a table row and a repeated prose sentence) in the same section.

Strengths:

• The round-1 fixes aren't just patched over — the WS-101 exclusion and the unhandled-errors alert both got defence-in-depth (server-side fix and a belt-and-suspenders guard in the alert/panel expression itself), which is more robust than what I asked for.
• The new Health check documentation (single-flight DB probe, the sqlite_master probe query, the "unknown"/no-uptime_seconds response on an unexpected internal error, the exact HEALTHCHECK timings) all cross-checked exactly against server/controllers/api/v1/health.py on the base branch — no inaccuracies there.

🤖 Generated with Claude Code

[Tim020]

Round 2 addressed in 4daa393: the HEALTHCHECK port-override example is now coherent (8081, --port, ports mapping, healthcheck, Prometheus target note), and the duplicated unexpected-error text was removed. Replies posted on both threads.

[Tim020]

Round 2 addendum, c907f9e: reworded the database-probe paragraph in deployment.md to match #1463's updated single-flight behaviour (overlapping polls await the same in-flight probe; each poll is bounded by 3s and reports fail while a probe is hung; no second probe starts until the first finishes). The earlier wording was no longer accurate.

[sonarqubecloud]

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

[Tim020]

Review round 3

Head SHA reviewed: 9f55fbd

Verdict: Ready to merge, as far as this skill's review is concerned — no outstanding findings, pending the rest of CI (several checks were still running at review time; everything that had finished was green, including SonarCloud).

Previous round follow-up: 2 fixed, 0 still open, 0 incomplete, 0 needs human decision.

• Port-9090 HEALTHCHECK example — verified fixed: now uses 8081 with a matching command, ports: mapping, healthcheck:, and a note about the Prometheus scrape target and the overlay's reserved 3000/9090 ports. Cross-checked the --port=8081 flag against server/main.py:52 and the scrape-target advice against monitoring/prometheus/prometheus.yml — both correct.
• Duplicated "unexpected internal error" text — verified fixed: the detail now lives only in the status table, the prose is re-wrapped.

Both threads replied to and resolved. (All 6 threads raised across rounds 1–2 are now resolved.)

Separately, this round's diff also reworded the "Database check" paragraph to describe updated single-flight-probe behaviour (overlapping polls now await the same in-flight probe, each bounded by its own 3s timeout, rather than failing immediately). I cross-checked this against server/controllers/api/v1/health.py's current _check_database on feature/obs-app-metrics and it matches exactly (asyncio.shield(asyncio.wrap_future(probe)), reuse of _probe while not done).

SonarQube: Quality gate still passing — 0 new issues, 0 security hotspots, 0%/0% new coverage/duplication. Source: GitHub check-run output on the current head.

New findings: None. I had a second reviewer independently check this round's diff (restricted to documentation/deployment.md, the only file that changed) against the actual code and the other monitoring/ files; it surfaced only one sub-50%-confidence, take-it-or-leave-it observation — that if the new port-override YAML snippet were dropped into a separate compose override file (rather than edited directly into docker-compose.yaml), Compose's list-merge behaviour would concatenate the ports: entries instead of replacing them, leaving host port 8080 still reserved. The doc doesn't actually tell anyone to use a second file for this case, so I'm not flagging it — noting it here only for the record.

Strengths:

• Every round's fixes have held up under re-verification with no regressions introduced along the way — round 2's fixes for round 1 didn't reopen anything, and round 3's fixes for round 2 didn't either.
• The docs have converged on matching the actual server behaviour precisely, including a subtle recent change to the health-check's concurrency handling.

🤖 Generated with Claude Code