feat(nextjs): Add `excludedServersideEntrypoints` option #6125

lforst · 2022-11-03T10:49:52Z

Resolves #6119

This PR adds a excludedServersideEntrypoints to our withSentryConfig Next.js config wrapper. It allows users to opt specific routes out of Sentry instrumentation, meaning the Sentry SDK won't be initialized for these routes.

The reason for this change is that we want to allow users to use different JavaScript runtimes for their Next.js routes. Up until now we just crashed the route.

The option takes an array containing strings or RegExes - strings match exactly, and RegExes match like they usually do.

AbhiPrasad · 2022-11-03T12:36:45Z

packages/nextjs/src/config/loaders/proxyLoader.ts

+  const relativePagePath = path
+    .join('pages', path.relative(pagesDir, this.resourcePath))
+    // Pull off the file extension
+    .replace(new RegExp(`\\.(${pageExtensionRegex})`), '');


m: do we need to worry about windows paths here?

Yes and that should be taken care of by this implementation.

Ok actually went back and made sure that the path we match against is posix because it is just way easier for users to configure that way

packages/nextjs/src/config/types.ts

Co-authored-by: Abhijeet Prasad <aprasad@sentry.io>

lobsterkatie

This approach looks good!

A few questions:

Are we meaning this to apply only to API routes, or to regular pages as well? I ask because as it stands, one couldn't exempt, say, pages/somePage. Your only option would be to block _app, which would turn the SDK off for all non-API pages besides _error. I think that if we are going to say it also applies to regular pages, we need to change our approach and (on the server side) insert the SDK into all page entrypoints (other than _app and _document) instead of just inserting it into _app.

If it really is for API routes only, should we reflect that reality in the option's name?

Also on the topic of naming (hardest problem in CS, right?): I have this dream that at some point we won't have to mess with entrypoints at all, and will do everything with our loaders. This makes me wonder if talking about entrypoints in the option name is too implementation-specific. Users think in terms of pages (or if this is really only for API routes, routes). Could we call it something like excludedServerSidePages or excludedAPIRoutes?

And one more thought - and then I'm done picking on the option name, I swear! - our naming convention for options tends to be verbNoun, not verbedNoun (see ignoreErrors, allowUrls, etc.) so regardless of any of the other questions, IMHO we should stick to that convention here as well.

(EDIT: Okay, I lied. One other thing I noticed is that in getServerSideProps, they capitalize the second S. Should we do that, too, just to be consistent?)

(Can you tell I used to have a side hustle as a copy editor? 😛)

lobsterkatie · 2022-11-04T08:03:52Z

packages/nextjs/src/config/loaders/proxyLoader.ts

@@ -34,11 +36,28 @@ export default async function proxyLoader(this: LoaderThis<LoaderOptions>, userC
    // homepage), sub back in the root route
    .replace(/^$/, '/');

+  // For the `excludedServersideEntrypoints` option we need the calculate the relative path to the file in question without file extension.


Suggested change

// For the `excludedServersideEntrypoints` option we need the calculate the relative path to the file in question without file extension.

// For the `excludedServersideEntrypoints` option we need to calculate the relative path to the file in question without file extension.

lobsterkatie · 2022-11-04T08:04:00Z

packages/nextjs/src/config/loaders/proxyLoader.ts

+  // For the `excludedServersideEntrypoints` option we need the calculate the relative path to the file in question without file extension.
+  const relativePosixPagePath = path
+    .join('pages', path.relative(pagesDir, this.resourcePath))
+    // Make sure that path is in posix style - this should make it easiser for users to configure and copy & paste from docs


Suggested change

// Make sure that path is in posix style - this should make it easiser for users to configure and copy & paste from docs

// Make sure that path is in posix style (using forward slashes) - this should make it easier for users to configure and will make docs examples work on all operating systems

lobsterkatie · 2022-11-04T08:04:25Z

packages/nextjs/src/config/loaders/proxyLoader.ts

+  const isExcluded = excludedServersideEntrypoints.some(exludeEntry => {
+    if (typeof exludeEntry === 'string') {
+      return relativePosixPagePath === exludeEntry;
+    } else {
+      return relativePosixPagePath.match(exludeEntry);
+    }
+  });


L: I don't immediately have a great suggestion here, but the fact that we're comparing the excluded paths to the current path rather than vice versa makes my brain hurt a little. (When I put it into words in my head, it feels more intuitive to say "if the current path matches any of the excluded paths" rather than "if any of the excluded paths match the current path.")

I honestly wouldn't know how to stop the brain-hurting here :D We only have an array and a string to work with - can we even reverse this logic?

lobsterkatie · 2022-11-04T08:04:49Z

packages/nextjs/src/config/types.ts

@@ -59,6 +59,15 @@ export type UserSentryOptions = {

  // Automatically instrument Next.js data fetching methods and Next.js API routes
  autoInstrumentServerFunctions?: boolean;
+
+  // Used to exclude certain serverside API routes or pages from being instrumented with Sentry. This option takes an


H: See above question re: if non-API pages should be part of this system.

packages/nextjs/src/config/types.ts

lobsterkatie · 2022-11-04T08:05:15Z

packages/nextjs/src/config/webpack.ts

+function shouldAddSentryToEntryPoint(
+  entryPointName: string,
+  isServer: boolean,
+  excludedServersideEntrypoints: (string | RegExp)[],


L: If we set a default value here, we don't have to make sure to include the ?? [] every time we call this function.

Suggested change

excludedServersideEntrypoints: (string | RegExp)[],

excludedServersideEntrypoints: (string | RegExp)[] = [],

packages/nextjs/src/config/webpack.ts

packages/nextjs/test/integration/test/server/excludedApiEndpoints.js

lobsterkatie · 2022-11-15T14:50:59Z

Closing this in favor of a combination of #6206 and #6207.

@lforst

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.

Add failing tests

09dc8bd

lforst added this to the NextJS Improvements milestone Nov 3, 2022

Add excludedServersideEntrypoints option

dbf7728

lforst changed the title ~~feat(nextjs): Add excludedServersideEntrypoints option allowing for opt-out of Sentry instrumentation~~ feat(nextjs): Add excludedServersideEntrypoints option Nov 3, 2022

Typo

480198b

lforst requested a review from lobsterkatie November 3, 2022 12:22

lforst marked this pull request as ready for review November 3, 2022 12:22

AbhiPrasad approved these changes Nov 3, 2022

View reviewed changes

lforst and others added 2 commits November 3, 2022 13:51

Update packages/nextjs/src/config/types.ts

509387f

Co-authored-by: Abhijeet Prasad <aprasad@sentry.io>

Convert path to posix before matching

a03a09e

lforst requested a review from AbhiPrasad November 3, 2022 13:46

smeubank mentioned this pull request Nov 3, 2022

Next.js middleware support #4206

Closed

2 tasks

lobsterkatie reviewed Nov 4, 2022

View reviewed changes

lforst added 2 commits November 4, 2022 15:35

Clean up include/exclude logic

63b713c

Make option optional

21d70c4

lforst mentioned this pull request Nov 14, 2022

Issues running @vercel/og when @sentry/nextjs is installed #5964

Closed

3 tasks

lobsterkatie mentioned this pull request Nov 15, 2022

feat(nextjs): Add excludeServerRoutes config option #6207

Merged

lobsterkatie closed this Nov 15, 2022

AbhiPrasad deleted the lforst-route-opt-out branch November 20, 2022 16:11

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(nextjs): Add `excludedServersideEntrypoints` option #6125

feat(nextjs): Add `excludedServersideEntrypoints` option #6125

lforst commented Nov 3, 2022 •

edited

AbhiPrasad Nov 3, 2022

lforst Nov 3, 2022

lforst Nov 3, 2022

lobsterkatie left a comment

lobsterkatie Nov 4, 2022

lobsterkatie Nov 4, 2022

lobsterkatie Nov 4, 2022

lforst Nov 4, 2022

lobsterkatie Nov 4, 2022

lobsterkatie Nov 4, 2022

lobsterkatie commented Nov 15, 2022

	// For the `excludedServersideEntrypoints` option we need the calculate the relative path to the file in question without file extension.
	// For the `excludedServersideEntrypoints` option we need to calculate the relative path to the file in question without file extension.

	// Make sure that path is in posix style - this should make it easiser for users to configure and copy & paste from docs
	// Make sure that path is in posix style (using forward slashes) - this should make it easier for users to configure and will make docs examples work on all operating systems

	excludedServersideEntrypoints: (string \| RegExp)[],
	excludedServersideEntrypoints: (string \| RegExp)[] = [],

feat(nextjs): Add excludedServersideEntrypoints option #6125

feat(nextjs): Add excludedServersideEntrypoints option #6125

Conversation

lforst commented Nov 3, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lobsterkatie left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lobsterkatie commented Nov 15, 2022

feat(nextjs): Add `excludedServersideEntrypoints` option #6125

feat(nextjs): Add `excludedServersideEntrypoints` option #6125

lforst commented Nov 3, 2022 •

edited