From dfbcb108767bffca5657d119dfcd1554b2a14a65 Mon Sep 17 00:00:00 2001 From: VortexUK Date: Sat, 25 Jul 2026 14:50:33 +0100 Subject: [PATCH] =?UTF-8?q?fix(metrics):=20cap=20label=20cardinality=20?= =?UTF-8?q?=E2=80=94=20drop=20per-user=20views,=20*=5Fcreated,=20raw-path?= =?UTF-8?q?=20leak?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Grafana Cloud free tier (10k series) was blown out at 14.5k+: - user_page_views_total + its _created twin were 12.9k series (88%) — username(286) x path. Dropped entirely along with the three dashboard panels that read it. - disable_created_metrics(): the OpenMetrics *_created companions doubled every counter/histogram and nothing read them (all "Unused" in the cardinality dashboard). - http_request_duration_seconds: method label removed (no dashboard queried it) + buckets 10 -> 6; ~1,260 series -> ~450. - Hardening: unmatched routes (bot POST probes) collapse to "(unmatched)" instead of minting a permanent series per raw URL; garbage verbs (PROPFIND, ...) collapse to OTHER; /class-icons/ joins the skip-list. Expected steady state ~1.5-2k active series. Co-Authored-By: Claude Fable 5 --- backend/server/app.py | 29 ++++---------- backend/server/metrics.py | 56 +++++++++++++++------------- ops/grafana/dashboards/frontend.json | 48 ------------------------ tests/server/test_metrics.py | 43 +++++++++++++++++++++ 4 files changed, 82 insertions(+), 94 deletions(-) create mode 100644 tests/server/test_metrics.py diff --git a/backend/server/app.py b/backend/server/app.py index 1a983ab7..9e1cd7fe 100644 --- a/backend/server/app.py +++ b/backend/server/app.py @@ -96,11 +96,10 @@ async def get_response(self, path: str, scope): # type: ignore[override] APP_INFO_LEGACY, HTTP_REQUEST_DURATION, HTTP_REQUESTS, - USER_PAGE_VIEWS, _register_db_collector, check_metrics_auth, + normalize_http_labels, should_track_path, - should_track_user_view, ) from backend.server.server_context import ServerContextMiddleware @@ -235,7 +234,10 @@ async def dispatch(self, request: Request, call_next): # type: ignore[override] class _MetricsMiddleware(BaseHTTPMiddleware): """Records per-route request count and latency using the matched route template (e.g. /api/character/{name}) rather than the raw path, so - label cardinality stays low.""" + label cardinality stays low. Requests that match no route (bot probes) + collapse into a single "(unmatched)" path label — labelling them with + the raw URL minted a permanent series per probe path and blew through + the Grafana Cloud free-tier series limit (2026-07).""" async def dispatch(self, request: Request, call_next): # type: ignore[override] start = time.perf_counter() @@ -244,18 +246,14 @@ async def dispatch(self, request: Request, call_next): # type: ignore[override] path = request.url.path if should_track_path(path): - # Prefer the route template; fall back to raw path route = request.scope.get("route") - label_path = getattr(route, "path", path) + method, label_path = normalize_http_labels(request.method, getattr(route, "path", None)) HTTP_REQUESTS.labels( - method=request.method, + method=method, path=label_path, status_code=str(response.status_code), ).inc() - HTTP_REQUEST_DURATION.labels( - method=request.method, - path=label_path, - ).observe(elapsed) + HTTP_REQUEST_DURATION.labels(path=label_path).observe(elapsed) # Server-error counter for the Databases dashboard. Only 5xx — 4xx # is mostly user error (auth, validation) and would drown out the @@ -265,17 +263,6 @@ async def dispatch(self, request: Request, call_next): # type: ignore[override] if response.status_code >= 500: APP_ERRORS.labels(source=label_path).inc() - # Per-user page views: authenticated GET requests only. - # Session is already populated by SessionMiddleware (runs before us). - # Polling/background endpoints are excluded via should_track_user_view. - if request.method == "GET" and response.status_code < 400 and should_track_user_view(label_path): - user = request.session.get("user") - if user: - USER_PAGE_VIEWS.labels( - username=user.get("username", "unknown"), - path=label_path, - ).inc() - return response diff --git a/backend/server/metrics.py b/backend/server/metrics.py index bf0bddb0..24341386 100644 --- a/backend/server/metrics.py +++ b/backend/server/metrics.py @@ -23,17 +23,28 @@ Gauge, Histogram, Info, + disable_created_metrics, ) from prometheus_client.core import GaugeMetricFamily from prometheus_client.registry import Collector from backend.sql_loader import load_sql +# The OpenMetrics *_created companion series double the series count of every +# counter/histogram and nothing reads them (Grafana cardinality dashboard +# flagged them all "Unused"). Kill them globally before any metric is defined. +disable_created_metrics() + _SQL = load_sql(__file__) _log = logging.getLogger(__name__) # ── HTTP request metrics ────────────────────────────────────────────────────── +# Cardinality discipline (2026-07 Grafana free-tier overrun): labels only ever +# take bounded values — route templates via normalize_http_labels (never raw +# URL paths, which bot scans mint by the thousand) and a fixed method +# vocabulary. The latency histogram deliberately has NO method label: each +# extra label combo costs ~(buckets + 2) series and no dashboard queried it. HTTP_REQUESTS = Counter( "http_requests_total", @@ -44,18 +55,8 @@ HTTP_REQUEST_DURATION = Histogram( "http_request_duration_seconds", "HTTP request latency", - ["method", "path"], - buckets=(0.005, 0.01, 0.025, 0.05, 0.1, 0.25, 0.5, 1.0, 2.5, 5.0), -) - -# ── Per-user page view metrics ───────────────────────────────────────────────── -# Tracks authenticated GET requests by Discord username and route template. -# Cardinality: ~users × ~routes — stays well under 1 k series for a guild tool. - -USER_PAGE_VIEWS = Counter( - "user_page_views_total", - "Successful authenticated GET requests by user and route", - ["username", "path"], + ["path"], + buckets=(0.01, 0.05, 0.1, 0.25, 1.0, 2.5), ) # ── Cache metrics ───────────────────────────────────────────────────────────── @@ -348,27 +349,32 @@ def census_endpoint_label(url: str) -> str: "/icons/", "/aa-assets/", "/spell-icons/", + "/class-icons/", "/metrics", ) -# Routes excluded from per-user page-view tracking (still counted in -# HTTP_REQUESTS). Add polling/background endpoints here so they don't -# inflate individual user activity graphs. -_USER_VIEW_SKIP = frozenset( - [ - "/api/notifications", - ] -) - def should_track_path(path: str) -> bool: return not any(path.startswith(p) for p in _SKIP_PREFIXES) -def should_track_user_view(route_path: str) -> bool: - """Return False for background/polling routes that shouldn't count as - meaningful page views in the per-user USER_PAGE_VIEWS metric.""" - return route_path not in _USER_VIEW_SKIP +# ── Label normalisation (cardinality guard) ────────────────────────────────── +# Requests that match no route (bot probes with POST/PUT on arbitrary paths) +# have no template — labelling them with the raw URL mints a permanent series +# per probe path (prometheus_client never forgets a label combo). Collapse +# them all into one bucket; ditto garbage HTTP verbs (PROPFIND, TRACK, …). + +UNMATCHED_PATH = "(unmatched)" + +_KNOWN_METHODS = frozenset({"GET", "HEAD", "POST", "PUT", "PATCH", "DELETE", "OPTIONS"}) + + +def normalize_http_labels(method: str, route_path: str | None) -> tuple[str, str]: + """Return (method, path) label values with bounded cardinality.""" + method = method.upper() + if method not in _KNOWN_METHODS: + method = "OTHER" + return method, route_path if route_path else UNMATCHED_PATH # ── Token check ─────────────────────────────────────────────────────────────── diff --git a/ops/grafana/dashboards/frontend.json b/ops/grafana/dashboards/frontend.json index c35af5da..c8ffdafb 100644 --- a/ops/grafana/dashboards/frontend.json +++ b/ops/grafana/dashboards/frontend.json @@ -105,21 +105,6 @@ "legendFormat": "p95", "refId": "A" }] }, - { - "id": 4, "type": "stat", "title": "Active users (last 1 h)", - "gridPos": { "h": 4, "w": 6, "x": 18, "y": 1 }, - "datasource": { "type": "prometheus", "uid": "${datasource}" }, - "description": "Distinct authenticated usernames that hit at least one tracked page in the last hour.", - "options": { - "reduceOptions": { "calcs": ["lastNotNull"], "fields": "", "values": false }, - "colorMode": "value", "graphMode": "none", "textMode": "auto" - }, - "fieldConfig": { "defaults": { "unit": "short", "decimals": 0, "thresholds": { "mode": "absolute", "steps": [{ "color": "blue", "value": null }] } }, "overrides": [] }, - "targets": [{ "datasource": { "type": "prometheus", "uid": "${datasource}" }, - "expr": "count(count by(username) (increase(user_page_views_total{job=\"eq2companion\"}[1h]) > 0))", - "legendFormat": "users", "refId": "A" }] - }, - { "id": 5, "type": "timeseries", "title": "HTTP requests / sec by status code", "gridPos": { "h": 9, "w": 12, "x": 0, "y": 5 }, @@ -152,39 +137,6 @@ "targets": [{ "datasource": { "type": "prometheus", "uid": "${datasource}" }, "expr": "histogram_quantile(0.95, sum by(le, path) (rate(http_request_duration_seconds_bucket{job=\"eq2companion\"}[5m])))", "legendFormat": "{{path}}", "refId": "A" }] - }, - - { "collapsed": false, "gridPos": { "h": 1, "w": 24, "x": 0, "y": 14 }, "id": 101, "title": "Per-user activity", "type": "row" }, - - { - "id": 10, "type": "timeseries", "title": "Page views per user (5 m rate)", - "gridPos": { "h": 10, "w": 12, "x": 0, "y": 15 }, - "datasource": { "type": "prometheus", "uid": "${datasource}" }, - "description": "Cardinality bounded by ~users × ~routes — currently well under 1k series.", - "fieldConfig": { - "defaults": { "unit": "reqps", "custom": { "lineWidth": 1, "fillOpacity": 5, "showPoints": "never" } }, "overrides": [] - }, - "options": { "legend": { "displayMode": "table", "placement": "bottom", "calcs": ["sum"] }, "tooltip": { "mode": "multi", "sort": "desc" } }, - "targets": [{ "datasource": { "type": "prometheus", "uid": "${datasource}" }, - "expr": "sum by(username) (rate(user_page_views_total{job=\"eq2companion\"}[5m]))", - "legendFormat": "{{username}}", "refId": "A" }] - }, - - { - "id": 11, "type": "table", "title": "Top pages by user (last 1 h)", - "gridPos": { "h": 10, "w": 12, "x": 12, "y": 15 }, - "datasource": { "type": "prometheus", "uid": "${datasource}" }, - "description": "Username × route view counts for the last hour. Sort by Value desc.", - "fieldConfig": { "defaults": { "custom": { "align": "auto" } }, "overrides": [] }, - "options": { "showHeader": true, "footer": { "show": false } }, - "targets": [{ - "datasource": { "type": "prometheus", "uid": "${datasource}" }, - "expr": "topk(20, sum by(username, path) (increase(user_page_views_total{job=\"eq2companion\"}[1h])))", - "format": "table", "instant": true, "refId": "A" - }], - "transformations": [ - { "id": "organize", "options": { "excludeByName": { "Time": true, "job": true, "instance": true, "__name__": true } } } - ] } ] diff --git a/tests/server/test_metrics.py b/tests/server/test_metrics.py new file mode 100644 index 00000000..2c94d91d --- /dev/null +++ b/tests/server/test_metrics.py @@ -0,0 +1,43 @@ +"""Tests for the metrics cardinality guards (2026-07 Grafana free-tier overrun). + +The failure mode being guarded: prometheus_client never forgets a label combo, +so any unbounded label value (raw URL paths from bot probes, garbage HTTP +verbs, per-user labels) mints permanent series until the process restarts. +""" + +from __future__ import annotations + +from prometheus_client import REGISTRY, generate_latest + +from backend.server import metrics + + +def test_normalize_http_labels_keeps_route_templates(): + assert metrics.normalize_http_labels("GET", "/api/character/{name}") == ("GET", "/api/character/{name}") + assert metrics.normalize_http_labels("get", "/api/server") == ("GET", "/api/server") + + +def test_normalize_http_labels_collapses_unmatched_and_garbage_verbs(): + # No matched route (bot probe) → single bucket, never the raw path. + assert metrics.normalize_http_labels("POST", None) == ("POST", metrics.UNMATCHED_PATH) + assert metrics.normalize_http_labels("POST", "") == ("POST", metrics.UNMATCHED_PATH) + # Scanner verbs (PROPFIND, TRACK, …) → OTHER. + assert metrics.normalize_http_labels("PROPFIND", "/{full_path:path}") == ("OTHER", "/{full_path:path}") + + +def test_duration_histogram_has_no_method_label(): + """Each label combo on the histogram costs ~(buckets + 2) series and no + dashboard queries latency by method — keep it path-only.""" + assert metrics.HTTP_REQUEST_DURATION._labelnames == ("path",) + + +def test_created_series_disabled(): + """The OpenMetrics *_created companions double every counter/histogram's + series count and nothing reads them — metrics.py disables them at import.""" + assert b"_created" not in generate_latest(REGISTRY) + + +def test_should_track_path_skips_static_mounts(): + for skipped in ("/assets/app.js", "/class-icons/13.png", "/spell-icons/1.png", "/metrics"): + assert not metrics.should_track_path(skipped) + assert metrics.should_track_path("/api/character/Foo")