fix(journal): match metric route label against known templates
Follow-up to the route-label cardinality fix. The first cut collapsed dynamic segments via regex (:id/:username/:provider) and capped the distinct-route set at 200 with a /other overflow. In production that cap filled almost entirely with vulnerability-scanner junk (`/.ssh/id_rsa`, `/%00.aws/credentials`, …) on a first-come-first-served basis: only 3 real templates made it in before the cap saturated, so legitimate routes hit afterward were misbucketed into /other — the metric became useless for per-route latency even though memory was bounded. Replace the regex+cap approach with explicit matching against the journal's known route templates (mirroring app/routes.ts). A `:param` segment matches any single path segment; literals are preferred over params (so /routes/new beats /routes/:id); anything matching no template collapses to /other immediately — no per-path tracking, no cap race. Cardinality is hard-bounded to (templates + 1) regardless of traffic, and scanner noise can never crowd out real routes. A co-located drift guard flattens the real route config and fails if ROUTE_TEMPLATES and app/routes.ts ever diverge. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This commit is contained in:
parent
e6b88ff294
commit
5643ce257a
2 changed files with 190 additions and 87 deletions
|
|
@ -1,11 +1,8 @@
|
||||||
import { describe, it, expect, beforeEach } from "vitest";
|
import { describe, it, expect } from "vitest";
|
||||||
import { normalizeRoute, _resetRouteNormalizationForTest } from "./metrics.server.ts";
|
import { normalizeRoute, ROUTE_TEMPLATES } from "./metrics.server.ts";
|
||||||
|
import routesConfig from "../routes.ts";
|
||||||
|
|
||||||
describe("normalizeRoute", () => {
|
describe("normalizeRoute", () => {
|
||||||
beforeEach(() => {
|
|
||||||
_resetRouteNormalizationForTest();
|
|
||||||
});
|
|
||||||
|
|
||||||
it("passes static routes through unchanged", () => {
|
it("passes static routes through unchanged", () => {
|
||||||
expect(normalizeRoute("/")).toBe("/");
|
expect(normalizeRoute("/")).toBe("/");
|
||||||
expect(normalizeRoute("/feed")).toBe("/feed");
|
expect(normalizeRoute("/feed")).toBe("/feed");
|
||||||
|
|
@ -15,25 +12,19 @@ describe("normalizeRoute", () => {
|
||||||
expect(normalizeRoute("/legal/terms")).toBe("/legal/terms");
|
expect(normalizeRoute("/legal/terms")).toBe("/legal/terms");
|
||||||
});
|
});
|
||||||
|
|
||||||
it("collapses UUID segments to :id", () => {
|
it("maps dynamic id segments to the :id template", () => {
|
||||||
expect(
|
expect(
|
||||||
normalizeRoute("/activities/0257f241-d880-4ca6-b073-c8706f07a293"),
|
normalizeRoute("/activities/0257f241-d880-4ca6-b073-c8706f07a293"),
|
||||||
).toBe("/activities/:id");
|
).toBe("/activities/:id");
|
||||||
expect(
|
expect(
|
||||||
normalizeRoute("/routes/0257f241-d880-4ca6-b073-c8706f07a293/edit"),
|
normalizeRoute("/routes/0257f241-d880-4ca6-b073-c8706f07a293/edit"),
|
||||||
).toBe("/routes/:id/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(
|
expect(normalizeRoute("/api/notifications/12345/read")).toBe(
|
||||||
"/api/notifications/:id/read",
|
"/api/notifications/:id/read",
|
||||||
);
|
);
|
||||||
});
|
});
|
||||||
|
|
||||||
it("collapses usernames to :username after a `users` segment", () => {
|
it("maps usernames to the :username template", () => {
|
||||||
expect(normalizeRoute("/users/alice")).toBe("/users/:username");
|
expect(normalizeRoute("/users/alice")).toBe("/users/:username");
|
||||||
expect(normalizeRoute("/users/bob/followers")).toBe(
|
expect(normalizeRoute("/users/bob/followers")).toBe(
|
||||||
"/users/:username/followers",
|
"/users/:username/followers",
|
||||||
|
|
@ -43,47 +34,84 @@ describe("normalizeRoute", () => {
|
||||||
);
|
);
|
||||||
});
|
});
|
||||||
|
|
||||||
it("collapses sync providers to :provider", () => {
|
it("maps sync providers to the :provider template", () => {
|
||||||
expect(normalizeRoute("/api/sync/connect/wahoo")).toBe(
|
expect(normalizeRoute("/api/sync/connect/wahoo")).toBe(
|
||||||
"/api/sync/connect/:provider",
|
"/api/sync/connect/:provider",
|
||||||
);
|
);
|
||||||
expect(normalizeRoute("/api/sync/callback/garmin")).toBe(
|
|
||||||
"/api/sync/callback/:provider",
|
|
||||||
);
|
|
||||||
expect(
|
expect(
|
||||||
normalizeRoute(
|
normalizeRoute(
|
||||||
"/api/sync/push/komoot/0257f241-d880-4ca6-b073-c8706f07a293",
|
"/api/sync/push/komoot/0257f241-d880-4ca6-b073-c8706f07a293",
|
||||||
),
|
),
|
||||||
).toBe("/api/sync/push/:provider/:id");
|
).toBe("/api/sync/push/:provider/:routeId");
|
||||||
});
|
});
|
||||||
|
|
||||||
it("strips query strings and fragments before normalizing", () => {
|
it("prefers a literal template over a parametric one", () => {
|
||||||
|
// /routes/new must NOT be swallowed by /routes/:id
|
||||||
|
expect(normalizeRoute("/routes/new")).toBe("/routes/new");
|
||||||
|
expect(normalizeRoute("/activities/new")).toBe("/activities/new");
|
||||||
|
// a known provider literal wins over /sync/import/:provider
|
||||||
|
expect(normalizeRoute("/sync/import/komoot")).toBe("/sync/import/komoot");
|
||||||
|
expect(normalizeRoute("/sync/import/strava")).toBe("/sync/import/:provider");
|
||||||
|
});
|
||||||
|
|
||||||
|
it("collapses anything matching no template to /other", () => {
|
||||||
|
expect(normalizeRoute("/.ssh/id_rsa")).toBe("/other");
|
||||||
|
expect(normalizeRoute("/wp-login.php")).toBe("/other");
|
||||||
|
expect(normalizeRoute("/.aws/credentials")).toBe("/other");
|
||||||
|
// right prefix, wrong arity also falls through
|
||||||
|
expect(normalizeRoute("/routes/abc/def/ghi")).toBe("/other");
|
||||||
|
// a username-shaped path under an unknown collection
|
||||||
|
expect(normalizeRoute("/profile/alice")).toBe("/other");
|
||||||
|
});
|
||||||
|
|
||||||
|
it("strips query strings and fragments and trailing slashes", () => {
|
||||||
expect(
|
expect(
|
||||||
normalizeRoute("/activities/0257f241-d880-4ca6-b073-c8706f07a293?foo=bar"),
|
normalizeRoute("/activities/0257f241-d880-4ca6-b073-c8706f07a293?foo=1"),
|
||||||
).toBe("/activities/:id");
|
).toBe("/activities/:id");
|
||||||
expect(normalizeRoute("/feed#section")).toBe("/feed");
|
expect(normalizeRoute("/feed#section")).toBe("/feed");
|
||||||
|
expect(normalizeRoute("/feed/")).toBe("/feed");
|
||||||
});
|
});
|
||||||
|
|
||||||
it("preserves a dotted version segment (does not treat 2.1 as numeric)", () => {
|
it("bounds cardinality: distinct outputs are limited to templates + /other", () => {
|
||||||
expect(normalizeRoute("/nodeinfo/2.1")).toBe("/nodeinfo/2.1");
|
const outputs = new Set<string>();
|
||||||
});
|
for (let i = 0; i < 500; i++) {
|
||||||
|
outputs.add(normalizeRoute(`/activities/id-${i}`));
|
||||||
it("returns the same template for two different ids (bounded cardinality)", () => {
|
outputs.add(normalizeRoute(`/scanner-junk-${i}`));
|
||||||
const a = normalizeRoute("/activities/0257f241-d880-4ca6-b073-c8706f07a293");
|
outputs.add(normalizeRoute(`/users/user${i}`));
|
||||||
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.
|
// All 1500 distinct paths collapse to just 3 labels.
|
||||||
expect(normalizeRoute("/scan-200")).toBe("/other");
|
expect([...outputs].sort()).toEqual([
|
||||||
expect(normalizeRoute("/another-novel-path")).toBe("/other");
|
"/activities/:id",
|
||||||
// Already-tracked paths are still returned verbatim.
|
"/other",
|
||||||
expect(normalizeRoute("/scan-0")).toBe("/scan-0");
|
"/users/:username",
|
||||||
|
]);
|
||||||
|
});
|
||||||
|
});
|
||||||
|
|
||||||
|
describe("ROUTE_TEMPLATES drift guard", () => {
|
||||||
|
// Flatten the real route config to full path templates. If a route is
|
||||||
|
// added/removed/renamed in app/routes.ts without updating ROUTE_TEMPLATES,
|
||||||
|
// this fails — keeping the metric label set honest.
|
||||||
|
type Entry = { path?: string; index?: boolean; children?: Entry[] };
|
||||||
|
function flatten(entries: Entry[], prefix: string): string[] {
|
||||||
|
const out: string[] = [];
|
||||||
|
for (const e of entries) {
|
||||||
|
if (e.index) {
|
||||||
|
out.push(prefix === "" ? "/" : `/${prefix}`);
|
||||||
|
continue;
|
||||||
|
}
|
||||||
|
const full = prefix === "" ? e.path ?? "" : `${prefix}/${e.path ?? ""}`;
|
||||||
|
if (e.children && e.children.length > 0) {
|
||||||
|
out.push(...flatten(e.children, full));
|
||||||
|
} else {
|
||||||
|
out.push(`/${full}`);
|
||||||
|
}
|
||||||
|
}
|
||||||
|
return out;
|
||||||
|
}
|
||||||
|
|
||||||
|
it("matches every full path declared in app/routes.ts", () => {
|
||||||
|
const fromConfig = flatten(routesConfig as unknown as Entry[], "").sort();
|
||||||
|
expect(fromConfig).toEqual([...ROUTE_TEMPLATES].sort());
|
||||||
});
|
});
|
||||||
});
|
});
|
||||||
|
|
|
||||||
|
|
@ -55,59 +55,134 @@ export const registry = client.register;
|
||||||
// The `route` label on httpRequestDuration MUST be bounded. prom-client
|
// The `route` label on httpRequestDuration MUST be bounded. prom-client
|
||||||
// stores one label-set per distinct combination and NEVER evicts, so
|
// stores one label-set per distinct combination and NEVER evicts, so
|
||||||
// feeding it raw request paths (`/activities/<uuid>`, probed usernames,
|
// feeding it raw request paths (`/activities/<uuid>`, probed usernames,
|
||||||
// crawler junk) leaks memory linearly for the life of the process — it
|
// crawler junk) leaks memory linearly for the life of the process.
|
||||||
// only resets on restart. `normalizeRoute` collapses dynamic path
|
//
|
||||||
// segments back to the `:param` templates declared in app/routes.ts so
|
// We match each request path against the journal's known route templates
|
||||||
// cardinality stays proportional to the route table, not to traffic.
|
// (mirroring app/routes.ts) and label it with the matched template; a
|
||||||
|
// `:param` segment matches any single path segment. Anything matching no
|
||||||
|
// template — vulnerability scanners, typos, unmounted paths — collapses to
|
||||||
|
// a single `/other` bucket. Cardinality is therefore hard-bounded to
|
||||||
|
// (number of templates + 1), independent of traffic, and real routes can
|
||||||
|
// never be crowded out by scanner noise (the failure mode of the earlier
|
||||||
|
// first-come-first-served cap).
|
||||||
|
//
|
||||||
|
// IMPORTANT: ROUTE_TEMPLATES must stay in sync with app/routes.ts. The
|
||||||
|
// co-located test imports the route config, flattens it to full paths, and
|
||||||
|
// fails if the two ever drift — so adding a route there forces an update
|
||||||
|
// here.
|
||||||
|
const ROUTE_TEMPLATES: readonly string[] = [
|
||||||
|
"/",
|
||||||
|
"/.well-known/trails-cool",
|
||||||
|
"/.well-known/webfinger",
|
||||||
|
"/.well-known/nodeinfo",
|
||||||
|
"/nodeinfo/2.1",
|
||||||
|
"/users/:username/inbox",
|
||||||
|
"/users/:username/outbox",
|
||||||
|
"/oauth/authorize",
|
||||||
|
"/oauth/token",
|
||||||
|
"/auth/register",
|
||||||
|
"/auth/login",
|
||||||
|
"/auth/verify",
|
||||||
|
"/auth/logout",
|
||||||
|
"/auth/accept-terms",
|
||||||
|
"/api/auth/register",
|
||||||
|
"/api/auth/login",
|
||||||
|
"/routes",
|
||||||
|
"/routes/new",
|
||||||
|
"/routes/:id",
|
||||||
|
"/routes/:id/edit",
|
||||||
|
"/api/e2e/seed",
|
||||||
|
"/api/e2e/route/:id",
|
||||||
|
"/api/e2e/komoot",
|
||||||
|
"/api/routes/:id/callback",
|
||||||
|
"/api/routes/:id/edit-in-planner",
|
||||||
|
"/api/routes/:id/gpx",
|
||||||
|
"/activities",
|
||||||
|
"/activities/new",
|
||||||
|
"/activities/:id",
|
||||||
|
"/users/:username",
|
||||||
|
"/users/:username/followers",
|
||||||
|
"/users/:username/following",
|
||||||
|
"/api/users/:username/follow",
|
||||||
|
"/api/users/:username/unfollow",
|
||||||
|
"/follows/requests",
|
||||||
|
"/follows/outgoing",
|
||||||
|
"/api/follows/:id/approve",
|
||||||
|
"/api/follows/:id/reject",
|
||||||
|
"/feed",
|
||||||
|
"/explore",
|
||||||
|
"/api/events",
|
||||||
|
"/notifications",
|
||||||
|
"/api/notifications/:id/read",
|
||||||
|
"/api/notifications/read-all",
|
||||||
|
"/settings",
|
||||||
|
"/settings/profile",
|
||||||
|
"/settings/account",
|
||||||
|
"/settings/security",
|
||||||
|
"/settings/connections",
|
||||||
|
"/settings/connections/komoot",
|
||||||
|
"/api/settings/profile",
|
||||||
|
"/api/settings/email",
|
||||||
|
"/api/settings/passkey/delete",
|
||||||
|
"/api/settings/delete-account",
|
||||||
|
"/sync/import/komoot",
|
||||||
|
"/sync/import/garmin",
|
||||||
|
"/sync/import/:provider",
|
||||||
|
"/api/sync/komoot/verify",
|
||||||
|
"/api/sync/komoot/connect",
|
||||||
|
"/api/sync/komoot/import",
|
||||||
|
"/api/sync/komoot/import-status",
|
||||||
|
"/api/sync/connect/:provider",
|
||||||
|
"/api/sync/callback/:provider",
|
||||||
|
"/api/sync/disconnect/:provider",
|
||||||
|
"/api/sync/webhook/:provider",
|
||||||
|
"/api/sync/push/:provider/:routeId",
|
||||||
|
"/privacy",
|
||||||
|
"/legal/imprint",
|
||||||
|
"/legal/terms",
|
||||||
|
"/legal/privacy",
|
||||||
|
"/api/v1/routes",
|
||||||
|
"/api/v1/routes/compute",
|
||||||
|
"/api/v1/routes/:id",
|
||||||
|
"/api/v1/activities",
|
||||||
|
"/api/v1/activities/:id",
|
||||||
|
"/api/v1/auth/devices",
|
||||||
|
"/api/v1/auth/devices/:id",
|
||||||
|
"/api/v1/uploads",
|
||||||
|
];
|
||||||
|
|
||||||
const UUID_RE =
|
export { ROUTE_TEMPLATES };
|
||||||
/^[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,
|
const isParam = (seg: string): boolean => seg.startsWith(":");
|
||||||
// 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<string, string>([
|
|
||||||
["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
|
// Pre-split templates into segments, ordered most-specific first (fewest
|
||||||
// have seen this many distinct normalized routes (e.g. an aggressive
|
// `:param` segments) so a literal like `/routes/new` is matched before the
|
||||||
// scanner hitting paths that match no `:param` rule), everything new
|
// parametric `/routes/:id`.
|
||||||
// collapses to a single `/other` bucket so cardinality can never run away.
|
const TEMPLATE_SEGMENTS = ROUTE_TEMPLATES.map((template) => ({
|
||||||
const MAX_ROUTES = 200;
|
template,
|
||||||
const seenRoutes = new Set<string>();
|
segments: template === "/" ? [] : template.slice(1).split("/"),
|
||||||
|
})).sort(
|
||||||
|
(a, b) =>
|
||||||
|
a.segments.filter(isParam).length - b.segments.filter(isParam).length,
|
||||||
|
);
|
||||||
|
|
||||||
/** Collapse a request path's dynamic segments to a bounded route template. */
|
/** Map a request path to one of the known route templates, or `/other`. */
|
||||||
export function normalizeRoute(pathname: string): string {
|
export function normalizeRoute(pathname: string): string {
|
||||||
const path = (pathname.split("?")[0] ?? "/").split("#")[0] || "/";
|
const path = (pathname.split("?")[0] ?? "/").split("#")[0] || "/";
|
||||||
if (path === "/") return "/";
|
const segs = path.replace(/\/+$/, "").split("/").filter((s) => s !== "");
|
||||||
|
if (segs.length === 0) return "/";
|
||||||
|
|
||||||
const out: string[] = [];
|
for (const { template, segments } of TEMPLATE_SEGMENTS) {
|
||||||
for (const seg of path.split("/")) {
|
if (segments.length !== segs.length) continue;
|
||||||
if (seg === "") continue;
|
let matched = true;
|
||||||
const prev = out.length > 0 ? out[out.length - 1]! : "";
|
for (let i = 0; i < segments.length; i++) {
|
||||||
if (UUID_RE.test(seg) || /^\d+$/.test(seg)) {
|
const t = segments[i]!;
|
||||||
out.push(":id");
|
if (!isParam(t) && t !== segs[i]) {
|
||||||
} else if (DYNAMIC_AFTER.has(prev)) {
|
matched = false;
|
||||||
out.push(DYNAMIC_AFTER.get(prev)!);
|
break;
|
||||||
} else {
|
}
|
||||||
out.push(seg);
|
|
||||||
}
|
}
|
||||||
|
if (matched) return template;
|
||||||
}
|
}
|
||||||
const normalized = "/" + out.join("/");
|
return "/other";
|
||||||
|
|
||||||
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();
|
|
||||||
}
|
}
|
||||||
|
|
|
||||||
Loading…
Add table
Add a link
Reference in a new issue