-
-
Notifications
You must be signed in to change notification settings - Fork 1.7k
feat(remix): Add parameterized transaction naming for routes #17951
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
base: develop
Are you sure you want to change the base?
Conversation
0460a75 to
8886ddc
Compare
node-overhead report 🧳Note: This is a synthetic benchmark with a minimal express app and does not necessarily reflect the real-world performance impact in an application.
|
| if: needs.job_build.outputs.changed_remix == 'true' || github.event_name != 'pull_request' | ||
| runs-on: ubuntu-24.04 | ||
| timeout-minutes: 10 | ||
| timeout-minutes: 15 |
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.
Why did this increase so much?
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.
After adding the new tests, some of them started randomly timing out slightly above the 10-minute threshold on CI. It can be 11-12. I just increased this much to be ready for when we add more tests.
|
|
||
| // No manifest available (legacy app without Vite plugin) | ||
| // Fall back to route ID for backward compatibility | ||
| return { name: routeId, source: 'route' }; |
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.
Shouldn't this be url here then? In the legacy case this will not be parameterized right?
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.
So this is still not an unparameterized url really, in the previous implementation we were using route for the routeId (e.g. routes/users.$userId.posts.$postId) as well, so while it's not a parameterized route by its format, it still contains the parameterized route info.
This PR adds parameterized routes for Remix SDK, similarly to the NextJS SDK
Resolves: #16690
Needs docs PR for the Vite plugin
Adds automatic parameterization of Remix route IDs for transaction naming. Converts internal Remix route file patterns to parameterized paths similar to the other SDKs (
routes/user.$id->/user/:id).sentryRemixVitePluginthat extracts route information from Remix builds and generates a manifest file containing route ID to parameterized route mappings.remixRouteParameterization.tswith regex-based route conversion that handles dynamic segments ($param→:param), splat routes ($→:*), and nested routes.performance.tsxandinstrumentServer.tsto use parameterized transaction names.Vite plugin is used like:
Updated all Remix E2E test applications to expect parameterized route names. Added new E2E test application
create-remix-app-v2-non-vitefor testing backward compatibility for non-Vite builds - these continue to useroutes/user.$idformat.Summary
_index.tsx/routes/_index/user.$id.tsx/user/123routes/user.$id/user/:idusers.$userId.posts.$postId.tsx/users/123/posts/456routes/users.$userId.posts.$postId/users/:userId/posts/:postId$.tsx(splat)/docs/guide/introroutes/$/:*docs.$.tsx(scoped splat)/docs/guide/introroutes/docs.$/docs/:*