Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
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
Expand Up @@ -34,11 +34,8 @@ test('Sends a transaction for a request to app router', async ({ page }) => {
trace_id: expect.stringMatching(/[a-f0-9]{32}/),
});

expect(transactionEvent.request).toEqual({
cookies: {},
headers: expect.objectContaining({
'user-agent': expect.any(String),
}),
expect(transactionEvent.request).toMatchObject({
url: expect.stringContaining('/server-component/parameter/1337/42'),
});

// The transaction should not contain any spans with the same name as the transaction
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -66,8 +66,7 @@ test('Should record exceptions and transactions for faulty route handlers', asyn
expect(routehandlerError.exception?.values?.[0].value).toBe('Dynamic route handler error');

expect(routehandlerError.request?.method).toBe('GET');
// todo: make sure url is attached to request object
// expect(routehandlerError.request?.url).toContain('/route-handlers/boop/error');
expect(routehandlerError.request?.url).toContain('/route-handlers/boop/error');

expect(routehandlerError.transaction).toBe('/route-handlers/[param]/error');
});
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);
});
42 changes: 42 additions & 0 deletions packages/nextjs/src/common/utils/setUrlProcessingMetadata.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,42 @@
import type { Event } from '@sentry/core';
import { getClient } 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;
}

// Only add URL if sendDefaultPii is enabled, as URLs may contain PII
const client = getClient();
if (!client?.getOptions().sendDefaultPii) {
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;
Comment on lines +31 to +40
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 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.

💡 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

Copy link
Collaborator Author

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.

}
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Fix in Cursor Fix in Web

Copy link
Collaborator Author

Choose a reason for hiding this comment

The 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.

}
3 changes: 3 additions & 0 deletions packages/nextjs/src/edge/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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';
Expand Down Expand Up @@ -126,6 +127,8 @@ export function init(options: VercelEdgeOptions = {}): void {
}
}
}

setUrlProcessingMetadata(event);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could gate this already with sendDefaultPii

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Got it, I added a check for it inside the setUrlProcessingMetadata in 6089a70

});

client?.on('spanEnd', span => {
Expand Down
3 changes: 3 additions & 0 deletions packages/nextjs/src/server/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,7 @@ import {
} from '../common/span-attributes-with-logic-attached';
import { addHeadersAsAttributes } from '../common/utils/addHeadersAsAttributes';
import { isBuild } from '../common/utils/isBuild';
import { setUrlProcessingMetadata } from '../common/utils/setUrlProcessingMetadata';
import { distDirRewriteFramesIntegration } from './distDirRewriteFramesIntegration';

export * from '@sentry/node';
Expand Down Expand Up @@ -391,6 +392,8 @@ export function init(options: NodeOptions): NodeClient | undefined {
event.contexts.trace.parent_span_id = traceparentData.parentSpanId;
}
}

setUrlProcessingMetadata(event);
});

if (process.env.NODE_ENV === 'development') {
Expand Down
Loading