From b79ee0951b1606baae3309dd8fd12aaf90f0d90a Mon Sep 17 00:00:00 2001 From: Katie Byers Date: Tue, 11 May 2021 11:22:19 -0700 Subject: [PATCH 1/7] page transactions working, API transactions not working --- packages/nextjs/package.json | 1 + packages/nextjs/src/utils/instrumentServer.ts | 102 +++++++++++++++++- packages/node/src/handlers.ts | 7 +- packages/node/src/index.ts | 1 + packages/node/src/utils.ts | 26 +++++ 5 files changed, 130 insertions(+), 7 deletions(-) create mode 100644 packages/node/src/utils.ts diff --git a/packages/nextjs/package.json b/packages/nextjs/package.json index dfde398a18f2..c938cafdb085 100644 --- a/packages/nextjs/package.json +++ b/packages/nextjs/package.json @@ -26,6 +26,7 @@ "tslib": "^1.9.3" }, "devDependencies": { + "@sentry/tracing": "6.3.6", "@sentry/types": "6.3.6", "@types/webpack": "^5.28.0", "eslint": "7.20.0", diff --git a/packages/nextjs/src/utils/instrumentServer.ts b/packages/nextjs/src/utils/instrumentServer.ts index 106a822d30ea..658201fd67ea 100644 --- a/packages/nextjs/src/utils/instrumentServer.ts +++ b/packages/nextjs/src/utils/instrumentServer.ts @@ -1,6 +1,10 @@ +import { deepReadDirSync } from '@sentry/node'; +import { hasTracingEnabled } from '@sentry/tracing'; +import { Transaction } from '@sentry/types'; import { fill } from '@sentry/utils'; import * as http from 'http'; import { default as createNextServer } from 'next'; +import * as path from 'path'; import * as url from 'url'; import * as Sentry from '../index.server'; @@ -10,23 +14,33 @@ type PlainObject = { [key: string]: T }; interface NextServer { server: Server; + createServer: (options: PlainObject) => Server; } 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 = {}; @@ -61,12 +75,16 @@ function makeWrappedHandlerGetter(origHandlerGetter: HandlerGetter): WrappedHand const wrappedHandlerGetter = async function(this: NextServer): Promise { if (!closure.wrappingComplete) { 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` fill(serverPrototype, 'logError', makeWrappedErrorLogger); + fill(serverPrototype, 'handleRequest', makeWrappedReqHandler); + closure.wrappingComplete = true; } @@ -89,3 +107,77 @@ 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 + // https://github.com/vercel/next.js/blob/4443d6f3d36b107e833376c2720c1e206eee720d/packages/next/next-server/server/next-server.ts#L1166 + const publicDirFiles = new Set( + deepReadDirSync(liveServer.publicDir).map(p => + encodeURI( + // switch any backslashes in the path to regular slashes + p.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..d2090372e7ce --- /dev/null +++ b/packages/node/src/utils.ts @@ -0,0 +1,26 @@ +import * as fs from 'fs'; +import * as path from 'path'; + +/** + * Recursively read the contents of a directory. + * + * @param targetDir The directory to scan. All returned paths will be relative to this directory. + * @param _paths Array to hold results, passed for purposes of recursion. Not meant to be provided by the caller. + * @returns Array holding all relative paths + */ +export function deepReadDirSync(targetDir: string, _paths?: string[]): string[] { + const paths = _paths || []; + const currentDirContents = fs.readdirSync(targetDir); + + currentDirContents.forEach((fileOrDirName: string) => { + const fileOrDirAbsPath = path.join(targetDir, fileOrDirName); + + if (fs.statSync(fileOrDirAbsPath).isDirectory()) { + deepReadDirSync(fileOrDirAbsPath, paths); + return; + } + paths.push(fileOrDirAbsPath.replace(targetDir, '')); + }); + + return paths; +} From d5293dac1d01acd7a827856b9222505cb7cf33b7 Mon Sep 17 00:00:00 2001 From: Katie Byers Date: Tue, 11 May 2021 14:10:42 -0700 Subject: [PATCH 2/7] inject SDK init earlier - API transactions working --- packages/nextjs/src/utils/config.ts | 88 ++++++++++--------- packages/nextjs/src/utils/instrumentServer.ts | 6 ++ 2 files changed, 53 insertions(+), 41 deletions(-) diff --git a/packages/nextjs/src/utils/config.ts b/packages/nextjs/src/utils/config.ts index 767a038a8e26..18ca8e5f6c55 100644 --- a/packages/nextjs/src/utils/config.ts +++ b/packages/nextjs/src/utils/config.ts @@ -3,33 +3,36 @@ import { logger } from '@sentry/utils'; import defaultWebpackPlugin, { SentryCliPluginOptions } from '@sentry/webpack-plugin'; import * as SentryWebpackPlugin from '@sentry/webpack-plugin'; +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_INIT_OUTPUT_LOCATION = 'sentry/serverInit'; + // 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 } }; 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 +44,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 +69,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 +79,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_INIT_OUTPUT_LOCATION] = 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 +108,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,17 +128,17 @@ 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.", ); } @@ -161,8 +167,8 @@ export function withSentryConfig( new ((SentryWebpackPlugin as unknown) as typeof defaultWebpackPlugin)({ dryRun: options.dev, release: getSentryRelease(options.buildId), - ...defaultWebpackPluginOptions, - ...providedWebpackPluginOptions, + ...defaultSentryWebpackPluginOptions, + ...providedSentryWebpackPluginOptions, }), ); diff --git a/packages/nextjs/src/utils/instrumentServer.ts b/packages/nextjs/src/utils/instrumentServer.ts index 658201fd67ea..0c26c362aa9c 100644 --- a/packages/nextjs/src/utils/instrumentServer.ts +++ b/packages/nextjs/src/utils/instrumentServer.ts @@ -45,6 +45,8 @@ type WrappedReqHandler = ReqHandler; // TODO is it necessary for this to be an object? const closure: PlainObject = {}; +let sdkInitialized = 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`. @@ -58,6 +60,10 @@ export function instrumentServer(): string { // 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 fill(nextServerPrototype, 'getServerRequestHandler', makeWrappedHandlerGetter); + if (!sdkInitialized) { + require(path.join(process.cwd(), 'sentry.server.config.js')); + sdkInitialized = true; + } return closure.projectRootDir; } From de075a42007e113db380fb9f5ddf4665e68c31dd Mon Sep 17 00:00:00 2001 From: Katie Byers Date: Wed, 12 May 2021 18:35:14 -0700 Subject: [PATCH 3/7] fix requiring server init code --- packages/nextjs/src/utils/config.ts | 41 +++++++++++++++++-- packages/nextjs/src/utils/instrumentServer.ts | 25 +++++++---- 2 files changed, 54 insertions(+), 12 deletions(-) diff --git a/packages/nextjs/src/utils/config.ts b/packages/nextjs/src/utils/config.ts index 18ca8e5f6c55..1bc45866d2fd 100644 --- a/packages/nextjs/src/utils/config.ts +++ b/packages/nextjs/src/utils/config.ts @@ -2,11 +2,13 @@ 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_INIT_OUTPUT_LOCATION = 'sentry/serverInit'; +export const SERVER_SDK_INIT_PATH = 'sentry/initServer.js'; // eslint-disable-next-line @typescript-eslint/no-explicit-any type PlainObject = { [key: string]: T }; @@ -15,7 +17,14 @@ type PlainObject = { [key: string]: T }; 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; output: { path: string } }; +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 @@ -87,7 +96,7 @@ 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_INIT_OUTPUT_LOCATION] = SENTRY_SERVER_CONFIG_FILE; + 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 { @@ -144,6 +153,12 @@ export function withSentryConfig( } 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') { + memoizeOutputPath(config); + } + let newConfig = config; if (typeof providedExports.webpack === 'function') { @@ -181,3 +196,23 @@ export function withSentryConfig( webpack: newWebpackExport, }; } + +/** + * Store the future location of the webpack-processed version of `sentry.server.config.js` in a runtime env variable, so + * we know where to find it when we want to initialize the SDK. + * + * @param config The webpack config object for this build + */ +function memoizeOutputPath(config: WebpackConfig): void { + const builtServerSDKInitPath = path.join(config.output.path, SERVER_SDK_INIT_PATH); + const projectDir = config.context; + // next will automatically set variables from `.env.local` in `process.env` at runtime + const envFilePath = path.join(projectDir, '.env.local'); + const envVarString = `SENTRY_SERVER_INIT_PATH=${builtServerSDKInitPath}\n`; + + if (fs.existsSync(envFilePath)) { + fs.appendFileSync(envFilePath, `\n${envVarString}`); + } else { + fs.writeFileSync(envFilePath, envVarString); + } +} diff --git a/packages/nextjs/src/utils/instrumentServer.ts b/packages/nextjs/src/utils/instrumentServer.ts index 0c26c362aa9c..c650571567c9 100644 --- a/packages/nextjs/src/utils/instrumentServer.ts +++ b/packages/nextjs/src/utils/instrumentServer.ts @@ -1,10 +1,9 @@ import { deepReadDirSync } from '@sentry/node'; import { hasTracingEnabled } from '@sentry/tracing'; import { Transaction } from '@sentry/types'; -import { fill } from '@sentry/utils'; +import { fill, logger } from '@sentry/utils'; import * as http from 'http'; import { default as createNextServer } from 'next'; -import * as path from 'path'; import * as url from 'url'; import * as Sentry from '../index.server'; @@ -45,7 +44,7 @@ type WrappedReqHandler = ReqHandler; // TODO is it necessary for this to be an object? const closure: PlainObject = {}; -let sdkInitialized = false; +let sdkSetupComplete = false; /** * Do the monkeypatching and wrapping necessary to catch errors in page routes. Along the way, as a bonus, grab (and @@ -60,11 +59,8 @@ export function instrumentServer(): string { // 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 fill(nextServerPrototype, 'getServerRequestHandler', makeWrappedHandlerGetter); - if (!sdkInitialized) { - require(path.join(process.cwd(), 'sentry.server.config.js')); - sdkInitialized = true; - } + // TODO replace with an env var, since at this point we don't have a value yet return closure.projectRootDir; } @@ -79,7 +75,18 @@ 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; @@ -91,7 +98,7 @@ function makeWrappedHandlerGetter(origHandlerGetter: HandlerGetter): WrappedHand fill(serverPrototype, 'handleRequest', makeWrappedReqHandler); - closure.wrappingComplete = true; + sdkSetupComplete = true; } return origHandlerGetter.call(this); From c9f78ce58da4460d52696f01064d9bb732b8a4a9 Mon Sep 17 00:00:00 2001 From: Katie Byers Date: Wed, 12 May 2021 18:37:07 -0700 Subject: [PATCH 4/7] improve documentation --- packages/nextjs/src/utils/instrumentServer.ts | 38 ++++++++++++++++--- 1 file changed, 32 insertions(+), 6 deletions(-) diff --git a/packages/nextjs/src/utils/instrumentServer.ts b/packages/nextjs/src/utils/instrumentServer.ts index c650571567c9..bb701983dedd 100644 --- a/packages/nextjs/src/utils/instrumentServer.ts +++ b/packages/nextjs/src/utils/instrumentServer.ts @@ -11,11 +11,15 @@ 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; @@ -47,17 +51,37 @@ 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: + + // 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` - // 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 + // 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 @@ -93,9 +117,11 @@ function makeWrappedHandlerGetter(origHandlerGetter: HandlerGetter): WrappedHand 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); + // 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; From f352ff2d0c6b861fc84c3de6741bb27419502331 Mon Sep 17 00:00:00 2001 From: Katie Byers Date: Thu, 13 May 2021 17:08:13 -0700 Subject: [PATCH 5/7] move tracing package to regular dependencies --- packages/nextjs/package.json | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/nextjs/package.json b/packages/nextjs/package.json index c938cafdb085..37214026eeac 100644 --- a/packages/nextjs/package.json +++ b/packages/nextjs/package.json @@ -21,12 +21,12 @@ "@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" }, "devDependencies": { - "@sentry/tracing": "6.3.6", "@sentry/types": "6.3.6", "@types/webpack": "^5.28.0", "eslint": "7.20.0", From 0ec15fdd324c7d5a721e887a07e4c9be25e1bc8b Mon Sep 17 00:00:00 2001 From: Katie Byers Date: Thu, 13 May 2021 17:10:37 -0700 Subject: [PATCH 6/7] fix deepReadDirSync --- packages/nextjs/src/utils/instrumentServer.ts | 13 +++-- packages/node/src/utils.ts | 40 +++++++++----- .../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 +++++++++++++++++++ 13 files changed, 95 insertions(+), 21 deletions(-) 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/src/utils/instrumentServer.ts b/packages/nextjs/src/utils/instrumentServer.ts index bb701983dedd..dbc818928976 100644 --- a/packages/nextjs/src/utils/instrumentServer.ts +++ b/packages/nextjs/src/utils/instrumentServer.ts @@ -156,15 +156,14 @@ function makeWrappedErrorLogger(origErrorLogger: ErrorLogger): WrappedErrorLogge function makeWrappedReqHandler(origReqHandler: ReqHandler): WrappedReqHandler { const liveServer = closure.server as Server; - // inspired by + // 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( - deepReadDirSync(liveServer.publicDir).map(p => - encodeURI( - // switch any backslashes in the path to regular slashes - p.replace(/\\/g, '/'), - ), - ), + // 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 diff --git a/packages/node/src/utils.ts b/packages/node/src/utils.ts index d2090372e7ce..87fb8f6157e3 100644 --- a/packages/node/src/utils.ts +++ b/packages/node/src/utils.ts @@ -4,23 +4,35 @@ import * as path from 'path'; /** * Recursively read the contents of a directory. * - * @param targetDir The directory to scan. All returned paths will be relative to this directory. - * @param _paths Array to hold results, passed for purposes of recursion. Not meant to be provided by the caller. + * @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, _paths?: string[]): string[] { - const paths = _paths || []; - const currentDirContents = fs.readdirSync(targetDir); +export function deepReadDirSync(targetDir: string): string[] { + const targetDirAbsPath = path.resolve(targetDir); - currentDirContents.forEach((fileOrDirName: string) => { - const fileOrDirAbsPath = path.join(targetDir, fileOrDirName); + if (!fs.existsSync(targetDirAbsPath)) { + throw new Error(`Cannot read contents of ${targetDirAbsPath}. Directory does not exist.`); + } - if (fs.statSync(fileOrDirAbsPath).isDirectory()) { - deepReadDirSync(fileOrDirAbsPath, paths); - return; - } - paths.push(fileOrDirAbsPath.replace(targetDir, '')); - }); + if (!fs.statSync(targetDirAbsPath).isDirectory()) { + throw new Error(`Cannot read contents of ${targetDirAbsPath}, because it is not a directory.`); + } - return paths; + // 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 188c982c79f2b955e738baf17f90d679f211c5ea Mon Sep 17 00:00:00 2001 From: Katie Byers Date: Mon, 17 May 2021 09:35:46 -0700 Subject: [PATCH 7/7] refactor setting env vars --- packages/nextjs/src/utils/config.ts | 48 ++++++++++++++++++++--------- 1 file changed, 33 insertions(+), 15 deletions(-) diff --git a/packages/nextjs/src/utils/config.ts b/packages/nextjs/src/utils/config.ts index 1bc45866d2fd..de2d40343273 100644 --- a/packages/nextjs/src/utils/config.ts +++ b/packages/nextjs/src/utils/config.ts @@ -8,7 +8,7 @@ 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/initServer.js'; +export const SERVER_SDK_INIT_PATH = 'sentry/initServerSDK.js'; // eslint-disable-next-line @typescript-eslint/no-explicit-any type PlainObject = { [key: string]: T }; @@ -156,7 +156,9 @@ export function withSentryConfig( // 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') { - memoizeOutputPath(config); + 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; @@ -198,21 +200,37 @@ export function withSentryConfig( } /** - * Store the future location of the webpack-processed version of `sentry.server.config.js` in a runtime env variable, so - * we know where to find it when we want to initialize the SDK. + * 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 config The webpack config object for this build + * @param projectDir The path to the project root + * @param vars Object containing vars to set */ -function memoizeOutputPath(config: WebpackConfig): void { - const builtServerSDKInitPath = path.join(config.output.path, SERVER_SDK_INIT_PATH); - const projectDir = config.context; - // next will automatically set variables from `.env.local` in `process.env` at runtime +function setRuntimeEnvVars(projectDir: string, vars: PlainObject): void { + // ensure the file exists const envFilePath = path.join(projectDir, '.env.local'); - const envVarString = `SENTRY_SERVER_INIT_PATH=${builtServerSDKInitPath}\n`; - - if (fs.existsSync(envFilePath)) { - fs.appendFileSync(envFilePath, `\n${envVarString}`); - } else { - fs.writeFileSync(envFilePath, envVarString); + 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`); }