Navigation Menu

Skip to content
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

feat(nextjs): Add excludeServerRoutes config option #6207

Merged
merged 7 commits into from Nov 15, 2022

Conversation

lobsterkatie
Copy link
Member

@lobsterkatie lobsterkatie commented Nov 15, 2022

Currently, in the nextjs SDK, we inject the user's Sentry.init() code (by way of their sentry.server.config.js file) into all serverside routes. This adds a new option to the sentry object in next.config.js which allows users to prevent specific routes from being instrumented in this way. In this option, excluded routes can be specified using either strings (which need to exactly match the route) or regexes.

Note: Heavily inspired by #6125. h/t to @lforst for his work there. Compared to that PR, this one allows non-API routes to be excluded and allows excluded pages to be specified as routes rather than filepaths. (Using routes a) obviates the need for users to add pages/ to the beginning of every entry, b) abstracts away the differences between windows and POSIX paths, and c) futureproofs users' config values against underlying changes to project file organization.)

Docs for this feature are being added in getsentry/sentry-docs#5789.

Fixes #6119.
Fixes #5964.
Fixes #5667.

@github-actions
Copy link
Contributor

github-actions bot commented Nov 15, 2022

size-limit report 📦

Path Size
@sentry/browser - ES5 CDN Bundle (gzipped + minified) 19.51 KB (+0.06% 🔺)
@sentry/browser - ES5 CDN Bundle (minified) 60.36 KB (+0.05% 🔺)
@sentry/browser - ES6 CDN Bundle (gzipped + minified) 18.17 KB (+0.08% 🔺)
@sentry/browser - ES6 CDN Bundle (minified) 53.71 KB (+0.03% 🔺)
@sentry/browser - Webpack (gzipped + minified) 19.9 KB (+0.04% 🔺)
@sentry/browser - Webpack (minified) 65.12 KB (+0.04% 🔺)
@sentry/react - Webpack (gzipped + minified) 19.93 KB (+0.07% 🔺)
@sentry/nextjs Client - Webpack (gzipped + minified) 45.89 KB (+0.03% 🔺)
@sentry/browser + @sentry/tracing - ES5 CDN Bundle (gzipped + minified) 26.33 KB (+0.03% 🔺)
@sentry/browser + @sentry/tracing - ES6 CDN Bundle (gzipped + minified) 24.74 KB (+0.06% 🔺)

@lobsterkatie lobsterkatie force-pushed the kmclb-nextjs-invert-serverside-injection-criteria branch from 78dc8b8 to c98a180 Compare November 15, 2022 08:02
@lobsterkatie lobsterkatie force-pushed the kmclb-nextjs-add-excludeServerPages-option branch from 4d699da to 172f933 Compare November 15, 2022 08:03
@lforst lforst self-requested a review November 15, 2022 09:02
Copy link
Member

@lforst lforst left a comment

Choose a reason for hiding this comment

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

LGTM. Left just one small thought regarding compatibility with Next.js 13's new app folder.

Comment on lines +63 to +68
// Exclude certain serverside API routes or pages from being instrumented with Sentry. This option takes an array of
// strings or regular expressions.
//
// NOTE: Pages should be specified as routes (`/animals` or `/api/animals/[animalType]/habitat`), not filepaths
// (`pages/animals/index.js` or `.\src\pages\api\animals\[animalType]\habitat.tsx`), and strings must be be a full,
// exact match.
Copy link
Member

Choose a reason for hiding this comment

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

m: Just something we should maybe think about: I am wondering if we should have users include "pages/" in the strings/regexes they provide here just to future-proof us against Next.js 13's app/ folder. However, the more I think about it it shouldn't matter, as the routes in the app/ folder will also just be routes like we have in the pages folder and I believe they can't collide - so we're probably good here.

Copy link
Member Author

Choose a reason for hiding this comment

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

It's a good question, but I think you're right, a route is a route and you can't have two with the same route path. I also think that it's better that we abstract away the underlying file structure. That way, as users do move their routes from pages/ to app/, they won't have to come and update this setting.

@lobsterkatie lobsterkatie force-pushed the kmclb-nextjs-invert-serverside-injection-criteria branch from c98a180 to d04f3bc Compare November 15, 2022 14:32
@lobsterkatie lobsterkatie force-pushed the kmclb-nextjs-add-excludeServerPages-option branch from 172f933 to 7ec84be Compare November 15, 2022 14:33
@lobsterkatie lobsterkatie force-pushed the kmclb-nextjs-invert-serverside-injection-criteria branch from d04f3bc to 054796b Compare November 15, 2022 14:43
@lobsterkatie lobsterkatie force-pushed the kmclb-nextjs-add-excludeServerPages-option branch from 7ec84be to 0e98d62 Compare November 15, 2022 14:44
@lobsterkatie lobsterkatie force-pushed the kmclb-nextjs-invert-serverside-injection-criteria branch from 054796b to 3eeff43 Compare November 15, 2022 16:07
Base automatically changed from kmclb-nextjs-invert-serverside-injection-criteria to master November 15, 2022 17:51
@lobsterkatie lobsterkatie force-pushed the kmclb-nextjs-add-excludeServerPages-option branch from 0e98d62 to a92e32f Compare November 15, 2022 17:53
@lobsterkatie lobsterkatie merged commit fd6ae0f into master Nov 15, 2022
@lobsterkatie lobsterkatie deleted the kmclb-nextjs-add-excludeServerPages-option branch November 15, 2022 18:53
@lobsterkatie lobsterkatie self-assigned this Nov 17, 2022
lobsterkatie added a commit to getsentry/sentry-docs that referenced this pull request Nov 17, 2022
…5789)

This adds the new `excludeServerRoutes` option in the nextjs SDK (added in getsentry/sentry-javascript#6207) to the docs. It also pulls all of the auto-instrumentation config into its own section in the manual setup page.

(Note: The only part of this which is actually new content is the `Opt Out of Auto-instrumentation on Specific Routes` section. Everything else is just stuff moving around.)
@zlwaterfield
Copy link

@lobsterkatie this doesn't seem to be working on the latest release.

 "@sentry/nextjs": "7.20.0",
 "@vercel/og": "^0.0.20",
 "next": "^13.0.0",

My api route is using the Vercel OG package with the experimental-edge runtime.

Screenshot 2022-11-19 at 10 02 41 PM

Screenshot 2022-11-19 at 10 02 49 PM

Screenshot 2022-11-19 at 10 02 58 PM

@lobsterkatie
Copy link
Member Author

@zlwaterfield - you've got the new option in your webpack plugin options, where as it needs to be in the sentry section of the main next options.

https://docs.sentry.io/platforms/javascript/guides/nextjs/manual-setup/#extend-nextjs-configuration
https://docs.sentry.io/platforms/javascript/guides/nextjs/manual-setup/#opt-out-of-auto-instrumentation-on-specific-routes

@zlwaterfield
Copy link

Wow I can't believe I missed that. Thank you for pointing that out. This PR is a life saver.

lforst pushed a commit to getsentry/sentry-docs that referenced this pull request Nov 22, 2022
…5789)

This adds the new `excludeServerRoutes` option in the nextjs SDK (added in getsentry/sentry-javascript#6207) to the docs. It also pulls all of the auto-instrumentation config into its own section in the manual setup page.

(Note: The only part of this which is actually new content is the `Opt Out of Auto-instrumentation on Specific Routes` section. Everything else is just stuff moving around.)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants