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
Merged

Conversation

lobsterkatie
Copy link
Member

@lobsterkatie lobsterkatie commented Sep 20, 2022

As part of #5505, this applies to API route handlers the same kind of auto-wrapping we've done with the data fetchers (withServerSideProps and the like). Though the general idea is the same, the one extra complicating factor here is that there's a good chance the handlers get to us already wrapped in withSentry, which we've up until now been telling users to use as a manual wrapper. This is handled by making withSentry idempotent - if it detects that it's already been run on the current request, it simply acts as a pass-through to the function it wraps.

Notes:

  • A new template has been created to act as a proxy module for API routes, but the proxying work itself is done by the same proxyLoader as before - it just loads one template or the other depending on an individual page's path.

  • Doing this auto-wrapping gives us a chance to do one thing manual withSentry wrapping isn't able to do, which is set the route config to use an external resolver, which will prevent next's dev server from throwing warnings about API routes not sending responses. (In other words, it should solve [@sentry/nextjs] Errored API routes trigger "API resolved without sending a response" #3852.)

@lobsterkatie lobsterkatie mentioned this pull request Sep 20, 2022
43 tasks
@lobsterkatie lobsterkatie changed the base branch from master to kmclb-nextjs-add-sampling-context-in-data-fetcher-wapper September 21, 2022 05:45
Copy link
Member

@lforst lforst left a comment

Choose a reason for hiding this comment

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

This PR is so satisfying. I have minor remarks and a small requested change that's supposed to reduce potential flakiness.

packages/nextjs/src/utils/withSentry.ts Outdated Show resolved Hide resolved
packages/nextjs/src/utils/withSentry.ts Show resolved Hide resolved
Comment on lines 34 to 42
// In the case where the exported handler is already of the form `withSentry(origHandler)`, `maybeWrappedHandler` is
// the wrapped handler returned by `withSentry`, which now has the parameterized route as a property on itself. By
// default in JS, functions have `global` as their `this` value, so to be able to get at the route value, we need to
// make sure it's called with itself as `this`. Since ultimately we need to give nextjs something *it* will call (and
// it won't set the `this` value we want when it does), this means one more wrapper.
const newWrapper: WrappedNextApiHandler = (req, res) => {
// Make `maybeyWrappedHandler` its own `this`
return maybeWrappedHandler.call(maybeWrappedHandler, req, res);
};
Copy link
Member

Choose a reason for hiding this comment

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

I don't believe this is necessary since we also turned the function returned from withSentry from an arrow function into an actual function.

To confirm this, I changed this block to return maybeWrappedHandler; and quickly ran the integration tests you wrote, and they all seem to pass.

Am I missing something?

Copy link
Member Author

@lobsterkatie lobsterkatie Sep 21, 2022

Choose a reason for hiding this comment

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

Huh. My first step was going from arrow to regular function, for exactly this reason, but when I logged this inside of the wrapped handler in withSentry, it gave me global. It wasn't until I used call that logging this gave me the wrapped handler.

Lemme try it again to be sure.

UPDATE: Man, was that a whole lot of debugging for an answer that turned out to be extremely simple. I ran my logging-of-this test again, and it confirmed what I found before - without newWrapper, the this value is global, not the wrapped handler. But I also found what you found, which is that the integration tests still passed. Way longer than I'd care to admit later, and after a whole bunch of poking at the integration tests, it finally dawned on me that the current parameterization method is still there as a fallback even after this change.

To test it out, I got rid of newWrapper as you did, and then separately tried both commenting out the fallback and changing the value of wrappedDynamicURL in the test from /api/withSentryAPI/wrapped/dog to /api/withSentryAPI/wrapped/wrapped. In the first case, the transaction name in the wrappedDynamicURL test doesn't get parameterized at all. In the second case, the fallback computes a transaction name of GET /api/withSentryAPI/[animal]/wrapped rather than the correct GET /api/withSentryAPI/wrapped/[animal]. Either way, the tests fail. Put newWrapper back, though, and both scenarios pass.

Copy link
Member

@lforst lforst Sep 22, 2022

Choose a reason for hiding this comment

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

Ok got it. I am still confused as to how this works and I'll probably never get it. Always, when I think I have it, some thing comes up that completely nukes the concept I have of this.

Anyways, I somehow still don't like our current implementation for the following reasons:

  • Messing with this/call/bind is always a bit funky - also from a user perspective. Are we sure we're not stomping on user-provided this values?
  • If the user wraps the API route with some other wrapper (e.g. export dfault myWrapper(withSentry(handler))) we double wrap the handler.

I then thought about doing the following which seems more or less bulletproof: We wrap the handler in any case with withSentryAPI, but we make withSentry idempotent by writing a flag to req when the request was handled by a withSentry wrapper, and bail in the beginning of withSentry when we see that req already has that flag. Wdyt?

(Just realized that that way we lose the way of passing the route name to with sentry, but we can also do that via the req object.)

Copy link
Member Author

Choose a reason for hiding this comment

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

Messing with this/call/bind is always a bit funky - also from a user perspective. Are we sure we're not stomping on user-provided this values?

With all due respect, I don't agree. Call, apply, and bind are perfectly respectable parts of the language and we use them as intended, all over the SDK. I will grant you that they're not used as commonly as some other language features, but that's exactly why I'm not especially worried about users having set their own this (generally, and especially in this particular situation, where there's nothing that lends itself to this-ing).

If the user wraps the API route with some other wrapper (e.g. export dfault myWrapper(withSentry(handler))) we double wrap the handler.

This, however, is a good point. We (should, if we don't already) tell people to have us be on the outside, but I know as well as anyone from my time doing support that users don't always take our advice.

I then thought about doing the following which seems more or less bulletproof: We wrap the handler in any case with withSentryAPI, but we make withSentry idempotent by writing a flag to req when the request was handled by a withSentry wrapper, and bail in the beginning of withSentry when we see that req already has that flag. Wdyt?

I think I wish we'd had this conversation before I spent hours in Higher Order Function Brain Melt City, but that's no one's fault but the time difference. I think it's a good solution to your second point, which I've now implemented.

Copy link
Member

Choose a reason for hiding this comment

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

Call, apply, and bind are perfectly respectable parts of the language and we use them as intended

Agree, but imo they are by far the most common source of confusion in our code base. So if we can, I would avoid them if possible. (I am aware that in many circumstances they are necessary - in these situations, they are fine ofc)

@lobsterkatie lobsterkatie force-pushed the kmclb-nextjs-add-sampling-context-in-data-fetcher-wapper branch from ab6d376 to 050c525 Compare September 21, 2022 14:01
Base automatically changed from kmclb-nextjs-add-sampling-context-in-data-fetcher-wapper to master September 21, 2022 14:24
@lobsterkatie lobsterkatie force-pushed the kmclb-nextjs-wrap-api-routes branch 4 times, most recently from f7f8dbd to e1dbf7c Compare September 23, 2022 07:44
Comment on lines +32 to +36
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_}.`,
);
}
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.)

Copy link
Contributor

@vladanpaunovic vladanpaunovic left a comment

Choose a reason for hiding this comment

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

Left a few comments here.

Overall, I think that there are too many comments in this PR making the code harder to read.

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.

@@ -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

@@ -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...)

@@ -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.

@lobsterkatie lobsterkatie marked this pull request as ready for review September 26, 2022 04:11
@lobsterkatie lobsterkatie merged commit 1d8370e into master Sep 26, 2022
@lobsterkatie lobsterkatie deleted the kmclb-nextjs-wrap-api-routes branch September 26, 2022 04:12
@daniellin215
Copy link

So we don't need to manually wrap routes anymore for next.js? This isn't updated/reflected in the documentation.
https://docs.sentry.io/platforms/javascript/guides/nextjs/

@lforst
Copy link
Member

lforst commented Oct 1, 2022

@daniellin215 This feature is currently sort of in an experimental state and we only really want newly onboarding users to have it - hence no mentioning of this in the docs. People currently have to either manually set the autoInstrumentServerFunctions option or install the SDK via @sentry/wizard to get this feature.

We plan to make the auto wrapping the default in the coming week and with that, we will also update the documentation. The docs will probably not mention withSentry in the default setup guide anymore since the wrapping is done automatically anyways.

@youssefm
Copy link

youssefm commented Mar 11, 2023

@lobsterkatie @lforst it sounds like you guys went to really great lengths to try to wrap everything that NextJS does. It's pretty crazy to have to write a custom webpack loader that renders a template to call specific wrapper code just to do something that's pretty basic in other frameworks with middleware.

Did you all try to talk to the NextJS team to see if they could provide a simpler, first-class way to handle NextJS errors?

@lforst
Copy link
Member

lforst commented Mar 13, 2023

@youssefm Yeah the logic in our Next.js SDK is a bit hacky. It had its hiccups at first but now it is pretty stable. Unfortunately, we had to go that route because Next.js doesn't provide a "neat" way of doing things. I think you are correct that stuff like this is quite basic in other frameworks.

We are in direct communication with Vercel but getting in changes from our side is not very easy at times and it also always takes ages. It seems like the Next.js team at Vercel has a pretty clear vision of how things should be and they don't want to steer away from it too much.

@youssefm
Copy link

Thanks for the context @lforst. And nicely done finding a workaround for Next.js' limitations.

I'm finding myself trying to do something similar (wrapping all the API routes with custom code) and it's cool to see Sentry found a way to do so, despite how involved the solution is.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants