-
-
Notifications
You must be signed in to change notification settings - Fork 1.7k
feat(nextjs): Add URL to server-side transaction events #18230
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 3 commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,48 @@ | ||
| import { expect, test } from '@playwright/test'; | ||
| import { waitForTransaction } from '@sentry-internal/test-utils'; | ||
|
|
||
| test('Sends a transaction for a request to app router with URL', async ({ page }) => { | ||
| const serverComponentTransactionPromise = waitForTransaction('nextjs-13', transactionEvent => { | ||
| return ( | ||
| transactionEvent?.transaction === 'GET /parameterized/[one]/beep/[two]' && | ||
| transactionEvent.contexts?.trace?.data?.['http.target']?.startsWith('/parameterized/1337/beep/42') | ||
| ); | ||
| }); | ||
|
|
||
| await page.goto('/parameterized/1337/beep/42'); | ||
|
|
||
| const transactionEvent = await serverComponentTransactionPromise; | ||
|
|
||
| expect(transactionEvent.contexts?.trace).toEqual({ | ||
| data: expect.objectContaining({ | ||
| 'sentry.op': 'http.server', | ||
| 'sentry.origin': 'auto', | ||
| 'sentry.sample_rate': 1, | ||
| 'sentry.source': 'route', | ||
| 'http.method': 'GET', | ||
| 'http.response.status_code': 200, | ||
| 'http.route': '/parameterized/[one]/beep/[two]', | ||
| 'http.status_code': 200, | ||
| 'http.target': '/parameterized/1337/beep/42', | ||
| 'otel.kind': 'SERVER', | ||
| 'next.route': '/parameterized/[one]/beep/[two]', | ||
| }), | ||
| op: 'http.server', | ||
| origin: 'auto', | ||
| span_id: expect.stringMatching(/[a-f0-9]{16}/), | ||
| status: 'ok', | ||
| trace_id: expect.stringMatching(/[a-f0-9]{32}/), | ||
| }); | ||
|
|
||
| expect(transactionEvent.request).toMatchObject({ | ||
| url: expect.stringContaining('/parameterized/1337/beep/42'), | ||
| }); | ||
|
|
||
| // The transaction should not contain any spans with the same name as the transaction | ||
| // e.g. "GET /parameterized/[one]/beep/[two]" | ||
| expect( | ||
| transactionEvent.spans?.filter(span => { | ||
| return span.description === transactionEvent.transaction; | ||
| }), | ||
| ).toHaveLength(0); | ||
| }); |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,48 @@ | ||
| import { expect, test } from '@playwright/test'; | ||
| import { waitForTransaction } from '@sentry-internal/test-utils'; | ||
|
|
||
| test('Sends a transaction for a request to app router with URL', async ({ page }) => { | ||
| const serverComponentTransactionPromise = waitForTransaction('nextjs-15', transactionEvent => { | ||
| return ( | ||
| transactionEvent?.transaction === 'GET /parameterized/[one]/beep/[two]' && | ||
| transactionEvent.contexts?.trace?.data?.['http.target']?.startsWith('/parameterized/1337/beep/42') | ||
| ); | ||
| }); | ||
|
|
||
| await page.goto('/parameterized/1337/beep/42'); | ||
|
|
||
| const transactionEvent = await serverComponentTransactionPromise; | ||
|
|
||
| expect(transactionEvent.contexts?.trace).toEqual({ | ||
| data: expect.objectContaining({ | ||
| 'sentry.op': 'http.server', | ||
| 'sentry.origin': 'auto', | ||
| 'sentry.sample_rate': 1, | ||
| 'sentry.source': 'route', | ||
| 'http.method': 'GET', | ||
| 'http.response.status_code': 200, | ||
| 'http.route': '/parameterized/[one]/beep/[two]', | ||
| 'http.status_code': 200, | ||
| 'http.target': '/parameterized/1337/beep/42', | ||
| 'otel.kind': 'SERVER', | ||
| 'next.route': '/parameterized/[one]/beep/[two]', | ||
| }), | ||
| op: 'http.server', | ||
| origin: 'auto', | ||
| span_id: expect.stringMatching(/[a-f0-9]{16}/), | ||
| status: 'ok', | ||
| trace_id: expect.stringMatching(/[a-f0-9]{32}/), | ||
| }); | ||
|
|
||
| expect(transactionEvent.request).toMatchObject({ | ||
| url: expect.stringContaining('/parameterized/1337/beep/42'), | ||
| }); | ||
|
|
||
| // The transaction should not contain any spans with the same name as the transaction | ||
| // e.g. "GET /parameterized/[one]/beep/[two]" | ||
| expect( | ||
| transactionEvent.spans?.filter(span => { | ||
| return span.description === transactionEvent.transaction; | ||
| }), | ||
| ).toHaveLength(0); | ||
| }); |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,48 @@ | ||
| import { expect, test } from '@playwright/test'; | ||
| import { waitForTransaction } from '@sentry-internal/test-utils'; | ||
|
|
||
| test('Sends a transaction for a request to app router with URL', async ({ page }) => { | ||
| const serverComponentTransactionPromise = waitForTransaction('nextjs-16', transactionEvent => { | ||
| return ( | ||
| transactionEvent?.transaction === 'GET /parameterized/[one]/beep/[two]' && | ||
| transactionEvent.contexts?.trace?.data?.['http.target']?.startsWith('/parameterized/1337/beep/42') | ||
| ); | ||
| }); | ||
|
|
||
| await page.goto('/parameterized/1337/beep/42'); | ||
|
|
||
| const transactionEvent = await serverComponentTransactionPromise; | ||
|
|
||
| expect(transactionEvent.contexts?.trace).toEqual({ | ||
| data: expect.objectContaining({ | ||
| 'sentry.op': 'http.server', | ||
| 'sentry.origin': 'auto', | ||
| 'sentry.sample_rate': 1, | ||
| 'sentry.source': 'route', | ||
| 'http.method': 'GET', | ||
| 'http.response.status_code': 200, | ||
| 'http.route': '/parameterized/[one]/beep/[two]', | ||
| 'http.status_code': 200, | ||
| 'http.target': '/parameterized/1337/beep/42', | ||
| 'otel.kind': 'SERVER', | ||
| 'next.route': '/parameterized/[one]/beep/[two]', | ||
| }), | ||
| op: 'http.server', | ||
| origin: 'auto', | ||
| span_id: expect.stringMatching(/[a-f0-9]{16}/), | ||
| status: 'ok', | ||
| trace_id: expect.stringMatching(/[a-f0-9]{32}/), | ||
| }); | ||
|
|
||
| expect(transactionEvent.request).toMatchObject({ | ||
| url: expect.stringContaining('/parameterized/1337/beep/42'), | ||
| }); | ||
|
|
||
| // The transaction should not contain any spans with the same name as the transaction | ||
| // e.g. "GET /parameterized/[one]/beep/[two]" | ||
| expect( | ||
| transactionEvent.spans?.filter(span => { | ||
| return span.description === transactionEvent.transaction; | ||
| }), | ||
| ).toHaveLength(0); | ||
| }); |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,48 @@ | ||
| import { expect, test } from '@playwright/test'; | ||
| import { waitForTransaction } from '@sentry-internal/test-utils'; | ||
|
|
||
| test('Sends a transaction for a request to app router with URL', async ({ page }) => { | ||
| const serverComponentTransactionPromise = waitForTransaction('nextjs-turbo', transactionEvent => { | ||
| return ( | ||
| transactionEvent?.transaction === 'GET /parameterized/[one]/beep/[two]' && | ||
| transactionEvent.contexts?.trace?.data?.['http.target']?.startsWith('/parameterized/1337/beep/42') | ||
| ); | ||
| }); | ||
|
|
||
| await page.goto('/parameterized/1337/beep/42'); | ||
|
|
||
| const transactionEvent = await serverComponentTransactionPromise; | ||
|
|
||
| expect(transactionEvent.contexts?.trace).toEqual({ | ||
| data: expect.objectContaining({ | ||
| 'sentry.op': 'http.server', | ||
| 'sentry.origin': 'auto', | ||
| 'sentry.sample_rate': 1, | ||
| 'sentry.source': 'route', | ||
| 'http.method': 'GET', | ||
| 'http.response.status_code': 200, | ||
| 'http.route': '/parameterized/[one]/beep/[two]', | ||
| 'http.status_code': 200, | ||
| 'http.target': '/parameterized/1337/beep/42', | ||
| 'otel.kind': 'SERVER', | ||
| 'next.route': '/parameterized/[one]/beep/[two]', | ||
| }), | ||
| op: 'http.server', | ||
| origin: 'auto', | ||
| span_id: expect.stringMatching(/[a-f0-9]{16}/), | ||
| status: 'ok', | ||
| trace_id: expect.stringMatching(/[a-f0-9]{32}/), | ||
| }); | ||
|
|
||
| expect(transactionEvent.request).toMatchObject({ | ||
| url: expect.stringContaining('/parameterized/1337/beep/42'), | ||
| }); | ||
|
|
||
| // The transaction should not contain any spans with the same name as the transaction | ||
| // e.g. "GET /parameterized/[one]/beep/[two]" | ||
| expect( | ||
| transactionEvent.spans?.filter(span => { | ||
| return span.description === transactionEvent.transaction; | ||
| }), | ||
| ).toHaveLength(0); | ||
| }); |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,35 @@ | ||
| import type { Event } from '@sentry/core'; | ||
| import { getSanitizedRequestUrl } from './urls'; | ||
|
|
||
| /** | ||
| * Sets the URL processing metadata for the event. | ||
| */ | ||
| export function setUrlProcessingMetadata(event: Event): void { | ||
| // Skip if not a server-side transaction | ||
| if (event.type !== 'transaction' || event.contexts?.trace?.op !== 'http.server' || !event.contexts?.trace?.data) { | ||
| return; | ||
| } | ||
|
|
||
| const traceData = event.contexts.trace.data; | ||
|
|
||
| // Get the route from trace data | ||
| const componentRoute = traceData['next.route'] || traceData['http.route']; | ||
| const httpTarget = traceData['http.target'] as string | undefined; | ||
|
|
||
| if (!componentRoute) { | ||
| return; | ||
| } | ||
|
|
||
| // Extract headers | ||
| 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Bug: Transaction Event Request Data Uses Wrong ScopeThe function modifies
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Data is merged from the trace data and the isolation scope, also tests pass so I don't think this is valid. |
||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -22,6 +22,7 @@ import { getDefaultIntegrations, init as vercelEdgeInit } from '@sentry/vercel-e | |
| import { addHeadersAsAttributes } from '../common/utils/addHeadersAsAttributes'; | ||
| import { isBuild } from '../common/utils/isBuild'; | ||
| import { flushSafelyWithTimeout } from '../common/utils/responseEnd'; | ||
| import { setUrlProcessingMetadata } from '../common/utils/setUrlProcessingMetadata'; | ||
| import { distDirRewriteFramesIntegration } from './distDirRewriteFramesIntegration'; | ||
|
|
||
| export * from '@sentry/vercel-edge'; | ||
|
|
@@ -126,6 +127,8 @@ export function init(options: VercelEdgeOptions = {}): void { | |
| } | ||
| } | ||
| } | ||
|
|
||
| setUrlProcessingMetadata(event); | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We could gate this already with
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Got it, I added a check for it inside the |
||
| }); | ||
|
|
||
| client?.on('spanEnd', span => { | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bug:
setUrlProcessingMetadataruns too late in thebeforeSendEventhook, afterrequestDataIntegrationhas processed the event, causing URLs to be missed.Severity: CRITICAL | Confidence: 0.95
🔍 Detailed Analysis
The
setUrlProcessingMetadatafunction executes in thebeforeSendEventhook, which occurs afterrequestDataIntegration.processEvent()has already run. Consequently, whensetUrlProcessingMetadataattempts to modifyisolationScopeData.sdkProcessingMetadata.normalizedRequest.url,requestDataIntegrationhas already processed the event and populatedevent.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 setevent.request.urlwithin thebeforeSendEventhook instead of modifying scope metadata.🤖 Prompt for AI Agent
Did we get this right? 👍 / 👎 to inform future reviews.
Reference_id: 2725898
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well tests pass with the fix and fail without it, so the timing looks correct.