From cd73093bebac4a0cbd6aa7d0f4f9ff12df6fded3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ullrich=20Sch=C3=A4fer?= Date: Thu, 18 Jun 2026 15:58:01 +0200 Subject: [PATCH] fix(journal): bound Prometheus route label cardinality MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The httpRequestDuration histogram labeled every request with the raw URL path (`url.split("?")[0]`), so every distinct path — `/activities/`, `/routes/`, probed usernames, crawler junk — became a permanent label-set. prom-client never evicts label-sets, so RSS grew linearly for the life of the process and reset only on restart. On the flagship this showed as ~25 MiB/day of unbounded journal memory growth; a live `/api/metrics` scrape held 2,655 histogram series across 291 distinct `route` values (344 KB body), dominated by per-UUID activity paths. Add `normalizeRoute()` in metrics.server.ts: it collapses dynamic path segments back to the `:param` templates declared in app/routes.ts (UUID and numeric segments -> `:id`; `:username`/`:provider` slots keyed by their preceding static segment), strips query/fragment, and enforces a hard cap (MAX_ROUTES) that funnels anything beyond the known route table into a single `/other` bucket as an absolute backstop. Cardinality is now proportional to the route table, not to traffic. Logs still record the raw path (`logger.info`) — only the metric label is bounded. Co-Authored-By: Claude Opus 4.8 (1M context) --- apps/journal/app/lib/metrics.server.test.ts | 89 +++++++++++++++++++++ apps/journal/app/lib/metrics.server.ts | 62 ++++++++++++++ apps/journal/server.ts | 4 +- 3 files changed, 153 insertions(+), 2 deletions(-) create mode 100644 apps/journal/app/lib/metrics.server.test.ts diff --git a/apps/journal/app/lib/metrics.server.test.ts b/apps/journal/app/lib/metrics.server.test.ts new file mode 100644 index 0000000..5237d30 --- /dev/null +++ b/apps/journal/app/lib/metrics.server.test.ts @@ -0,0 +1,89 @@ +import { describe, it, expect, beforeEach } from "vitest"; +import { normalizeRoute, _resetRouteNormalizationForTest } from "./metrics.server.ts"; + +describe("normalizeRoute", () => { + beforeEach(() => { + _resetRouteNormalizationForTest(); + }); + + it("passes static routes through unchanged", () => { + expect(normalizeRoute("/")).toBe("/"); + expect(normalizeRoute("/feed")).toBe("/feed"); + expect(normalizeRoute("/explore")).toBe("/explore"); + expect(normalizeRoute("/settings/security")).toBe("/settings/security"); + expect(normalizeRoute("/api/v1/routes")).toBe("/api/v1/routes"); + expect(normalizeRoute("/legal/terms")).toBe("/legal/terms"); + }); + + it("collapses UUID segments to :id", () => { + expect( + normalizeRoute("/activities/0257f241-d880-4ca6-b073-c8706f07a293"), + ).toBe("/activities/:id"); + expect( + normalizeRoute("/routes/0257f241-d880-4ca6-b073-c8706f07a293/edit"), + ).toBe("/routes/:id/edit"); + expect( + normalizeRoute("/api/v1/activities/0257F241-D880-4CA6-B073-C8706F07A293"), + ).toBe("/api/v1/activities/:id"); + }); + + it("collapses purely-numeric segments to :id", () => { + expect(normalizeRoute("/api/notifications/12345/read")).toBe( + "/api/notifications/:id/read", + ); + }); + + it("collapses usernames to :username after a `users` segment", () => { + expect(normalizeRoute("/users/alice")).toBe("/users/:username"); + expect(normalizeRoute("/users/bob/followers")).toBe( + "/users/:username/followers", + ); + expect(normalizeRoute("/api/users/carol/follow")).toBe( + "/api/users/:username/follow", + ); + }); + + it("collapses sync providers to :provider", () => { + expect(normalizeRoute("/api/sync/connect/wahoo")).toBe( + "/api/sync/connect/:provider", + ); + expect(normalizeRoute("/api/sync/callback/garmin")).toBe( + "/api/sync/callback/:provider", + ); + expect( + normalizeRoute( + "/api/sync/push/komoot/0257f241-d880-4ca6-b073-c8706f07a293", + ), + ).toBe("/api/sync/push/:provider/:id"); + }); + + it("strips query strings and fragments before normalizing", () => { + expect( + normalizeRoute("/activities/0257f241-d880-4ca6-b073-c8706f07a293?foo=bar"), + ).toBe("/activities/:id"); + expect(normalizeRoute("/feed#section")).toBe("/feed"); + }); + + it("preserves a dotted version segment (does not treat 2.1 as numeric)", () => { + expect(normalizeRoute("/nodeinfo/2.1")).toBe("/nodeinfo/2.1"); + }); + + it("returns the same template for two different ids (bounded cardinality)", () => { + const a = normalizeRoute("/activities/0257f241-d880-4ca6-b073-c8706f07a293"); + const b = normalizeRoute("/activities/ffffffff-d880-4ca6-b073-c8706f07a293"); + expect(a).toBe(b); + expect(a).toBe("/activities/:id"); + }); + + it("collapses unbounded junk to /other once the cap is exceeded", () => { + // Fill the tracking set with distinct unmatched paths up to the cap. + for (let i = 0; i < 200; i++) { + expect(normalizeRoute(`/scan-${i}`)).toBe(`/scan-${i}`); + } + // The next novel path can't be tracked, so it collapses to /other. + expect(normalizeRoute("/scan-200")).toBe("/other"); + expect(normalizeRoute("/another-novel-path")).toBe("/other"); + // Already-tracked paths are still returned verbatim. + expect(normalizeRoute("/scan-0")).toBe("/scan-0"); + }); +}); diff --git a/apps/journal/app/lib/metrics.server.ts b/apps/journal/app/lib/metrics.server.ts index 300bfab..d256f07 100644 --- a/apps/journal/app/lib/metrics.server.ts +++ b/apps/journal/app/lib/metrics.server.ts @@ -49,3 +49,65 @@ export const demoBotSyntheticActivitiesTotal = getOrCreate( ); export const registry = client.register; + +// --- Route label normalization ------------------------------------------- +// +// The `route` label on httpRequestDuration MUST be bounded. prom-client +// stores one label-set per distinct combination and NEVER evicts, so +// feeding it raw request paths (`/activities/`, probed usernames, +// crawler junk) leaks memory linearly for the life of the process — it +// only resets on restart. `normalizeRoute` collapses dynamic path +// segments back to the `:param` templates declared in app/routes.ts so +// cardinality stays proportional to the route table, not to traffic. + +const UUID_RE = + /^[0-9a-f]{8}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{12}$/i; + +// A dynamic segment is keyed by the static segment that precedes it, +// mirroring the non-id `:param` slots in app/routes.ts. (`:id` params are +// all UUIDs and get caught by UUID_RE on their own.) +const DYNAMIC_AFTER = new Map([ + ["users", ":username"], // /users/:username, /api/users/:username/... + ["connect", ":provider"], // /api/sync/connect/:provider + ["callback", ":provider"], // /api/sync/callback/:provider + ["disconnect", ":provider"], // /api/sync/disconnect/:provider + ["webhook", ":provider"], // /api/sync/webhook/:provider + ["push", ":provider"], // /api/sync/push/:provider/:routeId +]); + +// Absolute backstop. Real route templates number well under this; once we +// have seen this many distinct normalized routes (e.g. an aggressive +// scanner hitting paths that match no `:param` rule), everything new +// collapses to a single `/other` bucket so cardinality can never run away. +const MAX_ROUTES = 200; +const seenRoutes = new Set(); + +/** Collapse a request path's dynamic segments to a bounded route template. */ +export function normalizeRoute(pathname: string): string { + const path = (pathname.split("?")[0] ?? "/").split("#")[0] || "/"; + if (path === "/") return "/"; + + const out: string[] = []; + for (const seg of path.split("/")) { + if (seg === "") continue; + const prev = out.length > 0 ? out[out.length - 1]! : ""; + if (UUID_RE.test(seg) || /^\d+$/.test(seg)) { + out.push(":id"); + } else if (DYNAMIC_AFTER.has(prev)) { + out.push(DYNAMIC_AFTER.get(prev)!); + } else { + out.push(seg); + } + } + const normalized = "/" + out.join("/"); + + if (seenRoutes.has(normalized)) return normalized; + if (seenRoutes.size >= MAX_ROUTES) return "/other"; + seenRoutes.add(normalized); + return normalized; +} + +/** Test-only: clear the distinct-route tracking set. */ +export function _resetRouteNormalizationForTest(): void { + seenRoutes.clear(); +} diff --git a/apps/journal/server.ts b/apps/journal/server.ts index 839e3e3..00a7687 100644 --- a/apps/journal/server.ts +++ b/apps/journal/server.ts @@ -6,7 +6,7 @@ import { createReadStream, statSync } from "node:fs"; import { join, extname, resolve } from "node:path"; import { logger, requestContext } from "./app/lib/logger.server.ts"; import { randomUUID } from "node:crypto"; -import { httpRequestDuration, registry } from "./app/lib/metrics.server.ts"; +import { httpRequestDuration, normalizeRoute, registry } from "./app/lib/metrics.server.ts"; import { createBoss, startWorker } from "@trails-cool/jobs"; import { getDatabaseUrl } from "@trails-cool/db"; import postgres, { type Sql } from "postgres"; @@ -117,7 +117,7 @@ const server = createServer((req, res) => { const duration = Date.now() - start; logger.info({ method: req.method, path: url, status: res.statusCode, duration }, "request"); httpRequestDuration.observe( - { method: req.method ?? "GET", route: url.split("?")[0]!, status: String(res.statusCode) }, + { method: req.method ?? "GET", route: normalizeRoute(url), status: String(res.statusCode) }, duration / 1000, ); });