From 594e5ca33e72d7f321a1a0cf4186eddebab4b1c1 Mon Sep 17 00:00:00 2001 From: Katie Byers Date: Mon, 17 May 2021 11:26:17 -0700 Subject: [PATCH 01/17] feat(nextjs): Add server-side request transactions (#3533) --- packages/nextjs/package.json | 1 + packages/nextjs/src/utils/config.ts | 141 +++++++++++----- packages/nextjs/src/utils/instrumentServer.ts | 158 ++++++++++++++++-- packages/node/src/handlers.ts | 7 +- packages/node/src/index.ts | 1 + packages/node/src/utils.ts | 38 +++++ .../testDeepReadDirSync/cats/eddy.txt | 1 + .../testDeepReadDirSync/cats/persephone.txt | 1 + .../testDeepReadDirSync/cats/piper.txt | 1 + .../testDeepReadDirSync/cats/sassafras.txt | 1 + .../testDeepReadDirSync/cats/teaberry.txt | 1 + .../fixtures/testDeepReadDirSync/debra.txt | 1 + .../dogs/theBigs/charlie.txt | 1 + .../dogs/theBigs/maisey.txt | 1 + .../dogs/theSmalls/bodhi.txt | 1 + .../dogs/theSmalls/cory.txt | 1 + packages/node/test/utils.test.ts | 53 ++++++ 17 files changed, 352 insertions(+), 57 deletions(-) create mode 100644 packages/node/src/utils.ts create mode 100644 packages/node/test/fixtures/testDeepReadDirSync/cats/eddy.txt create mode 100644 packages/node/test/fixtures/testDeepReadDirSync/cats/persephone.txt create mode 100644 packages/node/test/fixtures/testDeepReadDirSync/cats/piper.txt create mode 100644 packages/node/test/fixtures/testDeepReadDirSync/cats/sassafras.txt create mode 100644 packages/node/test/fixtures/testDeepReadDirSync/cats/teaberry.txt create mode 100644 packages/node/test/fixtures/testDeepReadDirSync/debra.txt create mode 100644 packages/node/test/fixtures/testDeepReadDirSync/dogs/theBigs/charlie.txt create mode 100644 packages/node/test/fixtures/testDeepReadDirSync/dogs/theBigs/maisey.txt create mode 100644 packages/node/test/fixtures/testDeepReadDirSync/dogs/theSmalls/bodhi.txt create mode 100644 packages/node/test/fixtures/testDeepReadDirSync/dogs/theSmalls/cory.txt create mode 100644 packages/node/test/utils.test.ts diff --git a/packages/nextjs/package.json b/packages/nextjs/package.json index dfde398a18f2..37214026eeac 100644 --- a/packages/nextjs/package.json +++ b/packages/nextjs/package.json @@ -21,6 +21,7 @@ "@sentry/integrations": "6.3.6", "@sentry/node": "6.3.6", "@sentry/react": "6.3.6", + "@sentry/tracing": "6.3.6", "@sentry/utils": "6.3.6", "@sentry/webpack-plugin": "1.15.0", "tslib": "^1.9.3" diff --git a/packages/nextjs/src/utils/config.ts b/packages/nextjs/src/utils/config.ts index 767a038a8e26..de2d40343273 100644 --- a/packages/nextjs/src/utils/config.ts +++ b/packages/nextjs/src/utils/config.ts @@ -2,34 +2,46 @@ import { getSentryRelease } from '@sentry/node'; import { logger } from '@sentry/utils'; import defaultWebpackPlugin, { SentryCliPluginOptions } from '@sentry/webpack-plugin'; import * as SentryWebpackPlugin from '@sentry/webpack-plugin'; +import * as fs from 'fs'; +import * as path from 'path'; + +const SENTRY_CLIENT_CONFIG_FILE = './sentry.client.config.js'; +const SENTRY_SERVER_CONFIG_FILE = './sentry.server.config.js'; +// this is where the transpiled/bundled version of `USER_SERVER_CONFIG_FILE` will end up +export const SERVER_SDK_INIT_PATH = 'sentry/initServerSDK.js'; // eslint-disable-next-line @typescript-eslint/no-explicit-any type PlainObject = { [key: string]: T }; -// Man are these types hard to name well. "Entry" = an item in some collection of items, but in our case, one of the -// things we're worried about here is property (entry) in an object called... entry. So henceforth, the specific -// property we're modifying is going to be known as an EntryProperty. - // The function which is ultimately going to be exported from `next.config.js` under the name `webpack` type WebpackExport = (config: WebpackConfig, options: WebpackOptions) => WebpackConfig; // The two arguments passed to the exported `webpack` function, as well as the thing it returns -type WebpackConfig = { devtool: string; plugins: PlainObject[]; entry: EntryProperty }; +type WebpackConfig = { + devtool: string; + plugins: PlainObject[]; + entry: EntryProperty; + output: { path: string }; + target: string; + context: string; +}; type WebpackOptions = { dev: boolean; isServer: boolean; buildId: string }; // For our purposes, the value for `entry` is either an object, or a function which returns such an object type EntryProperty = (() => Promise) | EntryPropertyObject; - // Each value in that object is either a string representing a single entry point, an array of such strings, or an // object containing either of those, along with other configuration options. In that third case, the entry point(s) are // listed under the key `import`. -type EntryPropertyObject = PlainObject | EntryPointObject>; +type EntryPropertyObject = PlainObject | PlainObject> | PlainObject; type EntryPointObject = { import: string | Array }; -const sentryClientConfig = './sentry.client.config.js'; -const sentryServerConfig = './sentry.server.config.js'; - -/** Add a file (`injectee`) to a given element (`injectionPoint`) of the `entry` property */ +/** + * Add a file to a specific element of the given `entry` webpack config property. + * + * @param entryProperty The existing `entry` config object + * @param injectionPoint The key where the file should be injected + * @param injectee The path to the injected file + */ const _injectFile = (entryProperty: EntryPropertyObject, injectionPoint: string, injectee: string): void => { // can be a string, array of strings, or object whose `import` property is one of those two let injectedInto = entryProperty[injectionPoint]; @@ -41,21 +53,19 @@ const _injectFile = (entryProperty: EntryPropertyObject, injectionPoint: string, return; } - // In case we inject our client config, we need to add it after the frontend code - // otherwise the runtime config isn't loaded. See: https://github.com/getsentry/sentry-javascript/issues/3485 - const isClient = injectee === sentryClientConfig; - + // We inject the user's client config file after the existing code so that the config file has access to + // `publicRuntimeConfig`. See https://github.com/getsentry/sentry-javascript/issues/3485 if (typeof injectedInto === 'string') { - injectedInto = isClient ? [injectedInto, injectee] : [injectee, injectedInto]; + injectedInto = [injectedInto, injectee]; } else if (Array.isArray(injectedInto)) { - injectedInto = isClient ? [...injectedInto, injectee] : [injectee, ...injectedInto]; + injectedInto = [...injectedInto, injectee]; } else { - let importVal: string | string[] | EntryPointObject; + let importVal: string | string[]; + if (typeof injectedInto.import === 'string') { - importVal = isClient ? [injectedInto.import, injectee] : [injectee, injectedInto.import]; + importVal = [injectedInto.import, injectee]; } else { - // If it's not a string, the inner value is an array - importVal = isClient ? [...injectedInto.import, injectee] : [injectee, ...injectedInto.import]; + importVal = [...injectedInto.import, injectee]; } injectedInto = { @@ -68,8 +78,6 @@ const _injectFile = (entryProperty: EntryPropertyObject, injectionPoint: string, }; const injectSentry = async (origEntryProperty: EntryProperty, isServer: boolean): Promise => { - // Out of the box, nextjs uses the `() => Promise)` flavor of EntryProperty, where the returned - // object has string arrays for values. // 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 // someone else has come along before us and changed that, we need to check a few things along the way. The one thing @@ -80,19 +88,19 @@ const injectSentry = async (origEntryProperty: EntryProperty, isServer: boolean) newEntryProperty = await origEntryProperty(); } newEntryProperty = newEntryProperty as EntryPropertyObject; - // On the server, we need to inject the SDK into both into the base page (`_document`) and into individual API routes - // (which have no common base). + // Add a new element to the `entry` array, we force webpack to create a bundle out of the user's + // `sentry.server.config.js` file and output it to `SERVER_INIT_LOCATION`. (See + // https://webpack.js.org/guides/code-splitting/#entry-points.) We do this so that the user's config file is run + // through babel (and any other processors through which next runs the rest of the user-provided code - pages, API + // routes, etc.). Specifically, we need any ESM-style `import` code to get transpiled into ES5, so that we can call + // `require()` on the resulting file when we're instrumenting the sesrver. (We can't use a dynamic import there + // because that then forces the user into a particular TS config.) if (isServer) { - Object.keys(newEntryProperty).forEach(key => { - if (key === 'pages/_document' || key.includes('pages/api')) { - // for some reason, because we're now in a function, we have to cast again - _injectFile(newEntryProperty as EntryPropertyObject, key, sentryServerConfig); - } - }); + newEntryProperty[SERVER_SDK_INIT_PATH] = SENTRY_SERVER_CONFIG_FILE; } // On the client, it's sufficient to inject it into the `main` JS code, which is included in every browser page. else { - _injectFile(newEntryProperty, 'main', sentryClientConfig); + _injectFile(newEntryProperty, 'main', SENTRY_CLIENT_CONFIG_FILE); } // TODO: hack made necessary because the async-ness of this function turns our object back into a promise, meaning the // internal `next` code which should do this doesn't @@ -109,11 +117,18 @@ type NextConfigExports = { webpack?: WebpackExport; }; +/** + * Add Sentry options to the config to be exported from the user's `next.config.js` file. + * + * @param providedExports The existing config to be exported ,prior to adding Sentry + * @param providedSentryWebpackPluginOptions Configuration for SentryWebpackPlugin + * @returns The modified config to be exported + */ export function withSentryConfig( providedExports: NextConfigExports = {}, - providedWebpackPluginOptions: Partial = {}, + providedSentryWebpackPluginOptions: Partial = {}, ): NextConfigExports { - const defaultWebpackPluginOptions = { + const defaultSentryWebpackPluginOptions = { url: process.env.SENTRY_URL, org: process.env.SENTRY_ORG, project: process.env.SENTRY_PROJECT, @@ -122,22 +137,30 @@ export function withSentryConfig( stripPrefix: ['webpack://_N_E/'], urlPrefix: `~/_next`, include: '.next/', - ignore: ['node_modules', 'webpack.config.js'], + ignore: ['.next/cache', 'server/ssr-module-cache.js', 'static/*/_ssgManifest.js', 'static/*/_buildManifest.js'], }; // warn if any of the default options for the webpack plugin are getting overridden - const webpackPluginOptionOverrides = Object.keys(defaultWebpackPluginOptions) + const sentryWebpackPluginOptionOverrides = Object.keys(defaultSentryWebpackPluginOptions) .concat('dryrun') - .filter(key => key in Object.keys(providedWebpackPluginOptions)); - if (webpackPluginOptionOverrides.length > 0) { + .filter(key => key in Object.keys(providedSentryWebpackPluginOptions)); + if (sentryWebpackPluginOptionOverrides.length > 0) { logger.warn( '[Sentry] You are overriding the following automatically-set SentryWebpackPlugin config options:\n' + - `\t${webpackPluginOptionOverrides.toString()},\n` + + `\t${sentryWebpackPluginOptionOverrides.toString()},\n` + "which has the possibility of breaking source map upload and application. This is only a good idea if you know what you're doing.", ); } const newWebpackExport = (config: WebpackConfig, options: WebpackOptions): WebpackConfig => { + // if we're building server code, store the webpack output path as an env variable, so we know where to look for the + // webpack-processed version of `sentry.server.config.js` when we need it + if (config.target === 'node') { + const serverSDKInitOutputPath = path.join(config.output.path, SERVER_SDK_INIT_PATH); + const projectDir = config.context; + setRuntimeEnvVars(projectDir, { SENTRY_SERVER_INIT_PATH: serverSDKInitOutputPath }); + } + let newConfig = config; if (typeof providedExports.webpack === 'function') { @@ -161,8 +184,8 @@ export function withSentryConfig( new ((SentryWebpackPlugin as unknown) as typeof defaultWebpackPlugin)({ dryRun: options.dev, release: getSentryRelease(options.buildId), - ...defaultWebpackPluginOptions, - ...providedWebpackPluginOptions, + ...defaultSentryWebpackPluginOptions, + ...providedSentryWebpackPluginOptions, }), ); @@ -175,3 +198,39 @@ export function withSentryConfig( webpack: newWebpackExport, }; } + +/** + * Set variables to be added to the env at runtime, by storing them in `.env.local` (which `next` automatically reads + * into memory at server startup). + * + * @param projectDir The path to the project root + * @param vars Object containing vars to set + */ +function setRuntimeEnvVars(projectDir: string, vars: PlainObject): void { + // ensure the file exists + const envFilePath = path.join(projectDir, '.env.local'); + if (!fs.existsSync(envFilePath)) { + fs.writeFileSync(envFilePath, ''); + } + + let fileContents = fs + .readFileSync(envFilePath) + .toString() + .trim(); + + Object.entries(vars).forEach(entry => { + const [varName, value] = entry; + const envVarString = `${varName}=${value}`; + + // new entry + if (!fileContents.includes(varName)) { + fileContents = `${fileContents}\n${envVarString}`; + } + // existing entry; make sure value is up to date + else { + fileContents = fileContents.replace(new RegExp(`${varName}=\\S+`), envVarString); + } + }); + + fs.writeFileSync(envFilePath, `${fileContents.trim()}\n`); +} diff --git a/packages/nextjs/src/utils/instrumentServer.ts b/packages/nextjs/src/utils/instrumentServer.ts index 106a822d30ea..dbc818928976 100644 --- a/packages/nextjs/src/utils/instrumentServer.ts +++ b/packages/nextjs/src/utils/instrumentServer.ts @@ -1,4 +1,7 @@ -import { fill } from '@sentry/utils'; +import { deepReadDirSync } from '@sentry/node'; +import { hasTracingEnabled } from '@sentry/tracing'; +import { Transaction } from '@sentry/types'; +import { fill, logger } from '@sentry/utils'; import * as http from 'http'; import { default as createNextServer } from 'next'; import * as url from 'url'; @@ -8,43 +11,80 @@ import * as Sentry from '../index.server'; // eslint-disable-next-line @typescript-eslint/no-explicit-any type PlainObject = { [key: string]: T }; +// class used by `next` as a proxy to the real server; see +// https://github.com/vercel/next.js/blob/4443d6f3d36b107e833376c2720c1e206eee720d/packages/next/server/next.ts#L32 interface NextServer { server: Server; + createServer: (options: PlainObject) => Server; } +// `next`'s main server class; see +// https://github.com/vercel/next.js/blob/4443d6f3d36b107e833376c2720c1e206eee720d/packages/next/next-server/server/next-server.ts#L132 interface Server { dir: string; + publicDir: string; +} + +interface NextRequest extends http.IncomingMessage { + cookies: Record; + url: string; +} + +interface NextResponse extends http.ServerResponse { + __sentry__: { + transaction?: Transaction; + }; } type HandlerGetter = () => Promise; -type ReqHandler = ( - req: http.IncomingMessage, - res: http.ServerResponse, - parsedUrl?: url.UrlWithParsedQuery, -) => Promise; +type ReqHandler = (req: NextRequest, res: NextResponse, parsedUrl?: url.UrlWithParsedQuery) => Promise; type ErrorLogger = (err: Error) => void; // these aliases are purely to make the function signatures more easily understandable type WrappedHandlerGetter = HandlerGetter; type WrappedErrorLogger = ErrorLogger; +type WrappedReqHandler = ReqHandler; // TODO is it necessary for this to be an object? const closure: PlainObject = {}; +let sdkSetupComplete = false; + /** - * Do the monkeypatching and wrapping necessary to catch errors in page routes. Along the way, as a bonus, grab (and - * return) the path of the project root, for use in `RewriteFrames`. + * Do the monkeypatching and wrapping necessary to catch errors in page routes and record transactions for both page and + * API routes. Along the way, as a bonus, grab (and return) the path of the project root, for use in `RewriteFrames`. * * @returns The absolute path of the project root directory * */ export function instrumentServer(): string { - const nextServerPrototype = Object.getPrototypeOf(createNextServer({})); + // The full implementation here involves a lot of indirection and multiple layers of callbacks and wrapping, and is + // therefore potentially a little hard to follow. Here's the overall idea: - // wrap this getter because it runs before the request handler runs, which gives us a chance to wrap the logger before - // it's called for the first time + // Next.js uses two server classes, `NextServer` and `Server`, with the former proxying calls to the latter, which + // then does the all real work. The only access we have to either is through Next's default export, + // `createNextServer()`, which returns a `NextServer` instance. + + // At server startup: + // `next.config.js` imports SDK -> SDK index.ts -> `instrumentServer()` (the function we're in right now) -> + // `createNextServer()` -> `NextServer` instance -> `NextServer` prototype -> wrap + // `NextServer.getServerRequestHandler()`, purely to get us to the next step + + // At time of first request: + // Wrapped `getServerRequestHandler` runs for the first time -> live `NextServer` instance (via `this`) -> live + // `Server` instance -> `Server` prototype -> wrap `Server.logError` and `Server.handleRequest` methods, then pass + // wrapped version of `handleRequest` to caller of `getServerRequestHandler` + + // Whenever caller of `NextServer.getServerRequestHandler` calls the wrapped `Server.handleRequest`: + // Trace request + + // Whenever something calls the wrapped `Server.logError`: + // Capture error + + const nextServerPrototype = Object.getPrototypeOf(createNextServer({})); fill(nextServerPrototype, 'getServerRequestHandler', makeWrappedHandlerGetter); + // TODO replace with an env var, since at this point we don't have a value yet return closure.projectRootDir; } @@ -59,15 +99,32 @@ function makeWrappedHandlerGetter(origHandlerGetter: HandlerGetter): WrappedHand // We wrap this purely in order to be able to grab data and do further monkeypatching the first time it runs. // Otherwise, it's just a pass-through to the original method. const wrappedHandlerGetter = async function(this: NextServer): Promise { - if (!closure.wrappingComplete) { + if (!sdkSetupComplete) { + try { + // `SENTRY_SERVER_INIT_PATH` is set at build time, and points to a webpack-processed version of the user's + // `sentry.server.config.js`. Requiring it starts the SDK. + require(process.env.SENTRY_SERVER_INIT_PATH as string); + } catch (err) { + // Log the error but don't bail - we still want the wrapping to happen, in case the user is doing something weird + // and manually calling `Sentry.init()` somewhere else. + logger.error(`[Sentry] Could not initialize SDK. Received error:\n${err}`); + } + + // TODO: Replace projectRootDir with env variables closure.projectRootDir = this.server.dir; + closure.server = this.server; + closure.publicDir = this.server.publicDir; const serverPrototype = Object.getPrototypeOf(this.server); - // wrap the logger so we can capture errors in page-level functions like `getServerSideProps` + // wrap for error capturing (`logError` gets called by `next` for all server-side errors) fill(serverPrototype, 'logError', makeWrappedErrorLogger); - closure.wrappingComplete = true; + // wrap for request transaction creation (`handleRequest` is called for all incoming requests, and dispatches them + // to the appropriate handlers) + fill(serverPrototype, 'handleRequest', makeWrappedReqHandler); + + sdkSetupComplete = true; } return origHandlerGetter.call(this); @@ -89,3 +146,76 @@ function makeWrappedErrorLogger(origErrorLogger: ErrorLogger): WrappedErrorLogge return origErrorLogger.call(this, err); }; } + +/** + * Wrap the server's request handler to be able to create request transactions. + * + * @param origReqHandler The original request handler from the `Server` class + * @returns A wrapped version of that handler + */ +function makeWrappedReqHandler(origReqHandler: ReqHandler): WrappedReqHandler { + const liveServer = closure.server as Server; + + // inspired by next's public file routing; see + // https://github.com/vercel/next.js/blob/4443d6f3d36b107e833376c2720c1e206eee720d/packages/next/next-server/server/next-server.ts#L1166 + const publicDirFiles = new Set( + // we need the paths here to match the format of a request url, which means they must: + // - start with a slash + // - use forward slashes rather than backslashes + // - be URL-encoded + deepReadDirSync(liveServer.publicDir).map(filepath => encodeURI(`/${filepath.replace(/\\/g, '/')}`)), + ); + + // add transaction start and stop to the normal request handling + const wrappedReqHandler = async function( + this: Server, + req: NextRequest, + res: NextResponse, + parsedUrl?: url.UrlWithParsedQuery, + ): Promise { + // We only want to record page and API requests + if (hasTracingEnabled() && shouldTraceRequest(req.url, publicDirFiles)) { + const transaction = Sentry.startTransaction({ + name: `${(req.method || 'GET').toUpperCase()} ${req.url}`, + op: 'http.server', + }); + Sentry.getCurrentHub() + .getScope() + ?.setSpan(transaction); + + res.__sentry__ = {}; + res.__sentry__.transaction = transaction; + } + + res.once('finish', () => { + const transaction = res.__sentry__?.transaction; + if (transaction) { + // Push `transaction.finish` to the next event loop so open spans have a chance to finish before the transaction + // closes + setImmediate(() => { + // TODO + // addExpressReqToTransaction(transaction, req); + transaction.setHttpStatus(res.statusCode); + transaction.finish(); + }); + } + }); + + return origReqHandler.call(this, req, res, parsedUrl); + }; + + return wrappedReqHandler; +} + +/** + * Determine if the request should be traced, by filtering out requests for internal next files and static resources. + * + * @param url The URL of the request + * @param publicDirFiles A set containing relative paths to all available static resources (note that this does not + * include static *pages*, but rather images and the like) + * @returns false if the URL is for an internal or static resource + */ +function shouldTraceRequest(url: string, publicDirFiles: Set): boolean { + // `static` is a deprecated but still-functional location for static resources + return !url.startsWith('/_next/') && !url.startsWith('/static/') && !publicDirFiles.has(url); +} diff --git a/packages/node/src/handlers.ts b/packages/node/src/handlers.ts index 7496357851c2..9ad9bbe14d71 100644 --- a/packages/node/src/handlers.ts +++ b/packages/node/src/handlers.ts @@ -255,8 +255,11 @@ export function extractRequestData( if (method === 'GET' || method === 'HEAD') { break; } - // body data: - // node, express, koa: req.body + // body data: express, koa: req.body + + // when using node by itself, you have to read the incoming stream(see + // https://nodejs.dev/learn/get-http-request-body-data-using-nodejs); if a user is doing that, we can't know + // where they're going to store the final result, so they'll have to capture this data themselves if (req.body !== undefined) { requestData.data = isString(req.body) ? req.body : JSON.stringify(normalize(req.body)); } diff --git a/packages/node/src/index.ts b/packages/node/src/index.ts index f37b0abae2de..4204f944506c 100644 --- a/packages/node/src/index.ts +++ b/packages/node/src/index.ts @@ -41,6 +41,7 @@ export { export { NodeBackend, NodeOptions } from './backend'; export { NodeClient } from './client'; export { defaultIntegrations, init, lastEventId, flush, close, getSentryRelease } from './sdk'; +export { deepReadDirSync } from './utils'; export { SDK_NAME } from './version'; import { Integrations as CoreIntegrations } from '@sentry/core'; diff --git a/packages/node/src/utils.ts b/packages/node/src/utils.ts new file mode 100644 index 000000000000..87fb8f6157e3 --- /dev/null +++ b/packages/node/src/utils.ts @@ -0,0 +1,38 @@ +import * as fs from 'fs'; +import * as path from 'path'; + +/** + * Recursively read the contents of a directory. + * + * @param targetDir Absolute or relative path of the directory to scan. All returned paths will be relative to this + * directory. + * @returns Array holding all relative paths + */ +export function deepReadDirSync(targetDir: string): string[] { + const targetDirAbsPath = path.resolve(targetDir); + + if (!fs.existsSync(targetDirAbsPath)) { + throw new Error(`Cannot read contents of ${targetDirAbsPath}. Directory does not exist.`); + } + + if (!fs.statSync(targetDirAbsPath).isDirectory()) { + throw new Error(`Cannot read contents of ${targetDirAbsPath}, because it is not a directory.`); + } + + // This does the same thing as its containing function, `deepReadDirSync` (except that - purely for convenience - it + // deals in absolute paths rather than relative ones). We need this to be separate from the outer function to preserve + // the difference between `targetDirAbsPath` and `currentDirAbsPath`. + const deepReadCurrentDir = (currentDirAbsPath: string): string[] => { + return fs.readdirSync(currentDirAbsPath).reduce((absPaths: string[], itemName: string) => { + const itemAbsPath = path.join(currentDirAbsPath, itemName); + + if (fs.statSync(itemAbsPath).isDirectory()) { + return [...absPaths, ...deepReadCurrentDir(itemAbsPath)]; + } + + return [...absPaths, itemAbsPath]; + }, []); + }; + + return deepReadCurrentDir(targetDirAbsPath).map(absPath => path.relative(targetDirAbsPath, absPath)); +} diff --git a/packages/node/test/fixtures/testDeepReadDirSync/cats/eddy.txt b/packages/node/test/fixtures/testDeepReadDirSync/cats/eddy.txt new file mode 100644 index 000000000000..a34113db4801 --- /dev/null +++ b/packages/node/test/fixtures/testDeepReadDirSync/cats/eddy.txt @@ -0,0 +1 @@ +Lived to be almost 23. Named Eddy because she would sometimes just spin around and around and around, for no reason. diff --git a/packages/node/test/fixtures/testDeepReadDirSync/cats/persephone.txt b/packages/node/test/fixtures/testDeepReadDirSync/cats/persephone.txt new file mode 100644 index 000000000000..ef98cb18d7ba --- /dev/null +++ b/packages/node/test/fixtures/testDeepReadDirSync/cats/persephone.txt @@ -0,0 +1 @@ +Originally a stray. Adopted the humans rather than vice-versa. diff --git a/packages/node/test/fixtures/testDeepReadDirSync/cats/piper.txt b/packages/node/test/fixtures/testDeepReadDirSync/cats/piper.txt new file mode 100644 index 000000000000..0e3fa7aca948 --- /dev/null +++ b/packages/node/test/fixtures/testDeepReadDirSync/cats/piper.txt @@ -0,0 +1 @@ +A polydactyl with an enormous fluffy tail. diff --git a/packages/node/test/fixtures/testDeepReadDirSync/cats/sassafras.txt b/packages/node/test/fixtures/testDeepReadDirSync/cats/sassafras.txt new file mode 100644 index 000000000000..2fd44c1fba33 --- /dev/null +++ b/packages/node/test/fixtures/testDeepReadDirSync/cats/sassafras.txt @@ -0,0 +1 @@ +All black. Once ran away for two weeks, but eventually came back. diff --git a/packages/node/test/fixtures/testDeepReadDirSync/cats/teaberry.txt b/packages/node/test/fixtures/testDeepReadDirSync/cats/teaberry.txt new file mode 100644 index 000000000000..83e4df4bb879 --- /dev/null +++ b/packages/node/test/fixtures/testDeepReadDirSync/cats/teaberry.txt @@ -0,0 +1 @@ +Named by popular consensus, after the plant that makes wintergreen. diff --git a/packages/node/test/fixtures/testDeepReadDirSync/debra.txt b/packages/node/test/fixtures/testDeepReadDirSync/debra.txt new file mode 100644 index 000000000000..b7cd5d098fed --- /dev/null +++ b/packages/node/test/fixtures/testDeepReadDirSync/debra.txt @@ -0,0 +1 @@ +A black and white hooded rat, who loved to eat pizza crusts. diff --git a/packages/node/test/fixtures/testDeepReadDirSync/dogs/theBigs/charlie.txt b/packages/node/test/fixtures/testDeepReadDirSync/dogs/theBigs/charlie.txt new file mode 100644 index 000000000000..c51ee65682db --- /dev/null +++ b/packages/node/test/fixtures/testDeepReadDirSync/dogs/theBigs/charlie.txt @@ -0,0 +1 @@ +Named after the Charles River. A big dog who loves to play with tiny dogs. diff --git a/packages/node/test/fixtures/testDeepReadDirSync/dogs/theBigs/maisey.txt b/packages/node/test/fixtures/testDeepReadDirSync/dogs/theBigs/maisey.txt new file mode 100644 index 000000000000..29d690041354 --- /dev/null +++ b/packages/node/test/fixtures/testDeepReadDirSync/dogs/theBigs/maisey.txt @@ -0,0 +1 @@ +Has fluff between her toes. Slow to warm, but incredibly loyal thereafter. diff --git a/packages/node/test/fixtures/testDeepReadDirSync/dogs/theSmalls/bodhi.txt b/packages/node/test/fixtures/testDeepReadDirSync/dogs/theSmalls/bodhi.txt new file mode 100644 index 000000000000..e4b2ff5e6c4c --- /dev/null +++ b/packages/node/test/fixtures/testDeepReadDirSync/dogs/theSmalls/bodhi.txt @@ -0,0 +1 @@ +Loves to explore. Has spots on his tongue. diff --git a/packages/node/test/fixtures/testDeepReadDirSync/dogs/theSmalls/cory.txt b/packages/node/test/fixtures/testDeepReadDirSync/dogs/theSmalls/cory.txt new file mode 100644 index 000000000000..c581fffbf1e1 --- /dev/null +++ b/packages/node/test/fixtures/testDeepReadDirSync/dogs/theSmalls/cory.txt @@ -0,0 +1 @@ +Resembles a small sheep. Sneezes when playing with another dog. diff --git a/packages/node/test/utils.test.ts b/packages/node/test/utils.test.ts new file mode 100644 index 000000000000..73887aad7cd7 --- /dev/null +++ b/packages/node/test/utils.test.ts @@ -0,0 +1,53 @@ +import * as fs from 'fs'; +import * as os from 'os'; +import * as path from 'path'; + +import { deepReadDirSync } from '../src/utils'; + +// The parent directory for the new temporary directory + +describe('deepReadDirSync', () => { + it('handles nested files', () => { + // compare sets so that order doesn't matter + expect(new Set(deepReadDirSync('./test/fixtures/testDeepReadDirSync'))).toEqual( + new Set([ + // root level + 'debra.txt', + // one level deep + 'cats/eddy.txt', + 'cats/persephone.txt', + 'cats/piper.txt', + 'cats/sassafras.txt', + 'cats/teaberry.txt', + // two levels deep + 'dogs/theBigs/charlie.txt', + 'dogs/theBigs/maisey.txt', + 'dogs/theSmalls/bodhi.txt', + 'dogs/theSmalls/cory.txt', + ]), + ); + }); + + it('handles empty target directory', (done: (error?: Error) => void) => { + expect.assertions(1); + const tmpDir = os.tmpdir(); + + fs.mkdtemp(`${tmpDir}${path.sep}`, (err, dirPath) => { + if (err) throw err; + try { + expect(deepReadDirSync(dirPath)).toEqual([]); + done(); + } catch (error) { + done(error); + } + }); + }); + + it('errors if directory does not exist', () => { + expect(() => deepReadDirSync('./IDontExist')).toThrowError('Directory does not exist.'); + }); + + it('errors if given path is not a directory', () => { + expect(() => deepReadDirSync('package.json')).toThrowError('it is not a directory'); + }); +}); From b6b922a7f374457faeb71a70ddb9f000e3813f47 Mon Sep 17 00:00:00 2001 From: Katie Byers Date: Mon, 17 May 2021 15:59:11 -0700 Subject: [PATCH 02/17] add missing tracing dependency back in --- packages/nextjs/package.json | 1 + 1 file changed, 1 insertion(+) diff --git a/packages/nextjs/package.json b/packages/nextjs/package.json index 85add37c45c3..4fe51c899fea 100644 --- a/packages/nextjs/package.json +++ b/packages/nextjs/package.json @@ -21,6 +21,7 @@ "@sentry/integrations": "6.4.0", "@sentry/node": "6.4.0", "@sentry/react": "6.4.0", + "@sentry/tracing": "6.4.0", "@sentry/utils": "6.4.0", "@sentry/webpack-plugin": "1.15.0", "tslib": "^1.9.3" From 5d4dcab3c7c8e61096573a3728b127e85eaa409d Mon Sep 17 00:00:00 2001 From: iker barriocanal <32816711+iker-barriocanal@users.noreply.github.com> Date: Thu, 20 May 2021 16:31:05 +0200 Subject: [PATCH 03/17] feat(nextjs): Client performance monitoring (#3552) Adds front-end performance monitoring in the Next.js SDK, by providing a router instrumentation. --- packages/nextjs/src/index.client.ts | 1 + packages/nextjs/src/performance/client.ts | 115 +++++++++++++++++ .../nextjs/test/performance/client.test.ts | 117 ++++++++++++++++++ 3 files changed, 233 insertions(+) create mode 100644 packages/nextjs/src/performance/client.ts create mode 100644 packages/nextjs/test/performance/client.test.ts diff --git a/packages/nextjs/src/index.client.ts b/packages/nextjs/src/index.client.ts index 1c792523148c..db1aba8413fe 100644 --- a/packages/nextjs/src/index.client.ts +++ b/packages/nextjs/src/index.client.ts @@ -4,6 +4,7 @@ import { MetadataBuilder } from './utils/metadataBuilder'; import { NextjsOptions } from './utils/nextjsOptions'; export * from '@sentry/react'; +export { nextRouterInstrumentation } from './performance/client'; /** Inits the Sentry NextJS SDK on the browser with the React SDK. */ export function init(options: NextjsOptions): void { diff --git a/packages/nextjs/src/performance/client.ts b/packages/nextjs/src/performance/client.ts new file mode 100644 index 000000000000..e33e3243db47 --- /dev/null +++ b/packages/nextjs/src/performance/client.ts @@ -0,0 +1,115 @@ +import { Primitive, Transaction, TransactionContext } from '@sentry/types'; +import { fill, getGlobalObject } from '@sentry/utils'; +import { default as Router } from 'next/router'; + +const global = getGlobalObject(); + +type StartTransactionCb = (context: TransactionContext) => Transaction | undefined; + +const DEFAULT_TAGS = Object.freeze({ + 'routing.instrumentation': 'next-router', +}); + +const QUERY_PARAM_REGEX = /\?(.*)/; + +let activeTransaction: Transaction | undefined = undefined; +let prevTransactionName: string | undefined = undefined; +let startTransaction: StartTransactionCb | undefined = undefined; + +/** + * Creates routing instrumention for Next Router. Only supported for + * client side routing. Works for Next >= 10. + * + * Leverages the SingletonRouter from the `next/router` to + * generate pageload/navigation transactions and parameterize + * transaction names. + */ +export function nextRouterInstrumentation( + startTransactionCb: StartTransactionCb, + startTransactionOnPageLoad: boolean = true, + startTransactionOnLocationChange: boolean = true, +): void { + startTransaction = startTransactionCb; + Router.ready(() => { + // We can only start the pageload transaction when we have access to the parameterized + // route name. Setting the transaction name after the transaction is started could lead + // to possible race conditions with the router, so this approach was taken. + if (startTransactionOnPageLoad) { + prevTransactionName = Router.route !== null ? removeQueryParams(Router.route) : global.location.pathname; + activeTransaction = startTransactionCb({ + name: prevTransactionName, + op: 'pageload', + tags: DEFAULT_TAGS, + }); + } + + // Spans that aren't attached to any transaction are lost; so if transactions aren't + // created (besides potentially the onpageload transaction), no need to wrap the router. + if (!startTransactionOnLocationChange) return; + + // `withRouter` uses `useRouter` underneath: + // https://github.com/vercel/next.js/blob/de42719619ae69fbd88e445100f15701f6e1e100/packages/next/client/with-router.tsx#L21 + // Router events also use the router: + // https://github.com/vercel/next.js/blob/de42719619ae69fbd88e445100f15701f6e1e100/packages/next/client/router.ts#L92 + // `Router.changeState` handles the router state changes, so it may be enough to only wrap it + // (instead of wrapping all of the Router's functions). + const routerPrototype = Object.getPrototypeOf(Router.router); + fill(routerPrototype, 'changeState', changeStateWrapper); + }); +} + +type RouterChangeState = ( + method: string, + url: string, + as: string, + options: Record, + ...args: any[] +) => void; +type WrappedRouterChangeState = RouterChangeState; + +/** + * Wraps Router.changeState() + * https://github.com/vercel/next.js/blob/da97a18dafc7799e63aa7985adc95f213c2bf5f3/packages/next/next-server/lib/router/router.ts#L1204 + * Start a navigation transaction every time the router changes state. + */ +function changeStateWrapper(originalChangeStateWrapper: RouterChangeState): WrappedRouterChangeState { + const wrapper = function( + this: any, + method: string, + // The parameterized url, ex. posts/[id]/[comment] + url: string, + // The actual url, ex. posts/85/my-comment + as: string, + options: Record, + // At the moment there are no additional arguments (meaning the rest parameter is empty). + // This is meant to protect from future additions to Next.js API, especially since this is an + // internal API. + ...args: any[] + ): Promise { + if (startTransaction !== undefined) { + if (activeTransaction) { + activeTransaction.finish(); + } + const tags: Record = { + ...DEFAULT_TAGS, + method, + ...options, + }; + if (prevTransactionName) { + tags.from = prevTransactionName; + } + prevTransactionName = removeQueryParams(url); + activeTransaction = startTransaction({ + name: prevTransactionName, + op: 'navigation', + tags, + }); + } + return originalChangeStateWrapper.call(this, method, url, as, options, ...args); + }; + return wrapper; +} + +export function removeQueryParams(route: string): string { + return route.replace(QUERY_PARAM_REGEX, ''); +} diff --git a/packages/nextjs/test/performance/client.test.ts b/packages/nextjs/test/performance/client.test.ts new file mode 100644 index 000000000000..46ec3e60f051 --- /dev/null +++ b/packages/nextjs/test/performance/client.test.ts @@ -0,0 +1,117 @@ +import { default as Router } from 'next/router'; + +import { nextRouterInstrumentation, removeQueryParams } from '../../src/performance/client'; + +let readyCalled = false; +jest.mock('next/router', () => { + const router = {}; + Object.setPrototypeOf(router, { changeState: () => undefined }); + return { + default: { + router, + route: '/[user]/posts/[id]', + readyCallbacks: [], + ready(cb: () => void) { + readyCalled = true; + return cb(); + }, + }, + }; +}); + +type Table = Array<{ in: I; out: O }>; + +describe('client', () => { + describe('nextRouterInstrumentation', () => { + it('waits for Router.ready()', () => { + const mockStartTransaction = jest.fn(); + expect(readyCalled).toBe(false); + nextRouterInstrumentation(mockStartTransaction); + expect(readyCalled).toBe(true); + }); + + it('creates a pageload transaction', () => { + const mockStartTransaction = jest.fn(); + nextRouterInstrumentation(mockStartTransaction); + expect(mockStartTransaction).toHaveBeenCalledTimes(1); + expect(mockStartTransaction).toHaveBeenLastCalledWith({ + name: '/[user]/posts/[id]', + op: 'pageload', + tags: { + 'routing.instrumentation': 'next-router', + }, + }); + }); + + it('does not create a pageload transaction if option not given', () => { + const mockStartTransaction = jest.fn(); + nextRouterInstrumentation(mockStartTransaction, false); + expect(mockStartTransaction).toHaveBeenCalledTimes(0); + }); + + it('creates navigation transactions', () => { + const mockStartTransaction = jest.fn(); + nextRouterInstrumentation(mockStartTransaction, false); + expect(mockStartTransaction).toHaveBeenCalledTimes(0); + + const table: Table, Record> = [ + { + in: ['pushState', '/posts/[id]', '/posts/32', {}], + out: { + name: '/posts/[id]', + op: 'navigation', + tags: { + from: '/posts/[id]', + method: 'pushState', + 'routing.instrumentation': 'next-router', + }, + }, + }, + { + in: ['replaceState', '/posts/[id]?name=cat', '/posts/32?name=cat', {}], + out: { + name: '/posts/[id]', + op: 'navigation', + tags: { + from: '/posts/[id]', + method: 'replaceState', + 'routing.instrumentation': 'next-router', + }, + }, + }, + { + in: ['pushState', '/about', '/about', {}], + out: { + name: '/about', + op: 'navigation', + tags: { + from: '/about', + method: 'pushState', + 'routing.instrumentation': 'next-router', + }, + }, + }, + ]; + + table.forEach(test => { + // @ts-ignore changeState can be called with array spread + Router.router?.changeState(...test.in); + expect(mockStartTransaction).toHaveBeenLastCalledWith(test.out); + }); + }); + }); + + describe('removeQueryParams()', () => { + it('removes query params from an url', () => { + const table: Table = [ + { in: '/posts/[id]/[comment]?name=ferret&color=purple', out: '/posts/[id]/[comment]' }, + { in: '/posts/[id]/[comment]?', out: '/posts/[id]/[comment]' }, + { in: '/about?', out: '/about' }, + ]; + + table.forEach(test => { + expect(removeQueryParams(test.in)).toEqual(test.out); + }); + }); + }); +}); From da07ea7355b6d939db2d6170b5feec506f019988 Mon Sep 17 00:00:00 2001 From: Katie Byers Date: Thu, 20 May 2021 11:13:41 -0700 Subject: [PATCH 04/17] fix(nextjs): Use domains to prevent scope bleed on backend (#3574) --- packages/nextjs/src/utils/instrumentServer.ts | 61 +++++++++++-------- 1 file changed, 35 insertions(+), 26 deletions(-) diff --git a/packages/nextjs/src/utils/instrumentServer.ts b/packages/nextjs/src/utils/instrumentServer.ts index dbc818928976..ce097740e4b3 100644 --- a/packages/nextjs/src/utils/instrumentServer.ts +++ b/packages/nextjs/src/utils/instrumentServer.ts @@ -2,6 +2,7 @@ import { deepReadDirSync } from '@sentry/node'; import { hasTracingEnabled } from '@sentry/tracing'; import { Transaction } from '@sentry/types'; import { fill, logger } from '@sentry/utils'; +import * as domain from 'domain'; import * as http from 'http'; import { default as createNextServer } from 'next'; import * as url from 'url'; @@ -173,35 +174,43 @@ function makeWrappedReqHandler(origReqHandler: ReqHandler): WrappedReqHandler { res: NextResponse, parsedUrl?: url.UrlWithParsedQuery, ): Promise { - // We only want to record page and API requests - if (hasTracingEnabled() && shouldTraceRequest(req.url, publicDirFiles)) { - const transaction = Sentry.startTransaction({ - name: `${(req.method || 'GET').toUpperCase()} ${req.url}`, - op: 'http.server', - }); - Sentry.getCurrentHub() - .getScope() - ?.setSpan(transaction); - - res.__sentry__ = {}; - res.__sentry__.transaction = transaction; - } - - res.once('finish', () => { - const transaction = res.__sentry__?.transaction; - if (transaction) { - // Push `transaction.finish` to the next event loop so open spans have a chance to finish before the transaction - // closes - setImmediate(() => { - // TODO - // addExpressReqToTransaction(transaction, req); - transaction.setHttpStatus(res.statusCode); - transaction.finish(); + // wrap everything in a domain in order to prevent scope bleed between requests + const local = domain.create(); + local.add(req); + local.add(res); + // TODO could this replace wrapping the error logger? + // local.on('error', Sentry.captureException); + + local.run(() => { + // We only want to record page and API requests + if (hasTracingEnabled() && shouldTraceRequest(req.url, publicDirFiles)) { + const transaction = Sentry.startTransaction({ + name: `${(req.method || 'GET').toUpperCase()} ${req.url}`, + op: 'http.server', + }); + Sentry.getCurrentHub() + .getScope() + ?.setSpan(transaction); + + res.__sentry__ = { transaction }; + + res.once('finish', () => { + const transaction = res.__sentry__?.transaction; + if (transaction) { + // Push `transaction.finish` to the next event loop so open spans have a chance to finish before the transaction + // closes + setImmediate(() => { + // TODO + // addExpressReqToTransaction(transaction, req); + transaction.setHttpStatus(res.statusCode); + transaction.finish(); + }); + } }); + + return origReqHandler.call(this, req, res, parsedUrl); } }); - - return origReqHandler.call(this, req, res, parsedUrl); }; return wrappedReqHandler; From a0bab2faa62ba8bce4c5f6c95817f4b11ab9429e Mon Sep 17 00:00:00 2001 From: Katie Byers Date: Tue, 25 May 2021 06:34:32 -0700 Subject: [PATCH 05/17] feat(nextjs): Parameterize server-side transaction names and harvest request data (#3577) This PR does two things: 1) It wraps two more methods on `next`'s `Server` class, one of which is called on every API request, and the other of which is called on every page request. Both methods are passed a parameterized version of the path, which our wrappers grab and use to modify the name of the currently active transaction. 2) It adds an event processor which pulls data off of the request and adds it to the event. This runs whether or not tracing is enabled, in order to add data to both error and transaction events. The only other thing of minor interest is that the type for `Event.request.query_string` is expanded to include objects, which is okay according to the dev docs: https://develop.sentry.dev/sdk/event-payloads/request/#attribute). --- packages/nextjs/src/utils/instrumentServer.ts | 125 ++++++++++++++---- packages/types/src/index.ts | 2 +- packages/types/src/request.ts | 4 +- packages/types/src/transaction.ts | 3 + 4 files changed, 105 insertions(+), 29 deletions(-) diff --git a/packages/nextjs/src/utils/instrumentServer.ts b/packages/nextjs/src/utils/instrumentServer.ts index ce097740e4b3..127ce3e7b344 100644 --- a/packages/nextjs/src/utils/instrumentServer.ts +++ b/packages/nextjs/src/utils/instrumentServer.ts @@ -1,10 +1,11 @@ import { deepReadDirSync } from '@sentry/node'; -import { hasTracingEnabled } from '@sentry/tracing'; -import { Transaction } from '@sentry/types'; +import { getActiveTransaction, hasTracingEnabled } from '@sentry/tracing'; +import { Event as SentryEvent, Transaction } from '@sentry/types'; import { fill, logger } from '@sentry/utils'; import * as domain from 'domain'; import * as http from 'http'; import { default as createNextServer } from 'next'; +import * as querystring from 'querystring'; import * as url from 'url'; import * as Sentry from '../index.server'; @@ -29,6 +30,8 @@ interface Server { interface NextRequest extends http.IncomingMessage { cookies: Record; url: string; + query: { [key: string]: string }; + headers: { [key: string]: string }; } interface NextResponse extends http.ServerResponse { @@ -40,11 +43,19 @@ interface NextResponse extends http.ServerResponse { type HandlerGetter = () => Promise; type ReqHandler = (req: NextRequest, res: NextResponse, parsedUrl?: url.UrlWithParsedQuery) => Promise; type ErrorLogger = (err: Error) => void; +type ApiPageEnsurer = (path: string) => Promise; +type PageComponentFinder = ( + pathname: string, + query: querystring.ParsedUrlQuery, + params: { [key: string]: any } | null, +) => Promise<{ [key: string]: any } | null>; // these aliases are purely to make the function signatures more easily understandable type WrappedHandlerGetter = HandlerGetter; type WrappedErrorLogger = ErrorLogger; type WrappedReqHandler = ReqHandler; +type WrappedApiPageEnsurer = ApiPageEnsurer; +type WrappedPageComponentFinder = PageComponentFinder; // TODO is it necessary for this to be an object? const closure: PlainObject = {}; @@ -125,6 +136,12 @@ function makeWrappedHandlerGetter(origHandlerGetter: HandlerGetter): WrappedHand // to the appropriate handlers) fill(serverPrototype, 'handleRequest', makeWrappedReqHandler); + // Wrap as a way to grab the parameterized request URL to use as the transaction name for API requests and page + // requests, respectively. These methods are chosen because they're the first spot in the request-handling process + // where the parameterized path is provided as an argument, so it's easy to grab. + fill(serverPrototype, 'ensureApiPage', makeWrappedMethodForGettingParameterizedPath); + fill(serverPrototype, 'findPageComponents', makeWrappedMethodForGettingParameterizedPath); + sdkSetupComplete = true; } @@ -182,40 +199,80 @@ function makeWrappedReqHandler(origReqHandler: ReqHandler): WrappedReqHandler { // local.on('error', Sentry.captureException); local.run(() => { - // We only want to record page and API requests - if (hasTracingEnabled() && shouldTraceRequest(req.url, publicDirFiles)) { - const transaction = Sentry.startTransaction({ - name: `${(req.method || 'GET').toUpperCase()} ${req.url}`, - op: 'http.server', - }); - Sentry.getCurrentHub() - .getScope() - ?.setSpan(transaction); - - res.__sentry__ = { transaction }; - - res.once('finish', () => { - const transaction = res.__sentry__?.transaction; - if (transaction) { - // Push `transaction.finish` to the next event loop so open spans have a chance to finish before the transaction - // closes - setImmediate(() => { - // TODO - // addExpressReqToTransaction(transaction, req); + const currentScope = Sentry.getCurrentHub().getScope(); + + if (currentScope) { + currentScope.addEventProcessor(event => addRequestDataToEvent(event, req)); + + // We only want to record page and API requests + if (hasTracingEnabled() && shouldTraceRequest(req.url, publicDirFiles)) { + // pull off query string, if any + const reqPath = req.url.split('?')[0]; + + // requests for pages will only ever be GET requests, so don't bother to include the method in the transaction + // name; requests to API routes could be GET, POST, PUT, etc, so do include it there + const namePrefix = req.url.startsWith('/api') ? `${(req.method || 'GET').toUpperCase()} ` : ''; + + const transaction = Sentry.startTransaction({ + name: `${namePrefix}${reqPath}`, + op: 'http.server', + metadata: { requestPath: req.url.split('?')[0] }, + }); + + currentScope.setSpan(transaction); + + res.once('finish', () => { + const transaction = getActiveTransaction(); + if (transaction) { transaction.setHttpStatus(res.statusCode); - transaction.finish(); - }); - } - }); - return origReqHandler.call(this, req, res, parsedUrl); + // we'll collect this data in a more targeted way in the event processor we added above, + // `addRequestDataToEvent` + delete transaction.metadata.requestPath; + + // Push `transaction.finish` to the next event loop so open spans have a chance to finish before the + // transaction closes + setImmediate(() => { + transaction.finish(); + }); + } + }); + } } + + return origReqHandler.call(this, req, res, parsedUrl); }); }; return wrappedReqHandler; } +/** + * Wrap the given method in order to use the parameterized path passed to it in the transaction name. + * + * @param origMethod Either `ensureApiPage` (called for every API request) or `findPageComponents` (called for every + * page request), both from the `Server` class + * @returns A wrapped version of the given method + */ +function makeWrappedMethodForGettingParameterizedPath( + origMethod: ApiPageEnsurer | PageComponentFinder, +): WrappedApiPageEnsurer | WrappedPageComponentFinder { + // eslint-disable-next-line @typescript-eslint/no-explicit-any + const wrappedMethod = async function(this: Server, parameterizedPath: string, ...args: any[]): Promise { + const transaction = getActiveTransaction(); + + // replace specific URL with parameterized version + if (transaction && transaction.metadata.requestPath) { + const origPath = transaction.metadata.requestPath; + transaction.name = transaction.name.replace(origPath, parameterizedPath); + } + + return origMethod.call(this, parameterizedPath, ...args); + }; + + return wrappedMethod; +} + /** * Determine if the request should be traced, by filtering out requests for internal next files and static resources. * @@ -228,3 +285,17 @@ function shouldTraceRequest(url: string, publicDirFiles: Set): boolean { // `static` is a deprecated but still-functional location for static resources return !url.startsWith('/_next/') && !url.startsWith('/static/') && !publicDirFiles.has(url); } + +function addRequestDataToEvent(event: SentryEvent, req: NextRequest): SentryEvent { + event.request = { + ...event.request, + // TODO body/data + url: req.url.split('?')[0], + cookies: req.cookies, + headers: req.headers, + method: req.method, + query_string: req.query, + }; + + return event; +} diff --git a/packages/types/src/index.ts b/packages/types/src/index.ts index 0123fb42ec12..9674be34db9b 100644 --- a/packages/types/src/index.ts +++ b/packages/types/src/index.ts @@ -15,7 +15,7 @@ export { Mechanism } from './mechanism'; export { ExtractedNodeRequestData, Primitive, WorkerLocation } from './misc'; export { Options } from './options'; export { Package } from './package'; -export { Request, SentryRequest, SentryRequestType } from './request'; +export { QueryParams, Request, SentryRequest, SentryRequestType } from './request'; export { Response } from './response'; export { Runtime } from './runtime'; export { CaptureContext, Scope, ScopeContext } from './scope'; diff --git a/packages/types/src/request.ts b/packages/types/src/request.ts index 1995919d11f9..84b135fcfb3d 100644 --- a/packages/types/src/request.ts +++ b/packages/types/src/request.ts @@ -13,8 +13,10 @@ export interface Request { url?: string; method?: string; data?: any; - query_string?: string; + query_string?: QueryParams; cookies?: { [key: string]: string }; env?: { [key: string]: string }; headers?: { [key: string]: string }; } + +export type QueryParams = string | { [key: string]: string } | Array<[string, string]>; diff --git a/packages/types/src/transaction.ts b/packages/types/src/transaction.ts index f937c28bedc7..805d1350bff0 100644 --- a/packages/types/src/transaction.ts +++ b/packages/types/src/transaction.ts @@ -132,4 +132,7 @@ export interface TransactionMetadata { sentry?: string; thirdparty?: string; }; + + /** For transactions tracing server-side request handling, the path of the request being tracked. */ + requestPath?: string; } From 596f83a78805d0f4ee6f6ec61f91d54bf42a528b Mon Sep 17 00:00:00 2001 From: Katie Byers Date: Tue, 25 May 2021 07:23:31 -0700 Subject: [PATCH 06/17] chore(nextjs): Generalized server-side performance cleanup (#3581) Just a bunch of random housekeeping. - No longer return project root directory from `instrumentServer` since it's not yet defined at that point. - No longer store transaction on `res`, since our new use of domains means that `getActiveTransaction` is safe. As a result, change `NextResponse` from an interface to a type which is purely an alias. - Store a pointer to the live `Server` instance directly on the top scope rather than in an object, and get rid of the other stuff being stored in that object (`closure`) since it can all be gotten off of the aforementioned `Server` instance. - Fix/expand/tweak various docstrings and comments. - Improve log message when tracing an outgoing HTTP request. --- packages/nextjs/src/index.server.ts | 3 +- packages/nextjs/src/utils/instrumentServer.ts | 77 ++++++++++--------- packages/node/src/integrations/http.ts | 4 +- 3 files changed, 44 insertions(+), 40 deletions(-) diff --git a/packages/nextjs/src/index.server.ts b/packages/nextjs/src/index.server.ts index 365c6bfa82e7..c7b36642b0d7 100644 --- a/packages/nextjs/src/index.server.ts +++ b/packages/nextjs/src/index.server.ts @@ -17,6 +17,7 @@ export function init(options: NextjsOptions): void { const metadataBuilder = new MetadataBuilder(options, ['nextjs', 'node']); metadataBuilder.addSdkMetadata(); options.environment = options.environment || process.env.NODE_ENV; + // TODO capture project root and store in an env var for RewriteFrames? addServerIntegrations(options); // Right now we only capture frontend sessions for Next.js options.autoSessionTracking = false; @@ -47,5 +48,5 @@ function addServerIntegrations(options: NextjsOptions): void { export { withSentryConfig } from './utils/config'; export { withSentry } from './utils/handlers'; -// TODO capture project root (which this returns) for RewriteFrames? +// wrap various server methods to enable error monitoring and tracing instrumentServer(); diff --git a/packages/nextjs/src/utils/instrumentServer.ts b/packages/nextjs/src/utils/instrumentServer.ts index 127ce3e7b344..acdf9af234b5 100644 --- a/packages/nextjs/src/utils/instrumentServer.ts +++ b/packages/nextjs/src/utils/instrumentServer.ts @@ -1,6 +1,6 @@ import { deepReadDirSync } from '@sentry/node'; import { getActiveTransaction, hasTracingEnabled } from '@sentry/tracing'; -import { Event as SentryEvent, Transaction } from '@sentry/types'; +import { Event as SentryEvent } from '@sentry/types'; import { fill, logger } from '@sentry/utils'; import * as domain from 'domain'; import * as http from 'http'; @@ -33,13 +33,9 @@ interface NextRequest extends http.IncomingMessage { query: { [key: string]: string }; headers: { [key: string]: string }; } +type NextResponse = http.ServerResponse; -interface NextResponse extends http.ServerResponse { - __sentry__: { - transaction?: Transaction; - }; -} - +// the methods we'll wrap type HandlerGetter = () => Promise; type ReqHandler = (req: NextRequest, res: NextResponse, parsedUrl?: url.UrlWithParsedQuery) => Promise; type ErrorLogger = (err: Error) => void; @@ -47,8 +43,8 @@ type ApiPageEnsurer = (path: string) => Promise; type PageComponentFinder = ( pathname: string, query: querystring.ParsedUrlQuery, - params: { [key: string]: any } | null, -) => Promise<{ [key: string]: any } | null>; + params: { [key: string]: unknown } | null, +) => Promise<{ [key: string]: unknown } | null>; // these aliases are purely to make the function signatures more easily understandable type WrappedHandlerGetter = HandlerGetter; @@ -57,19 +53,14 @@ type WrappedReqHandler = ReqHandler; type WrappedApiPageEnsurer = ApiPageEnsurer; type WrappedPageComponentFinder = PageComponentFinder; -// TODO is it necessary for this to be an object? -const closure: PlainObject = {}; - +let liveServer: Server; let sdkSetupComplete = false; /** * Do the monkeypatching and wrapping necessary to catch errors in page routes and record transactions for both page and - * API routes. Along the way, as a bonus, grab (and return) the path of the project root, for use in `RewriteFrames`. - * - * @returns The absolute path of the project root directory - * + * API routes. */ -export function instrumentServer(): string { +export function instrumentServer(): void { // The full implementation here involves a lot of indirection and multiple layers of callbacks and wrapping, and is // therefore potentially a little hard to follow. Here's the overall idea: @@ -78,26 +69,34 @@ export function instrumentServer(): string { // `createNextServer()`, which returns a `NextServer` instance. // At server startup: - // `next.config.js` imports SDK -> SDK index.ts -> `instrumentServer()` (the function we're in right now) -> - // `createNextServer()` -> `NextServer` instance -> `NextServer` prototype -> wrap - // `NextServer.getServerRequestHandler()`, purely to get us to the next step + // `next.config.js` imports SDK -> + // SDK's `index.ts` runs -> + // `instrumentServer()` (the function we're in right now) -> + // `createNextServer()` -> + // `NextServer` instance -> + // `NextServer` prototype -> + // Wrap `NextServer.getServerRequestHandler()`, purely to get us to the next step // At time of first request: - // Wrapped `getServerRequestHandler` runs for the first time -> live `NextServer` instance (via `this`) -> live - // `Server` instance -> `Server` prototype -> wrap `Server.logError` and `Server.handleRequest` methods, then pass - // wrapped version of `handleRequest` to caller of `getServerRequestHandler` + // Wrapped `getServerRequestHandler` runs for the first time -> + // Live `NextServer` instance(via`this`) -> + // Live `Server` instance (via `NextServer.server`) -> + // `Server` prototype -> + // Wrap `Server.logError`, `Server.handleRequest`, `Server.ensureApiPage`, and `Server.findPageComponents` methods, + // then fulfill original purpose of function by passing wrapped version of `handleRequest` to caller // Whenever caller of `NextServer.getServerRequestHandler` calls the wrapped `Server.handleRequest`: - // Trace request + // Trace request // Whenever something calls the wrapped `Server.logError`: - // Capture error + // Capture error + + // Whenever an API request is handled and the wrapped `Server.ensureApiPage` is called, or whenever a page request is + // handled and the wrapped `Server.findPageComponents` is called: + // Replace URL in transaction name with parameterized version const nextServerPrototype = Object.getPrototypeOf(createNextServer({})); fill(nextServerPrototype, 'getServerRequestHandler', makeWrappedHandlerGetter); - - // TODO replace with an env var, since at this point we don't have a value yet - return closure.projectRootDir; } /** @@ -122,17 +121,14 @@ function makeWrappedHandlerGetter(origHandlerGetter: HandlerGetter): WrappedHand logger.error(`[Sentry] Could not initialize SDK. Received error:\n${err}`); } - // TODO: Replace projectRootDir with env variables - closure.projectRootDir = this.server.dir; - closure.server = this.server; - closure.publicDir = this.server.publicDir; - - const serverPrototype = Object.getPrototypeOf(this.server); + // stash this in the closure so that `makeWrappedReqHandler` can use it + liveServer = this.server; + const serverPrototype = Object.getPrototypeOf(liveServer); - // wrap for error capturing (`logError` gets called by `next` for all server-side errors) + // Wrap for error capturing (`logError` gets called by `next` for all server-side errors) fill(serverPrototype, 'logError', makeWrappedErrorLogger); - // wrap for request transaction creation (`handleRequest` is called for all incoming requests, and dispatches them + // Wrap for request transaction creation (`handleRequest` is called for all incoming requests, and dispatches them // to the appropriate handlers) fill(serverPrototype, 'handleRequest', makeWrappedReqHandler); @@ -172,8 +168,6 @@ function makeWrappedErrorLogger(origErrorLogger: ErrorLogger): WrappedErrorLogge * @returns A wrapped version of that handler */ function makeWrappedReqHandler(origReqHandler: ReqHandler): WrappedReqHandler { - const liveServer = closure.server as Server; - // inspired by next's public file routing; see // https://github.com/vercel/next.js/blob/4443d6f3d36b107e833376c2720c1e206eee720d/packages/next/next-server/server/next-server.ts#L1166 const publicDirFiles = new Set( @@ -286,6 +280,13 @@ function shouldTraceRequest(url: string, publicDirFiles: Set): boolean { return !url.startsWith('/_next/') && !url.startsWith('/static/') && !publicDirFiles.has(url); } +/** + * Harvest specific data from the request, and add it to the event. + * + * @param event The event to which to add request data + * @param req The request whose data is being added + * @returns The modified event + */ function addRequestDataToEvent(event: SentryEvent, req: NextRequest): SentryEvent { event.request = { ...event.request, diff --git a/packages/node/src/integrations/http.ts b/packages/node/src/integrations/http.ts index e331f69b109f..77e72228ee66 100644 --- a/packages/node/src/integrations/http.ts +++ b/packages/node/src/integrations/http.ts @@ -116,7 +116,9 @@ function _createWrappedRequestMethodFactory( }); const sentryTraceHeader = span.toTraceparent(); - logger.log(`[Tracing] Adding sentry-trace header to outgoing request: ${sentryTraceHeader}`); + logger.log( + `[Tracing] Adding sentry-trace header ${sentryTraceHeader} to outgoing request to ${requestUrl}: `, + ); requestOptions.headers = { ...requestOptions.headers, 'sentry-trace': sentryTraceHeader }; } } From 1bee1f31b2a59dfc1b200bbd4124c3466577c151 Mon Sep 17 00:00:00 2001 From: Katie Byers Date: Tue, 25 May 2021 07:51:09 -0700 Subject: [PATCH 07/17] feat(nextjs): Continue trace using incoming `sentry-trace` header (#3587) Adds support for continuing a trace started in another service, by reading and applying the `sentry-trace` header on incoming requests. It also passes the request as extra context in the `tracesSampler`. --- packages/nextjs/src/utils/instrumentServer.ts | 26 ++++++++++++++----- packages/node/src/handlers.ts | 1 + 2 files changed, 20 insertions(+), 7 deletions(-) diff --git a/packages/nextjs/src/utils/instrumentServer.ts b/packages/nextjs/src/utils/instrumentServer.ts index acdf9af234b5..d1effd5003ee 100644 --- a/packages/nextjs/src/utils/instrumentServer.ts +++ b/packages/nextjs/src/utils/instrumentServer.ts @@ -1,7 +1,7 @@ import { deepReadDirSync } from '@sentry/node'; -import { getActiveTransaction, hasTracingEnabled } from '@sentry/tracing'; +import { extractTraceparentData, getActiveTransaction, hasTracingEnabled } from '@sentry/tracing'; import { Event as SentryEvent } from '@sentry/types'; -import { fill, logger } from '@sentry/utils'; +import { fill, isString, logger } from '@sentry/utils'; import * as domain from 'domain'; import * as http from 'http'; import { default as createNextServer } from 'next'; @@ -200,6 +200,13 @@ function makeWrappedReqHandler(origReqHandler: ReqHandler): WrappedReqHandler { // We only want to record page and API requests if (hasTracingEnabled() && shouldTraceRequest(req.url, publicDirFiles)) { + // If there is a trace header set, extract the data from it (parentSpanId, traceId, and sampling decision) + let traceparentData; + if (req.headers && isString(req.headers['sentry-trace'])) { + traceparentData = extractTraceparentData(req.headers['sentry-trace'] as string); + logger.log(`[Tracing] Continuing trace ${traceparentData?.traceId}.`); + } + // pull off query string, if any const reqPath = req.url.split('?')[0]; @@ -207,11 +214,16 @@ function makeWrappedReqHandler(origReqHandler: ReqHandler): WrappedReqHandler { // name; requests to API routes could be GET, POST, PUT, etc, so do include it there const namePrefix = req.url.startsWith('/api') ? `${(req.method || 'GET').toUpperCase()} ` : ''; - const transaction = Sentry.startTransaction({ - name: `${namePrefix}${reqPath}`, - op: 'http.server', - metadata: { requestPath: req.url.split('?')[0] }, - }); + const transaction = Sentry.startTransaction( + { + name: `${namePrefix}${reqPath}`, + op: 'http.server', + metadata: { requestPath: req.url.split('?')[0] }, + ...traceparentData, + }, + // extra context passed to the `tracesSampler` + { request: req }, + ); currentScope.setSpan(transaction); diff --git a/packages/node/src/handlers.ts b/packages/node/src/handlers.ts index 361d16efa0c2..fec2a2846b05 100644 --- a/packages/node/src/handlers.ts +++ b/packages/node/src/handlers.ts @@ -65,6 +65,7 @@ export function tracingHandler(): ( op: 'http.server', ...traceparentData, }, + // extra context passed to the tracesSampler { request: extractRequestData(req) }, ); From 65aea8ab6bcb312f3b138da23d6c29a46889d791 Mon Sep 17 00:00:00 2001 From: Katie Byers Date: Tue, 25 May 2021 08:50:46 -0700 Subject: [PATCH 08/17] ref(nextjs): Improve webpack config merging (#3596) 1) Fix a bug which prevented the "don't overwrite these keys" warning from working correctly. 2) Restrict the warning to appearing when overwriting keys which actually have values. --- packages/nextjs/src/utils/config.ts | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/packages/nextjs/src/utils/config.ts b/packages/nextjs/src/utils/config.ts index de2d40343273..dd341bf3cc90 100644 --- a/packages/nextjs/src/utils/config.ts +++ b/packages/nextjs/src/utils/config.ts @@ -1,5 +1,5 @@ import { getSentryRelease } from '@sentry/node'; -import { logger } from '@sentry/utils'; +import { dropUndefinedKeys, logger } from '@sentry/utils'; import defaultWebpackPlugin, { SentryCliPluginOptions } from '@sentry/webpack-plugin'; import * as SentryWebpackPlugin from '@sentry/webpack-plugin'; import * as fs from 'fs'; @@ -83,11 +83,13 @@ const injectSentry = async (origEntryProperty: EntryProperty, isServer: boolean) // someone else has come along before us and changed that, we need to check a few things along the way. The one thing // we know is that it won't have gotten *simpler* in form, so we only need to worry about the object and function // options. See https://webpack.js.org/configuration/entry-context/#entry. + let newEntryProperty = origEntryProperty; if (typeof origEntryProperty === 'function') { newEntryProperty = await origEntryProperty(); } newEntryProperty = newEntryProperty as EntryPropertyObject; + // Add a new element to the `entry` array, we force webpack to create a bundle out of the user's // `sentry.server.config.js` file and output it to `SERVER_INIT_LOCATION`. (See // https://webpack.js.org/guides/code-splitting/#entry-points.) We do this so that the user's config file is run @@ -128,7 +130,7 @@ export function withSentryConfig( providedExports: NextConfigExports = {}, providedSentryWebpackPluginOptions: Partial = {}, ): NextConfigExports { - const defaultSentryWebpackPluginOptions = { + const defaultSentryWebpackPluginOptions = dropUndefinedKeys({ url: process.env.SENTRY_URL, org: process.env.SENTRY_ORG, project: process.env.SENTRY_PROJECT, @@ -138,12 +140,12 @@ export function withSentryConfig( urlPrefix: `~/_next`, include: '.next/', ignore: ['.next/cache', 'server/ssr-module-cache.js', 'static/*/_ssgManifest.js', 'static/*/_buildManifest.js'], - }; + }); // warn if any of the default options for the webpack plugin are getting overridden const sentryWebpackPluginOptionOverrides = Object.keys(defaultSentryWebpackPluginOptions) .concat('dryrun') - .filter(key => key in Object.keys(providedSentryWebpackPluginOptions)); + .filter(key => key in providedSentryWebpackPluginOptions); if (sentryWebpackPluginOptionOverrides.length > 0) { logger.warn( '[Sentry] You are overriding the following automatically-set SentryWebpackPlugin config options:\n' + From 4d37cd1af2a0fc448437c871d2a0bc834a473b71 Mon Sep 17 00:00:00 2001 From: iker barriocanal <32816711+iker-barriocanal@users.noreply.github.com> Date: Tue, 25 May 2021 18:24:21 +0200 Subject: [PATCH 09/17] feat(nextjs): Automatic performance monitoring (#3586) Automatically enable `BrowserTracing` if a traceSampler or tracesSample Rate is set. Co-authored-by: Abhijeet Prasad --- packages/nextjs/src/index.client.ts | 31 +++- packages/nextjs/src/utils/userIntegrations.ts | 66 ++++++++- packages/nextjs/test/index.client.test.ts | 137 ++++++++++++++++++ .../test/{ => utils}/userIntegrations.test.ts | 2 +- 4 files changed, 227 insertions(+), 9 deletions(-) create mode 100644 packages/nextjs/test/index.client.test.ts rename packages/nextjs/test/{ => utils}/userIntegrations.test.ts (95%) diff --git a/packages/nextjs/src/index.client.ts b/packages/nextjs/src/index.client.ts index db1aba8413fe..7a55b4d0bce2 100644 --- a/packages/nextjs/src/index.client.ts +++ b/packages/nextjs/src/index.client.ts @@ -1,18 +1,47 @@ import { configureScope, init as reactInit } from '@sentry/react'; +import { Integrations } from '@sentry/tracing'; +import { nextRouterInstrumentation } from './performance/client'; import { MetadataBuilder } from './utils/metadataBuilder'; import { NextjsOptions } from './utils/nextjsOptions'; +import { addIntegration, UserIntegrations } from './utils/userIntegrations'; export * from '@sentry/react'; export { nextRouterInstrumentation } from './performance/client'; +const { BrowserTracing } = Integrations; + /** Inits the Sentry NextJS SDK on the browser with the React SDK. */ export function init(options: NextjsOptions): void { const metadataBuilder = new MetadataBuilder(options, ['nextjs', 'react']); metadataBuilder.addSdkMetadata(); options.environment = options.environment || process.env.NODE_ENV; - reactInit(options); + + // Only add BrowserTracing if a tracesSampleRate or tracesSampler is set + const integrations = + options.tracesSampleRate === undefined && options.tracesSampler === undefined + ? options.integrations + : createClientIntegrations(options.integrations); + + reactInit({ + ...options, + integrations, + }); configureScope(scope => { scope.setTag('runtime', 'browser'); }); } + +const defaultBrowserTracingIntegration = new BrowserTracing({ + routingInstrumentation: nextRouterInstrumentation, +}); + +function createClientIntegrations(integrations?: UserIntegrations): UserIntegrations { + if (integrations) { + return addIntegration(defaultBrowserTracingIntegration, integrations, { + BrowserTracing: { keyPath: 'options.routingInstrumentation', value: nextRouterInstrumentation }, + }); + } else { + return [defaultBrowserTracingIntegration]; + } +} diff --git a/packages/nextjs/src/utils/userIntegrations.ts b/packages/nextjs/src/utils/userIntegrations.ts index 006b5ebc8971..9f45e6c3d362 100644 --- a/packages/nextjs/src/utils/userIntegrations.ts +++ b/packages/nextjs/src/utils/userIntegrations.ts @@ -1,7 +1,36 @@ import { Integration } from '@sentry/types'; export type UserFunctionIntegrations = (integrations: Integration[]) => Integration[]; -type UserIntegrations = Integration[] | UserFunctionIntegrations; +export type UserIntegrations = Integration[] | UserFunctionIntegrations; + +type Options = { + [integrationName: string]: + | { + keyPath: string; + value: unknown; + } + | undefined; +}; + +/** + * Recursively traverses an object to update an existing nested key. + * Note: The provided key path must include existing properties, + * the function will not create objects while traversing. + * + * @param obj An object to update + * @param value The value to update the nested key with + * @param keyPath The path to the key to update ex. fizz.buzz.foo + */ +// eslint-disable-next-line @typescript-eslint/no-explicit-any +function setNestedKey(obj: Record, keyPath: string, value: unknown): void { + // Ex. foo.bar.zoop will extract foo and bar.zoop + const match = keyPath.match(/([a-z]+)\.(.*)/i); + if (match === null) { + obj[keyPath] = value; + } else { + setNestedKey(obj[match[1]], match[2], value); + } +} /** * Retrieves the patched integrations with the provided integration. @@ -12,18 +41,40 @@ type UserIntegrations = Integration[] | UserFunctionIntegrations; * * @param integration The integration to patch, if necessary. * @param userIntegrations Integrations defined by the user. + * @param options options to update for a particular integration * @returns Final integrations, patched if necessary. */ -export function addIntegration(integration: Integration, userIntegrations: UserIntegrations): UserIntegrations { +export function addIntegration( + integration: Integration, + userIntegrations: UserIntegrations, + options: Options = {}, +): UserIntegrations { if (Array.isArray(userIntegrations)) { - return addIntegrationToArray(integration, userIntegrations); + return addIntegrationToArray(integration, userIntegrations, options); } else { - return addIntegrationToFunction(integration, userIntegrations); + return addIntegrationToFunction(integration, userIntegrations, options); } } -function addIntegrationToArray(integration: Integration, userIntegrations: Integration[]): Integration[] { - if (userIntegrations.map(int => int.name).includes(integration.name)) { +function addIntegrationToArray( + integration: Integration, + userIntegrations: Integration[], + options: Options, +): Integration[] { + let includesName = false; + // eslint-disable-next-line @typescript-eslint/prefer-for-of + for (let x = 0; x < userIntegrations.length; x++) { + if (userIntegrations[x].name === integration.name) { + includesName = true; + } + + const op = options[userIntegrations[x].name]; + if (op) { + setNestedKey(userIntegrations[x], op.keyPath, op.value); + } + } + + if (includesName) { return userIntegrations; } return [...userIntegrations, integration]; @@ -32,10 +83,11 @@ function addIntegrationToArray(integration: Integration, userIntegrations: Integ function addIntegrationToFunction( integration: Integration, userIntegrationsFunc: UserFunctionIntegrations, + options: Options, ): UserFunctionIntegrations { const wrapper: UserFunctionIntegrations = defaultIntegrations => { const userFinalIntegrations = userIntegrationsFunc(defaultIntegrations); - return addIntegrationToArray(integration, userFinalIntegrations); + return addIntegrationToArray(integration, userFinalIntegrations, options); }; return wrapper; } diff --git a/packages/nextjs/test/index.client.test.ts b/packages/nextjs/test/index.client.test.ts new file mode 100644 index 000000000000..738ecc24dc89 --- /dev/null +++ b/packages/nextjs/test/index.client.test.ts @@ -0,0 +1,137 @@ +import { Integrations as TracingIntegrations } from '@sentry/tracing'; +import { Integration } from '@sentry/types'; + +import { init, Integrations, nextRouterInstrumentation, Scope } from '../src/index.client'; +import { NextjsOptions } from '../src/utils/nextjsOptions'; + +const { BrowserTracing } = TracingIntegrations; + +const mockInit = jest.fn(); +let configureScopeCallback: (scope: Scope) => void = () => undefined; + +jest.mock('@sentry/react', () => { + const actual = jest.requireActual('@sentry/react'); + return { + ...actual, + init: (options: NextjsOptions) => { + mockInit(options); + }, + configureScope: (callback: (scope: Scope) => void) => { + configureScopeCallback = callback; + }, + }; +}); + +describe('Client init()', () => { + afterEach(() => { + mockInit.mockClear(); + configureScopeCallback = () => undefined; + }); + + it('inits the React SDK', () => { + expect(mockInit).toHaveBeenCalledTimes(0); + init({}); + expect(mockInit).toHaveBeenCalledTimes(1); + expect(mockInit).toHaveBeenLastCalledWith({ + _metadata: { + sdk: { + name: 'sentry.javascript.nextjs', + version: expect.any(String), + packages: expect.any(Array), + }, + }, + environment: 'test', + integrations: undefined, + }); + }); + + it('sets runtime on scope', () => { + const mockScope = new Scope(); + init({}); + configureScopeCallback(mockScope); + // @ts-ignore need access to protected _tags attribute + expect(mockScope._tags).toEqual({ runtime: 'browser' }); + }); + + describe('integrations', () => { + it('does not add BrowserTracing integration by default if tracesSampleRate is not set', () => { + init({}); + + const reactInitOptions: NextjsOptions = mockInit.mock.calls[0][0]; + expect(reactInitOptions.integrations).toBeUndefined(); + }); + + it('adds BrowserTracing integration by default if tracesSampleRate is set', () => { + init({ tracesSampleRate: 1.0 }); + + const reactInitOptions: NextjsOptions = mockInit.mock.calls[0][0]; + expect(reactInitOptions.integrations).toHaveLength(1); + + const integrations = reactInitOptions.integrations as Integration[]; + expect(integrations[0]).toEqual(expect.any(BrowserTracing)); + // eslint-disable-next-line @typescript-eslint/unbound-method + expect((integrations[0] as InstanceType).options.routingInstrumentation).toEqual( + nextRouterInstrumentation, + ); + }); + + it('adds BrowserTracing integration by default if tracesSampler is set', () => { + init({ tracesSampler: () => true }); + + const reactInitOptions: NextjsOptions = mockInit.mock.calls[0][0]; + expect(reactInitOptions.integrations).toHaveLength(1); + + const integrations = reactInitOptions.integrations as Integration[]; + expect(integrations[0]).toEqual(expect.any(BrowserTracing)); + // eslint-disable-next-line @typescript-eslint/unbound-method + expect((integrations[0] as InstanceType).options.routingInstrumentation).toEqual( + nextRouterInstrumentation, + ); + }); + + it('supports passing integration through options', () => { + init({ tracesSampleRate: 1.0, integrations: [new Integrations.Breadcrumbs({ console: false })] }); + const reactInitOptions: NextjsOptions = mockInit.mock.calls[0][0]; + expect(reactInitOptions.integrations).toHaveLength(2); + + const integrations = reactInitOptions.integrations as Integration[]; + expect(integrations).toEqual([expect.any(Integrations.Breadcrumbs), expect.any(BrowserTracing)]); + }); + + it('uses custom BrowserTracing with array option with nextRouterInstrumentation', () => { + init({ + tracesSampleRate: 1.0, + integrations: [new BrowserTracing({ idleTimeout: 5000, startTransactionOnLocationChange: false })], + }); + + const reactInitOptions: NextjsOptions = mockInit.mock.calls[0][0]; + expect(reactInitOptions.integrations).toHaveLength(1); + const integrations = reactInitOptions.integrations as Integration[]; + expect((integrations[0] as InstanceType).options).toEqual( + expect.objectContaining({ + idleTimeout: 5000, + startTransactionOnLocationChange: false, + routingInstrumentation: nextRouterInstrumentation, + }), + ); + }); + + it('uses custom BrowserTracing with function option with nextRouterInstrumentation', () => { + init({ + tracesSampleRate: 1.0, + integrations: () => [new BrowserTracing({ idleTimeout: 5000, startTransactionOnLocationChange: false })], + }); + + const reactInitOptions: NextjsOptions = mockInit.mock.calls[0][0]; + const integrationFunc = reactInitOptions.integrations as () => Integration[]; + const integrations = integrationFunc(); + expect((integrations[0] as InstanceType).options).toEqual( + expect.objectContaining({ + idleTimeout: 5000, + startTransactionOnLocationChange: false, + routingInstrumentation: nextRouterInstrumentation, + }), + ); + }); + }); +}); diff --git a/packages/nextjs/test/userIntegrations.test.ts b/packages/nextjs/test/utils/userIntegrations.test.ts similarity index 95% rename from packages/nextjs/test/userIntegrations.test.ts rename to packages/nextjs/test/utils/userIntegrations.test.ts index 011143151a45..b6f603be5cd0 100644 --- a/packages/nextjs/test/userIntegrations.test.ts +++ b/packages/nextjs/test/utils/userIntegrations.test.ts @@ -1,7 +1,7 @@ import { RewriteFrames } from '@sentry/integrations'; import { Integration } from '@sentry/types'; -import { addIntegration, UserFunctionIntegrations } from '../src/utils/userIntegrations'; +import { addIntegration, UserFunctionIntegrations } from '../../src/utils/userIntegrations'; const testIntegration = new RewriteFrames(); From b6f9dc6931b67aaa9d2ef8a06c5dcc71787dc1a1 Mon Sep 17 00:00:00 2001 From: Katie Byers Date: Tue, 25 May 2021 11:16:11 -0700 Subject: [PATCH 10/17] fix(nextjs): Fix duplicated extension when compiling `sentry.server.config.js` (#3597) --- packages/nextjs/src/utils/config.ts | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/packages/nextjs/src/utils/config.ts b/packages/nextjs/src/utils/config.ts index dd341bf3cc90..a8842bce8d77 100644 --- a/packages/nextjs/src/utils/config.ts +++ b/packages/nextjs/src/utils/config.ts @@ -98,7 +98,9 @@ const injectSentry = async (origEntryProperty: EntryProperty, isServer: boolean) // `require()` on the resulting file when we're instrumenting the sesrver. (We can't use a dynamic import there // because that then forces the user into a particular TS config.) if (isServer) { - newEntryProperty[SERVER_SDK_INIT_PATH] = SENTRY_SERVER_CONFIG_FILE; + // slice off the final `.js` since webpack is going to add it back in for us, and we don't want to end up with + // `.js.js` as the extension + newEntryProperty[SERVER_SDK_INIT_PATH.slice(0, -3)] = SENTRY_SERVER_CONFIG_FILE; } // On the client, it's sufficient to inject it into the `main` JS code, which is included in every browser page. else { From 6afe3814d558082cd3af06acc89efb2073ce0f61 Mon Sep 17 00:00:00 2001 From: Abhijeet Prasad Date: Wed, 26 May 2021 07:03:06 -0400 Subject: [PATCH 11/17] ref(nextjs): Use common util to strip query params (#3598) Use the stripUrlQueryAndFragment function from @sentry/utils instead of using our implementation. --- packages/nextjs/src/performance/client.ts | 12 +++--------- packages/nextjs/src/utils/instrumentServer.ts | 6 +++--- packages/nextjs/test/performance/client.test.ts | 16 +--------------- 3 files changed, 7 insertions(+), 27 deletions(-) diff --git a/packages/nextjs/src/performance/client.ts b/packages/nextjs/src/performance/client.ts index e33e3243db47..b326781ba5fd 100644 --- a/packages/nextjs/src/performance/client.ts +++ b/packages/nextjs/src/performance/client.ts @@ -1,5 +1,5 @@ import { Primitive, Transaction, TransactionContext } from '@sentry/types'; -import { fill, getGlobalObject } from '@sentry/utils'; +import { fill, getGlobalObject, stripUrlQueryAndFragment } from '@sentry/utils'; import { default as Router } from 'next/router'; const global = getGlobalObject(); @@ -10,8 +10,6 @@ const DEFAULT_TAGS = Object.freeze({ 'routing.instrumentation': 'next-router', }); -const QUERY_PARAM_REGEX = /\?(.*)/; - let activeTransaction: Transaction | undefined = undefined; let prevTransactionName: string | undefined = undefined; let startTransaction: StartTransactionCb | undefined = undefined; @@ -35,7 +33,7 @@ export function nextRouterInstrumentation( // route name. Setting the transaction name after the transaction is started could lead // to possible race conditions with the router, so this approach was taken. if (startTransactionOnPageLoad) { - prevTransactionName = Router.route !== null ? removeQueryParams(Router.route) : global.location.pathname; + prevTransactionName = Router.route !== null ? stripUrlQueryAndFragment(Router.route) : global.location.pathname; activeTransaction = startTransactionCb({ name: prevTransactionName, op: 'pageload', @@ -98,7 +96,7 @@ function changeStateWrapper(originalChangeStateWrapper: RouterChangeState): Wrap if (prevTransactionName) { tags.from = prevTransactionName; } - prevTransactionName = removeQueryParams(url); + prevTransactionName = stripUrlQueryAndFragment(url); activeTransaction = startTransaction({ name: prevTransactionName, op: 'navigation', @@ -109,7 +107,3 @@ function changeStateWrapper(originalChangeStateWrapper: RouterChangeState): Wrap }; return wrapper; } - -export function removeQueryParams(route: string): string { - return route.replace(QUERY_PARAM_REGEX, ''); -} diff --git a/packages/nextjs/src/utils/instrumentServer.ts b/packages/nextjs/src/utils/instrumentServer.ts index d1effd5003ee..3514f436a010 100644 --- a/packages/nextjs/src/utils/instrumentServer.ts +++ b/packages/nextjs/src/utils/instrumentServer.ts @@ -1,7 +1,7 @@ import { deepReadDirSync } from '@sentry/node'; import { extractTraceparentData, getActiveTransaction, hasTracingEnabled } from '@sentry/tracing'; import { Event as SentryEvent } from '@sentry/types'; -import { fill, isString, logger } from '@sentry/utils'; +import { fill, isString, logger, stripUrlQueryAndFragment } from '@sentry/utils'; import * as domain from 'domain'; import * as http from 'http'; import { default as createNextServer } from 'next'; @@ -208,7 +208,7 @@ function makeWrappedReqHandler(origReqHandler: ReqHandler): WrappedReqHandler { } // pull off query string, if any - const reqPath = req.url.split('?')[0]; + const reqPath = stripUrlQueryAndFragment(req.url); // requests for pages will only ever be GET requests, so don't bother to include the method in the transaction // name; requests to API routes could be GET, POST, PUT, etc, so do include it there @@ -218,7 +218,7 @@ function makeWrappedReqHandler(origReqHandler: ReqHandler): WrappedReqHandler { { name: `${namePrefix}${reqPath}`, op: 'http.server', - metadata: { requestPath: req.url.split('?')[0] }, + metadata: { requestPath: reqPath }, ...traceparentData, }, // extra context passed to the `tracesSampler` diff --git a/packages/nextjs/test/performance/client.test.ts b/packages/nextjs/test/performance/client.test.ts index 46ec3e60f051..ac632349275c 100644 --- a/packages/nextjs/test/performance/client.test.ts +++ b/packages/nextjs/test/performance/client.test.ts @@ -1,6 +1,6 @@ import { default as Router } from 'next/router'; -import { nextRouterInstrumentation, removeQueryParams } from '../../src/performance/client'; +import { nextRouterInstrumentation } from '../../src/performance/client'; let readyCalled = false; jest.mock('next/router', () => { @@ -100,18 +100,4 @@ describe('client', () => { }); }); }); - - describe('removeQueryParams()', () => { - it('removes query params from an url', () => { - const table: Table = [ - { in: '/posts/[id]/[comment]?name=ferret&color=purple', out: '/posts/[id]/[comment]' }, - { in: '/posts/[id]/[comment]?', out: '/posts/[id]/[comment]' }, - { in: '/about?', out: '/about' }, - ]; - - table.forEach(test => { - expect(removeQueryParams(test.in)).toEqual(test.out); - }); - }); - }); }); From b3ffadd9ec3e9e9dcd581ffe6d88361d1eaac2d8 Mon Sep 17 00:00:00 2001 From: Abhijeet Prasad Date: Wed, 26 May 2021 14:23:09 -0400 Subject: [PATCH 12/17] fix(nextjs): Check if public dir exists (#3602) When instrumenting the nextjs server, we should check if the public directory exists. --- packages/nextjs/src/utils/instrumentServer.ts | 28 ++++++++++++------- 1 file changed, 18 insertions(+), 10 deletions(-) diff --git a/packages/nextjs/src/utils/instrumentServer.ts b/packages/nextjs/src/utils/instrumentServer.ts index 3514f436a010..87bc48ed6f45 100644 --- a/packages/nextjs/src/utils/instrumentServer.ts +++ b/packages/nextjs/src/utils/instrumentServer.ts @@ -161,6 +161,23 @@ function makeWrappedErrorLogger(origErrorLogger: ErrorLogger): WrappedErrorLogge }; } +// inspired by next's public file routing; see +// https://github.com/vercel/next.js/blob/4443d6f3d36b107e833376c2720c1e206eee720d/packages/next/next-server/server/next-server.ts#L1166 +function getPublicDirFiles(): Set { + try { + // we need the paths here to match the format of a request url, which means they must: + // - start with a slash + // - use forward slashes rather than backslashes + // - be URL-encoded + const dirContents = deepReadDirSync(liveServer.publicDir).map(filepath => + encodeURI(`/${filepath.replace(/\\/g, '/')}`), + ); + return new Set(dirContents); + } catch (_) { + return new Set(); + } +} + /** * Wrap the server's request handler to be able to create request transactions. * @@ -168,16 +185,7 @@ function makeWrappedErrorLogger(origErrorLogger: ErrorLogger): WrappedErrorLogge * @returns A wrapped version of that handler */ function makeWrappedReqHandler(origReqHandler: ReqHandler): WrappedReqHandler { - // inspired by next's public file routing; see - // https://github.com/vercel/next.js/blob/4443d6f3d36b107e833376c2720c1e206eee720d/packages/next/next-server/server/next-server.ts#L1166 - const publicDirFiles = new Set( - // we need the paths here to match the format of a request url, which means they must: - // - start with a slash - // - use forward slashes rather than backslashes - // - be URL-encoded - deepReadDirSync(liveServer.publicDir).map(filepath => encodeURI(`/${filepath.replace(/\\/g, '/')}`)), - ); - + const publicDirFiles = getPublicDirFiles(); // add transaction start and stop to the normal request handling const wrappedReqHandler = async function( this: Server, From 41b6ee1b2430c6954d73f975abc17b4b67024442 Mon Sep 17 00:00:00 2001 From: Daniel Griesser Date: Thu, 27 May 2021 11:52:05 +0200 Subject: [PATCH 13/17] feat: transactions in withSentry handler --- packages/nextjs/src/utils/handlers.ts | 56 +++++++++++++++++++++++++-- 1 file changed, 52 insertions(+), 4 deletions(-) diff --git a/packages/nextjs/src/utils/handlers.ts b/packages/nextjs/src/utils/handlers.ts index 8b4bdcf2ae0c..96b6677a17d6 100644 --- a/packages/nextjs/src/utils/handlers.ts +++ b/packages/nextjs/src/utils/handlers.ts @@ -1,7 +1,10 @@ -import { captureException, flush, Handlers, withScope } from '@sentry/node'; -import { addExceptionMechanism } from '@sentry/utils'; +import { captureException, flush, getCurrentHub, Handlers, startTransaction, withScope } from '@sentry/node'; +import { extractTraceparentData, getActiveTransaction, hasTracingEnabled } from '@sentry/tracing'; +import { addExceptionMechanism, isString, logger, stripUrlQueryAndFragment } from '@sentry/utils'; import { NextApiHandler } from 'next'; +import { addRequestDataToEvent, NextRequest } from './instrumentServer'; + const { parseRequest } = Handlers; // purely for clarity @@ -12,9 +15,43 @@ export const withSentry = (handler: NextApiHandler): WrappedNextApiHandler => { // eslint-disable-next-line @typescript-eslint/explicit-module-boundary-types return async (req, res) => { try { - // TODO: Start Transaction + const currentScope = getCurrentHub().getScope(); + + if (currentScope) { + currentScope.addEventProcessor(event => addRequestDataToEvent(event, req as NextRequest)); + + // We only want to record page and API requests + if (hasTracingEnabled()) { + // If there is a trace header set, extract the data from it (parentSpanId, traceId, and sampling decision) + let traceparentData; + if (req.headers && isString(req.headers['sentry-trace'])) { + traceparentData = extractTraceparentData(req.headers['sentry-trace'] as string); + logger.log(`[Tracing] Continuing trace ${traceparentData?.traceId}.`); + } + + const url = `${req.url}`; + // pull off query string, if any + const reqPath = stripUrlQueryAndFragment(url); + + // requests for pages will only ever be GET requests, so don't bother to include the method in the transaction + // name; requests to API routes could be GET, POST, PUT, etc, so do include it there + const namePrefix = `${(req.method || 'GET').toUpperCase()} `; + + const transaction = startTransaction( + { + name: `${namePrefix}${reqPath}`, + op: 'http.server', + metadata: { requestPath: reqPath }, + ...traceparentData, + }, + // extra context passed to the `tracesSampler` + { request: req }, + ); + currentScope.setSpan(transaction); + } + } + return await handler(req, res); // Call Handler - // TODO: Finish Transaction } catch (e) { withScope(scope => { scope.addEventProcessor(event => { @@ -27,6 +64,17 @@ export const withSentry = (handler: NextApiHandler): WrappedNextApiHandler => { }); throw e; } finally { + const transaction = getActiveTransaction(); + if (transaction) { + transaction.setHttpStatus(res.statusCode); + + // we'll collect this data in a more targeted way in the event processor we added above, + // `addRequestDataToEvent` + // eslint-disable-next-line @typescript-eslint/no-unsafe-member-access + delete transaction.metadata.requestPath; + + transaction.finish(); + } await flush(2000); } }; From 82318d21bb240bea5dcf2bf58f4983fcfcf89df2 Mon Sep 17 00:00:00 2001 From: Daniel Griesser Date: Thu, 27 May 2021 12:59:22 +0200 Subject: [PATCH 14/17] fix: Build --- packages/nextjs/src/utils/instrumentServer.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/packages/nextjs/src/utils/instrumentServer.ts b/packages/nextjs/src/utils/instrumentServer.ts index 87bc48ed6f45..a591147b3b82 100644 --- a/packages/nextjs/src/utils/instrumentServer.ts +++ b/packages/nextjs/src/utils/instrumentServer.ts @@ -27,7 +27,7 @@ interface Server { publicDir: string; } -interface NextRequest extends http.IncomingMessage { +export interface NextRequest extends http.IncomingMessage { cookies: Record; url: string; query: { [key: string]: string }; @@ -307,7 +307,7 @@ function shouldTraceRequest(url: string, publicDirFiles: Set): boolean { * @param req The request whose data is being added * @returns The modified event */ -function addRequestDataToEvent(event: SentryEvent, req: NextRequest): SentryEvent { +export function addRequestDataToEvent(event: SentryEvent, req: NextRequest): SentryEvent { event.request = { ...event.request, // TODO body/data From 11a7a263329d1c28647cc0adc304ceea23110712 Mon Sep 17 00:00:00 2001 From: Daniel Griesser Date: Thu, 27 May 2021 14:49:43 +0200 Subject: [PATCH 15/17] fix: Parameter urls for API requests --- packages/nextjs/src/utils/handlers.ts | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/packages/nextjs/src/utils/handlers.ts b/packages/nextjs/src/utils/handlers.ts index 96b6677a17d6..c8b61f08fded 100644 --- a/packages/nextjs/src/utils/handlers.ts +++ b/packages/nextjs/src/utils/handlers.ts @@ -31,7 +31,13 @@ export const withSentry = (handler: NextApiHandler): WrappedNextApiHandler => { const url = `${req.url}`; // pull off query string, if any - const reqPath = stripUrlQueryAndFragment(url); + let reqPath = stripUrlQueryAndFragment(url); + // Replace with placeholder + if (req.query) { + for (const [key, value] of Object.entries(req.query)) { + reqPath = reqPath.replace(`${value}`, `[${key}]`); + } + } // requests for pages will only ever be GET requests, so don't bother to include the method in the transaction // name; requests to API routes could be GET, POST, PUT, etc, so do include it there From 800fc8864cd4fef306045502665671f01e49e47d Mon Sep 17 00:00:00 2001 From: Abhijeet Prasad Date: Thu, 27 May 2021 11:40:05 -0400 Subject: [PATCH 16/17] fix(nextjs): Add `api/` to BrowserTracing tracingOrigins (#3607) Update default tracingOrigins for NextJS BrowserTracing to include api routes so we can add sentry-trace header correctly. --- packages/nextjs/src/index.client.ts | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/packages/nextjs/src/index.client.ts b/packages/nextjs/src/index.client.ts index 7a55b4d0bce2..a93799509bce 100644 --- a/packages/nextjs/src/index.client.ts +++ b/packages/nextjs/src/index.client.ts @@ -1,5 +1,5 @@ import { configureScope, init as reactInit } from '@sentry/react'; -import { Integrations } from '@sentry/tracing'; +import { defaultRequestInstrumentationOptions, Integrations } from '@sentry/tracing'; import { nextRouterInstrumentation } from './performance/client'; import { MetadataBuilder } from './utils/metadataBuilder'; @@ -33,6 +33,7 @@ export function init(options: NextjsOptions): void { } const defaultBrowserTracingIntegration = new BrowserTracing({ + tracingOrigins: [...defaultRequestInstrumentationOptions.tracingOrigins, /^(api\/)/], routingInstrumentation: nextRouterInstrumentation, }); From ab21117d8924bf7ef81ad54dc0eb8ba8c85d67e4 Mon Sep 17 00:00:00 2001 From: Katie Byers Date: Mon, 31 May 2021 00:48:45 -0700 Subject: [PATCH 17/17] fix(nextjs): Fix return type when injecting sentry code using webpack (#3625) --- packages/nextjs/src/utils/config.ts | 20 +++++++++++--------- 1 file changed, 11 insertions(+), 9 deletions(-) diff --git a/packages/nextjs/src/utils/config.ts b/packages/nextjs/src/utils/config.ts index a8842bce8d77..0cc240facd34 100644 --- a/packages/nextjs/src/utils/config.ts +++ b/packages/nextjs/src/utils/config.ts @@ -77,7 +77,7 @@ const _injectFile = (entryProperty: EntryPropertyObject, injectionPoint: string, entryProperty[injectionPoint] = injectedInto; }; -const injectSentry = async (origEntryProperty: EntryProperty, isServer: boolean): Promise => { +const injectSentry = async (origEntryProperty: EntryProperty, isServer: boolean): 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 // someone else has come along before us and changed that, we need to check a few things along the way. The one thing @@ -106,11 +106,7 @@ const injectSentry = async (origEntryProperty: EntryProperty, isServer: boolean) else { _injectFile(newEntryProperty, 'main', SENTRY_CLIENT_CONFIG_FILE); } - // TODO: hack made necessary because the async-ness of this function turns our object back into a promise, meaning the - // internal `next` code which should do this doesn't - if ('main.js' in newEntryProperty) { - delete newEntryProperty['main.js']; - } + return newEntryProperty; }; @@ -178,9 +174,15 @@ export function withSentryConfig( newConfig.devtool = 'source-map'; } - // Inject user config files (`sentry.client.confg.js` and `sentry.server.config.js`), which is where `Sentry.init()` - // is called. By adding them here, we ensure that they're bundled by webpack as part of both server code and client code. - newConfig.entry = (injectSentry(newConfig.entry, options.isServer) as unknown) as EntryProperty; + // Tell webpack to inject user config files (containing the two `Sentry.init()` calls) into the appropriate output + // bundles. Store a separate reference to the original `entry` value to avoid an infinite loop. (In a synchronous + // world, `x = () => f(x)` is fine, because the dereferencing is guaranteed to happen before the assignment, meaning + // we know f will get the original value of x. But in an async world, if we do `x = async () => f(x)`, the + // assignment happens *before* the dereferencing, meaning f is passed the new value. In other words, in that + // scenario, the new value is defined in terms of itself, with predictably bad consequences. Theoretically this + // could also be fixed by using `bind`, but this is way simpler.) + const origEntryProperty = newConfig.entry; + newConfig.entry = () => injectSentry(origEntryProperty, options.isServer); // Add the Sentry plugin, which uploads source maps to Sentry when not in dev newConfig.plugins.push(