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

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

Open
3 tasks done
harry-gocity opened this issue Aug 29, 2024 · 9 comments
Open
3 tasks done
Labels
Package: nextjs Issues related to the Sentry Nextjs SDK

Comments

@harry-gocity
Copy link

harry-gocity commented Aug 29, 2024

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/@[email protected]/node_modules/@sentry/utils/build/cjs/syncpromise.js and /usr/app/node_modules/.pnpm/@[email protected]/node_modules/@sentry/core/build/cjs/eventProcessors.js in a loop.

To upgrade from 7 to 8 we:

  • Moved out sentry.server.config.ts and sentry.edge.config.ts to Next.js instrumentation.ts (We don't actually use the edge runtime, we are self hosting on Node, but we moved the edge config anyway)
  • Combined our SentryWebpackPluginOptions and UserSentryOptions into a single SentryBuildOptions param for withSentryConfig
  • Replaced out httpClientIntegration and replayIntegration with the functional alternative

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 🙏

@github-actions github-actions bot added the Package: nextjs Issues related to the Sentry Nextjs SDK label Aug 29, 2024
@harry-gocity harry-gocity changed the title Maximum call stack size exceeded after upgrading to V8 Maximum call stack size exceeded after upgrading 7.114.0 -> 8.27.0 Aug 29, 2024
@lforst
Copy link
Member

lforst commented Aug 29, 2024

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

@harry-gocity
Copy link
Author

harry-gocity commented Aug 29, 2024

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
Copy link
Member

lforst commented Aug 29, 2024

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
Copy link
Author

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
Copy link
Author

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

#5252 (comment)

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
Copy link
Author

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: vercel/next.js#51450 (comment)

@lforst
Copy link
Member

lforst commented Aug 29, 2024

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
Copy link
Author

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
Copy link
Member

lforst commented Aug 30, 2024

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Package: nextjs Issues related to the Sentry Nextjs SDK
Projects
Status: No status
Development

No branches or pull requests

2 participants