getsentry / sentry-javascript

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

Critical Dependency Warning After Upgrading to @sentry/nextjs v8 #12077

Closed saraluk closed 2 weeks ago

saraluk commented 5 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.1.0

Framework Version

next 14.2.3

Link to Sentry event

No response

SDK Setup

Sentry.init({
  dsn: "change me",
  tracesSampleRate: 1,
  debug: false,
  integrations: [
    Sentry.feedbackIntegration({
      autoInject: false,
    }),
  ],
});

Steps to Reproduce

Upgrade @sentry/nextjs from v7.113.0 to v8.1.0 and run the project build process

Expected Result

The build process should complete without warnings related to critical dependencies.

Actual Result

After upgrading to @sentry/nextjs version 8, I encountered the following warning in the build logs:

⚠ ./node_modules/@prisma/instrumentation/node_modules/@opentelemetry/instrumentation/build/esm/platform/node/instrumentation.js
Critical dependency: the request of a dependency is an expression

Import trace for requested module:
./node_modules/@prisma/instrumentation/node_modules/@opentelemetry/instrumentation/build/esm/platform/node/instrumentation.js
./node_modules/@prisma/instrumentation/node_modules/@opentelemetry/instrumentation/build/esm/platform/node/index.js
./node_modules/@prisma/instrumentation/node_modules/@opentelemetry/instrumentation/build/esm/platform/index.js
./node_modules/@prisma/instrumentation/node_modules/@opentelemetry/instrumentation/build/esm/index.js
./node_modules/@prisma/instrumentation/dist/chunk-64MZJIQ5.js
./node_modules/@prisma/instrumentation/dist/index.js
./node_modules/@sentry/node/cjs/integrations/tracing/prisma.js
./node_modules/@sentry/node/cjs/index.js
./node_modules/@sentry/nextjs/cjs/index.server.js
./app/global-error.tsx
mydea commented 5 months ago

Hmm, let's see if this: https://github.com/getsentry/sentry-javascript/pull/12081 possibly fixes this 🤔

gusfune commented 5 months ago

I can also confirm this is going on 8.2.1 version.

MarwanAlsoltany commented 5 months ago

I can also confirm this is going on 8.2.1 version.

+1

lforst commented 5 months ago

This warning can be safely ignored. In theory, the SDK should add an ignore rule for this warning. I wonder why that doesn't work for you 🤔

https://github.com/getsentry/sentry-javascript/blob/7e59832817f20becb304b82a05bb555a7f315c6e/packages/nextjs/src/config/webpack.ts#L662-L669

Would you mind sharing your next.config.js? Can you try adding the following to your ignoreWarnings in your Next.js webpack config?

[
    { module: /@opentelemetry\/instrumentation/, message: /Critical dependency/ },
    { module: /@prisma\/instrumentation/, message: /Critical dependency/ },
]
GiancarlosIO commented 4 months ago

Hi there. I'm using @sentry/nextjs v8.7.0 and I still get the warning in the console:

- warn ../../node_modules/.pnpm/@opentelemetry+instrumentation@0.51.1_@opentelemetry+api@1.8.0/node_modules/@opentelemetry/instrumentation/build/src/platform/node/instrumentation.js
Critical dependency: the request of a dependency is an expression

Import trace for requested module:
../../node_modules/.pnpm/@opentelemetry+instrumentation@0.51.1_@opentelemetry+api@1.8.0/node_modules/@opentelemetry/instrumentation/build/src/platform/node/instrumentation.js
../../node_modules/.pnpm/@opentelemetry+instrumentation@0.51.1_@opentelemetry+api@1.8.0/node_modules/@opentelemetry/instrumentation/build/src/platform/node/index.js
../../node_modules/.pnpm/@opentelemetry+instrumentation@0.51.1_@opentelemetry+api@1.8.0/node_modules/@opentelemetry/instrumentation/build/src/platform/index.js
../../node_modules/.pnpm/@opentelemetry+instrumentation@0.51.1_@opentelemetry+api@1.8.0/node_modules/@opentelemetry/instrumentation/build/src/index.js
../../node_modules/.pnpm/@sentry+opentelemetry@8.7.0_@opentelemetry+api@1.8.0_@opentelemetry+core@1.24.1_@opentelemetr_easlgnajsmadvvyfptwg27pvmy/node_modules/@sentry/opentelemetry/cjs/index.js
../../node_modules/.pnpm/@sentry+node@8.7.0/node_modules/@sentry/node/cjs/index.js
../../node_modules/.pnpm/@sentry+nextjs@8.7.0_@opentelemetry+api@1.8.0_@opentelemetry+core@1.24.1_@opentelemetry+api@1_n5hycvwuc2wadypxzgqutrwu2y/node_modules/@sentry/nextjs/cjs/index.server.js

Is the issue closed because the fix is going to be released in a new version? 🤔

lforst commented 4 months ago

@GiancarlosIO It's not really a fix we published but we are ignoring the warning automatically. The code for this is here: https://github.com/getsentry/sentry-javascript/blob/7de5ea40e29125b0092138e419e0a3be20c296d5/packages/nextjs/src/config/webpack.ts#L663

I struggle a bit to understand how this is happening. Would you mind sharing reproduction? Thanks!

cyrus-za commented 4 months ago

I updated to @sentry/nextjs 8.9.2 and still getting this warning log

lforst commented 4 months ago

@cyrus-za would you mind sharing more information? The import trace would be helpful.

cyrus-za commented 4 months ago

@cyrus-za would you mind sharing more information? The import trace would be helpful.

I setup my nextjs project with the instrumentation hook defined here

Using next 14.2.3, nx 19.2.2, pnpm 9.1.0 and node 20.14.0

I got a nx lib for my wrapper around sentry. Not sure if that makes a difference.

Everything worked just fine before the switch to v8

Critical dependency: the request of a dependency is an expression

Import trace for requested module:
../../node_modules/.pnpm/@opentelemetry+instrumentation@0.51.1_@opentelemetry+api@1.9.0/node_modules/@opentelemetry/instrumentation/build/esm/platform/node/instrumentation.js
../../node_modules/.pnpm/@opentelemetry+instrumentation@0.51.1_@opentelemetry+api@1.9.0/node_modules/@opentelemetry/instrumentation/build/esm/platform/node/index.js
../../node_modules/.pnpm/@opentelemetry+instrumentation@0.51.1_@opentelemetry+api@1.9.0/node_modules/@opentelemetry/instrumentation/build/esm/platform/index.js
../../node_modules/.pnpm/@opentelemetry+instrumentation@0.51.1_@opentelemetry+api@1.9.0/node_modules/@opentelemetry/instrumentation/build/esm/index.js
../../node_modules/.pnpm/@prisma+instrumentation@5.15.0/node_modules/@prisma/instrumentation/dist/chunk-B5GLSQ4E.js
../../node_modules/.pnpm/@prisma+instrumentation@5.15.0/node_modules/@prisma/instrumentation/dist/index.js
../../node_modules/.pnpm/@sentry+node@8.9.2/node_modules/@sentry/node/cjs/integrations/tracing/prisma.js
../../node_modules/.pnpm/@sentry+node@8.9.2/node_modules/@sentry/node/cjs/index.js
../../node_modules/.pnpm/@sentry+nextjs@8.9.2_@opentelemetry+api@1.9.0_@opentelemetry+core@1.25.0_@opentelemetry+api@1_ujmyanxtchbaumo3rdotqhac5q/node_modules/@sentry/nextjs/cjs/index.server.js
../utils/sentry/src/lib/sentry.ts
../utils/sentry/src/index.ts
./src/app/layout.tsx
lforst commented 4 months ago

It might be nx that is messing with our ignoring of the warning.

@GiancarlosIO are you also using nx?

Please note: It is safe to ignore this warning. All it does, is say that webpack is unable to statically analyze an import. This may have some implications with regards to bundling server code but in 99.999% of the cases this should be fine.

cyrus-za commented 4 months ago

In case it helps, here's my nextjs config

/* eslint-disable  n/no-process-env */
import { composePlugins, withNx } from '@nx/next'
import { withSentryConfig } from '@sentry/nextjs'
import webpack from 'webpack'
import { BundleAnalyzerPlugin } from 'webpack-bundle-analyzer'

/** @type {import('@nx/next/plugins/with-nx').WithNxOptions} */

const config = {
  experimental: {
    instrumentationHook: true,
  },
  typescript: {
    tsconfigPath: 'tsconfig.app.json',
    // `build` depends on `type-check` so we don't need to run `type-check` separately
    ignoreBuildErrors: true,
  },
  nx: {
    // Set this to true if you would like to use SVGR
    // See: https://github.com/gregberge/svgr
    svgr: false,
  },
  webpack(cfg, { isServer, nextRuntime }) {
    // eslint-disable-next-line no-param-reassign
    cfg.resolve.alias.canvas = false

    if (isServer && nextRuntime === 'nodejs') {
      cfg.plugins.push(new webpack.IgnorePlugin({ resourceRegExp: /@aws-sdk\/signature-v4-crt/ }))
    }

    return cfg
  },
  reactStrictMode: true,
  images: {
    remotePatterns: [
      {
        protocol: 'https',
        hostname: 'lh3.googleusercontent.com',
        port: '',
        pathname: '/**/*',
      },
      {
        protocol: 'https',
        hostname: 's.gravatar.com',
        port: '',
        pathname: '/**/*',
      },
    ],
  },
  transpilePackages: ['jotai-devtools', 'react-pdf'],
}

const sentryConfig = {
  // For all available options, see:
  // https://docs.sentry.io/platforms/javascript/guides/nextjs/manual-setup/

  // Upload a larger set of source maps for prettier stack traces (increases build time)
  widenClientFileUpload: true,

  // Transpiles SDK to be compatible with IE11 (increases bundle size)
  transpileClientSDK: false,

  // Hides source maps from generated client bundles
  hideSourceMaps: true,

  automaticVercelMonitors: true,

  // Suppresses source map uploading logs during build
  silent: true,
}

const plugins = [
  // Add more Next.js plugins to this list if needed.
  withNx,
]

/** @type {import('@nx/next/plugins/with-nx').WithNxOptions} */

const configWithSentry = process.env.VERCEL ? withSentryConfig(config, sentryConfig) : config

export default composePlugins(...plugins)(configWithSentry)
matmannion commented 4 months ago

I've managed to get this working by effectively duplicating the Sentry code for this myself in my Next config:

/** @type {import('next').NextConfig} */
const nextConfig = withSentryConfig(
  ...
  webpack(
    /** @type {import('webpack').WebpackOptionsNormalized} */ config,
    /** @type {import('next/dist/server/config-shared').WebpackConfigContext} */ context
  ) {
    config.ignoreWarnings = [
      ...(config.ignoreWarnings ?? []),
      (warning, { requestShortener }) => {
        const isOtelModule =
          !!warning.module &&
          (/@opentelemetry\/instrumentation/.test(warning.module.readableIdentifier(requestShortener)) ||
            /@prisma\/instrumentation/.test(warning.module.readableIdentifier(requestShortener)))

        const isCriticalDependencyMessage = /Critical dependency/.test(warning.message)

        return isOtelModule && isCriticalDependencyMessage
      },
    ]
  },
  ...
)

I'm not sure if the reason the Sentry code only catches one of the two errors is because it's only looking at the server context?

cyrus-za commented 3 months ago

@lforst Why is this issue closed?

lforst commented 3 months ago

Since we fixed the original issue and mostly due to lack of minimal reproduction. Also, I am pretty confident that it is a downstream that does unorthodox stuff. However, I want to try @matmannion's approach.

matmannion commented 3 months ago

Since we fixed the original issue and mostly due to lack of minimal reproduction. Also, I am pretty confident that it is a downstream that does unorthodox stuff. However, I want to try @matmannion's approach.

I haven't tried it but I think Next has some variance in config parsing if you use a next.config.mjs file instead of .js, may help with reproduction

hpohl-pley commented 2 months ago

Even the attempt at ignoring the warning is not fixing it for us, NextJS 14.2.4, @sentry/nextjs 8.26.0.

berci-i commented 1 month ago

I am also encoutering this issue

mikaeledgren commented 1 month ago

Same here

My bad, I had setup my sentry config in next.config.js, but had forgotten to add it to my plugins list (I use composePlugins from nx) 😇

ming535 commented 1 month ago

I am also having this issue: next: 14.2.10 @sentry/nextjs@8.30.0

taylor-lindores-reeves commented 1 month ago

I only seem to be facing this issue when importing @sentry/nextjs in client components.

lforst commented 1 month ago

If possible please share reproduction. We cannot act on "+1" comments.

mattsacks commented 1 month ago

@lforst I've reproduced the error while debugging a separate issue with dd-trace here. To remove the stack trace, @sentry/nextjs would need to be added as an external package by adding this configuration.

It seems like this wasn't added to the default list due to the impact it has on boot times: https://github.com/vercel/next.js/pull/61194

lforst commented 1 month ago

@mattsacks we explicitly do not want to be added to the external list. It comes with a plethora of issues.

mrmckeb commented 4 weeks ago

We're seeing this issue after upgrading to the latest Next.js canary release too.

taylor-lindores-reeves commented 4 weeks ago

@mattsacks we explicitly do not want to be added to the external list. It comes with a plethora of issues.

Edit: reviewed #61194 which answers my question. Boot / cold start increase on require statements by 350ms. Hopefully Sentry can do something about this but will be removing @sentry/nextjs from externals list for this reason.

@lforst please could you share some insight as to why adding this package to the externals list would cause issues? Is this specifically regarding otel instrumentation, or a much wider range of issues?

I've added @sentey/nextjs to my externals list, warning has disappeared and I'm not facing any issues thus far. It would be nice to understand a bit about what's happening under the hood.

lforst commented 4 weeks ago

@taylor-lindores-reeves much wider range. The way Next.js works is that it would even prevent us from having client components (ie components that use use client) in our package. Also, otel.

Sicria commented 3 weeks ago

I'm seeing the same thing whenever import * as Sentry from '@sentry/nextjs'; is used in a "use client" component in NextJS without the withSentryConfig being applied to the next.config.js, eg for local development when there's no NEXT_PUBLIC_SENTRY_DSN set we don't apply the withSentryConfig wrapper.

max-degterev commented 3 weeks ago

Hilarious, an obvious bug in error tracing module left open for months.

And by open I mean closed without fixing of course.

chris-erickson commented 3 weeks ago

This began happening from Sentry v8.28.0 upgrading to v8.29.0 for us, NextJS v14.2.13

hayata-suenaga commented 2 weeks ago

I can confirm that the issue started happening with @sentry/nextjs 8.32.0 and next 14.2.13.

lforst commented 2 weeks ago

Hilarious, an obvious bug in error tracing module left open for months.

And by open I mean closed without fixing of course.

@max-degterev I wouldn't classify this as a bug. It is merely a warning emitted by Webpack. Nothing will (or should) break because of it.

The problem raised in the original post was fixed, but it appears that in certain situations the warning is still logged.

I am gonna reopen this issue. I just tried @mattsacks' repro but I failed to reproduce the issue reaised.

If somebody is able to provide a minimal reproduction example we can take a closer look.

mattsacks commented 2 weeks ago

@lforst Here's a stack trace running on the sentry-nextjs-expression-dependency branch of that repo:

~/code/dd-trace-next sentry-nextjs-expression-dependency
❯ nvm install latest
Installing Node v22.9.0 latest
Fetching https://nodejs.org/dist/v22.9.0/node-v22.9.0-darwin-arm64.tar.gz
Now using Node v22.9.0 (npm 10.8.3) ~/.local/share/nvm/v22.9.0/bin/node

~/code/dd-trace-next sentry-nextjs-expression-dependency
❯ rm -rf node_modules .next

~/code/dd-trace-next sentry-nextjs-expression-dependency
❯ npm i
npm warn deprecated inflight@1.0.6: This module is not supported, and leaks memory. Do not use it. Check out lru-cache if you want a good and tested way to coalesce async requests by a key value, which is much more comprehensive and powerful.
npm warn deprecated @humanwhocodes/config-array@0.11.14: Use @eslint/config-array instead
npm warn deprecated rimraf@3.0.2: Rimraf versions prior to v4 are no longer supported
npm warn deprecated @humanwhocodes/object-schema@2.0.3: Use @eslint/object-schema instead
npm warn deprecated glob@7.2.3: Glob versions prior to v9 are no longer supported

added 601 packages, and audited 602 packages in 3s

153 packages are looking for funding
  run `npm fund` for details

4 vulnerabilities (1 moderate, 3 high)

To address all issues, run:
  npm audit fix

Run `npm audit` for details.

~/code/dd-trace-next sentry-nextjs-expression-dependency
❯ npm run dev

> dd-trace@0.1.0 dev
> next dev

 ⚠ Port 3000 is in use, trying 3001 instead.
  ▲ Next.js 14.2.11
  - Local:        http://localhost:3001
  - Experiments (use with caution):
    · instrumentationHook

 ✓ Starting...
 ○ Compiling /instrumentation ...
 ⚠ ./node_modules/@prisma/instrumentation/node_modules/@opentelemetry/instrumentation/build/esm/platform/node/instrumentation.js
Critical dependency: the request of a dependency is an expression

Import trace for requested module:
./node_modules/@prisma/instrumentation/node_modules/@opentelemetry/instrumentation/build/esm/platform/node/instrumentation.js
./node_modules/@prisma/instrumentation/node_modules/@opentelemetry/instrumentation/build/esm/platform/node/index.js
./node_modules/@prisma/instrumentation/node_modules/@opentelemetry/instrumentation/build/esm/platform/index.js
./node_modules/@prisma/instrumentation/node_modules/@opentelemetry/instrumentation/build/esm/index.js
./node_modules/@prisma/instrumentation/dist/chunk-6MQXHI6Y.js
./node_modules/@prisma/instrumentation/dist/index.js
./node_modules/@sentry/node/build/cjs/integrations/tracing/prisma.js
./node_modules/@sentry/node/build/cjs/index.js
./node_modules/@sentry/nextjs/build/cjs/index.server.js
(node:71250) [DEP0005] DeprecationWarning: Buffer() is deprecated due to security and usability issues. Please use the Buffer.alloc(), Buffer.allocUnsafe(), or Buffer.from() methods instead.
(Use `node --trace-deprecation ...` to show where the warning was created)
 ✓ Ready in 4.8s

Let me know if there's anything else I can provide.

lforst commented 2 weeks ago

@mattsacks Ok so you're lacking like a really critical piece of the SDK setup which is wrapping your nextConfig in next.config.mjs in withSentryConfig().

It is outlined here: https://docs.sentry.io/platforms/javascript/guides/nextjs/manual-setup/#extend-your-nextjs-configuration

lforst commented 2 weeks ago

I am gonna carefully close this again because I think we're good. (?) Lmk if this is still an issue with the intended setup.

mattsacks commented 2 weeks ago

@lforst that fixes the error in my repro case. Thanks!

tom2strobl commented 1 week ago

FYI: I still experience the warning on node 20.17.0 with esm-style type: "module" @sentry/electron: 5.6.0 @sentry/node: ^8.34.0 webpack: 5.75.0 (not on @sentry/nextjs)

WARNING in ../node_modules/@opentelemetry/instrumentation/build/esm/platform/node/instrumentation.js 139:37-58
Critical dependency: the request of a dependency is an expression
 @ ../node_modules/@opentelemetry/instrumentation/build/esm/platform/node/index.js 16:0-56 16:0-56
 @ ../node_modules/@opentelemetry/instrumentation/build/esm/platform/index.js 16:0-56 16:0-56 16:0-56
 @ ../node_modules/@opentelemetry/instrumentation/build/esm/index.js 17:0-55 17:0-55
 @ ../node_modules/@sentry/node/node_modules/@sentry/opentelemetry/build/esm/index.js 10:0-74 2185:2-26
 @ ../node_modules/@sentry/node/build/esm/index.js 43:0-163 43:0-163 43:0-163 43:0-163
 @ ./app/services/utils/captureExceptionNode.ts 57:0-43 69:16-43
 @ ./app/main/main.ts 234:24-75

WARNING in ../node_modules/@prisma/instrumentation/node_modules/@opentelemetry/instrumentation/build/esm/platform/node/instrumentation.js 143:37-58
Critical dependency: the request of a dependency is an expression
 @ ../node_modules/@prisma/instrumentation/node_modules/@opentelemetry/instrumentation/build/esm/platform/node/index.js 16:0-56 16:0-56
 @ ../node_modules/@prisma/instrumentation/node_modules/@opentelemetry/instrumentation/build/esm/platform/index.js 16:0-56 16:0-56 16:0-56
 @ ../node_modules/@prisma/instrumentation/node_modules/@opentelemetry/instrumentation/build/esm/index.js 17:0-55 17:0-55
 @ ../node_modules/@prisma/instrumentation/dist/chunk-6MQXHI6Y.js 26:29-70
 @ ../node_modules/@prisma/instrumentation/dist/index.js 24:28-58
 @ ../node_modules/@sentry/node/build/esm/integrations/tracing/prisma.js 2:0-65 12:20-41 12:123-166
 @ ../node_modules/@sentry/node/build/esm/index.js 24:0-69 24:0-69
 @ ./app/services/utils/captureExceptionNode.ts 57:0-43 69:16-43
 @ ./app/main/main.ts 234:24-75

From what I've experienced it doesn't really matter how or where I import Sentry and also both @sentry/electron and @sentry/node are affected.

I mean I just added the

ignoreWarnings: [{ module: /@opentelemetry\/instrumentation/ }]

to my webpack config to hide it and that works. If you say it doesn't matter, I'm fine with it. Just wanted to report my findings.

lforst commented 4 days ago

@tom2strobl This issue is about Next.js. For Next.js we simply ignore the warning OOTB because it actually has no effect whatsoever on the application.

For all other use-cases (like yours), people need to decide themselves whether they wanna ignore the warning or not. It may be the case that it breaks your build in some way, but since you are a bit more free-form, we cannot know that.

We never "fixed" this issue, we simply ignored it because we deemed it non-harmful and it caused confusion.

In your case ignoring it like you did is probably fine :) Actually, I think ignoring the warning is fine for 99.99999% of the cases.