-
-
Notifications
You must be signed in to change notification settings - Fork 1.7k
fix(nextjs): Avoid sending http.client transactions for Sentry fetch requests on edge & dev env #16772
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
Conversation
size-limit report 📦
|
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: Event Processor Registration Fails on Early SDK Initialization
The change from getGlobalScope().addEventProcessor()
to client?.addEventProcessor()
prevents critical event processors from being registered when the client
is undefined. This occurs when the SDK's init()
function returns early, such as when isBuild()
or sdkAlreadyInitialized()
is true. Consequently, essential functionality like transaction filtering (e.g., for static assets, Sentry requests, 404s) and error handling (e.g., React control flow errors like postpone/suspense) is bypassed, leading to unwanted data being sent to Sentry or important errors not being filtered.
packages/nextjs/src/server/index.ts#L206-L378
sentry-javascript/packages/nextjs/src/server/index.ts
Lines 206 to 378 in 188a8ff
client?.addEventProcessor( | |
Object.assign( | |
(event => { | |
if (event.type === 'transaction') { | |
// Filter out transactions for static assets | |
// This regex matches the default path to the static assets (`_next/static`) and could potentially filter out too many transactions. | |
// We match `/_next/static/` anywhere in the transaction name because its location may change with the basePath setting. | |
if (event.transaction?.match(/^GET (\/.*)?\/_next\/static\//)) { | |
return null; | |
} | |
// Filter out transactions for requests to the tunnel route | |
if ( | |
(globalWithInjectedValues._sentryRewritesTunnelPath && | |
event.transaction === `POST ${globalWithInjectedValues._sentryRewritesTunnelPath}`) || | |
(process.env._sentryRewritesTunnelPath && | |
event.transaction === `POST ${process.env._sentryRewritesTunnelPath}`) | |
) { | |
return null; | |
} | |
// Filter out requests to resolve source maps for stack frames in dev mode | |
if (event.transaction?.match(/\/__nextjs_original-stack-frame/)) { | |
return null; | |
} | |
// Filter out /404 transactions which seem to be created excessively | |
if ( | |
// Pages router | |
event.transaction === '/404' || | |
// App router (could be "GET /404", "POST /404", ...) | |
event.transaction?.match(/^(GET|HEAD|POST|PUT|DELETE|CONNECT|OPTIONS|TRACE|PATCH) \/(404|_not-found)$/) | |
) { | |
return null; | |
} | |
// Filter transactions that we explicitly want to drop. | |
if (event.contexts?.trace?.data?.[TRANSACTION_ATTR_SHOULD_DROP_TRANSACTION]) { | |
return null; | |
} | |
// Next.js 13 sometimes names the root transactions like this containing useless tracing. | |
if (event.transaction === 'NextServer.getRequestHandler') { | |
return null; | |
} | |
// Next.js 13 is not correctly picking up tracing data for trace propagation so we use a back-fill strategy | |
if (typeof event.contexts?.trace?.data?.[TRANSACTION_ATTR_SENTRY_TRACE_BACKFILL] === 'string') { | |
const traceparentData = extractTraceparentData( | |
event.contexts.trace.data[TRANSACTION_ATTR_SENTRY_TRACE_BACKFILL], | |
); | |
if (traceparentData?.parentSampled === false) { | |
return null; | |
} | |
} | |
// On Edge Runtime, fetch requests can leak to the Node runtime fetch instrumentation | |
// and end up being sent as transactions to Sentry. | |
// We filter them here based on URL | |
if (event.contexts?.trace?.data?.[SEMANTIC_ATTRIBUTE_SENTRY_OP] === 'http.client') { | |
const url = event.contexts?.trace?.data?.['url']; | |
if (url && isSentryRequestUrl(url, client)) { | |
return null; | |
} | |
} | |
return event; | |
} else { | |
return event; | |
} | |
}) satisfies EventProcessor, | |
{ id: 'NextLowQualityTransactionsFilter' }, | |
), | |
); | |
client?.addEventProcessor( | |
Object.assign( | |
((event, hint) => { | |
if (event.type !== undefined) { | |
return event; | |
} | |
const originalException = hint.originalException; | |
const isPostponeError = | |
typeof originalException === 'object' && | |
originalException !== null && | |
'$$typeof' in originalException && | |
originalException.$$typeof === Symbol.for('react.postpone'); | |
if (isPostponeError) { | |
// Postpone errors are used for partial-pre-rendering (PPR) | |
return null; | |
} | |
// We don't want to capture suspense errors as they are simply used by React/Next.js for control flow | |
const exceptionMessage = event.exception?.values?.[0]?.value; | |
if ( | |
exceptionMessage?.includes('Suspense Exception: This is not a real error!') || | |
exceptionMessage?.includes('Suspense Exception: This is not a real error, and should not leak') | |
) { | |
return null; | |
} | |
return event; | |
}) satisfies EventProcessor, | |
{ id: 'DropReactControlFlowErrors' }, | |
), | |
); | |
// Use the preprocessEvent hook instead of an event processor, so that the users event processors receive the most | |
// up-to-date value, but also so that the logic that detects changes to the transaction names to set the source to | |
// "custom", doesn't trigger. | |
client?.on('preprocessEvent', event => { | |
// Enhance route handler transactions | |
if ( | |
event.type === 'transaction' && | |
event.contexts?.trace?.data?.['next.span_type'] === 'BaseServer.handleRequest' | |
) { | |
event.contexts.trace.data[SEMANTIC_ATTRIBUTE_SENTRY_OP] = 'http.server'; | |
event.contexts.trace.op = 'http.server'; | |
if (event.transaction) { | |
event.transaction = stripUrlQueryAndFragment(event.transaction); | |
} | |
// eslint-disable-next-line deprecation/deprecation | |
const method = event.contexts.trace.data[SEMATTRS_HTTP_METHOD]; | |
// eslint-disable-next-line deprecation/deprecation | |
const target = event.contexts?.trace?.data?.[SEMATTRS_HTTP_TARGET]; | |
const route = event.contexts.trace.data[ATTR_HTTP_ROUTE] || event.contexts.trace.data['next.route']; | |
if (typeof method === 'string' && typeof route === 'string') { | |
const cleanRoute = route.replace(/\/route$/, ''); | |
event.transaction = `${method} ${cleanRoute}`; | |
event.contexts.trace.data[SEMANTIC_ATTRIBUTE_SENTRY_SOURCE] = 'route'; | |
// Preserve next.route in case it did not get hoisted | |
event.contexts.trace.data['next.route'] = cleanRoute; | |
} | |
// backfill transaction name for pages that would otherwise contain unparameterized routes | |
if (event.contexts.trace.data[TRANSACTION_ATTR_SENTRY_ROUTE_BACKFILL] && event.transaction !== 'GET /_app') { | |
event.transaction = `${method} ${event.contexts.trace.data[TRANSACTION_ATTR_SENTRY_ROUTE_BACKFILL]}`; | |
} | |
// Next.js overrides transaction names for page loads that throw an error | |
// but we want to keep the original target name | |
if (event.transaction === 'GET /_error' && target) { | |
event.transaction = `${method ? `${method} ` : ''}${target}`; | |
} | |
} | |
// Next.js 13 is not correctly picking up tracing data for trace propagation so we use a back-fill strategy | |
if ( | |
event.type === 'transaction' && | |
typeof event.contexts?.trace?.data?.[TRANSACTION_ATTR_SENTRY_TRACE_BACKFILL] === 'string' | |
) { | |
const traceparentData = extractTraceparentData(event.contexts.trace.data[TRANSACTION_ATTR_SENTRY_TRACE_BACKFILL]); | |
if (traceparentData?.traceId) { | |
event.contexts.trace.trace_id = traceparentData.traceId; | |
} | |
if (traceparentData?.parentSpanId) { | |
event.contexts.trace.parent_span_id = traceparentData.parentSpanId; | |
} | |
} | |
}); | |
if (process.env.NODE_ENV === 'development') { | |
client?.addEventProcessor(devErrorSymbolicationEventProcessor); |
Was this report helpful? Give feedback by reacting with 👍 or 👎
return null; | ||
} | ||
} | ||
|
||
return event; | ||
} else { | ||
return event; |
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.
A bit unrelated to this PR, is there a reason why we are returning the event in an else
block?
Wouldn't it make more sense to remove the else
and return the event after the if
statement?
Update: This is not reliable enough, other fetch requests still leak to the node SDK, so closing this in favor of just warning about this. |
This should fix #16502, but we do not have a fix yet.
But tests show the problem....