Uh oh!
There was an error while loading. Please reload this page.
feat(web): record HTTP request duration metrics - #1571
Merged
Conversation
The web process now reports runtime metrics, but nothing about how long requests actually take, so per-endpoint latency is still invisible. That is the signal needed to see a stall from the outside: /api/health does almost no work, so its duration is essentially event loop queueing delay. Add an http_request_duration_seconds histogram labelled by method, route, and status, populated by subscribing to Node's built-in http.server.request.start and http.server.response.finish diagnostics channels. Next.js owns the server instance in a standalone build, so there is no request pipeline to wrap; the channels observe every request without patching anything. Paths are collapsed to a bounded route label. Repository and file paths are unbounded, so labelling by full path would mint a time series per file viewed. Requests to the metrics port are skipped, since the channels are process-wide and every scrape would otherwise record itself. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This comment has been minimized.
This comment has been minimized.
Contributor
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
WalkthroughThe web server now exports ChangesHTTP metrics instrumentation
Estimated code review effort: 3 (Moderate) | ~30 minutes Sequence Diagram(s)sequenceDiagram
participant NodeRegistration
participant NodeDiagnosticChannels
participant httpMetrics
participant httpRequestDuration
NodeRegistration->>httpMetrics: Start HTTP metrics collection
NodeDiagnosticChannels->>httpMetrics: Publish request start and completion messages
httpMetrics->>httpMetrics: Normalize route and exclude metrics-server traffic
httpMetrics->>httpRequestDuration: Observe method, route, status, and duration
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Uh oh!
There was an error while loading. Please reload this page.
Truncating path depth bounded depth, not breadth. The first path segment is client-supplied and /api/[...slug] is a catch-all, so /wp-admin, /.env, and /api/<anything> each minted a new time series. Scanner traffic could grow the series count without limit, which is exactly what the normalization was supposed to prevent. Match the truncated path against a known-route set and report anything else as `other`, bounding distinct route labels to that set plus one regardless of what is requested. A route missing from the set loses granularity rather than breaking, so it fails closed. Also strengthens the metrics-port exclusion assertion. It checked for the absence of a `/metrics` label, which became vacuous once unknown paths collapse to `other` — it now asserts the total observation count, and fails if the port filter is removed. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/web/src/httpMetrics.integration.test.ts`:
- Around line 38-42: Update the integration test setup around startHttpMetrics
so process.env.WEB_METRICS_PORT is assigned before the httpMetrics module is
imported, ensuring createEnv(options) reads the ephemeral metrics port during
module initialization. Replace the static import with a deferred import after
the environment assignment and invoke startHttpMetrics from that loaded module.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 7cb3db6d-2e81-48e4-a016-a97834000cd9
📒 Files selected for processing (6)
CHANGELOG.mdpackages/web/src/httpMetrics.integration.test.tspackages/web/src/httpMetrics.test.tspackages/web/src/httpMetrics.tspackages/web/src/instrumentation.tspackages/web/src/promClient.ts
Uh oh!
There was an error while loading. Please reload this page.
…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>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/web/src/httpMetrics.ts`:
- Around line 59-63: Update the catch block in initRouteTable to assign
routeTable = undefined before returning false, ensuring initialization failures
discard any previously loaded route table and subsequent requests use the
OTHER_ROUTE fallback.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: c5daa8f0-6a9f-4a10-b470-2185ac676f2c
📒 Files selected for processing (3)
packages/web/src/httpMetrics.integration.test.tspackages/web/src/httpMetrics.test.tspackages/web/src/httpMetrics.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/web/src/httpMetrics.test.ts
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using high effort and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 0867a0e. Configure here.
| method: request.method ?? 'UNKNOWN', | ||
| route: normalizeRoute(pathname), | ||
| status: response.statusCode, | ||
| }, |
There was a problem hiding this comment.
Unbounded HTTP method label cardinality
Medium Severity
The method label uses the raw client-supplied request.method with no allowlist. Route labels were carefully bounded via the manifest to stop scanner traffic from exploding series count, but arbitrary methods remain unbounded and multiply every route×status series under hostile or unusual clients.
Reviewed by Cursor Bugbot for commit 0867a0e. Configure here.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Follows #1570.
Problem
#1570 gave the web process runtime metrics (heap, GC, event loop lag) but nothing about how long requests take, so per-endpoint latency is still invisible. That gap is concrete: an attempt to build a Better Stack chart of
/api/healthresponse time found the data does not exist anywhere in telemetry — not as a span (traces go to Sentry), not as a metric, and Uptime monitor data isn't queryable from a Telemetry dashboard./api/healthis the useful case. It does almost no work, so its duration is essentially event loop queueing delay — which makes it a direct read on whether the process is stalling, and it is the exact endpoint the liveness probe hits with a 1 second timeout.Change
Adds an
http_request_duration_secondshistogram (labels:method,route,status) populated from Node's built-in HTTP diagnostics channels —http.server.request.startandhttp.server.response.finish.Why diagnostics channels. Next.js owns the
http.Serverinstance in a standalone build, so there is no request pipeline to wrap.proxy.tsmiddleware runs in the edge runtime, whereprom-clientdoesn't work. The channels are a supported, in-process observation point that sees every request without patching or monkey-wrapping anything.Two details that are load-bearing rather than incidental:
Route labels come from Next's own route table. The raw path can't be a label — repository/file paths are unbounded, the first segment is client-supplied, and
/api/[...slug]is a catch-all, so full paths or naive truncation both let scanner traffic grow the series count without limit. Instead, the build's.next/routes-manifest.json(78 static + 18 dynamic routes, each with a matching regex, ordered by the router's own resolution priority) is loaded at startup, and each request path is matched to its route pattern:Cardinality is bounded by the number of defined routes plus
/_nextandother, and the label set tracks the app automatically at build time — no hand-maintained list to rot. If the manifest is missing or unreadable (e.g.next dev), everything is labelledother: granularity lost, bound kept — it fails closed.Metrics scrapes are excluded. The channels are process-wide, so they also fire for the metrics server itself — without a filter on the metrics port, every scrape would record itself and the histogram would measure the observer. Verified in the integration test.
Recording is wrapped in try/catch and logs at debug: a metrics failure must never affect request handling.
Test plan
yarn workspace @sourcebot/web build— exit 0, and bothhttp_request_duration_secondsand the module's log line are present in the standalone server chunksnormalizeRouteunit tests against a manifest-shaped fixture: static exact-match, dynamic patterns, manifest ordering (specific routes beat catch-alls), duplicate/trailing slashes, missing-table fallbackother/api/healthis recorded, two distinct file paths collapse to a single/browseseries with count 2, andstatus="200"is labelledexpected 4 to be 3next buildnor a fixed port/app/packages/web) containsroutes-manifest.json— the standalone server chdirs to the app dir on booteslintclean;tsc --noEmitreports no errors in the new files and the non-test error count is unchanged at 0http_request_duration_secondsappears on:3070/metricsand build the/api/healthlatency chartFollow-up
Once this is deployed and scraped, the
/api/healthp50/p95/p99 chart is a singlehistogramQuantilequery, and the "does health latency spike during GC" correlation becomes one dashboard rather than an inference across two systems. The dashboard is already created and its charts group bysource, so the web process joins them automatically.🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Documentation
Note
Cursor Bugbot is generating a summary for commit d839501. Configure here.