Next.js auto instrumentation hides properties of wrapped api routes

[dhmacs]

Is there an existing issue for this?

• I have checked for existing issues https://github.com/getsentry/sentry-javascript/issues
• I have reviewed the documentation https://docs.sentry.io/
• I am using the latest SDK release https://github.com/getsentry/sentry-javascript/releases

How do you use Sentry?

Sentry Saas (sentry.io)

Which package are you using?

@sentry/nextjs

SDK Version

7.33.0

Framework Version

Next.js 12.2.3

Link to Sentry event

No response

SDK Setup

Sentry.init({
   dsn: SENTRY_DSN,
   integrations: [new BrowserTracing()],
   sampleRate: 1.0,
   normalizeDepth: 5,
   tracesSampleRate: 0.05,
   // ...
   // Note: if you want to override the automatic release value, do not set a
   // `release` value here - use the environment variable `SENTRY_RELEASE`, so
   // that it will also get attached to your source maps
   initialScope: {
      tags: {
         'next.runtime': 'client',
      },
   },
   ignoreErrors: [
      'TypeError: NetworkError when attempting to fetch resource.',
      'TypeError: Network request failed',
      // Algolia error when network requests fail.
      // Only happens on iOS devices (across all browsers)
      'RetryError: Unreachable hosts - your application id may be incorrect',
      // Can't reproduce, promise rejection with an instance of a CustomEvent
      // is unhandled.
      // Only happens on Macs, mostly Chrome, but some on safari
      'CustomEvent: Non-Error promise rejection captured with keys: currentTarget, detail, isTrusted, target',
   ],
   beforeSend: (event, hint) => {
      const ex = hint.originalException;
      if (ex && typeof ex == 'object' && ex.message) {
         // Sample hydration errors.
         if (hydrationErrors.some((msg) => ex.message.match(msg))) {
            return Math.random() < 0.05 ? event : null;
         }
      }
      return event;
   },
});

Steps to Reproduce

Context

We use our api endpoint to export a method to call the api within getServerSideProps without a network call.

Example:

const withFoo = (fn) => {
  const handler = async (req, res) => res.json(fn());
  handler.get = () => { return fn(); }
  return handler;
}

We then declare an endpoint:

// /api/hello-world.js
export default withFoo(() => "hello world!");

and we use it like this in out getServerSideProps method:

import HelloWorld from "@pages/api/hello-world";

export const getServerSideProps = async () => {
      const greeting = await HelloWorld.get();
      return {
         props: { greeting },
      };
   });

This is just a dummy example, if you want to check what we're using this for, here's the reference: https://github.com/iFixit/react-commerce/blob/add-product-caching/frontend/pages/api/nextjs/cache/product.ts (TLDR: An SWR server side caching mechanism with Redis)

Problem

With the introduction of autoInstrumentServerFunctions we are unable to access the .get static method that we export from our api endpoint because Sentry wraps the function without re-exporting any attached static prop.
To avoid breaking this use cases Sentry wrappers needs to copy over any static method (Just for reference this is similar to the problem of writing higher order component in React https://reactjs.org/docs/higher-order-components.html#static-methods-must-be-copied-over)

Expected Result

Sentry wrapper should re-export any static method to avoid breaking this advanced use cases

Actual Result

Webpack throws an error because it can't find HelloWorld.get method exported from the module

[lforst]

Thanks for the detailed description! Currently, I am unsure whether we should handle this case as it is a bit edge-casey.
Just from a breakage perspective, I get it, but there is nothing preventing users from exporting the static methods separately. I will discuss this with the team.

[Sparragus]

This is not edge-casey. The very same situation happened to me. It broke our Quirrel background jobs. In this case, I cannot export static methods separately. They were set by a third-party library I don't maintain.

To make it work, I had to use excludeServerRoutes to exclude the API routes that Quirrel uses.

And as a separate note, it's very disheartening to read these sort of responses from Sentry. This is the second thing that I read this week where Sentry breaks something, and "brushes off" users telling them it's up them to fix it by doing things differently. See #6782 and #6846.

[lforst]

@Sparragus yeah I totally get it. I am not happy either that this is happening. We pretty much poured our heart and soul into this SDK and issues keep creeping up 😢 The ecosystem is just moving very quickly all the time.

As for this issue, we have to make trade-offs. It is always the same: having good instrumentation with as little setup as possible vs. breaking some users who happen to do something in a way you didn't account for. What I want to avoid doing right now is drafting up a knee-jerk reaction fix that makes this work again but creates more issues down the line.

To make it work, I had to use excludeServerRoutes to exclude the API routes that Quirrel uses.

We introduced excludeServerRoutes for exactly this use case - as an escape hatch - so it does exactly the right thing and I even encourage you to use it. We generally aim to just make it work for the 99% and provide these escape hatches for the rest. We can never make everything work for everybody. It is an impossible task.

And as a separate note, it's very disheartening to read these sort of responses from Sentry. This is the second thing that I read this week where Sentry breaks something, and "brushes off" users telling them it's up them to fix it by doing things differently. See #6782 and #6846.

I really really try not to brush off people. With #6782 we identified an upstream issue, created an issue in the Next.js repository, pinged Vercel in an internal chat, and we gave people a temporary workaround. There is literally nothing more we can do. As for #6846 I don't see why this is brushing somebody off. The user simply made a suggestion that we could do something in the next major if we wanted to. It's only natural for us to prioritize community input - we can't jump onto every suggestion somebody brings up. Having to prioritize is the sad reality of open source. We would love to do everything, but can't.

I hope this gives a little insight into what our thought process is around these things. Hearing somebody say that something I said is disheartening is honestly breaking my own heart a little. It is definitely never my intention to brush somebody off. If you have suggestions on how we can do stuff better in the future let us know - it always helps to have concrete action items.

[Sparragus]

Hey @lforst. Sorry. I think I was too harsh with my words. I know you guys give it your all and that building software is complex and full of tradeoffs.

Thanks for replying and sharing all the insights and emotions.

Again, apologies for the harshness.

[dhmacs]

@lforst

As for this issue, we have to make trade-offs. It is always the same: having good instrumentation with as little setup as possible vs. breaking some users who happen to do something in a way you didn't account for. What I want to avoid doing right now is drafting up a knee-jerk reaction fix that makes this work again but creates more issues down the line.

You're doing a fantastic job at removing setup with this webpack plugin, but I think that the wrapping implementation is incomplete, because it doesn't preserve the default Javascript behaviour that a user would expect.
A plain JS code for preserving behaviour would look like this:

const withSentry = (fn) => {
  const wrapped = async (...args) => {
    // Do sentry stuff
    res = await fun(...args);
    // Do other sentry stuff
    return res;
  };
  return Object.assign(wrapped, fn);  
}

The key here is using Object.assign to copy over the static properties of the function being wrapped.

The implementation being incomplete results in an unexpected error and in more setup for us to do because we have to disable these routes and instrument them ourselves, so a worse DX overall imo