Remove per-player cardinality from websocket Prometheus metrics

[cesaregarza]

Motivation

Prometheus does not need SplatTop's per-player aggregation. Using player_id as a metric label creates an unbounded number of time series and risks substantial Prometheus memory growth when the FastAPI scrape is reconnected.

Changes

Remove the player_id label from:

• fastapi_websocket_connections
• fastapi_websocket_broadcast_duration_seconds
• fastapi_websocket_bytes_total
• fastapi_pubsub_bytes_total

The connection gauge now reports total active connections, while duration and byte metrics aggregate across players.

Metric names, application-level player routing, payloads, cache behavior, and player logging remain unchanged. Existing aggregate Grafana queries remain compatible.

Validation

• 8 focused websocket, cached-payload, and pubsub tests passed.
• Tests cover chunked and legacy pubsub paths and verify player routing is preserved.
• git diff --check passed.

The existing TestClient /metrics test hangs during app-fixture startup on clean origin/main; it was not counted as passing.

Rollout dependency

Deploy this application change before reconnecting the Prometheus FastAPI scrape. Otherwise, restoring the scrape against the current application version would recreate high-cardinality per-player series.

Companion GitOps change: GarzAICluster #402

Linear: CES-571

[claude]

Review: Remove per-player cardinality from websocket Prometheus metrics

Overview: Drops the player_id label from the four websocket/pubsub metrics (fastapi_websocket_connections, fastapi_websocket_broadcast_duration_seconds, fastapi_websocket_bytes_total, fastapi_pubsub_bytes_total), converting them to unlabeled aggregates. The metric definitions, call sites in connections.py/pubsub.py, and a new focused test file are all updated consistently. Confirmed via grep that no other code (dashboards, other modules) still references these labels, so nothing was missed.

Code quality

• The change is minimal and well-scoped — only the label removal and the aggregation math, no unrelated refactoring.
• connect()/disconnect() in connections.py:107-112,262-267 now recompute the total via sum(len(v) for v in self.active_connections.values()) on every connect/disconnect. Functionally correct, but it's an O(P) scan (P = distinct connected players) on every single connection event, where the old code did an O(1) .set()/.remove() per player label. Given P is usually the count of distinct players being viewed concurrently (not total connections), this is probably fine at current scale, but a running counter (self._connection_count, incremented/decremented directly) would be simpler and avoid the repeated full-dict walk if connection churn grows.
• The duplicated sum(len(connections) for connections in self.active_connections.values()) expression appears twice (connect and disconnect) — could be factored into a small private helper (e.g. self._total_connections()), which would also make the O(1)-counter refactor above a one-line change if adopted later.

Correctness

• Logic check on disconnect(): the old branch structure (delete label if room empties, else update label) collapses correctly into "delete if empty, then always recompute total" — verified this preserves behavior for all three cases (last connection removed, non-last connection removed, room already empty).
• No leftover calls to .labels(player_id=...) on the four changed metrics — confirmed via grep across the repo.

Performance

• This is the intended win: unbounded per-player label cardinality is replaced with fixed-cardinality metrics, which is the right fix for the stated Prometheus memory-growth risk.
• The only new cost is the O(P) recomputation noted above — worth a quick sanity check under production-like concurrent-player counts before/after the Prometheus scrape is reconnected, but not a blocker.

Security

• No security-relevant surface here (internal metrics only, no user input reflected into labels other than what's already removed).

Test coverage

• Good: tests/test_websocket_metrics.py adds direct unit tests for the label removal (_labelnames == ()) and for aggregate behavior across multi-player connect/disconnect sequences and both pubsub payload paths (chunked + legacy), including asserting player routing (progressive_only/legacy_only) is unaffected — this is exactly the regression the PR is guarding against.
• One nit: test_websocket_metrics_have_no_player_labels asserts on the private _labelnames attribute of prometheus_client metric objects. It's a common enough pattern for this library, but it does rely on a non-public API that could shift in a future prometheus_client upgrade — not a real risk here, just noting it as the one test that isn't using a public interface.
• The PR description flags a pre-existing /metrics TestClient test that hangs on clean main and wasn't counted as passing — worth confirming that's tracked separately, since it means there's currently no working end-to-end test confirming the /metrics endpoint actually renders these metrics correctly, only the unit-level spies added here.

Rollout

• The PR correctly calls out the deploy-before-reconnect-scrape dependency; that ordering constraint is the main operational risk and is already well-documented in the description.

Overall: a clean, well-tested, low-risk change that does what it says. The O(P) recompute-on-every-event pattern is the only thing I'd consider tightening, and only if connection churn turns out to be higher than expected in practice.

[cesaregarza]

Gate verdict — ADVISE-APPROVE ✅ (advise review; BLOCKED on branch protection)

Reviewed via a 3-dimension workflow (recording-code correctness/completeness, consumer impact, test honesty). Clean cardinality fix.

• ✅ Recording code correct + complete. player_id is removed consistently from both the declaration (prometheus.py) and every emit/observe/inc call site — all calls are now bare .set()/.observe()/.inc() with no .labels(), so no declaration-vs-call mismatch and no prometheus_client runtime error is possible. Repo-wide greps for .labels( and residual player_id labels return zero. The connections gauge correctly switched to a full global recompute (sum of len over active_connections) post-connect/post-disconnect, which balances to 0 — more robust than the old incremental .remove().
• ✅ Consumer impact clean. Swept the full monitoring stack (Grafana dashboards, prometheus/rules.yaml, alertmanager): the only PromQL consumers are two label-agnostic sum() panels in dashboard-realtime.yaml that survive the label removal intact; no alert or recording rule references these metrics, so nothing stops firing.
• ✅ Tests mutation-honest. test_websocket_metrics_have_no_player_labels asserts _labelnames == () on the four real metric objects (re-adding player_id fails it); spy-based emit tests assert concrete aggregate values.

Two non-blocking notes:

1. Heads-up — the PR removes player_id from four metrics, not the three named in the description (also fastapi_pubsub_bytes_total/PUBSUB_BYTES_BROADCAST). None has a player_id consumer, so it's benign — just flagging the description undercount.
2. (nit / caveat) Emit-path tests use spies, so no test drives a real Gauge/Counter/Histogram end-to-end (the acknowledged-hanging /metrics TestClient test leaves no live integration emit). And repo-scope can't see live UI-edited Grafana panels or ad-hoc explore queries that group by player_id — those would silently return no data after this lands. Worth a quick glance at any hand-built dashboards before/after deploy.

No blockers. Recommend merge (your call — non-Mandate; also currently BLOCKED, likely needs a review approval / up-to-date branch).