diff --git a/packages/nextjs/rollup.npm.config.js b/packages/nextjs/rollup.npm.config.js index b5083370fe3d..267bd5503e6e 100644 --- a/packages/nextjs/rollup.npm.config.js +++ b/packages/nextjs/rollup.npm.config.js @@ -14,7 +14,11 @@ export default [ ), ...makeNPMConfigVariants( makeBaseNPMConfig({ - entrypoints: ['src/config/templates/prefixLoaderTemplate.ts', 'src/config/templates/proxyLoaderTemplate.ts'], + entrypoints: [ + 'src/config/templates/prefixLoaderTemplate.ts', + 'src/config/templates/pageProxyLoaderTemplate.ts', + 'src/config/templates/apiProxyLoaderTemplate.ts', + ], packageSpecificConfig: { plugins: [plugins.makeRemoveMultiLineCommentsPlugin()], diff --git a/packages/nextjs/src/config/loaders/proxyLoader.ts b/packages/nextjs/src/config/loaders/proxyLoader.ts index f0cf96b11651..1c10bff39d01 100644 --- a/packages/nextjs/src/config/loaders/proxyLoader.ts +++ b/packages/nextjs/src/config/loaders/proxyLoader.ts @@ -33,12 +33,6 @@ export default async function proxyLoader(this: LoaderThis, userC // homepage), sub back in the root route .replace(/^$/, '/'); - // TODO: For the moment we skip API routes. Those will need to be handled slightly differently because of the manual - // wrapping we've already been having people do using `withSentry`. - if (parameterizedRoute.startsWith('api')) { - return userCode; - } - // We don't want to wrap twice (or infinitely), so in the proxy we add this query string onto references to the // wrapped file, so that we know that it's already been processed. (Adding this query string is also necessary to // convince webpack that it's a different file than the one it's in the middle of loading now, so that the originals @@ -47,7 +41,10 @@ export default async function proxyLoader(this: LoaderThis, userC return userCode; } - const templatePath = path.resolve(__dirname, '../templates/proxyLoaderTemplate.js'); + const templateFile = parameterizedRoute.startsWith('/api') + ? 'apiProxyLoaderTemplate.js' + : 'pageProxyLoaderTemplate.js'; + const templatePath = path.resolve(__dirname, `../templates/${templateFile}`); let templateCode = fs.readFileSync(templatePath).toString(); // Make sure the template is included when runing `webpack watch` this.addDependency(templatePath); diff --git a/packages/nextjs/src/config/templates/apiProxyLoaderTemplate.ts b/packages/nextjs/src/config/templates/apiProxyLoaderTemplate.ts new file mode 100644 index 000000000000..59d49856fb7b --- /dev/null +++ b/packages/nextjs/src/config/templates/apiProxyLoaderTemplate.ts @@ -0,0 +1,47 @@ +/** + * This file is a template for the code which will be substituted when our webpack loader handles API files in the + * `pages/` directory. + * + * We use `__RESOURCE_PATH__` as a placeholder for the path to the file being wrapped. Because it's not a real package, + * this causes both TS and ESLint to complain, hence the pragma comments below. + */ + +// @ts-ignore See above +// eslint-disable-next-line import/no-unresolved +import * as origModule from '__RESOURCE_PATH__'; +import * as Sentry from '@sentry/nextjs'; +import type { PageConfig } from 'next'; + +// We import this from `withSentry` rather than directly from `next` because our version can work simultaneously with +// multiple versions of next. See note in `withSentry` for more. +import type { NextApiHandler } from '../../utils/withSentry'; + +type NextApiModule = { + default: NextApiHandler; + config?: PageConfig; +}; + +const userApiModule = origModule as NextApiModule; + +const maybeWrappedHandler = userApiModule.default; +const origConfig = userApiModule.config || {}; + +// Setting `externalResolver` to `true` prevents nextjs from throwing a warning in dev about API routes resolving +// without sending a response. It's a false positive (a response is sent, but only after we flush our send queue), and +// we throw a warning of our own to tell folks that, but it's better if we just don't have to deal with it in the first +// place. +export const config = { + ...origConfig, + api: { + ...origConfig.api, + externalResolver: true, + }, +}; + +export default Sentry.withSentryAPI(maybeWrappedHandler, '__ROUTE__'); + +// Re-export anything exported by the page module we're wrapping. When processing this code, Rollup is smart enough to +// not include anything whose name matchs something we've explicitly exported above. +// @ts-ignore See above +// eslint-disable-next-line import/no-unresolved +export * from '__RESOURCE_PATH__'; diff --git a/packages/nextjs/src/config/templates/proxyLoaderTemplate.ts b/packages/nextjs/src/config/templates/pageProxyLoaderTemplate.ts similarity index 100% rename from packages/nextjs/src/config/templates/proxyLoaderTemplate.ts rename to packages/nextjs/src/config/templates/pageProxyLoaderTemplate.ts diff --git a/packages/nextjs/src/config/wrappers/index.ts b/packages/nextjs/src/config/wrappers/index.ts index d675812fbd99..9f710d1f5104 100644 --- a/packages/nextjs/src/config/wrappers/index.ts +++ b/packages/nextjs/src/config/wrappers/index.ts @@ -4,3 +4,4 @@ export { withSentryServerSideAppGetInitialProps } from './withSentryServerSideAp export { withSentryServerSideDocumentGetInitialProps } from './withSentryServerSideDocumentGetInitialProps'; export { withSentryServerSideErrorGetInitialProps } from './withSentryServerSideErrorGetInitialProps'; export { withSentryGetServerSideProps } from './withSentryGetServerSideProps'; +export { withSentryAPI } from './withSentryAPI'; diff --git a/packages/nextjs/src/config/wrappers/withSentryAPI.ts b/packages/nextjs/src/config/wrappers/withSentryAPI.ts new file mode 100644 index 000000000000..1dd095b90697 --- /dev/null +++ b/packages/nextjs/src/config/wrappers/withSentryAPI.ts @@ -0,0 +1,39 @@ +import { formatAsCode, nextLogger } from '../../utils/nextLogger'; +// We import these types from `withSentry` rather than directly from `next` because our version can work simultaneously +// with multiple versions of next. See note in `withSentry` for more. +import type { NextApiHandler, WrappedNextApiHandler } from '../../utils/withSentry'; +import { withSentry } from '../../utils/withSentry'; + +/** + * Wrap the given API route handler for tracing and error capturing. Thin wrapper around `withSentry`, which only + * applies it if it hasn't already been applied. + * + * @param maybeWrappedHandler The handler exported from the user's API page route file, which may or may not already be + * wrapped with `withSentry` + * @param parameterizedRoute The page's route, passed in via the proxy loader + * @returns The wrapped handler + */ +export function withSentryAPI( + maybeWrappedHandler: NextApiHandler | WrappedNextApiHandler, + parameterizedRoute: string, +): WrappedNextApiHandler { + // Log a warning if the user is still manually wrapping their route in `withSentry`. Doesn't work in cases where + // there's been an intermediate wrapper (like `withSentryAPI(someOtherWrapper(withSentry(handler)))`) but should catch + // most cases. Only runs once per route. (Note: Such double-wrapping isn't harmful, but we'll eventually deprecate and remove `withSentry`, so + // best to get people to stop using it.) + if (maybeWrappedHandler.name === 'sentryWrappedHandler') { + const [_sentryNextjs_, _autoWrapOption_, _withSentry_, _route_] = [ + '@sentry/nextjs', + 'autoInstrumentServerFunctions', + 'withSentry', + parameterizedRoute, + ].map(phrase => formatAsCode(phrase)); + + nextLogger.info( + `${_sentryNextjs_} is running with the ${_autoWrapOption_} flag set, which means API routes no longer need to ` + + `be manually wrapped with ${_withSentry_}. Detected manual wrapping in ${_route_}.`, + ); + } + + return withSentry(maybeWrappedHandler, parameterizedRoute); +} diff --git a/packages/nextjs/src/index.server.ts b/packages/nextjs/src/index.server.ts index cc1c314a5eda..28442999b2aa 100644 --- a/packages/nextjs/src/index.server.ts +++ b/packages/nextjs/src/index.server.ts @@ -134,6 +134,7 @@ export { withSentryServerSideAppGetInitialProps, withSentryServerSideDocumentGetInitialProps, withSentryServerSideErrorGetInitialProps, + withSentryAPI, } from './config/wrappers'; export { withSentry } from './utils/withSentry'; diff --git a/packages/nextjs/src/utils/nextLogger.ts b/packages/nextjs/src/utils/nextLogger.ts new file mode 100644 index 000000000000..14d701d6edb7 --- /dev/null +++ b/packages/nextjs/src/utils/nextLogger.ts @@ -0,0 +1,31 @@ +/* eslint-disable no-console */ +import * as chalk from 'chalk'; + +// This is nextjs's own logging formatting, vendored since it's not exported. See +// https://github.com/vercel/next.js/blob/c3ceeb03abb1b262032bd96457e224497d3bbcef/packages/next/build/output/log.ts#L3-L11 +// and +// https://github.com/vercel/next.js/blob/de7aa2d6e486c40b8be95a1327639cbed75a8782/packages/next/lib/eslint/runLintCheck.ts#L321-L323. + +const prefixes = { + wait: `${chalk.cyan('wait')} -`, + error: `${chalk.red('error')} -`, + warn: `${chalk.yellow('warn')} -`, + ready: `${chalk.green('ready')} -`, + info: `${chalk.cyan('info')} -`, + event: `${chalk.magenta('event')} -`, + trace: `${chalk.magenta('trace')} -`, +}; + +export const formatAsCode = (str: string): string => chalk.bold.cyan(str); + +export const nextLogger: { + [key: string]: (...message: unknown[]) => void; +} = { + wait: (...message) => console.log(prefixes.wait, ...message), + error: (...message) => console.error(prefixes.error, ...message), + warn: (...message) => console.warn(prefixes.warn, ...message), + ready: (...message) => console.log(prefixes.ready, ...message), + info: (...message) => console.log(prefixes.info, ...message), + event: (...message) => console.log(prefixes.event, ...message), + trace: (...message) => console.log(prefixes.trace, ...message), +}; diff --git a/packages/nextjs/src/utils/withSentry.ts b/packages/nextjs/src/utils/withSentry.ts index 65cadd78ed89..bd094d58d8c8 100644 --- a/packages/nextjs/src/utils/withSentry.ts +++ b/packages/nextjs/src/utils/withSentry.ts @@ -28,17 +28,32 @@ import { NextApiRequest, NextApiResponse } from 'next'; // the test app would refer to the other version of the type (from the test app's `node_modules`). By using a custom // version of the type compatible with both the old and new official versions, we can use any Next version we want in // a test app without worrying about type errors. -type NextApiHandler = (req: NextApiRequest, res: NextApiResponse) => void | Promise | unknown | Promise; +export type NextApiHandler = ( + req: NextApiRequest, + res: NextApiResponse, +) => void | Promise | unknown | Promise; export type WrappedNextApiHandler = (req: NextApiRequest, res: NextApiResponse) => Promise | Promise; +type AugmentedNextApiRequest = NextApiRequest & { + __withSentry_applied__?: boolean; +}; + export type AugmentedNextApiResponse = NextApiResponse & { __sentryTransaction?: Transaction; }; // eslint-disable-next-line @typescript-eslint/explicit-module-boundary-types -export const withSentry = (origHandler: NextApiHandler): WrappedNextApiHandler => { +export const withSentry = (origHandler: NextApiHandler, parameterizedRoute?: string): WrappedNextApiHandler => { // eslint-disable-next-line @typescript-eslint/explicit-module-boundary-types - return async (req, res) => { + return async function sentryWrappedHandler(req: AugmentedNextApiRequest, res: NextApiResponse) { + // We're now auto-wrapping API route handlers using `withSentryAPI` (which uses `withSentry` under the hood), but + // users still may have their routes manually wrapped with `withSentry`. This check makes `sentryWrappedHandler` + // idempotent so that those cases don't break anything. + if (req.__withSentry_applied__) { + return origHandler(req, res); + } + req.__withSentry_applied__ = true; + // first order of business: monkeypatch `res.end()` so that it will wait for us to send events to sentry before it // fires (if we don't do this, the lambda will close too early and events will be either delayed or lost) // eslint-disable-next-line @typescript-eslint/unbound-method @@ -69,17 +84,23 @@ export const withSentry = (origHandler: NextApiHandler): WrappedNextApiHandler = const baggageHeader = req.headers && req.headers.baggage; const dynamicSamplingContext = baggageHeaderToDynamicSamplingContext(baggageHeader); - const url = `${req.url}`; - // pull off query string, if any - let reqPath = stripUrlQueryAndFragment(url); - // Replace with placeholder - if (req.query) { - // TODO get this from next if possible, to avoid accidentally replacing non-dynamic parts of the path if - // they happen to match the values of any of the dynamic parts - for (const [key, value] of Object.entries(req.query)) { - reqPath = reqPath.replace(`${value}`, `[${key}]`); + // prefer the parameterized route, if we have it (which we will if we've auto-wrapped the route handler) + let reqPath = parameterizedRoute; + + // If not, fake it by just replacing parameter values with their names, hoping that none of them match either + // each other or any hard-coded parts of the path + if (!reqPath) { + const url = `${req.url}`; + // pull off query string, if any + reqPath = stripUrlQueryAndFragment(url); + // Replace with placeholder + if (req.query) { + for (const [key, value] of Object.entries(req.query)) { + reqPath = reqPath.replace(`${value}`, `[${key}]`); + } } } + const reqMethod = `${(req.method || 'GET').toUpperCase()} `; const transaction = startTransaction( diff --git a/packages/nextjs/test/integration/pages/api/withSentryAPI/unwrapped/[...pathParts].ts b/packages/nextjs/test/integration/pages/api/withSentryAPI/unwrapped/[...pathParts].ts new file mode 100644 index 000000000000..3307b12037d5 --- /dev/null +++ b/packages/nextjs/test/integration/pages/api/withSentryAPI/unwrapped/[...pathParts].ts @@ -0,0 +1,7 @@ +import { NextApiRequest, NextApiResponse } from 'next'; + +const handler = async (_req: NextApiRequest, res: NextApiResponse): Promise => { + res.status(200).json({}); +}; + +export default handler; diff --git a/packages/nextjs/test/integration/pages/api/withSentryAPI/unwrapped/[animal].ts b/packages/nextjs/test/integration/pages/api/withSentryAPI/unwrapped/[animal].ts new file mode 100644 index 000000000000..3307b12037d5 --- /dev/null +++ b/packages/nextjs/test/integration/pages/api/withSentryAPI/unwrapped/[animal].ts @@ -0,0 +1,7 @@ +import { NextApiRequest, NextApiResponse } from 'next'; + +const handler = async (_req: NextApiRequest, res: NextApiResponse): Promise => { + res.status(200).json({}); +}; + +export default handler; diff --git a/packages/nextjs/test/integration/pages/api/withSentryAPI/unwrapped/noParams.ts b/packages/nextjs/test/integration/pages/api/withSentryAPI/unwrapped/noParams.ts new file mode 100644 index 000000000000..3307b12037d5 --- /dev/null +++ b/packages/nextjs/test/integration/pages/api/withSentryAPI/unwrapped/noParams.ts @@ -0,0 +1,7 @@ +import { NextApiRequest, NextApiResponse } from 'next'; + +const handler = async (_req: NextApiRequest, res: NextApiResponse): Promise => { + res.status(200).json({}); +}; + +export default handler; diff --git a/packages/nextjs/test/integration/pages/api/withSentryAPI/wrapped/[...pathParts].ts b/packages/nextjs/test/integration/pages/api/withSentryAPI/wrapped/[...pathParts].ts new file mode 100644 index 000000000000..e048d9d27c0f --- /dev/null +++ b/packages/nextjs/test/integration/pages/api/withSentryAPI/wrapped/[...pathParts].ts @@ -0,0 +1,8 @@ +import { withSentry } from '@sentry/nextjs'; +import { NextApiRequest, NextApiResponse } from 'next'; + +const handler = async (_req: NextApiRequest, res: NextApiResponse): Promise => { + res.status(200).json({}); +}; + +export default withSentry(handler); diff --git a/packages/nextjs/test/integration/pages/api/withSentryAPI/wrapped/[animal].ts b/packages/nextjs/test/integration/pages/api/withSentryAPI/wrapped/[animal].ts new file mode 100644 index 000000000000..e048d9d27c0f --- /dev/null +++ b/packages/nextjs/test/integration/pages/api/withSentryAPI/wrapped/[animal].ts @@ -0,0 +1,8 @@ +import { withSentry } from '@sentry/nextjs'; +import { NextApiRequest, NextApiResponse } from 'next'; + +const handler = async (_req: NextApiRequest, res: NextApiResponse): Promise => { + res.status(200).json({}); +}; + +export default withSentry(handler); diff --git a/packages/nextjs/test/integration/pages/api/withSentryAPI/wrapped/noParams.ts b/packages/nextjs/test/integration/pages/api/withSentryAPI/wrapped/noParams.ts new file mode 100644 index 000000000000..e048d9d27c0f --- /dev/null +++ b/packages/nextjs/test/integration/pages/api/withSentryAPI/wrapped/noParams.ts @@ -0,0 +1,8 @@ +import { withSentry } from '@sentry/nextjs'; +import { NextApiRequest, NextApiResponse } from 'next'; + +const handler = async (_req: NextApiRequest, res: NextApiResponse): Promise => { + res.status(200).json({}); +}; + +export default withSentry(handler); diff --git a/packages/nextjs/test/integration/test/server/tracingWithSentryAPI.js b/packages/nextjs/test/integration/test/server/tracingWithSentryAPI.js new file mode 100644 index 000000000000..a9ceebc835c6 --- /dev/null +++ b/packages/nextjs/test/integration/test/server/tracingWithSentryAPI.js @@ -0,0 +1,54 @@ +const assert = require('assert'); + +const { sleep } = require('../utils/common'); +const { getAsync, interceptTracingRequest } = require('../utils/server'); + +module.exports = async ({ url: urlBase, argv }) => { + const urls = { + // testName: [url, route] + unwrappedNoParamURL: [`/api/withSentryAPI/unwrapped/noParams`, '/api/withSentryAPI/unwrapped/noParams'], + unwrappedDynamicURL: [`/api/withSentryAPI/unwrapped/dog`, '/api/withSentryAPI/unwrapped/[animal]'], + unwrappedCatchAllURL: [`/api/withSentryAPI/unwrapped/dog/facts`, '/api/withSentryAPI/unwrapped/[...pathParts]'], + wrappedNoParamURL: [`/api/withSentryAPI/wrapped/noParams`, '/api/withSentryAPI/wrapped/noParams'], + wrappedDynamicURL: [`/api/withSentryAPI/wrapped/dog`, '/api/withSentryAPI/wrapped/[animal]'], + wrappedCatchAllURL: [`/api/withSentryAPI/wrapped/dog/facts`, '/api/withSentryAPI/wrapped/[...pathParts]'], + }; + + const interceptedRequests = {}; + + Object.entries(urls).forEach(([testName, [url, route]]) => { + interceptedRequests[testName] = interceptTracingRequest( + { + contexts: { + trace: { + op: 'http.server', + status: 'ok', + tags: { 'http.status_code': '200' }, + }, + }, + transaction: `GET ${route}`, + type: 'transaction', + request: { + url: `${urlBase}${url}`, + }, + }, + argv, + testName, + ); + }); + + // Wait until all requests have completed + await Promise.all(Object.values(urls).map(([url]) => getAsync(`${urlBase}${url}`))); + + await sleep(250); + + const failingTests = Object.entries(interceptedRequests).reduce( + (failures, [testName, request]) => (!request.isDone() ? failures.concat(testName) : failures), + [], + ); + + assert.ok( + failingTests.length === 0, + `Did not intercept transaction request for the following tests: ${failingTests.join(', ')}.`, + ); +};