-
-
Notifications
You must be signed in to change notification settings - Fork 1.7k
feat(react-router): Ensure http.server route handling is consistent #16986
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
processEvent(event) { | ||
// Express generates bogus `*` routes for data loaders, which we want to remove here | ||
// we cannot do this earlier because some OTEL instrumentation adds this at some unexpected point | ||
if ( | ||
event.type === 'transaction' && | ||
event.contexts?.trace?.data && | ||
event.contexts.trace.data[ATTR_HTTP_ROUTE] === '*' && | ||
// This means the name has been adjusted before, but the http.route remains, so we need to remove it | ||
event.transaction !== 'GET *' && | ||
event.transaction !== 'POST *' | ||
) { | ||
// eslint-disable-next-line @typescript-eslint/no-dynamic-delete | ||
delete event.contexts.trace.data[ATTR_HTTP_ROUTE]; | ||
} | ||
|
||
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.
Don't we also need to update the tx name here?
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.
this is done already in the respective place via updateSpanName
, that forces a name over infernal. All we need to do here is remove the route attribute, as that can be misleading in product I think. (E2E tests should show this works xD)
927c18a
to
ef814f8
Compare
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: Undefined Data Access Causes Fallback Failure
A TypeError
can occur when accessing spanData.data[SEMATTRS_HTTP_TARGET]
if spanData.data
is undefined
. This prevents the intended fallback (e.g., url.pathname
) from being evaluated.
packages/react-router/src/server/wrapServerAction.ts#L47-L48
// eslint-disable-next-line deprecation/deprecation | |
const target = spanData.data[SEMATTRS_HTTP_TARGET]; |
packages/react-router/src/server/wrapServerLoader.ts#L48-L49
// eslint-disable-next-line deprecation/deprecation | |
const target = spanData.data[SEMATTRS_HTTP_TARGET]; |
packages/react-router/src/server/instrumentation/reactRouter.ts#L91-L92
sentry-javascript/packages/react-router/src/server/instrumentation/reactRouter.ts
Lines 91 to 92 in ef814f8
// eslint-disable-next-line deprecation/deprecation | |
const target = spanData.data[SEMATTRS_HTTP_TARGET] || url.pathname; |
Bug: Route Handling Bug in React Router Integration
The processEvent
method in reactRouterServerIntegration
incorrectly handles bogus *
routes. It only removes the http.route
attribute when the transaction name is not "GET *" or "POST *", failing to address cases where the transaction name remains "GET *" or "POST *". This leaves transactions with unhelpful *
names and http.route
attributes, contradicting the intended goal of removing them and missing a case previously handled by older logic that updated the transaction name.
packages/react-router/src/server/integration/reactRouterServer.ts#L32-L49
sentry-javascript/packages/react-router/src/server/integration/reactRouterServer.ts
Lines 32 to 49 in ef814f8
}, | |
processEvent(event) { | |
// Express generates bogus `*` routes for data loaders, which we want to remove here | |
// we cannot do this earlier because some OTEL instrumentation adds this at some unexpected point | |
if ( | |
event.type === 'transaction' && | |
event.contexts?.trace?.data && | |
event.contexts.trace.data[ATTR_HTTP_ROUTE] === '*' && | |
// This means the name has been adjusted before, but the http.route remains, so we need to remove it | |
event.transaction !== 'GET *' && | |
event.transaction !== 'POST *' | |
) { | |
// eslint-disable-next-line @typescript-eslint/no-dynamic-delete | |
delete event.contexts.trace.data[ATTR_HTTP_ROUTE]; | |
} | |
return event; | |
}, |
Was this report helpful? Give feedback by reacting with 👍 or 👎
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.
🚀
This updates react router http.server handling to ensure:
*
transaction name by just usingprocessEvent
in the integration. This means we now always add the server integration, it just does less in non-matching node versions.