Skip to content

Commit 829ecb3

Browse files
refactor(web): derive route labels from routes-manifest instead of a hardcoded list
The hardcoded KNOWN_ROUTES set bounded cardinality but rots: new routes silently degrade to `other` until someone updates the file, and two-segment truncation collapses distinct routes (everything under /api/ee/* became one label). Next's build already emits .next/routes-manifest.json with every defined route and a matching regex, ordered by the router's own resolution priority. Load that at startup: exact-match static routes, then first dynamic regex wins, mirroring how the server actually routes the request. Labels become the route pattern itself (/browse/[...path], /settings/connections/[id]), so cardinality is bounded by the number of defined routes plus /_next and `other`, and the label set tracks the app automatically at build time. Scanner traffic now lands on the catch-all routes that genuinely serve it (/[...slug], /api/[...slug]) rather than a synthetic bucket. If the manifest is missing or unreadable, everything is labelled `other` — granularity lost, bound kept. Verified against the production build's manifest (78 static + 18 dynamic routes) and the running pod: the standalone server chdirs to the app dir, so the cwd-relative manifest path resolves. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
1 parent 9e595eb commit 829ecb3

3 files changed

Lines changed: 175 additions & 108 deletions

File tree

packages/web/src/httpMetrics.integration.test.ts

Lines changed: 11 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,6 @@
11
import { afterAll, describe, expect, it } from 'vitest';
22
import { createServer, Server } from 'node:http';
3-
import { startHttpMetrics } from './httpMetrics';
3+
import { initRouteTable, startHttpMetrics } from './httpMetrics';
44
import { registry } from './promClient';
55

66
const app = createServer((_req, res) => {
@@ -32,7 +32,14 @@ const countLines = (output: string): string[] => {
3232
};
3333

3434
describe('httpMetrics', () => {
35-
it('records durations per normalized route and ignores the metrics port', async () => {
35+
it('records durations per route pattern and ignores the metrics port', async () => {
36+
// The table is injected rather than read from disk so the test doesn't
37+
// depend on a prior `next build` having produced routes-manifest.json.
38+
initRouteTable({
39+
staticRoutes: [{ page: '/api/health' }],
40+
dynamicRoutes: [{ page: '/browse/[...path]', regex: '^/browse/(.+?)(?:/)?$' }],
41+
});
42+
3643
// Both servers take an ephemeral port, then the metrics port is published
3744
// to env before subscribing, so the test never depends on a fixed port.
3845
const metricsPort = await listen(metricsServer);
@@ -54,8 +61,8 @@ describe('httpMetrics', () => {
5461
expect(counts.some(line => line.includes('route="/api/health"'))).toBe(true);
5562
expect(counts.some(line => line.includes('status="200"'))).toBe(true);
5663

57-
// Two distinct file paths must collapse to a single /browse series.
58-
const browse = counts.filter(line => line.includes('route="/browse"'));
64+
// Two distinct file paths must collapse to the single route-pattern series.
65+
const browse = counts.filter(line => line.includes('route="/browse/[...path]"'));
5966
expect(browse).toHaveLength(1);
6067
expect(browse[0].trim().endsWith('2')).toBe(true);
6168

Lines changed: 78 additions & 35 deletions
Original file line numberDiff line numberDiff line change
@@ -1,47 +1,92 @@
11
import { describe, expect, it } from 'vitest';
2-
import { MAX_ROUTE_LABELS, normalizeRoute } from './httpMetrics';
2+
import { buildRouteTable, normalizeRoute } from './httpMetrics';
3+
4+
// Mirrors the shape and ordering of the real .next/routes-manifest.json:
5+
// dynamic routes are listed in Next's resolution priority, with catch-alls
6+
// after specific routes and the root catch-all last.
7+
const table = buildRouteTable({
8+
staticRoutes: [
9+
{ page: '/' },
10+
{ page: '/search' },
11+
{ page: '/repos' },
12+
{ page: '/api/health' },
13+
{ page: '/api/commits' },
14+
],
15+
dynamicRoutes: [
16+
{ page: '/api/auth/[...nextauth]', regex: '^/api/auth/(.+?)(?:/)?$' },
17+
{ page: '/api/repos/[repoId]/image', regex: '^/api/repos/([^/]+?)/image(?:/)?$' },
18+
{ page: '/api/[...slug]', regex: '^/api/(.+?)(?:/)?$' },
19+
{ page: '/browse/[...path]', regex: '^/browse/(.+?)(?:/)?$' },
20+
{ page: '/settings/[...slug]', regex: '^/settings/(.+?)(?:/)?$' },
21+
{ page: '/[...slug]', regex: '^/(.+?)(?:/)?$' },
22+
],
23+
});
24+
25+
const maxLabels = table.staticPages.size + table.dynamicRoutes.length + 2; // + '/_next', 'other'
326

427
describe('normalizeRoute', () => {
528
it('maps the root path', () => {
6-
expect(normalizeRoute('/')).toBe('/');
7-
expect(normalizeRoute('')).toBe('/');
29+
expect(normalizeRoute('/', table)).toBe('/');
30+
expect(normalizeRoute('', table)).toBe('/');
31+
});
32+
33+
it('matches static routes exactly', () => {
34+
expect(normalizeRoute('/api/health', table)).toBe('/api/health');
35+
expect(normalizeRoute('/search', table)).toBe('/search');
836
});
937

10-
it('keeps two segments for known API routes', () => {
11-
expect(normalizeRoute('/api/health')).toBe('/api/health');
12-
expect(normalizeRoute('/api/commits')).toBe('/api/commits');
13-
expect(normalizeRoute('/api/auth/callback/github')).toBe('/api/auth');
38+
it('labels dynamic routes with their route pattern', () => {
39+
expect(normalizeRoute('/api/auth/callback/github', table)).toBe('/api/auth/[...nextauth]');
40+
expect(normalizeRoute('/api/repos/42/image', table)).toBe('/api/repos/[repoId]/image');
41+
expect(normalizeRoute('/settings/connections/42', table)).toBe('/settings/[...slug]');
1442
});
1543

16-
it('keeps one segment for known page routes', () => {
17-
expect(normalizeRoute('/search')).toBe('/search');
18-
expect(normalizeRoute('/settings/connections/42')).toBe('/settings');
44+
it('respects manifest ordering: specific routes win over catch-alls', () => {
45+
// /api/auth/... must hit [...nextauth], not the /api/[...slug] catch-all.
46+
expect(normalizeRoute('/api/auth/session', table)).toBe('/api/auth/[...nextauth]');
47+
// Unknown API paths fall through to the catch-all that actually serves them.
48+
expect(normalizeRoute('/api/not-a-real-route', table)).toBe('/api/[...slug]');
1949
});
2050

21-
it('collapses unbounded repository and file paths', () => {
22-
const a = normalizeRoute('/browse/github.com/org/repo/-/blob/src/index.ts');
23-
const b = normalizeRoute('/browse/github.com/other/repo/-/blob/lib/other.ts');
51+
it('collapses unbounded repository and file paths to one label', () => {
52+
const a = normalizeRoute('/browse/github.com/org/repo/-/blob/src/index.ts', table);
53+
const b = normalizeRoute('/browse/github.com/other/repo/-/blob/lib/other.ts', table);
2454

25-
expect(a).toBe('/browse');
55+
expect(a).toBe('/browse/[...path]');
2656
expect(b).toBe(a);
2757
});
2858

2959
it('is unaffected by trailing or duplicate slashes', () => {
30-
expect(normalizeRoute('/search/')).toBe('/search');
31-
expect(normalizeRoute('//search//')).toBe('/search');
60+
expect(normalizeRoute('/search/', table)).toBe('/search');
61+
expect(normalizeRoute('//search//', table)).toBe('/search');
62+
expect(normalizeRoute('/api/health/', table)).toBe('/api/health');
63+
});
64+
65+
it('labels asset requests /_next without consulting the table', () => {
66+
expect(normalizeRoute('/_next/static/chunks/main.js', table)).toBe('/_next');
67+
expect(normalizeRoute('/_next/image', undefined)).toBe('/_next');
68+
});
69+
70+
it('reports everything as other when no table is loaded', () => {
71+
expect(normalizeRoute('/api/health', undefined)).toBe('other');
72+
expect(normalizeRoute('/search', undefined)).toBe('other');
3273
});
3374

3475
describe('cardinality bounding', () => {
35-
it('reports unknown top-level paths as other', () => {
36-
expect(normalizeRoute('/wp-admin')).toBe('other');
37-
expect(normalizeRoute('/.env')).toBe('other');
38-
expect(normalizeRoute('/phpmyadmin/index.php')).toBe('other');
76+
it('routes scanner traffic to catch-alls, not new labels', () => {
77+
expect(normalizeRoute('/wp-admin', table)).toBe('/[...slug]');
78+
expect(normalizeRoute('/.env', table)).toBe('/[...slug]');
79+
expect(normalizeRoute('/api/12345', table)).toBe('/api/[...slug]');
3980
});
4081

41-
it('reports unknown API paths as other, despite the [...slug] catch-all', () => {
42-
expect(normalizeRoute('/api/not-a-real-route')).toBe('other');
43-
expect(normalizeRoute('/api/12345')).toBe('other');
44-
expect(normalizeRoute('/api/health-check')).toBe('other');
82+
it('reports unmatched paths as other when there is no root catch-all', () => {
83+
const noCatchAll = buildRouteTable({
84+
staticRoutes: [{ page: '/search' }],
85+
dynamicRoutes: [{ page: '/api/[...slug]', regex: '^/api/(.+?)(?:/)?$' }],
86+
});
87+
88+
expect(normalizeRoute('/wp-admin', noCatchAll)).toBe('other');
89+
expect(normalizeRoute('/api/anything', noCatchAll)).toBe('/api/[...slug]');
4590
});
4691

4792
it('stays bounded under scanner traffic', () => {
@@ -52,28 +97,26 @@ describe('normalizeRoute', () => {
5297
hostile.push(`/${i}/${i}/${i}`);
5398
}
5499

55-
const labels = new Set(hostile.map(normalizeRoute));
100+
const labels = new Set(hostile.map(p => normalizeRoute(p, table)));
56101

57-
// 3000 distinct hostile paths must produce exactly one label.
58-
expect(labels).toEqual(new Set(['other']));
102+
// 3,000 distinct hostile paths produce exactly the two catch-all labels.
103+
expect(labels).toEqual(new Set(['/[...slug]', '/api/[...slug]']));
59104
});
60105

61-
it('never exceeds the documented label bound for any input', () => {
106+
it('never exceeds the table-derived bound for any input', () => {
62107
const paths = [
63-
'/', '/search', '/repos', '/settings/general', '/browse/a/b/c',
64-
'/api/health', '/api/commits', '/api/auth/session', '/_next/static/x.js',
65-
'/wp-admin', '/api/bogus', '/random', '/api/9', '/..%2f', '/a/b/c/d/e',
108+
'/', '/search', '/repos', '/browse/a/b/c', '/api/health',
109+
'/api/commits', '/api/auth/session', '/_next/static/x.js',
110+
'/wp-admin', '/api/bogus', '/random', '/..%2f', '/a/b/c/d/e',
66111
];
67112
for (let i = 0; i < 500; i++) {
68113
paths.push(`/junk${i}`, `/api/junk${i}`);
69114
}
70115

71-
const labels = new Set(paths.map(normalizeRoute));
116+
const labels = new Set(paths.map(p => normalizeRoute(p, table)));
72117

73-
expect(labels.size).toBeLessThanOrEqual(MAX_ROUTE_LABELS);
74-
// Known routes still resolve; only the unknown ones collapse.
118+
expect(labels.size).toBeLessThanOrEqual(maxLabels);
75119
expect(labels).toContain('/api/health');
76-
expect(labels).toContain('other');
77120
});
78121
});
79122
});

packages/web/src/httpMetrics.ts

Lines changed: 86 additions & 69 deletions
Original file line numberDiff line numberDiff line change
@@ -1,93 +1,106 @@
11
import { createLogger, env } from '@sourcebot/shared';
22
import { subscribe } from 'node:diagnostics_channel';
3+
import { readFileSync } from 'node:fs';
34
import type { IncomingMessage, ServerResponse } from 'node:http';
5+
import path from 'node:path';
46
import { httpRequestDuration } from './promClient';
57

68
const logger = createLogger('web-http-metrics');
79

10+
interface RoutesManifest {
11+
staticRoutes: { page: string }[];
12+
dynamicRoutes: { page: string; regex: string }[];
13+
}
14+
15+
interface RouteTable {
16+
staticPages: Set<string>;
17+
dynamicRoutes: { page: string; regex: RegExp }[];
18+
}
19+
20+
const OTHER_ROUTE = 'other';
21+
822
/**
9-
* Every path that isn't in this set is reported as `other`.
10-
*
11-
* Truncating path depth alone does not bound cardinality: the first segment is
12-
* client-supplied, and `/api/[...slug]` is a catch-all, so `/wp-admin`,
13-
* `/.env`, and `/api/<anything>` would each mint a new time series. Scanner or
14-
* bot traffic would then grow the series count without limit. Matching against
15-
* a known set instead bounds it to this size plus one, whatever gets requested.
16-
*
17-
* Adding a route here is deliberate. A missing one is reported as `other`, so
18-
* new routes lose granularity rather than breaking, and cardinality holds.
23+
* Builds a route matcher from Next's routes-manifest. The manifest lists every
24+
* defined route with a matching regex, ordered by Next's own resolution
25+
* priority (specific routes before catch-alls), so first-match-wins here
26+
* agrees with how the server actually routes the request.
1927
*/
20-
const KNOWN_ROUTES = new Set([
21-
'/',
22-
'/_next',
23-
'/askgh',
24-
'/browse',
25-
'/chat',
26-
'/chats',
27-
'/invite',
28-
'/login',
29-
'/oauth',
30-
'/onboard',
31-
'/redeem',
32-
'/repos',
33-
'/search',
34-
'/settings',
35-
'/signup',
36-
'/slow',
37-
'/api/auth',
38-
'/api/avatar',
39-
'/api/blame',
40-
'/api/changelog',
41-
'/api/chat',
42-
'/api/commit',
43-
'/api/commits',
44-
'/api/diff',
45-
'/api/ee',
46-
'/api/files',
47-
'/api/find_definitions',
48-
'/api/find_references',
49-
'/api/folder_contents',
50-
'/api/health',
51-
'/api/minidenticon',
52-
'/api/models',
53-
'/api/offers',
54-
'/api/openapi.json',
55-
'/api/repo-status',
56-
'/api/repos',
57-
'/api/search',
58-
'/api/source',
59-
'/api/stream_search',
60-
'/api/symbols',
61-
'/api/tree',
62-
'/api/version',
63-
'/api/webhook',
64-
]);
28+
export const buildRouteTable = (manifest: RoutesManifest): RouteTable => {
29+
return {
30+
staticPages: new Set(manifest.staticRoutes.map(route => route.page)),
31+
dynamicRoutes: manifest.dynamicRoutes.map(route => ({
32+
page: route.page,
33+
regex: new RegExp(route.regex),
34+
})),
35+
};
36+
};
6537

66-
const OTHER_ROUTE = 'other';
38+
let routeTable: RouteTable | undefined;
6739

6840
/**
69-
* Collapses a request path into a bounded label.
41+
* Loads the route table from the build's routes-manifest. Next's standalone
42+
* server chdirs to the app directory on boot, so the manifest sits at
43+
* `.next/routes-manifest.json` relative to cwd.
7044
*
71-
* Depth is truncated first, because repository and file paths are unbounded and
72-
* `/browse/github.com/org/repo/-/blob/src/index.ts` must not mint a series per
73-
* file viewed. API paths keep two segments so `/api/health` stays distinct from
74-
* `/api/commits`; everything else keeps one. The result is then matched against
75-
* `KNOWN_ROUTES`, which is what actually bounds the label set.
45+
* Deriving routes from the manifest (rather than a hardcoded list) keeps the
46+
* label set in sync with the app automatically: new routes appear at build
47+
* time, and the label is the route pattern itself (`/browse/[...path]`), so
48+
* cardinality is bounded by the number of defined routes no matter what gets
49+
* requested.
7650
*/
77-
export const normalizeRoute = (pathname: string): string => {
51+
export const initRouteTable = (manifest?: RoutesManifest): boolean => {
52+
try {
53+
const resolved = manifest ?? (JSON.parse(
54+
readFileSync(path.join(process.cwd(), '.next', 'routes-manifest.json'), 'utf-8'),
55+
) as RoutesManifest);
56+
routeTable = buildRouteTable(resolved);
57+
logger.info(`Route table loaded: ${routeTable.staticPages.size} static, ${routeTable.dynamicRoutes.length} dynamic routes.`);
58+
return true;
59+
} catch (error) {
60+
// Fail closed: without a table every request is labelled `other`, which
61+
// loses granularity but can never grow the label set.
62+
logger.error(`Failed to load routes-manifest; all routes will be reported as '${OTHER_ROUTE}': ${error}`);
63+
return false;
64+
}
65+
};
66+
67+
/**
68+
* Maps a request path to its route pattern. The raw path can't be used as a
69+
* label: repository and file paths are unbounded, so `/browse/...` would mint
70+
* a new time series for every file anyone views, and unknown paths (scanners,
71+
* bots) would grow the set without limit. Matching against the app's own
72+
* routes bounds the label set to the number of defined routes plus `/_next`
73+
* and `other`.
74+
*/
75+
export const normalizeRoute = (pathname: string, table: RouteTable | undefined = routeTable): string => {
7876
const segments = pathname.split('/').filter(segment => segment.length > 0);
7977
if (segments.length === 0) {
8078
return '/';
8179
}
8280

83-
const depth = segments[0] === 'api' ? 2 : 1;
84-
const candidate = `/${segments.slice(0, depth).join('/')}`;
81+
// Asset requests are real traffic but not manifest routes.
82+
if (segments[0] === '_next') {
83+
return '/_next';
84+
}
8585

86-
return KNOWN_ROUTES.has(candidate) ? candidate : OTHER_ROUTE;
87-
};
86+
if (!table) {
87+
return OTHER_ROUTE;
88+
}
8889

89-
/** Upper bound on distinct `route` label values, for tests and review. */
90-
export const MAX_ROUTE_LABELS = KNOWN_ROUTES.size + 1;
90+
const canonical = `/${segments.join('/')}`;
91+
92+
if (table.staticPages.has(canonical)) {
93+
return canonical;
94+
}
95+
96+
for (const route of table.dynamicRoutes) {
97+
if (route.regex.test(canonical)) {
98+
return route.page;
99+
}
100+
}
101+
102+
return OTHER_ROUTE;
103+
};
91104

92105
interface RequestStartMessage {
93106
response?: ServerResponse;
@@ -117,6 +130,10 @@ export const startHttpMetrics = (): void => {
117130
}
118131
subscribed = true;
119132

133+
if (!routeTable) {
134+
initRouteTable();
135+
}
136+
120137
const metricsPort = Number(env.WEB_METRICS_PORT);
121138

122139
subscribe('http.server.request.start', (message) => {

0 commit comments

Comments
 (0)