Skip to content
Open
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,12 @@
import { PropsWithChildren } from 'react';

export const dynamic = 'force-dynamic';

export default function Layout({ children }: PropsWithChildren<{}>) {
return (
<div>
<p>DynamicLayout</p>
{children}
</div>
);
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
export const dynamic = 'force-dynamic';

export default async function Page() {
return (
<div>
<p>Dynamic Page</p>
</div>
);
}

export async function generateMetadata() {
return {
title: 'I am dynamic page generated metadata',
};
}
Original file line number Diff line number Diff line change
Expand Up @@ -14,8 +14,43 @@ test('Will create a transaction with spans for every server component and metada
return span.description;
});

expect(spanDescriptions).toContainEqual('Layout Server Component (/(nested-layout)/nested-layout)');
expect(spanDescriptions).toContainEqual('Layout Server Component (/(nested-layout))');
expect(spanDescriptions).toContainEqual('Page Server Component (/(nested-layout)/nested-layout)');
expect(spanDescriptions).toContainEqual('resolve page components');
expect(spanDescriptions).toContainEqual('render route (app) /nested-layout');
expect(spanDescriptions).toContainEqual('build component tree');
expect(spanDescriptions).toContainEqual('resolve root layout server component');
expect(spanDescriptions).toContainEqual('resolve layout server component "(nested-layout)"');
expect(spanDescriptions).toContainEqual('resolve layout server component "nested-layout"');
expect(spanDescriptions).toContainEqual('resolve page server component "/nested-layout"');
expect(spanDescriptions).toContainEqual('generateMetadata /(nested-layout)/nested-layout/page');
expect(spanDescriptions).toContainEqual('Page.generateMetadata (/(nested-layout)/nested-layout)');
expect(spanDescriptions).toContainEqual('start response');
expect(spanDescriptions).toContainEqual('NextNodeServer.clientComponentLoading');
});

test('Will create a transaction with spans for every server component and metadata generation functions when visiting a dynamic page', async ({
page,
}) => {
const serverTransactionEventPromise = waitForTransaction('nextjs-app-dir', async transactionEvent => {
console.log(transactionEvent?.transaction);
Copy link

Choose a reason for hiding this comment

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

Bug: Debug console.log left in test code

A console.log(transactionEvent?.transaction) statement appears to have been left in the test code, likely from debugging. While this is in test code, it will produce unnecessary output during test runs.

Fix in Cursor Fix in Web

return transactionEvent?.transaction === 'GET /nested-layout/[dynamic]';
});

await page.goto('/nested-layout/123');

const spanDescriptions = (await serverTransactionEventPromise).spans?.map(span => {
return span.description;
});

expect(spanDescriptions).toContainEqual('resolve page components');
expect(spanDescriptions).toContainEqual('render route (app) /nested-layout/[dynamic]');
expect(spanDescriptions).toContainEqual('build component tree');
expect(spanDescriptions).toContainEqual('resolve root layout server component');
expect(spanDescriptions).toContainEqual('resolve layout server component "(nested-layout)"');
expect(spanDescriptions).toContainEqual('resolve layout server component "nested-layout"');
expect(spanDescriptions).toContainEqual('resolve layout server component "[dynamic]"');
expect(spanDescriptions).toContainEqual('resolve page server component "/nested-layout/[dynamic]"');
expect(spanDescriptions).toContainEqual('generateMetadata /(nested-layout)/nested-layout/[dynamic]/page');
expect(spanDescriptions).toContainEqual('Page.generateMetadata (/(nested-layout)/nested-layout/[dynamic])');
expect(spanDescriptions).toContainEqual('start response');
expect(spanDescriptions).toContainEqual('NextNodeServer.clientComponentLoading');
});
Original file line number Diff line number Diff line change
Expand Up @@ -70,13 +70,20 @@ test('Should set a "not_found" status on a server component span when notFound()

const transactionEvent = await serverComponentTransactionPromise;

// Transaction should have status ok, because the http status is ok, but the server component span should be not_found
// Transaction should have status ok, because the http status is ok, but the render component span should be not_found
expect(transactionEvent.contexts?.trace?.status).toBe('ok');
expect(transactionEvent.spans).toContainEqual(
expect.objectContaining({
description: 'Page Server Component (/server-component/not-found)',
op: 'function.nextjs',
description: 'render route (app) /server-component/not-found',
status: 'not_found',
}),
);

// Page server component span should have the right name and attributes
expect(transactionEvent.spans).toContainEqual(
expect.objectContaining({
description: 'resolve page server component "/server-component/not-found"',
op: 'function.nextjs',
data: expect.objectContaining({
'sentry.nextjs.ssr.function.type': 'Page',
'sentry.nextjs.ssr.function.route': '/server-component/not-found',
Expand All @@ -102,13 +109,20 @@ test('Should capture an error and transaction for a app router page', async ({ p
// Error event should have the right transaction name
expect(errorEvent.transaction).toBe(`Page Server Component (/server-component/faulty)`);

// Transaction should have status ok, because the http status is ok, but the server component span should be internal_error
// Transaction should have status ok, because the http status is ok, but the render component span should be internal_error
expect(transactionEvent.contexts?.trace?.status).toBe('ok');
expect(transactionEvent.spans).toContainEqual(
expect.objectContaining({
description: 'Page Server Component (/server-component/faulty)',
op: 'function.nextjs',
description: 'render route (app) /server-component/faulty',
status: 'internal_error',
}),
);

// The page server component span should have the right name and attributes
expect(transactionEvent.spans).toContainEqual(
expect.objectContaining({
description: 'resolve page server component "/server-component/faulty"',
op: 'function.nextjs',
data: expect.objectContaining({
'sentry.nextjs.ssr.function.type': 'Page',
'sentry.nextjs.ssr.function.route': '/server-component/faulty',
Expand Down
2 changes: 2 additions & 0 deletions packages/nextjs/src/common/nextSpanAttributes.ts
Original file line number Diff line number Diff line change
@@ -1,3 +1,5 @@
export const ATTR_NEXT_SPAN_TYPE = 'next.span_type';
export const ATTR_NEXT_SPAN_NAME = 'next.span_name';
export const ATTR_NEXT_ROUTE = 'next.route';
export const ATTR_NEXT_SPAN_DESCRIPTION = 'next.span_description';
export const ATTR_NEXT_SEGMENT = 'next.segment';
75 changes: 73 additions & 2 deletions packages/nextjs/src/common/utils/tracingUtils.ts
Original file line number Diff line number Diff line change
@@ -1,10 +1,23 @@
import type { PropagationContext } from '@sentry/core';
import { debug, getActiveSpan, getRootSpan, GLOBAL_OBJ, Scope, spanToJSON, startNewTrace } from '@sentry/core';
import { ATTR_HTTP_ROUTE } from '@opentelemetry/semantic-conventions';
import type { PropagationContext, Span, SpanAttributes } from '@sentry/core';
import {
debug,
getActiveSpan,
getRootSpan,
GLOBAL_OBJ,
Scope,
SEMANTIC_ATTRIBUTE_SENTRY_OP,
spanToJSON,
startNewTrace,
} from '@sentry/core';
import { DEBUG_BUILD } from '../debug-build';
import { ATTR_NEXT_SEGMENT, ATTR_NEXT_SPAN_NAME, ATTR_NEXT_SPAN_TYPE } from '../nextSpanAttributes';
import { TRANSACTION_ATTR_SHOULD_DROP_TRANSACTION } from '../span-attributes-with-logic-attached';

const commonPropagationContextMap = new WeakMap<object, PropagationContext>();

const PAGE_SEGMENT = '__PAGE__';

/**
* Takes a shared (garbage collectable) object between resources, e.g. a headers object shared between Next.js server components and returns a common propagation context.
*
Expand Down Expand Up @@ -108,3 +121,61 @@ export function dropNextjsRootContext(): void {
}
}
}

/**
* Checks if the span is a resolve segment span.
* @param spanAttributes The attributes of the span to check.
* @returns True if the span is a resolve segment span, false otherwise.
*/
export function isResolveSegmentSpan(spanAttributes: SpanAttributes): boolean {
return (
spanAttributes[ATTR_NEXT_SPAN_TYPE] === 'NextNodeServer.getLayoutOrPageModule' &&
spanAttributes[ATTR_NEXT_SPAN_NAME] === 'resolve segment modules' &&
typeof spanAttributes[ATTR_NEXT_SEGMENT] === 'string'
);
}

/**
* Returns the enhanced name for a resolve segment span.
* @param segment The segment of the resolve segment span.
* @param route The route of the resolve segment span.
* @returns The enhanced name for the resolve segment span.
*/
export function getEnhancedResolveSegmentSpanName({ segment, route }: { segment: string; route: string }): string {
if (segment === PAGE_SEGMENT) {
return `resolve page server component "${route}"`;
}

if (segment === '') {
return 'resolve root layout server component';
}

return `resolve layout server component "${segment}"`;
}

/**
* Maybe enhances the span name for a resolve segment span.
* If the span is not a resolve segment span, this function does nothing.
* @param activeSpan The active span.
* @param spanAttributes The attributes of the span to check.
* @param rootSpanAttributes The attributes of the according root span.
*/
export function maybeEnhanceServerComponentSpanName(
activeSpan: Span,
spanAttributes: SpanAttributes,
rootSpanAttributes: SpanAttributes,
): void {
if (!isResolveSegmentSpan(spanAttributes)) {
return;
}

const segment = spanAttributes[ATTR_NEXT_SEGMENT] as string;
const route = rootSpanAttributes[ATTR_HTTP_ROUTE];
const enhancedName = getEnhancedResolveSegmentSpanName({ segment, route: typeof route === 'string' ? route : '' });
activeSpan.updateName(enhancedName);
activeSpan.setAttributes({
'sentry.nextjs.ssr.function.type': segment === PAGE_SEGMENT ? 'Page' : 'Layout',
'sentry.nextjs.ssr.function.route': route,
});
activeSpan.setAttribute(SEMANTIC_ATTRIBUTE_SENTRY_OP, 'function.nextjs');
}
117 changes: 33 additions & 84 deletions packages/nextjs/src/common/wrapServerComponentWithSentry.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2,26 +2,16 @@ import type { RequestEventData } from '@sentry/core';
import {
captureException,
getActiveSpan,
getCapturedScopesOnSpan,
getRootSpan,
getIsolationScope,
handleCallbackErrors,
propagationContextFromHeaders,
Scope,
SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN,
SEMANTIC_ATTRIBUTE_SENTRY_SOURCE,
setCapturedScopesOnSpan,
SPAN_STATUS_ERROR,
SPAN_STATUS_OK,
startSpanManual,
winterCGHeadersToDict,
withIsolationScope,
withScope,
} from '@sentry/core';
import { isNotFoundNavigationError, isRedirectNavigationError } from '../common/nextNavigationErrorUtils';
import type { ServerComponentContext } from '../common/types';
import { flushSafelyWithTimeout, waitUntil } from '../common/utils/responseEnd';
import { TRANSACTION_ATTR_SENTRY_TRACE_BACKFILL } from './span-attributes-with-logic-attached';
import { commonObjectToIsolationScope, commonObjectToPropagationContext } from './utils/tracingUtils';
import { commonObjectToIsolationScope } from './utils/tracingUtils';

/**
* Wraps an `app` directory server component with Sentry error instrumentation.
Expand All @@ -31,22 +21,13 @@ export function wrapServerComponentWithSentry<F extends (...args: any[]) => any>
appDirComponent: F,
context: ServerComponentContext,
): F {
const { componentRoute, componentType } = context;
// Even though users may define server components as async functions, for the client bundles
// Next.js will turn them into synchronous functions and it will transform any `await`s into instances of the `use`
// hook. 🤯
return new Proxy(appDirComponent, {
apply: (originalFunction, thisArg, args) => {
const requestTraceId = getActiveSpan()?.spanContext().traceId;
const isolationScope = commonObjectToIsolationScope(context.headers);

const activeSpan = getActiveSpan();
if (activeSpan) {
const rootSpan = getRootSpan(activeSpan);
const { scope } = getCapturedScopesOnSpan(rootSpan);
setCapturedScopesOnSpan(rootSpan, scope ?? new Scope(), isolationScope);
}

const headersDict = context.headers ? winterCGHeadersToDict(context.headers) : undefined;

isolationScope.setSDKProcessingMetadata({
Expand All @@ -55,74 +36,42 @@ export function wrapServerComponentWithSentry<F extends (...args: any[]) => any>
} satisfies RequestEventData,
});

return withIsolationScope(isolationScope, () => {
return withScope(scope => {
scope.setTransactionName(`${componentType} Server Component (${componentRoute})`);

if (process.env.NEXT_RUNTIME === 'edge') {
const propagationContext = commonObjectToPropagationContext(
context.headers,
propagationContextFromHeaders(headersDict?.['sentry-trace'], headersDict?.['baggage']),
);

if (requestTraceId) {
propagationContext.traceId = requestTraceId;
}

scope.setPropagationContext(propagationContext);
}
return handleCallbackErrors(
() => originalFunction.apply(thisArg, args),
error => {
const isolationScope = getIsolationScope();
const span = getActiveSpan();
const { componentRoute, componentType } = context;
let shouldCapture = false;
isolationScope.setTransactionName(`${componentType} Server Component (${componentRoute})`);

const activeSpan = getActiveSpan();
if (activeSpan) {
const rootSpan = getRootSpan(activeSpan);
const sentryTrace = headersDict?.['sentry-trace'];
if (sentryTrace) {
rootSpan.setAttribute(TRANSACTION_ATTR_SENTRY_TRACE_BACKFILL, sentryTrace);
if (span) {
if (isNotFoundNavigationError(error)) {
shouldCapture = false;
// We don't want to report "not-found"s
span.setStatus({ code: SPAN_STATUS_ERROR, message: 'not_found' });
} else if (isRedirectNavigationError(error)) {
shouldCapture = false;
// We don't want to report redirects
span.setStatus({ code: SPAN_STATUS_OK });
} else {
span.setStatus({ code: SPAN_STATUS_ERROR, message: 'internal_error' });
}
}

return startSpanManual(
{
op: 'function.nextjs',
name: `${componentType} Server Component (${componentRoute})`,
attributes: {
[SEMANTIC_ATTRIBUTE_SENTRY_SOURCE]: 'component',
[SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN]: 'auto.function.nextjs.server_component',
'sentry.nextjs.ssr.function.type': componentType,
'sentry.nextjs.ssr.function.route': componentRoute,
if (shouldCapture) {
captureException(error, {
Comment on lines +53 to +63
Copy link

Choose a reason for hiding this comment

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

Bug: The shouldCapture flag is never set to true for general errors, preventing captureException() from being called.
Severity: CRITICAL | Confidence: High

🔍 Detailed Analysis

The shouldCapture flag is initialized to false and is only explicitly set to false for isNotFoundNavigationError and isRedirectNavigationError. In the else branch, which handles all other errors, shouldCapture is never set to true. Consequently, the if (shouldCapture) block containing captureException(error, ...) never executes, preventing any actual errors from being reported to Sentry.

💡 Suggested Fix

In the else branch of the error handling logic, add shouldCapture = true; to ensure that general errors are captured by Sentry.

🤖 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/wrapServerComponentWithSentry.ts#L45-L63

Potential issue: The `shouldCapture` flag is initialized to `false` and is only
explicitly set to `false` for `isNotFoundNavigationError` and
`isRedirectNavigationError`. In the `else` branch, which handles all other errors,
`shouldCapture` is never set to `true`. Consequently, the `if (shouldCapture)` block
containing `captureException(error, ...)` never executes, preventing any actual errors
from being reported to Sentry.

Did we get this right? 👍 / 👎 to inform future reviews.
Reference ID: 5790254

mechanism: {
handled: false,
type: 'auto.function.nextjs.server_component',
},
},
span => {
return handleCallbackErrors(
() => originalFunction.apply(thisArg, args),
error => {
// When you read this code you might think: "Wait a minute, shouldn't we set the status on the root span too?"
// The answer is: "No." - The status of the root span is determined by whatever status code Next.js decides to put on the response.
if (isNotFoundNavigationError(error)) {
// We don't want to report "not-found"s
span.setStatus({ code: SPAN_STATUS_ERROR, message: 'not_found' });
} else if (isRedirectNavigationError(error)) {
// We don't want to report redirects
span.setStatus({ code: SPAN_STATUS_OK });
} else {
span.setStatus({ code: SPAN_STATUS_ERROR, message: 'internal_error' });
captureException(error, {
mechanism: {
handled: false,
type: 'auto.function.nextjs.server_component',
},
});
}
},
() => {
span.end();
waitUntil(flushSafelyWithTimeout());
},
);
},
);
});
});
});
}
Copy link

Choose a reason for hiding this comment

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

Bug: Exceptions never captured due to unset shouldCapture flag

The shouldCapture variable is initialized to false on line 45 and never set to true. In the original code, captureException was called directly in the else branch for actual errors. Now it's guarded by if (shouldCapture), but the else branch (lines 57-58) only sets the span status without setting shouldCapture = true. This means real exceptions from server components will never be captured by Sentry.

Fix in Cursor Fix in Web

},
() => {
waitUntil(flushSafelyWithTimeout());
},
);
Copy link

Choose a reason for hiding this comment

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

Bug: Missing isolation scope context breaks request metadata attachment

The refactored code retrieves an isolation scope via commonObjectToIsolationScope at line 29 and sets normalizedRequest metadata on it, but the handleCallbackErrors callback is not wrapped with withIsolationScope. When captureException is called in the error handler, getIsolationScope() at line 42 returns the current global isolation scope - not the one with the request metadata. This causes captured exceptions to be missing request context (like headers) that was set up earlier. The original code used withIsolationScope(isolationScope, ...) to ensure all nested code ran with the correct isolation scope.

Fix in Cursor Fix in Web

Copy link
Member Author

Choose a reason for hiding this comment

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

the scope already gets forked earlier

Comment on lines +64 to +74
Copy link

Choose a reason for hiding this comment

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

Bug: The created isolation scope is not activated with withIsolationScope(), causing metadata loss during error handling.
Severity: CRITICAL | Confidence: High

🔍 Detailed Analysis

An isolation scope is created and enriched with metadata at the beginning of wrapServerComponentWithSentry, but the subsequent execution of the server component's callback is not wrapped with withIsolationScope(). As a result, the error handler retrieves the current ambient isolation scope instead of the specifically created and enriched one, leading to a loss of context and metadata during error processing.

💡 Suggested Fix

Wrap the execution of the server component's callback within withIsolationScope(isolationScope, () => { ... }) to ensure the correct scope is active during error handling.

🤖 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/wrapServerComponentWithSentry.ts#L29-L74

Potential issue: An isolation scope is created and enriched with metadata at the
beginning of `wrapServerComponentWithSentry`, but the subsequent execution of the server
component's callback is not wrapped with `withIsolationScope()`. As a result, the error
handler retrieves the current ambient isolation scope instead of the specifically
created and enriched one, leading to a loss of context and metadata during error
processing.

Did we get this right? 👍 / 👎 to inform future reviews.
Reference ID: 5790254

},
});
}
Loading
Loading