Uh oh!
There was an error while loading. Please reload this page.
feat(nextjs): Add URL to server-side transaction events - #18230
Conversation
| isolationScopeData.sdkProcessingMetadata.normalizedRequest = | ||
| isolationScopeData.sdkProcessingMetadata.normalizedRequest || {}; | ||
| isolationScopeData.sdkProcessingMetadata.normalizedRequest.url = url; | ||
| } |
There was a problem hiding this comment.
Bug: Transaction Event Request Data Uses Wrong Scope
The function modifies capturedSpanIsolationScope metadata, but the transaction event's request field is populated from capturedSpanScope. Modifications to the isolation scope won't affect the URL included in the event since requestDataIntegration reads from the same scope used to populate the initial request field. Should modify capturedSpanScope instead.
There was a problem hiding this comment.
Data is merged from the trace data and the isolation scope, also tests pass so I don't think this is valid.
| const isolationScopeData = event.sdkProcessingMetadata?.capturedSpanIsolationScope?.getScopeData(); | ||
| const headersDict = isolationScopeData?.sdkProcessingMetadata?.normalizedRequest?.headers; | ||
| const url = getSanitizedRequestUrl(componentRoute, undefined, headersDict, httpTarget?.toString()); | ||
| // Add URL to the isolation scope's normalizedRequest so requestDataIntegration picks it up | ||
| if (url && isolationScopeData?.sdkProcessingMetadata) { | ||
| isolationScopeData.sdkProcessingMetadata.normalizedRequest = | ||
| isolationScopeData.sdkProcessingMetadata.normalizedRequest || {}; | ||
| isolationScopeData.sdkProcessingMetadata.normalizedRequest.url = url; |
There was a problem hiding this comment.
Bug:setUrlProcessingMetadata runs too late in the beforeSendEvent hook, after requestDataIntegration has processed the event, causing URLs to be missed.
Severity: CRITICAL | Confidence: 0.95
🔍 Detailed Analysis
The setUrlProcessingMetadata function executes in the beforeSendEvent hook, which occurs afterrequestDataIntegration.processEvent() has already run. Consequently, when setUrlProcessingMetadata attempts to modify isolationScopeData.sdkProcessingMetadata.normalizedRequest.url, requestDataIntegration has already processed the event and populated event.request, or failed to find the URL. This late modification means server-side transaction events will continue to lack URLs, rendering the intended feature non-functional.
💡 Suggested Fix
Move the URL addition logic to an earlier hook like preprocessEvent, or directly set event.request.url within the beforeSendEvent hook instead of modifying scope metadata.
🤖 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/common/utils/setUrlProcessingMetadata.ts#L24-L33
Potential issue: The `setUrlProcessingMetadata` function executes in the
`beforeSendEvent` hook, which occurs *after* `requestDataIntegration.processEvent()` has
already run. Consequently, when `setUrlProcessingMetadata` attempts to modify
`isolationScopeData.sdkProcessingMetadata.normalizedRequest.url`,
`requestDataIntegration` has already processed the event and populated `event.request`,
or failed to find the URL. This late modification means server-side transaction events
will continue to lack URLs, rendering the intended feature non-functional.
Did we get this right? 👍 / 👎 to inform future reviews.
Reference_id: 2725898
There was a problem hiding this comment.
Well tests pass with the fix and fail without it, so the timing looks correct.
size-limit report 📦
|
| } | ||
| } | ||
| setUrlProcessingMetadata(event); |
There was a problem hiding this comment.
We could gate this already with sendDefaultPii
There was a problem hiding this comment.
Got it, I added a check for it inside the setUrlProcessingMetadata in 6089a70
node-overhead report 🧳Note: This is a synthetic benchmark with a minimal express app and does not necessarily reflect the real-world performance impact in an application.
|
Uh oh!
There was an error while loading. Please reload this page.
URLs were missing from server-side transaction events (server components, generation functions) in Next.js. This was previously removed in #18113 because we tried to synchronously access
paramsandsearchParams, which cause builds to crash.This PR approach adds the URL at runtime using a
preprocessEventhook as suggested.Implementation
http.target(actual request path) andnext.route(parameterized route) from the transaction's trace datagetSanitizedRequestUrl()utilitynormalizedRequest.urlso therequestDataIntegrationincludes it in the eventThis works uniformly for both Webpack and Turbopack across all of our supported Next.js versions (13~16), I added missing tests for this case in the versions that did not have it.
Fixes#18115