search

getsentry / sentry-javascript

Official Sentry SDKs for JavaScript
https://sentry.io
MIT License
7.98k stars 1.57k forks source link

`Maximum call stack size exceeded` after upgrading 7.114.0 -> 8.27.0 #13519

Open harry-gocity opened 2 months ago

harry-gocity commented 2 months ago

Is there an existing issue for this?

How do you use Sentry?

Sentry Saas (sentry.io)

Which SDK are you using?

@sentry/nextjs

SDK Version

8.27.0

Framework Version

React 18.3, Next.js 14.1.4

Link to Sentry event

https://go-city.sentry.io/issues/5767473230

Reproduction Example/SDK Setup

// sentry.client.ts

Sentry.init({
  enabled: isProduction && isSupportedBrowser,
  dsn: process.env.NEXT_PUBLIC_SENTRY_DSN,
  environment: process.env.NEXT_PUBLIC_ENVIRONMENT ?? "local",
  release: isProduction ? process.env.SENTRY_RELEASE : `development-${process.env.USER}`,
  ignoreErrors: ["jQuery"],
  replaysSessionSampleRate: parseFloat(process.env.NEXT_PUBLIC_SENTRY_REPLAY_SESSION_SAMPLE_RATE ?? "0.0"),
  replaysOnErrorSampleRate: 1,
  allowUrls: isProduction
    ? [
        // Match the sites and GTM errors only
        /localhost/,
        /gocity/,
        /londonpass/,
        /parispass/,
        /newyorkpass/,
        /romeandvaticanpass/,
        /gtm.js/,
      ]
    : [],
  integrations: [
    // HttpClient to capture any "Failed to fetch" errors
    Sentry.httpClientIntegration(),
    // Sentry Replay to set up session recording
    Sentry.replayIntegration({
      maskAllText: false,
    }),
  ],
});

// instrumentation.node.ts and instrumentation.edge.ts

Sentry.init({
  enabled: isProduction,
  dsn: process.env.NEXT_PUBLIC_SENTRY_DSN,
  environment: process.env.NEXT_PUBLIC_ENVIRONMENT ?? "local",
  release: isProduction ? process.env.SENTRY_RELEASE : `development-${process.env.USER}`,
});

// instrumentation.ts

export async function register() {
  if (process.env.NEXT_RUNTIME === "nodejs") {
    await import("@/instrumentation.node");
  }

  if (process.env.NEXT_RUNTIME === "edge") {
    await import("@/instrumentation.edge");
  }
}

Steps to Reproduce

We just upgraded from Sentry 7.114.0 to 8.27.0 and have since had a few 'Maximum call stack size exceeded' errors appear in Sentry that trace back to /usr/app/node_modules/.pnpm/@sentry+utils@8.27.0/node_modules/@sentry/utils/build/cjs/syncpromise.js and /usr/app/node_modules/.pnpm/@sentry+core@8.27.0/node_modules/@sentry/core/build/cjs/eventProcessors.js in a loop.

To upgrade from 7 to 8 we:

Expected Result

We didn't see these issues before but we do now. All events seem to be happening Node, none from the browser.

Actual Result

No new errors 🙏

lforst commented 2 months ago

Hi, thanks for writing in! Are you adding any event processors somewhere?

harry-gocity commented 2 months ago

Sorry, I should have included those.

We have a basic traces sampler in the client and server config:

tracesSampler = (samplingContext: SamplingContext) => {
  if (
    samplingContext.transactionContext.name === "GET /api/health-check" ||
    samplingContext.transactionContext.name === "GET /api/metrics/prometheus"
  ) {
    return 0;
  } else {
    return defaultTraceSampleRate; // number parsed from env variable
  }
};

And a beforeSend in the client config only that filters out Failed to Fetch and Load Failed noise from the HttpClient integration (it's quite long but just a lot of conditional checks on breadcrumbs and ErrorEvent, I can sanitise it and upload if needed, but this issue appears to be happening in Node, not the browser).

We don't use anything like Sentry.addEventProcessor anywhere.

lforst commented 2 months ago

Hmm, I don't see anything anywhere that might cause this. Can you reproduce this with a minimal setup that you could share with us?

harry-gocity commented 2 months ago

Honestly, no. This is happening in production for us and since the stack trace is entirely Sentry's own modules I have absolutely no idea where this error is coming from. I can link more issues for the same problem that haven't been grouped together?

https://go-city.sentry.io/issues/5768588849 https://go-city.sentry.io/issues/5769616185 https://go-city.sentry.io/issues/5770174627 https://go-city.sentry.io/issues/5767522248 https://go-city.sentry.io/issues/5767533755 https://go-city.sentry.io/issues/5767473230

We have one other Maximum call stack size exceeded issue that has been ongoing at a much lower frequency for several months. The stack trace is different but still just Sentry modules: https://go-city.sentry.io/issues/4629472638

harry-gocity commented 2 months ago

Just been digging around existing issues further and came across this comment:

https://github.com/getsentry/sentry-javascript/issues/5252#issuecomment-1386433460

My tests indicate that @sentry/nextjs integration invokes Sentry.init() twice on the server under certain conditions. To reproduce:

Since we are self hosting Next.js in Node (no edge) I wonder if process.env.NEXT_RUNTIME is problematic and Sentry.init() is being called twice as both our node and edge instrumentation files are being imported. I'll investigate that a bit.

harry-gocity commented 2 months ago

I think this could be the issue. In Next 14.1.4, instrumentation.ts runs twice times, but process.env.NEXT_RUNTIME actually changes. So actually, the conditional import doesn't prevent instrumentation.edge.ts running, it just delays it, and it eventually runs twice but in the same effective env. So Sentry.init could be called multiple times.

Now that Sentry.edge.config.ts is not required, I'm going to remove our 'edge' Sentry initialisation altogether, and see if that helps.

It sounds like they've fixed this in Next 15 Canary: https://github.com/vercel/next.js/issues/51450#issuecomment-2298627701

lforst commented 2 months ago

All of this smells very funky to me. Can you double check that Sentry.init() is not called over and over again through imports or anything? That is usually what leads to these kinds of errors if there aren't any explicit Sentry.addEventProcessor() calls or similar.

harry-gocity commented 2 months ago

Yes, I think that's what's happening. We have an instrumentation.ts like this:

export async function register() {
  if (process.env.NEXT_RUNTIME === "nodejs") {
    await import("@/instrumentation.node");
  }

  if (process.env.NEXT_RUNTIME === "edge") {
    await import("@/instrumentation.edge");
  }
}

Which effectively does this:

export async function register() {
  if (process.env.NEXT_RUNTIME === "nodejs") {
    Sentry.init(...)
  }

  if (process.env.NEXT_RUNTIME === "edge") {
    Sentry.init(...)
  }
}

If you make the reasonable assumption register() is only called once per runtime environment, at a glance it looks like Sentry will only be initialised once, depending on the environment it's running in. As we only have one environment, I would expect the edge conditional to be a noop.

What I found actually happens, is Next.js runs register twice, and changes the environment variable. It runs once with NEXT_RUNTIME as nodejs and once as edge. So Sentry.init() is called twice.

This likely wasn't an issue for us in v7 since we were using sentry.server.config.ts and sentry.edge.config.ts, rather than the v8 shared instrumentation.ts approach.

It may be worth flagging this in the docs? https://docs.sentry.io/platforms/javascript/guides/nextjs/manual-setup/#create-initialization-config-files

lforst commented 2 months ago

@harry-gocity it's fine that register is called twice as long as things are guarded with the NEXT_RUNTIME check. This is totally by design. Next.js will spawn one process for edge and one for node and the two init calls will not interfere. Something else must be going on 🤔

I fear this is very hard to debug without having something locally reproducible or at least access to the relevant code. I am happy to take a quick look at your repo if you provide access. We ensure confidentiality.