From 3bf486c30f22bbadf458554debe1876e8d3f921b Mon Sep 17 00:00:00 2001 From: Katie Byers Date: Mon, 14 Nov 2022 23:00:11 -0800 Subject: [PATCH 1/7] add `excludeServerRoutes` option --- packages/nextjs/src/config/types.ts | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/packages/nextjs/src/config/types.ts b/packages/nextjs/src/config/types.ts index 6324b35fa36e..26279d19271c 100644 --- a/packages/nextjs/src/config/types.ts +++ b/packages/nextjs/src/config/types.ts @@ -59,6 +59,14 @@ export type UserSentryOptions = { // Automatically instrument Next.js data fetching methods and Next.js API routes autoInstrumentServerFunctions?: boolean; + + // 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. + excludeServerRoutes?: Array; }; export type NextConfigFunction = (phase: string, defaults: { defaultConfig: NextConfigObject }) => NextConfigObject; From 75c0035978820038cce8b3de4937b09e7906056e Mon Sep 17 00:00:00 2001 From: Katie Byers Date: Mon, 14 Nov 2022 23:04:24 -0800 Subject: [PATCH 2/7] use `excludeServerRoutes` in the proxy loader for skipping files --- packages/nextjs/src/config/loaders/proxyLoader.ts | 14 ++++++++++++-- packages/nextjs/src/config/webpack.ts | 6 +++++- 2 files changed, 17 insertions(+), 3 deletions(-) diff --git a/packages/nextjs/src/config/loaders/proxyLoader.ts b/packages/nextjs/src/config/loaders/proxyLoader.ts index 50e90317f223..c6bee0ec9f3a 100644 --- a/packages/nextjs/src/config/loaders/proxyLoader.ts +++ b/packages/nextjs/src/config/loaders/proxyLoader.ts @@ -1,4 +1,4 @@ -import { escapeStringForRegex, logger } from '@sentry/utils'; +import { escapeStringForRegex, logger, stringMatchesSomePattern } from '@sentry/utils'; import * as fs from 'fs'; import * as path from 'path'; @@ -8,6 +8,7 @@ import { LoaderThis } from './types'; type LoaderOptions = { pagesDir: string; pageExtensionRegex: string; + excludeServerRoutes: Array; }; /** @@ -17,7 +18,11 @@ type LoaderOptions = { */ export default async function proxyLoader(this: LoaderThis, userCode: string): Promise { // We know one or the other will be defined, depending on the version of webpack being used - const { pagesDir, pageExtensionRegex } = 'getOptions' in this ? this.getOptions() : this.query; + const { + pagesDir, + pageExtensionRegex, + excludeServerRoutes = [], + } = 'getOptions' in this ? this.getOptions() : this.query; // Get the parameterized route name from this page's filepath const parameterizedRoute = path @@ -34,6 +39,11 @@ export default async function proxyLoader(this: LoaderThis, userC // homepage), sub back in the root route .replace(/^$/, '/'); + // Skip explicitly-ignored pages + if (stringMatchesSomePattern(parameterizedRoute, excludeServerRoutes, true)) { + return userCode; + } + // We don't want to wrap twice (or infinitely), so in the proxy we add this query string onto references to the // wrapped file, so that we know that it's already been processed. (Adding this query string is also necessary to // convince webpack that it's a different file than the one it's in the middle of loading now, so that the originals diff --git a/packages/nextjs/src/config/webpack.ts b/packages/nextjs/src/config/webpack.ts index 83818aeb3cff..6b034f18dc80 100644 --- a/packages/nextjs/src/config/webpack.ts +++ b/packages/nextjs/src/config/webpack.ts @@ -91,7 +91,11 @@ export function constructWebpackConfigFunction( use: [ { loader: path.resolve(__dirname, 'loaders/proxyLoader.js'), - options: { pagesDir, pageExtensionRegex }, + options: { + pagesDir, + pageExtensionRegex, + excludeServerRoutes: userSentryOptions.excludeServerRoutes, + }, }, ], }); From cbdf3a1d10c8c0643f913c05d9bc28fbe2247762 Mon Sep 17 00:00:00 2001 From: Katie Byers Date: Mon, 14 Nov 2022 23:06:41 -0800 Subject: [PATCH 3/7] prevent `Sentry.init()` code from being injected into excluded routes --- packages/nextjs/src/config/webpack.ts | 17 +++++++++++++---- 1 file changed, 13 insertions(+), 4 deletions(-) diff --git a/packages/nextjs/src/config/webpack.ts b/packages/nextjs/src/config/webpack.ts index 6b034f18dc80..6aa675a28e13 100644 --- a/packages/nextjs/src/config/webpack.ts +++ b/packages/nextjs/src/config/webpack.ts @@ -1,6 +1,6 @@ /* eslint-disable max-lines */ import { getSentryRelease } from '@sentry/node'; -import { arrayify, dropUndefinedKeys, escapeStringForRegex, logger } from '@sentry/utils'; +import { arrayify, dropUndefinedKeys, escapeStringForRegex, logger, stringMatchesSomePattern } from '@sentry/utils'; import { default as SentryWebpackPlugin } from '@sentry/webpack-plugin'; import * as chalk from 'chalk'; import * as fs from 'fs'; @@ -139,7 +139,7 @@ export function constructWebpackConfigFunction( // will call the callback which will call `f` which will call `x.y`... and on and on. Theoretically this could also // be fixed by using `bind`, but this is way simpler.) const origEntryProperty = newConfig.entry; - newConfig.entry = async () => addSentryToEntryProperty(origEntryProperty, buildContext); + newConfig.entry = async () => addSentryToEntryProperty(origEntryProperty, buildContext, userSentryOptions); // Enable the Sentry plugin (which uploads source maps to Sentry when not in dev) by default if (shouldEnableWebpackPlugin(buildContext, userSentryOptions)) { @@ -252,6 +252,7 @@ function findTranspilationRules(rules: WebpackModuleRule[] | undefined, projectD async function addSentryToEntryProperty( currentEntryProperty: WebpackEntryProperty, buildContext: BuildContext, + userSentryOptions: UserSentryOptions, ): Promise { // The `entry` entry in a webpack config can be a string, array of strings, object, or function. By default, nextjs // sets it to an async function which returns the promise of an object of string arrays. Because we don't know whether @@ -272,7 +273,7 @@ async function addSentryToEntryProperty( // inject into all entry points which might contain user's code for (const entryPointName in newEntryProperty) { - if (shouldAddSentryToEntryPoint(entryPointName, isServer)) { + if (shouldAddSentryToEntryPoint(entryPointName, isServer, userSentryOptions.excludeServerRoutes)) { addFilesToExistingEntryPoint(newEntryProperty, entryPointName, filesToInject); } } @@ -381,13 +382,21 @@ function checkWebpackPluginOverrides( * * @param entryPointName The name of the entry point in question * @param isServer Whether or not this function is being called in the context of a server build + * @param excludeServerRoutes A list of excluded serverside entrypoints provided by the user * @returns `true` if sentry code should be injected, and `false` otherwise */ -function shouldAddSentryToEntryPoint(entryPointName: string, isServer: boolean): boolean { +function shouldAddSentryToEntryPoint( + entryPointName: string, + isServer: boolean, + excludeServerRoutes: Array = [], +): boolean { // On the server side, by default we inject the `Sentry.init()` code into every page (with a few exceptions). if (isServer) { const entryPointRoute = entryPointName.replace(/^pages/, ''); if ( + // User-specified pages to skip. (Note: For ease of use, `excludeServerRoutes` is specified in terms of routes, + // which don't have the `pages` prefix.) + stringMatchesSomePattern(entryPointRoute, excludeServerRoutes, true) || // All non-API pages contain both of these components, and we don't want to inject more than once, so as long as // we're doing the individual pages, it's fine to skip these. (Note: Even if a given user doesn't have either or // both of these in their `pages/` folder, they'll exist as entrypoints because nextjs will supply default From f962329a78f9bf4b69b47b93250029613dbca478 Mon Sep 17 00:00:00 2001 From: Katie Byers Date: Mon, 14 Nov 2022 23:07:17 -0800 Subject: [PATCH 4/7] prevent webpack plugin from injecting release file into excluded routes --- packages/nextjs/src/config/webpack.ts | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/packages/nextjs/src/config/webpack.ts b/packages/nextjs/src/config/webpack.ts index 6aa675a28e13..b8541ce1578f 100644 --- a/packages/nextjs/src/config/webpack.ts +++ b/packages/nextjs/src/config/webpack.ts @@ -475,7 +475,8 @@ export function getWebpackPluginOptions( configFile: hasSentryProperties ? 'sentry.properties' : undefined, stripPrefix: ['webpack://_N_E/'], urlPrefix, - entries: (entryPointName: string) => shouldAddSentryToEntryPoint(entryPointName, isServer), + entries: (entryPointName: string) => + shouldAddSentryToEntryPoint(entryPointName, isServer, userSentryOptions.excludeServerRoutes), release: getSentryRelease(buildId), dryRun: isDev, }); From 192e8dadc0419cc81f4f3710f4d0549860917e2c Mon Sep 17 00:00:00 2001 From: Katie Byers Date: Mon, 14 Nov 2022 23:07:33 -0800 Subject: [PATCH 5/7] log when a route has been excluded --- packages/nextjs/src/config/webpack.ts | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/packages/nextjs/src/config/webpack.ts b/packages/nextjs/src/config/webpack.ts index b8541ce1578f..45d64e9db857 100644 --- a/packages/nextjs/src/config/webpack.ts +++ b/packages/nextjs/src/config/webpack.ts @@ -275,6 +275,16 @@ async function addSentryToEntryProperty( for (const entryPointName in newEntryProperty) { if (shouldAddSentryToEntryPoint(entryPointName, isServer, userSentryOptions.excludeServerRoutes)) { addFilesToExistingEntryPoint(newEntryProperty, entryPointName, filesToInject); + } else { + if ( + isServer && + // If the user has asked to exclude pages, confirm for them that it's worked + userSentryOptions.excludeServerRoutes && + // We always skip these, so it's not worth telling the user that we've done so + !['pages/_app', 'pages/_document'].includes(entryPointName) + ) { + __DEBUG_BUILD__ && logger.log(`Skipping Sentry injection for ${entryPointName.replace(/^pages/, '')}`); + } } } From e3e30f62abd1644e43a7a6e951ba9c4091dc5316 Mon Sep 17 00:00:00 2001 From: Katie Byers Date: Mon, 14 Nov 2022 23:08:38 -0800 Subject: [PATCH 6/7] add integration tests --- .../nextjs/test/integration/next.config.js | 1 + .../test/integration/next10.config.template | 4 ++ .../test/integration/next11.config.template | 4 ++ .../excludedEndpoints/excludedWithRegExp.tsx | 6 ++ .../excludedEndpoints/excludedWithString.tsx | 6 ++ .../test/server/excludedApiEndpoints.js | 67 +++++++++++++++++++ 6 files changed, 88 insertions(+) create mode 100644 packages/nextjs/test/integration/pages/api/excludedEndpoints/excludedWithRegExp.tsx create mode 100644 packages/nextjs/test/integration/pages/api/excludedEndpoints/excludedWithString.tsx create mode 100644 packages/nextjs/test/integration/test/server/excludedApiEndpoints.js diff --git a/packages/nextjs/test/integration/next.config.js b/packages/nextjs/test/integration/next.config.js index 8992ed63d0e5..3f209311c332 100644 --- a/packages/nextjs/test/integration/next.config.js +++ b/packages/nextjs/test/integration/next.config.js @@ -9,6 +9,7 @@ const moduleExports = { // Suppress the warning message from `handleSourcemapHidingOptionWarning` in `src/config/webpack.ts` // TODO (v8): This can come out in v8, because this option will get a default value hideSourceMaps: false, + excludeServerRoutes: ['/api/excludedEndpoints/excludedWithString', /\/api\/excludedEndpoints\/excludedWithRegExp/], }, }; const SentryWebpackPluginOptions = { diff --git a/packages/nextjs/test/integration/next10.config.template b/packages/nextjs/test/integration/next10.config.template index 31c332cd25cd..6e55fa099a22 100644 --- a/packages/nextjs/test/integration/next10.config.template +++ b/packages/nextjs/test/integration/next10.config.template @@ -10,6 +10,10 @@ const moduleExports = { // Suppress the warning message from `handleSourcemapHidingOptionWarning` in `src/config/webpack.ts` // TODO (v8): This can come out in v8, because this option will get a default value hideSourceMaps: false, + excludeServerRoutes: [ + '/api/excludedEndpoints/excludedWithString', + /\/api\/excludedEndpoints\/excludedWithRegExp/, + ], }, }; diff --git a/packages/nextjs/test/integration/next11.config.template b/packages/nextjs/test/integration/next11.config.template index 6a7e849067b1..28aaba18639a 100644 --- a/packages/nextjs/test/integration/next11.config.template +++ b/packages/nextjs/test/integration/next11.config.template @@ -11,6 +11,10 @@ const moduleExports = { // Suppress the warning message from `handleSourcemapHidingOptionWarning` in `src/config/webpack.ts` // TODO (v8): This can come out in v8, because this option will get a default value hideSourceMaps: false, + excludeServerRoutes: [ + '/api/excludedEndpoints/excludedWithString', + /\/api\/excludedEndpoints\/excludedWithRegExp/, + ], }, }; diff --git a/packages/nextjs/test/integration/pages/api/excludedEndpoints/excludedWithRegExp.tsx b/packages/nextjs/test/integration/pages/api/excludedEndpoints/excludedWithRegExp.tsx new file mode 100644 index 000000000000..49099819c843 --- /dev/null +++ b/packages/nextjs/test/integration/pages/api/excludedEndpoints/excludedWithRegExp.tsx @@ -0,0 +1,6 @@ +// This file will test the `excludeServerRoutes` option when a route is provided as a RegExp. +const handler = async (): Promise => { + throw new Error('API Error'); +}; + +export default handler; diff --git a/packages/nextjs/test/integration/pages/api/excludedEndpoints/excludedWithString.tsx b/packages/nextjs/test/integration/pages/api/excludedEndpoints/excludedWithString.tsx new file mode 100644 index 000000000000..9e6bde70c490 --- /dev/null +++ b/packages/nextjs/test/integration/pages/api/excludedEndpoints/excludedWithString.tsx @@ -0,0 +1,6 @@ +// This file will test the `excludeServerRoutes` option when a route is provided as a string. +const handler = async (): Promise => { + throw new Error('API Error'); +}; + +export default handler; diff --git a/packages/nextjs/test/integration/test/server/excludedApiEndpoints.js b/packages/nextjs/test/integration/test/server/excludedApiEndpoints.js new file mode 100644 index 000000000000..db49bbbc57cf --- /dev/null +++ b/packages/nextjs/test/integration/test/server/excludedApiEndpoints.js @@ -0,0 +1,67 @@ +const assert = require('assert'); + +const { sleep } = require('../utils/common'); +const { getAsync, interceptEventRequest, interceptTracingRequest } = require('../utils/server'); + +module.exports = async ({ url: urlBase, argv }) => { + const regExpUrl = `${urlBase}/api/excludedEndpoints/excludedWithRegExp`; + const stringUrl = `${urlBase}/api/excludedEndpoints/excludedWithString`; + + const capturedRegExpErrorRequest = interceptEventRequest( + { + exception: { + values: [ + { + type: 'Error', + value: 'API Error', + }, + ], + }, + tags: { + runtime: 'node', + }, + request: { + url: regExpUrl, + method: 'GET', + }, + transaction: 'GET /api/excludedEndpoints/excludedWithRegExp', + }, + argv, + 'excluded API endpoint via RegExp', + ); + + const capturedStringErrorRequest = interceptEventRequest( + { + exception: { + values: [ + { + type: 'Error', + value: 'API Error', + }, + ], + }, + tags: { + runtime: 'node', + }, + request: { + url: regExpUrl, + method: 'GET', + }, + transaction: 'GET /api/excludedEndpoints/excludedWithString', + }, + argv, + 'excluded API endpoint via String', + ); + + await Promise.all([getAsync(regExpUrl), getAsync(stringUrl)]); + await sleep(250); + + assert.ok( + !capturedRegExpErrorRequest.isDone(), + 'Did intercept error request even though route should be excluded (RegExp)', + ); + assert.ok( + !capturedStringErrorRequest.isDone(), + 'Did intercept error request even though route should be excluded (String)', + ); +}; From a92e32f482033efbc04f47ae8dc626b27db8cb39 Mon Sep 17 00:00:00 2001 From: Katie Byers Date: Mon, 14 Nov 2022 23:16:19 -0800 Subject: [PATCH 7/7] add unit test for `excludeServerRoutes` --- .../webpack/constructWebpackConfig.test.ts | 25 +++++++++++++++++++ 1 file changed, 25 insertions(+) diff --git a/packages/nextjs/test/config/webpack/constructWebpackConfig.test.ts b/packages/nextjs/test/config/webpack/constructWebpackConfig.test.ts index 0646c00e0c9b..206050d56d38 100644 --- a/packages/nextjs/test/config/webpack/constructWebpackConfig.test.ts +++ b/packages/nextjs/test/config/webpack/constructWebpackConfig.test.ts @@ -241,5 +241,30 @@ describe('constructWebpackConfigFunction()', () => { simulatorBundle: './src/simulator/index.ts', }); }); + + it('does not inject into routes included in `excludeServerRoutes`', async () => { + const nextConfigWithExcludedRoutes = { + ...exportedNextConfig, + sentry: { + excludeServerRoutes: [/simulator/], + }, + }; + const finalWebpackConfig = await materializeFinalWebpackConfig({ + exportedNextConfig: nextConfigWithExcludedRoutes, + incomingWebpackConfig: serverWebpackConfig, + incomingWebpackBuildContext: serverBuildContext, + }); + + expect(finalWebpackConfig.entry).toEqual( + expect.objectContaining({ + 'pages/simulator/leaderboard': { + import: expect.not.arrayContaining([serverConfigFilePath]), + }, + 'pages/api/simulator/dogStats/[name]': { + import: expect.not.arrayContaining([serverConfigFilePath]), + }, + }), + ); + }); }); });