fix(nextjs): Make request data available to tracesSampler for edge middleware root spans#22232
fix(nextjs): Make request data available to tracesSampler for edge middleware root spans#22232s1gr1d wants to merge 4 commits into
tracesSampler for edge middleware root spans#22232Conversation
…middleware root spans
chargome
left a comment
There was a problem hiding this comment.
Can we add one (unskipped) test case to verify the intent of this pr works correctly e2e in a non-cf scenario?
|
👋 @mydea, @nicohrubec — Please review this PR when you get a chance! |
| if (normalizedRequest) { | ||
| getIsolationScope().setSDKProcessingMetadata({ normalizedRequest }); | ||
| } |
There was a problem hiding this comment.
Bug: In serverless warm starts, stale normalizedRequest data from a previous request can persist if a new request lacks HTTP attributes, leading to incorrect sampling decisions.
Severity: MEDIUM
Suggested Fix
The beforeSampling hook should always write to the isolation scope, even if getNormalizedRequestFromAttributes returns undefined. This will ensure that any stale normalizedRequest from a previous invocation is cleared. Modify the logic to call getIsolationScope().setSDKProcessingMetadata({ normalizedRequest }) outside of the if (normalizedRequest) guard, effectively overwriting the old value with the new one (or undefined).
Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent. Verify if this is a real issue. If it is, propose a fix; if not, explain why it's
not valid.
Location: packages/nextjs/src/edge/index.ts#L110-L112
Potential issue: In serverless environments with warm starts, the `beforeSampling` hook
only updates the `normalizedRequest` in the isolation scope if the current request span
has HTTP attributes. If a subsequent request lacks these attributes, the `if
(normalizedRequest)` check fails, and the stale `normalizedRequest` from the previous
invocation is not cleared because `setSDKProcessingMetadata` performs a merge, not a
replacement. When `maybeForkIsolationScopeForRootSpan` later clones the isolation scope,
it inherits this stale data, causing the `tracesSampler` to make incorrect sampling
decisions based on data from a previous request.
On the edge runtime, Next.js's OTel instrumentation creates and samples the
Middleware.executeroot span before the Sentry middleware wrapper runs. SonormalizedRequestwas never on the isolation scope at sampling time, and tracesSampler received undefined.This adds a
beforeSamplinghook in the edge SDK which populatesnormalizedRequest(method, URL, query string) based on HTTP span attributes. It also extends the edgespanStarthandler to fork the isolation scope forMiddleware.executeroot spans (similar to what #22013 did on the Node side after vercel/next.js#95357 made middleware a detached root span).The isolation-scope fork is extracted into a shared
maybeForkIsolationScopeForRootSpanutility used by both the Node and edge handlers.Fixes #22200