Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(nextjs): Auto-wrap API routes #5778

Merged
merged 10 commits into from
Sep 26, 2022
6 changes: 5 additions & 1 deletion packages/nextjs/rollup.npm.config.js
Original file line number Diff line number Diff line change
Expand Up @@ -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()],
Expand Down
11 changes: 4 additions & 7 deletions packages/nextjs/src/config/loaders/proxyLoader.ts
Original file line number Diff line number Diff line change
Expand Up @@ -33,12 +33,6 @@ export default async function proxyLoader(this: LoaderThis<LoaderOptions>, 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
Expand All @@ -47,7 +41,10 @@ export default async function proxyLoader(this: LoaderThis<LoaderOptions>, userC
return userCode;
}

const templatePath = path.resolve(__dirname, '../templates/proxyLoaderTemplate.js');
const templateFile = parameterizedRoute.startsWith('/api')
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

do we have access to API_ROUTE from next.js?

If so, I would recommend us using that instead when referring to /api folder

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We don't - it's not exported - but it's the just the regex version of this same check. Is your concern that they might change the value? (Feels like that'd be a pretty big breaking change on their part...)

? '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);
Expand Down
47 changes: 47 additions & 0 deletions packages/nextjs/src/config/templates/apiProxyLoaderTemplate.ts
Original file line number Diff line number Diff line change
@@ -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__';
1 change: 1 addition & 0 deletions packages/nextjs/src/config/wrappers/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -4,3 +4,4 @@ export { withSentryServerSideAppGetInitialProps } from './withSentryServerSideAp
export { withSentryServerSideDocumentGetInitialProps } from './withSentryServerSideDocumentGetInitialProps';
export { withSentryServerSideErrorGetInitialProps } from './withSentryServerSideErrorGetInitialProps';
export { withSentryGetServerSideProps } from './withSentryGetServerSideProps';
export { withSentryAPI } from './withSentryAPI';
39 changes: 39 additions & 0 deletions packages/nextjs/src/config/wrappers/withSentryAPI.ts
Original file line number Diff line number Diff line change
@@ -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(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I find this message as a good line in our CHANGELOG.md.

As a user, I would find it annoying to see this message. I don't care if the route is wrapped by Sentry automatically or by me. All I care is that it works.

`${_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_}.`,
);
}
Comment on lines +32 to +36
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am on the fence about whether I find this message useful or unnecessary. Can't hurt I guess, so it's definitely not blocking (or worth a discussion at this point in time).

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My thinking is that eventually, we'll deprecate withSentry, and in the meantime, it's a good way to let people know about the change. Will leave it in for now so as to get this merged, but am open to it not staying there forever.

(Bonus of eventually removing it: We can then merge withSentryAPI and withSentry into one.)


return withSentry(maybeWrappedHandler, parameterizedRoute);
}
1 change: 1 addition & 0 deletions packages/nextjs/src/index.server.ts
Original file line number Diff line number Diff line change
Expand Up @@ -134,6 +134,7 @@ export {
withSentryServerSideAppGetInitialProps,
withSentryServerSideDocumentGetInitialProps,
withSentryServerSideErrorGetInitialProps,
withSentryAPI,
} from './config/wrappers';
export { withSentry } from './utils/withSentry';

Expand Down
31 changes: 31 additions & 0 deletions packages/nextjs/src/utils/nextLogger.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,31 @@
/* eslint-disable no-console */
import * as chalk from 'chalk';
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is the purpose of this logger?

Yes, I checked the implementation on the nextjs repo that you referenced and I don't udnerstand the "why" behind it. :)

Also, I believe that the comment attached is more appropriate as a GH comment then part of the codebase


// 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),
};
45 changes: 33 additions & 12 deletions packages/nextjs/src/utils/withSentry.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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<void> | unknown | Promise<unknown>;
export type NextApiHandler = (
req: NextApiRequest,
res: NextApiResponse,
) => void | Promise<void> | unknown | Promise<unknown>;
export type WrappedNextApiHandler = (req: NextApiRequest, res: NextApiResponse) => Promise<void> | Promise<unknown>;

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
lobsterkatie marked this conversation as resolved.
Show resolved Hide resolved
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
Expand Down Expand Up @@ -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(
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
import { NextApiRequest, NextApiResponse } from 'next';

const handler = async (_req: NextApiRequest, res: NextApiResponse): Promise<void> => {
res.status(200).json({});
};

export default handler;
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
import { NextApiRequest, NextApiResponse } from 'next';

const handler = async (_req: NextApiRequest, res: NextApiResponse): Promise<void> => {
res.status(200).json({});
};

export default handler;
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
import { NextApiRequest, NextApiResponse } from 'next';
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I recommend we make a test checking for nested routes like [...all].ts

Docs https://nextjs.org/docs/routing/introduction#dynamic-route-segments

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.


const handler = async (_req: NextApiRequest, res: NextApiResponse): Promise<void> => {
res.status(200).json({});
};

export default handler;
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
import { withSentry } from '@sentry/nextjs';
import { NextApiRequest, NextApiResponse } from 'next';

const handler = async (_req: NextApiRequest, res: NextApiResponse): Promise<void> => {
res.status(200).json({});
};

export default withSentry(handler);
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
import { withSentry } from '@sentry/nextjs';
import { NextApiRequest, NextApiResponse } from 'next';

const handler = async (_req: NextApiRequest, res: NextApiResponse): Promise<void> => {
res.status(200).json({});
};

export default withSentry(handler);
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
import { withSentry } from '@sentry/nextjs';
import { NextApiRequest, NextApiResponse } from 'next';

const handler = async (_req: NextApiRequest, res: NextApiResponse): Promise<void> => {
res.status(200).json({});
};

export default withSentry(handler);
Original file line number Diff line number Diff line change
@@ -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(', ')}.`,
);
};