Skip to content
Draft
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
Expand Up @@ -27,5 +27,7 @@ test('Sends a server-side exception to Sentry', async ({ baseURL }) => {
expect(errorEvent.contexts?.trace).toEqual({
trace_id: expect.stringMatching(/[a-f0-9]{32}/),
span_id: expect.stringMatching(/[a-f0-9]{16}/),
// Will be present since we no longer drop Next.js spans in the wrapper
parent_span_id: expect.stringMatching(/[a-f0-9]{16}/),
});
});
Original file line number Diff line number Diff line change
Expand Up @@ -22,32 +22,33 @@ test('Sends server-side transactions to Sentry', async ({ baseURL }) => {
span_id: expect.stringMatching(/[a-f0-9]{16}/),
trace_id: expect.stringMatching(/[a-f0-9]{32}/),
op: 'http.server',
origin: 'auto.http.nextjs',
origin: 'auto',
data: expect.objectContaining({
'http.response.status_code': 200,
'sentry.op': 'http.server',
'sentry.origin': 'auto.http.nextjs',
'sentry.origin': 'auto',
'sentry.sample_rate': 1,
'sentry.source': 'route',
}),
status: 'ok',
},
}),
spans: [
{
spans: expect.arrayContaining([
expect.objectContaining({
data: {
'sentry.origin': 'manual',
},
description: 'test-span',
origin: 'manual',
parent_span_id: transactionEvent.contexts?.trace?.span_id,
// Won't be the trace span id because we don't wrap the Next.js span in the wrapper
parent_span_id: expect.stringMatching(/[a-f0-9]{16}/),
span_id: expect.stringMatching(/[a-f0-9]{16}/),
start_timestamp: expect.any(Number),
status: 'ok',
timestamp: expect.any(Number),
trace_id: transactionEvent.contexts?.trace?.trace_id,
},
],
}),
]),
request: {
headers: expect.any(Object),
method: 'GET',
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -39,12 +39,12 @@ test('should create a transaction for a CJS pages router API endpoint', async ({
data: {
'http.response.status_code': 200,
'sentry.op': 'http.server',
'sentry.origin': 'auto.http.nextjs',
'sentry.origin': 'auto',
'sentry.sample_rate': 1,
'sentry.source': 'route',
},
op: 'http.server',
origin: 'auto.http.nextjs',
origin: 'auto',
span_id: expect.stringMatching(/[a-f0-9]{16}/),
status: 'ok',
trace_id: expect.stringMatching(/[a-f0-9]{32}/),
Expand All @@ -57,7 +57,7 @@ test('should create a transaction for a CJS pages router API endpoint', async ({
cookies: expect.any(Object),
headers: expect.any(Object),
method: 'GET',
url: expect.stringMatching(/^http.*\/api\/cjs-api-endpoint$/),
url: expect.stringMatching(/^\/api\/cjs-api-endpoint$/),
},
spans: expect.arrayContaining([]),
start_timestamp: expect.any(Number),
Expand Down Expand Up @@ -102,12 +102,12 @@ test('should not mess up require statements in CJS API endpoints', async ({ requ
data: {
'http.response.status_code': 200,
'sentry.op': 'http.server',
'sentry.origin': 'auto.http.nextjs',
'sentry.origin': 'auto',
'sentry.sample_rate': 1,
'sentry.source': 'route',
},
op: 'http.server',
origin: 'auto.http.nextjs',
origin: 'auto',
span_id: expect.stringMatching(/[a-f0-9]{16}/),
status: 'ok',
trace_id: expect.stringMatching(/[a-f0-9]{32}/),
Expand All @@ -120,7 +120,7 @@ test('should not mess up require statements in CJS API endpoints', async ({ requ
cookies: expect.any(Object),
headers: expect.any(Object),
method: 'GET',
url: expect.stringMatching(/^http.*\/api\/cjs-api-endpoint-with-require$/),
url: expect.stringMatching(/^\/api\/cjs-api-endpoint-with-require$/),
},
spans: expect.arrayContaining([]),
start_timestamp: expect.any(Number),
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -98,11 +98,11 @@ test('Should report a transaction event for a successful pages router api route'
data: {
'http.response.status_code': 200,
'sentry.op': 'http.server',
'sentry.origin': 'auto.http.nextjs',
'sentry.origin': 'auto',
'sentry.source': 'route',
},
op: 'http.server',
origin: 'auto.http.nextjs',
origin: 'auto',
span_id: expect.stringMatching(/[a-f0-9]{16}/),
status: 'ok',
trace_id: expect.stringMatching(/[a-f0-9]{32}/),
Expand All @@ -112,7 +112,7 @@ test('Should report a transaction event for a successful pages router api route'
request: {
headers: expect.any(Object),
method: 'GET',
url: expect.stringMatching(/^http.*\/api\/foo\/success-api-route$/),
url: expect.stringMatching(/^\/api\/foo\/success-api-route$/),
},
start_timestamp: expect.any(Number),
timestamp: expect.any(Number),
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -39,11 +39,11 @@ cases.forEach(({ name, url, transactionName }) => {
data: {
'http.response.status_code': 200,
'sentry.op': 'http.server',
'sentry.origin': 'auto.http.nextjs',
'sentry.origin': 'auto',
'sentry.source': 'route',
},
op: 'http.server',
origin: 'auto.http.nextjs',
origin: 'auto',
span_id: expect.stringMatching(/[a-f0-9]{16}/),
status: 'ok',
trace_id: expect.stringMatching(/[a-f0-9]{32}/),
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -54,8 +54,12 @@ test('Faulty edge routes', async ({ request }) => {

test.step('should have scope isolation', () => {
expect(edgerouteTransaction.tags?.['my-isolated-tag']).toBe(true);
expect(edgerouteTransaction.tags?.['my-global-scope-isolated-tag']).not.toBeDefined();
// No longer valid since we removed the global scope isolation from the wrapper
// expect(edgerouteTransaction.tags?.['my-global-scope-isolated-tag']).not.toBeDefined();
expect(edgerouteTransaction.tags?.['my-global-scope-isolated-tag']).toBeDefined();
expect(errorEvent.tags?.['my-isolated-tag']).toBe(true);
expect(errorEvent.tags?.['my-global-scope-isolated-tag']).not.toBeDefined();
// No longer valid since we removed the global scope isolation from the wrapper
// expect(errorEvent.tags?.['my-global-scope-isolated-tag']).not.toBeDefined();
expect(errorEvent.tags?.['my-global-scope-isolated-tag']).toBeDefined();
});
});
Original file line number Diff line number Diff line change
@@ -1,21 +1,7 @@
import {
captureException,
continueTrace,
debug,
getActiveSpan,
httpRequestToRequestData,
isString,
objectify,
SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN,
SEMANTIC_ATTRIBUTE_SENTRY_SOURCE,
setHttpStatus,
startSpanManual,
withIsolationScope,
} from '@sentry/core';
import { captureException, debug, objectify } from '@sentry/core';
import type { NextApiRequest } from 'next';
import type { AugmentedNextApiResponse, NextApiHandler } from '../types';
import { flushSafelyWithTimeout, waitUntil } from '../utils/responseEnd';
import { dropNextjsRootContext, escapeNextjsTracing } from '../utils/tracingUtils';
import { flushSafelyWithTimeout } from '../utils/responseEnd';

export type AugmentedNextApiRequest = NextApiRequest & {
__withSentry_applied__?: boolean;
Expand All @@ -31,15 +17,13 @@ export type AugmentedNextApiRequest = NextApiRequest & {
*/
export function wrapApiHandlerWithSentry(apiHandler: NextApiHandler, parameterizedRoute: string): NextApiHandler {
return new Proxy(apiHandler, {
apply: (
apply: async (
wrappingTarget,
thisArg,
args: [AugmentedNextApiRequest | undefined, AugmentedNextApiResponse | undefined],
) => {
dropNextjsRootContext();
return escapeNextjsTracing(() => {
try {
const [req, res] = args;

if (!req) {
debug.log(
`Wrapped API handler on route "${parameterizedRoute}" was not passed a request object. Will not instrument.`,
Expand All @@ -56,86 +40,37 @@ export function wrapApiHandlerWithSentry(apiHandler: NextApiHandler, parameteriz
if (req.__withSentry_applied__) {
return wrappingTarget.apply(thisArg, args);
}

req.__withSentry_applied__ = true;

return withIsolationScope(isolationScope => {
// Normally, there is an active span here (from Next.js OTEL) and we just use that as parent
// Else, we manually continueTrace from the incoming headers
const continueTraceIfNoActiveSpan = getActiveSpan()
? <T>(_opts: unknown, callback: () => T) => callback()
: continueTrace;
return await wrappingTarget.apply(thisArg, args);
} catch (e) {
// In case we have a primitive, wrap it in the equivalent wrapper class (string -> String, etc.) so that we can
// store a seen flag on it. (Because of the one-way-on-Vercel-one-way-off-of-Vercel approach we've been forced
// to take, it can happen that the same thrown object gets caught in two different ways, and flagging it is a
// way to prevent it from actually being reported twice.)
const objectifiedErr = objectify(e);

return continueTraceIfNoActiveSpan(
{
sentryTrace:
req.headers && isString(req.headers['sentry-trace']) ? req.headers['sentry-trace'] : undefined,
baggage: req.headers?.baggage,
captureException(objectifiedErr, {
mechanism: {
type: 'auto.http.nextjs.api_handler',
handled: false,
data: {
wrapped_handler: wrappingTarget.name,
function: 'withSentry',
},
() => {
const reqMethod = `${(req.method || 'GET').toUpperCase()} `;
const normalizedRequest = httpRequestToRequestData(req);

isolationScope.setSDKProcessingMetadata({ normalizedRequest });
isolationScope.setTransactionName(`${reqMethod}${parameterizedRoute}`);

return startSpanManual(
{
name: `${reqMethod}${parameterizedRoute}`,
op: 'http.server',
forceTransaction: true,
attributes: {
[SEMANTIC_ATTRIBUTE_SENTRY_SOURCE]: 'route',
[SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN]: 'auto.http.nextjs',
},
},
async span => {
// eslint-disable-next-line @typescript-eslint/unbound-method
res.end = new Proxy(res.end, {
apply(target, thisArg, argArray) {
setHttpStatus(span, res.statusCode);
span.end();
waitUntil(flushSafelyWithTimeout());
return target.apply(thisArg, argArray);
},
});
try {
return await wrappingTarget.apply(thisArg, args);
} catch (e) {
// In case we have a primitive, wrap it in the equivalent wrapper class (string -> String, etc.) so that we can
// store a seen flag on it. (Because of the one-way-on-Vercel-one-way-off-of-Vercel approach we've been forced
// to take, it can happen that the same thrown object gets caught in two different ways, and flagging it is a
// way to prevent it from actually being reported twice.)
const objectifiedErr = objectify(e);

captureException(objectifiedErr, {
mechanism: {
type: 'auto.http.nextjs.api_handler',
handled: false,
data: {
wrapped_handler: wrappingTarget.name,
function: 'withSentry',
},
},
});

setHttpStatus(span, 500);
span.end();
},
});

// we need to await the flush here to ensure that the error is captured
// as the runtime freezes as soon as the error is thrown below
await flushSafelyWithTimeout();
// we need to await the flush here to ensure that the error is captured
// as the runtime freezes as soon as the error is thrown below
await flushSafelyWithTimeout();

// We rethrow here so that nextjs can do with the error whatever it would normally do. (Sometimes "whatever it
// would normally do" is to allow the error to bubble up to the global handlers - another reason we need to mark
// the error as already having been captured.)
throw objectifiedErr;
}
},
);
},
);
});
});
// We rethrow here so that nextjs can do with the error whatever it would normally do. (Sometimes "whatever it
// would normally do" is to allow the error to bubble up to the global handlers - another reason we need to mark
// the error as already having been captured.)
throw objectifiedErr;
}
},
});
}
Original file line number Diff line number Diff line change
Expand Up @@ -6,3 +6,5 @@ export const TRANSACTION_ATTR_SHOULD_DROP_TRANSACTION = 'sentry.drop_transaction
export const TRANSACTION_ATTR_SENTRY_TRACE_BACKFILL = 'sentry.sentry_trace_backfill';

export const TRANSACTION_ATTR_SENTRY_ROUTE_BACKFILL = 'sentry.route_backfill';

export const ATTR_NEXT_PAGES_API_ROUTE_TYPE = 'executing api route (pages)';
37 changes: 32 additions & 5 deletions packages/nextjs/src/edge/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,11 @@ import {
import { getScopesFromContext } from '@sentry/opentelemetry';
import type { VercelEdgeOptions } from '@sentry/vercel-edge';
import { getDefaultIntegrations, init as vercelEdgeInit } from '@sentry/vercel-edge';
import { TRANSACTION_ATTR_SHOULD_DROP_TRANSACTION } from '../common/span-attributes-with-logic-attached';
import { ATTR_NEXT_SPAN_NAME, ATTR_NEXT_SPAN_TYPE } from '../common/nextSpanAttributes';
import {
ATTR_NEXT_PAGES_API_ROUTE_TYPE,
TRANSACTION_ATTR_SHOULD_DROP_TRANSACTION,
} from '../common/span-attributes-with-logic-attached';
import { addHeadersAsAttributes } from '../common/utils/addHeadersAsAttributes';
import { dropMiddlewareTunnelRequests } from '../common/utils/dropMiddlewareTunnelRequests';
import { isBuild } from '../common/utils/isBuild';
Expand Down Expand Up @@ -82,12 +86,21 @@ export function init(options: VercelEdgeOptions = {}): void {
dropMiddlewareTunnelRequests(span, spanAttributes);

// Mark all spans generated by Next.js as 'auto'
if (spanAttributes?.['next.span_type'] !== undefined) {
if (spanAttributes?.[ATTR_NEXT_SPAN_TYPE] !== undefined) {
span.setAttribute(SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN, 'auto');
}

// Backfill span attributes for api route pages because we removed it from the wrapper
if (
spanAttributes?.[ATTR_NEXT_SPAN_TYPE] === 'Node.runHandler' &&
String(spanAttributes?.['next.span_name']).startsWith(ATTR_NEXT_PAGES_API_ROUTE_TYPE)
) {
span.setAttribute(SEMANTIC_ATTRIBUTE_SENTRY_OP, 'http.server');
span.setAttribute(SEMANTIC_ATTRIBUTE_SENTRY_SOURCE, 'route');
}

// Make sure middleware spans get the right op
if (spanAttributes?.['next.span_type'] === 'Middleware.execute') {
if (spanAttributes?.[ATTR_NEXT_SPAN_TYPE] === 'Middleware.execute') {
span.setAttribute(SEMANTIC_ATTRIBUTE_SENTRY_OP, 'http.server.middleware');
span.setAttribute(SEMANTIC_ATTRIBUTE_SENTRY_SOURCE, 'url');

Expand Down Expand Up @@ -119,8 +132,8 @@ export function init(options: VercelEdgeOptions = {}): void {
// The otel auto inference will clobber the transaction name because the span has an http.target
if (
event.type === 'transaction' &&
event.contexts?.trace?.data?.['next.span_type'] === 'Middleware.execute' &&
event.contexts?.trace?.data?.['next.span_name']
event.contexts?.trace?.data?.[ATTR_NEXT_SPAN_TYPE] === 'Middleware.execute' &&
event.contexts?.trace?.data?.[ATTR_NEXT_SPAN_NAME] !== undefined
) {
if (event.transaction) {
// Older nextjs versions pass the full url appended to the middleware name, which results in high cardinality transaction names.
Expand All @@ -139,6 +152,20 @@ export function init(options: VercelEdgeOptions = {}): void {
}
}

// Backfill the transaction name for api route pages because we removed it from the wrapper
if (
event.type === 'transaction' &&
event.contexts?.trace?.data?.[ATTR_NEXT_SPAN_TYPE] === 'Node.runHandler' &&
String(event.contexts.trace.data['next.span_name']).startsWith(ATTR_NEXT_PAGES_API_ROUTE_TYPE)
) {
let path = String(event.contexts.trace.data['next.span_name']).replace(ATTR_NEXT_PAGES_API_ROUTE_TYPE, '').trim();
// Set transaction name on isolation scope to ensure parameterized routes are used
// The HTTP server integration sets it on isolation scope, so we need to match that
const method = event.request?.method || 'GET';
path = path ?? event.request?.url ?? '/';
Copy link

Choose a reason for hiding this comment

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

Bug: Nullish coalescing won't handle empty path string

The path variable is assigned from String(...).replace(...).trim() on line 161, which always returns a string (possibly empty ""). On line 165, path = path ?? event.request?.url ?? '/' uses nullish coalescing (??), which only falls back for null/undefined, not for empty strings. If the span name is exactly 'executing api route (pages)' with no route suffix, path becomes "" and the fallback to event.request?.url or / never triggers. This results in malformed transaction names like "GET " instead of "GET /". Using the logical OR operator (||) would correctly handle empty strings.

Fix in Cursor Fix in Web

event.transaction = `${method} ${path}`;
}

setUrlProcessingMetadata(event);
});

Expand Down
Loading
Loading