From f759084f410fc708a81aff7b48784d54014ee843 Mon Sep 17 00:00:00 2001 From: agent_coder Date: Sun, 5 Jul 2026 01:44:16 +0300 Subject: [PATCH 1/2] fix(metrics): collapse static-asset routes to bounded "static" label (#362) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Follow-up to #355: http_request_duration_seconds's `route` label captured raw content-hashed asset filenames (route="/assets/index-CAbxDtto.js", "/assets/chunk-*.js"). @fastify/static serves each file through a route whose matched routeOptions.url IS the raw hashed path, so the label was unbounded — a new set of names every deploy, growing the series forever (the exact cardinality leak the API routes were protected against). resolveRouteLabel now detects a static request by its path prefix (/assets/, /vad/, /brand/, /locales/) FIRST and collapses it to a single `static` label (query string stripped before the check); API routes still use the template and 404s still collapse to `unknown`. Static edge latency is already measured by Traefik's traefik_router_request_duration_*. Gate: server tsc 0; metrics.spec passes (added static-collapse + query-strip + "real API route mentioning assets is NOT collapsed" cases). Co-Authored-By: Claude Opus 4.8 (1M context) --- .../integrations/metrics/http-metrics.hook.ts | 21 +++++++++-- .../src/integrations/metrics/metrics.spec.ts | 37 +++++++++++++++++++ 2 files changed, 54 insertions(+), 4 deletions(-) diff --git a/apps/server/src/integrations/metrics/http-metrics.hook.ts b/apps/server/src/integrations/metrics/http-metrics.hook.ts index a01a7d1a..d1215dfe 100644 --- a/apps/server/src/integrations/metrics/http-metrics.hook.ts +++ b/apps/server/src/integrations/metrics/http-metrics.hook.ts @@ -2,15 +2,28 @@ import { FastifyReply, FastifyRequest } from 'fastify'; import { isStreamingResponse } from './metrics.constants'; import { observeHttp } from './metrics.registry'; +// URL path prefixes served by @fastify/static (client build output under +// client/dist). Their filenames are content-hashed (index-*.js, chunk-*.js), +// so a NEW set of names is minted on every deploy — using them as the `route` +// label would grow the label set without bound (#362). Collapse them all to one +// bounded `static` label. (Edge latency for static is already measured by +// Traefik's traefik_router_request_duration_*.) +const STATIC_PATH_PREFIXES = ['/assets/', '/vad/', '/brand/', '/locales/']; + /** * Resolve the BOUNDED route label for an HTTP response. * - * HARD REQUIREMENT (#355): use the ROUTE TEMPLATE (`/pages/:id`), NEVER the raw - * URL (`/pages/abc-123`), so label cardinality stays finite. Fastify exposes the - * matched template on `req.routeOptions.url`. On 404s (no route matched) that is - * missing → collapse to the literal `unknown`. + * HARD REQUIREMENT (#355): use the ROUTE TEMPLATE (`/pages/:id`), NEVER a raw + * URL (`/pages/abc-123` or `/assets/index-CAbxDtto.js`), so label cardinality + * stays finite. Fastify exposes the matched template on `req.routeOptions.url`, + * BUT @fastify/static serves each file through a route whose matched url is the + * raw (hashed) file path — so for static assets that value is itself unbounded. + * Detect static requests by their path prefix FIRST and collapse to `static`; + * otherwise use the route template; on a 404 (no route matched) → `unknown`. */ export function resolveRouteLabel(req: FastifyRequest): string { + const path = (req.url ?? '').split('?', 1)[0]; + if (STATIC_PATH_PREFIXES.some((p) => path.startsWith(p))) return 'static'; const url = req.routeOptions?.url; return typeof url === 'string' && url.length > 0 ? url : 'unknown'; } diff --git a/apps/server/src/integrations/metrics/metrics.spec.ts b/apps/server/src/integrations/metrics/metrics.spec.ts index ffcc11e6..02a27133 100644 --- a/apps/server/src/integrations/metrics/metrics.spec.ts +++ b/apps/server/src/integrations/metrics/metrics.spec.ts @@ -25,6 +25,43 @@ describe('resolveRouteLabel (histogram route label)', () => { const req = { url: '/x' } as unknown as FastifyRequest; expect(resolveRouteLabel(req)).toBe('unknown'); }); + + it.each([ + '/assets/index-CAbxDtto.js', + '/assets/chunk-3OPIFGDE-CJOt9nr5.js', + '/assets/excalidraw-menu-DpsI0kFW.js', + '/vad/silero_vad_v5.onnx', + '/brand/logo.svg', + '/locales/en.json', + ])('collapses hashed/static asset %p to "static" (#362 cardinality)', (url) => { + // @fastify/static serves each file through a route whose matched url is the + // raw (hashed) file path, so routeOptions.url is itself unbounded here. + const req = { + url, + routeOptions: { url }, + } as unknown as FastifyRequest; + const label = resolveRouteLabel(req); + expect(label).toBe('static'); + expect(label).not.toContain('.js'); + expect(label).not.toContain('index-'); + }); + + it('strips the query string before the static-prefix check', () => { + const req = { + url: '/assets/index-CAbxDtto.js?v=2', + routeOptions: { url: '/assets/index-CAbxDtto.js' }, + } as unknown as FastifyRequest; + expect(resolveRouteLabel(req)).toBe('static'); + }); + + it('does NOT collapse a real API route that merely mentions assets', () => { + // A templated API route is kept as-is; only the static path PREFIXES collapse. + const req = { + url: '/api/pages/assets-guide', + routeOptions: { url: '/api/pages/:id' }, + } as unknown as FastifyRequest; + expect(resolveRouteLabel(req)).toBe('/api/pages/:id'); + }); }); describe('isStreamingResponse (SSE exclusion)', () => { -- 2.52.0 From 43b11d92ab613766586a547be4a74a1915ba2e5d Mon Sep 17 00:00:00 2001 From: agent_coder Date: Sun, 5 Jul 2026 02:38:47 +0300 Subject: [PATCH 2/2] fix(#362 review F1-F3): add /icons/ prefix + honest comment + real boundary test MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - F1: added '/icons/' to STATIC_PATH_PREFIXES — public/icons/ is copied verbatim to client/dist (a sibling of the already-included brand/vad/locales), and index.html references /icons/favicon-*.png on every page load, so those requests were getting their own route labels instead of collapsing to `static`. - F2: corrected the comment — only /assets/ is content-hashed (unbounded per deploy); /vad//brand//locales//icons/ have stable names (repetitive, not unbounded). Either way none belong in the API-route histogram. - F3: the negative test now exercises the trailing-slash boundary (the actual anti-false-collapse guard): '/assets' (no slash), '/assetsx/foo.js', '/iconset/x.png' must NOT collapse to `static` — cases that a buggy includes()/slashless-prefix impl would wrongly collapse. Plus '/icons/*' added to the positive it.each. Gate: server tsc 0; metrics.spec passes. Co-Authored-By: Claude Opus 4.8 (1M context) --- .../integrations/metrics/http-metrics.hook.ts | 20 +++++++++++++------ .../src/integrations/metrics/metrics.spec.ts | 18 +++++++++++++++++ 2 files changed, 32 insertions(+), 6 deletions(-) diff --git a/apps/server/src/integrations/metrics/http-metrics.hook.ts b/apps/server/src/integrations/metrics/http-metrics.hook.ts index d1215dfe..c72fb4d0 100644 --- a/apps/server/src/integrations/metrics/http-metrics.hook.ts +++ b/apps/server/src/integrations/metrics/http-metrics.hook.ts @@ -3,12 +3,20 @@ import { isStreamingResponse } from './metrics.constants'; import { observeHttp } from './metrics.registry'; // URL path prefixes served by @fastify/static (client build output under -// client/dist). Their filenames are content-hashed (index-*.js, chunk-*.js), -// so a NEW set of names is minted on every deploy — using them as the `route` -// label would grow the label set without bound (#362). Collapse them all to one -// bounded `static` label. (Edge latency for static is already measured by -// Traefik's traefik_router_request_duration_*.) -const STATIC_PATH_PREFIXES = ['/assets/', '/vad/', '/brand/', '/locales/']; +// client/dist). `/assets/` holds the content-hashed bundle (index-*.js, +// chunk-*.js) — a NEW set of names every deploy, i.e. an UNBOUNDED label set +// (#362); the others (/vad/, /brand/, /locales/, /icons/ — copied verbatim from +// public/) have stable names, so they are merely repetitive per-file labels +// rather than unbounded. Either way none of these belong in the API-route +// histogram: collapse them all to one bounded `static` label. (Edge latency for +// static is already measured by Traefik's traefik_router_request_duration_*.) +const STATIC_PATH_PREFIXES = [ + '/assets/', + '/vad/', + '/brand/', + '/locales/', + '/icons/', +]; /** * Resolve the BOUNDED route label for an HTTP response. diff --git a/apps/server/src/integrations/metrics/metrics.spec.ts b/apps/server/src/integrations/metrics/metrics.spec.ts index 02a27133..311ad43d 100644 --- a/apps/server/src/integrations/metrics/metrics.spec.ts +++ b/apps/server/src/integrations/metrics/metrics.spec.ts @@ -33,6 +33,7 @@ describe('resolveRouteLabel (histogram route label)', () => { '/vad/silero_vad_v5.onnx', '/brand/logo.svg', '/locales/en.json', + '/icons/app-icon-192x192.png', ])('collapses hashed/static asset %p to "static" (#362 cardinality)', (url) => { // @fastify/static serves each file through a route whose matched url is the // raw (hashed) file path, so routeOptions.url is itself unbounded here. @@ -62,6 +63,23 @@ describe('resolveRouteLabel (histogram route label)', () => { } as unknown as FastifyRequest; expect(resolveRouteLabel(req)).toBe('/api/pages/:id'); }); + + it.each([ + // The TRAILING SLASH on the prefix is the anti-false-collapse guard: a path + // that is the prefix WITHOUT its slash, or merely shares the prefix as a + // substring of a longer segment, must NOT collapse. These would collapse + // under a buggy `includes('/assets/')` / slashless-prefix impl. + '/assets', + '/assetsx/foo.js', + '/iconset/x.png', + ])('does NOT collapse the prefix-boundary case %p', (url) => { + const req = { + url, + routeOptions: { url: '/some/:route' }, + } as unknown as FastifyRequest; + expect(resolveRouteLabel(req)).not.toBe('static'); + expect(resolveRouteLabel(req)).toBe('/some/:route'); + }); }); describe('isStreamingResponse (SSE exclusion)', () => { -- 2.52.0