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

WebPack error: Critical dependency: the request of a dependency is an expression #4173

Closed
dvoytenko opened this issue Sep 29, 2023 · 13 comments
Labels
question User is asking a question not related to a new feature or bug

Comments

@dvoytenko
Copy link
Contributor

What happened?

Steps to Reproduce

Running const sdk = new NodeSDK({}); sdk.start(); pops up this WebPack error:

Critical dependency: the request of a dependency is an expression

Expected Result

No error.

Actual Result

Error.

Additional Details

It appears that the error is coming from a dynamic import here:

const version = require(path.join(baseDir, 'package.json')).version;

It's even shielded by the // eslint-disable-next-line @typescript-eslint/no-var-requires.

This seems like an auxiliary function and perhaps the SDK could allow users to bypass the need to execute this dynamic require?

OpenTelemetry Setup Code

const sdk = new NodeSDK({
  resource: new Resource({}),
});
sdk.start();

package.json

{
  "name": "otel",
  "version": "0.3.0",
  "private": true,
  "type": "module",
  "scripts": {
    "type-check": "tsc --noEmit"
  },
  "dependencies": {
    "@opentelemetry/api": "^1.6.0",
    "@opentelemetry/exporter-trace-otlp-grpc": "^0.43.0",
    "@opentelemetry/exporter-trace-otlp-http": "^0.43.0",
    "@opentelemetry/exporter-trace-otlp-proto": "^0.43.0",
    "@opentelemetry/resources": "^1.17.0",
    "@opentelemetry/sdk-node": "^0.43.0",
    "@opentelemetry/sdk-trace-node": "^1.17.0",
    "@opentelemetry/semantic-conventions": "^1.17.0"
  },
  "devDependencies": {
    "@types/node": "18.15.11",
    "typescript": "5.1.3"
  }
}

Relevant log output

Critical dependency: the request of a dependency is an expression

Import trace for requested module:
../../node_modules/.pnpm/@[email protected]_@[email protected]/node_modules/@opentelemetry/instrumentation/build/src/platform/node/instrumentation.js
vercel-site:dev: ../../node_modules/.pnpm/@[email protected]_@[email protected]/node_modules/@opentelemetry/instrumentation/build/src/platform/node/index.js
vercel-site:dev: ../../node_modules/.pnpm/@[email protected]_@[email protected]/node_modules/@opentelemetry/instrumentation/build/src/platform/index.js
vercel-site:dev: ../../node_modules/.pnpm/@[email protected]_@[email protected]/node_modules/@opentelemetry/instrumentation/build/src/index.js
vercel-site:dev: ../../node_modules/.pnpm/@[email protected]_@[email protected]/node_modules/@opentelemetry/sdk-node/build/src/sdk.js
vercel-site:dev: ../../node_modules/.pnpm/@[email protected]_@[email protected]/node_modules/@opentelemetry/sdk-node/build/src/index.js
../../packages/otel/src/index.ts
@dvoytenko dvoytenko added bug Something isn't working triage labels Sep 29, 2023
@dyladan
Copy link
Member

dyladan commented Oct 11, 2023

This is critical to the functioning of the instrumentation package and can't really be removed without causing other problems. Even if you get past this step, all node autoinstrumentations depend very heavily on require-in-the-middle which doesn't function in webpack contexts and you won't get any instrumentation anyway. Other agents solve this in different ways, for example elastic recommends adding all node_modules/ as an external using nodeExternals() but I can't guarantee that would work (https://www.elastic.co/guide/en/apm/agent/nodejs/current/starting-the-agent.html#start-webpack). If I'm missing something or there is a way to solve this, I would love to hear it because we have heard this from other users as well.

There are definitely people using otel in webpack contexts so it is definitely possible, but I think the autoinstrumentations are unlikely to work and you'll have to jump through quite some hoops and/or use manual instrumentation.

@dyladan dyladan added question User is asking a question not related to a new feature or bug and removed bug Something isn't working triage labels Oct 11, 2023
@dvoytenko
Copy link
Contributor Author

I'll close this for now. Overall, it makes sense for specific instrumentations to violate this rule - most of them won't be compatible with WebPack or any other pack in many cases. But it'd be good to keep it await from the the core library.

@moxious
Copy link

moxious commented Nov 22, 2023

Adding this note for posterity. Here's what the issue looks like in practice, with a work-around.

What it looks like

Import trace for requested module:
./node_modules/@opentelemetry/instrumentation-fetch/node_modules/@opentelemetry/instrumentation/build/esm/platform/node/instrumentation.js
./node_modules/@opentelemetry/instrumentation-fetch/node_modules/@opentelemetry/instrumentation/build/esm/platform/node/index.js
./node_modules/@opentelemetry/instrumentation-fetch/node_modules/@opentelemetry/instrumentation/build/esm/platform/index.js
./node_modules/@opentelemetry/instrumentation-fetch/node_modules/@opentelemetry/instrumentation/build/esm/index.js
./node_modules/@opentelemetry/instrumentation-fetch/build/esm/fetch.js
./node_modules/@opentelemetry/instrumentation-fetch/build/esm/index.js
./node_modules/@grafana/faro-web-tracing/dist/esm/getDefaultOTELInstrumentations.js
./node_modules/@grafana/faro-web-tracing/dist/esm/index.js
./app/components/nav.tsx
./app/components/navigatedcontent.tsx
./app/answers/page.tsx

./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/@opentelemetry/instrumentation/build/esm/platform/node/instrumentation.js
./node_modules/@opentelemetry/instrumentation/build/esm/platform/node/index.js
./node_modules/@opentelemetry/instrumentation/build/esm/platform/index.js
./node_modules/@opentelemetry/instrumentation/build/esm/index.js
./node_modules/@grafana/faro-web-tracing/dist/esm/instrumentation.js
./node_modules/@grafana/faro-web-tracing/dist/esm/index.js
./app/components/nav.tsx
./app/components/navigatedcontent.tsx
./app/answers/page.tsx
 ⚠ ./node_modules/@opentelemetry/instrumentation-fetch/node_modules/@opentelemetry/instrumentation/build/esm/platform/node/instrumentation.js
Critical dependency: the request of a dependency is an expression

A work-around:

In my particular case, the root of this came from NextJS (which does a lot of server side rendering) trying to run client-side code in server-side pre-rendering. In next.config.js you can add this block which will disable this warning for opentelemetry from webpack. It fixes nothing, but it doesn't blow up your console with things you can't fix.

    webpack: (
        config,
        { buildId, dev, isServer, defaultLoaders, nextRuntime, webpack }
    ) => {
        if (isServer) {
            config.ignoreWarnings = [
                { module: /opentelemetry/, },
            ]
        }
        return config
    },

@lforst
Copy link
Contributor

lforst commented Apr 21, 2024

Just fyi, the particular error in this issue should be fixed with the release of #4593. It would be cool if we could get it actually released soon!

@r34son
Copy link

r34son commented Apr 24, 2024

@timfish @lforst Tried to use updated version of @opentelemetry/instrumentation with fix #4593, but error still occurs.
require.resolve is still causing it https://github.com/open-telemetry/opentelemetry-js/blob/main/experimental/packages/opentelemetry-instrumentation/src/platform/node/instrumentation.ts#L149
image
Can we somehow get rid of dynamic resolve?

Should we resolve module.name before checking if it exists in cache? Or maybe module.name should be resolved already inside init()?

private _warnOnPreloadedModules(): void {
  this._modules.forEach((module: InstrumentationModuleDefinition) => {
    const { name } = module;
    try {
-    const resolvedModule = require.resolve(name);
-    if (require.cache[resolvedModule]) {
+    if (require.cache[name]) {
        // Module is already cached, which means the instrumentation hook might not work
        this._diag.warn(
          `Module ${name} has been loaded before ${this.instrumentationName} so it might not work, please initialize it before requiring ${name}`
        );
      }
    } catch {
      // Module isn't available, we can simply skip
    }
  });
}

@timfish
Copy link
Contributor

timfish commented Apr 24, 2024

Sorry I missed that. It appears I only fixed one of the issues!

The only quick fix I can think of is to hide the require.resolve from bundlers like this:

function resolveHiddenFromBundlers(
  mod: { require: { resolve: (name: string) => string } },
  name: string
): string {
  return mod.require.resolve(name);
}

const resolvedModule = resolveHiddenFromBundlers(module, name);

This is a common approach to hide require from bundlers:
https://github.com/umijs/umi/blob/9d160fc2bc9236b3df06e349412bbe0a6b2df451/packages/bundler-utils/compiled/babel/source-map-support.js#L17-L25

https://github.com/GChristensen/scrapyard/blob/f9b01c0a9d30312c046129eb5ac4b03c219fa7dd/addon/lib/unzipit.js#L426-L428

There are some other less desirable approaches:
https://github.com/yarnpkg/berry/blob/52252b0caa3ac6d753348b271f91090f2724d089/packages/yarnpkg-pnpify/sources/dynamicRequire.ts#L5

@lforst
Copy link
Contributor

lforst commented Apr 25, 2024

+1 on the hiding solution. I believe the @vercel/otel package does this too with some eval shenanigans. We should reopen this issue, and I would advise putting some lint rule or test in place so that this doesn't regress.

@trentm
Copy link
Contributor

trentm commented May 31, 2024

I would advise putting some lint rule or test in place so that this doesn't regress.

@lforst I agree. I've opened #4744 to start potentially adding bundler-related tests in this repo. Would you, @timfish, and/or others be able to help a bit on that? I grok the test infrastructure in the repo, but I don't know the bundlers that well at all.

my failed attempt at a simple webpack repro of the above

I have briefly tried -- and failed so far -- to repro the Critical dependency: the request of a dependency is an expression using webpack directly.
Please take a look at my attempted repro here: https://github.com/trentm/repro-webpack-dep-expr
I would appreciate help getting something that reproduces, ideally without needing a full Next.js or Nest.js or whatever usage. Thanks.

@lforst
Copy link
Contributor

lforst commented Jun 3, 2024

@trentm Happy to help out with build process! We happen to have experience with it and I think there's some room for improvement.

btw: Your attempted repro is currently private. Would you mind turning it public? 😄

@trentm
Copy link
Contributor

trentm commented Jun 5, 2024

btw: Your attempted repro is currently private. Would you mind turning it public? 😄

🤦 It is public now.

@t18n
Copy link

t18n commented Jul 4, 2024

If you are using NextJS 14.2, updating the packages to 0.52.1 fixes the issue for me:

    "@opentelemetry/exporter-trace-otlp-http": "^0.52.1",
    "@opentelemetry/sdk-node": "^0.52.1",

@wjoel
Copy link

wjoel commented Jul 19, 2024

FWIW, just updating those wasn't sufficient for me. I had to add "@opentelemetry/exporter-jaeger": "1.25.1", to package.json to silence the warning. A bit annoying, as we don't actually use that exporter.

@meowsus
Copy link

meowsus commented Nov 18, 2024

In my particular case, the root of this came from NextJS (which does a lot of server side rendering) trying to run client-side code in server-side pre-rendering.

In our Next.js v14 project, we were receiving these errors as well. They appeared after we upgraded Sentry from v7 to v8. Some of our files were importing directly from @sentry/node. Adding this package name to the nextConfig.experimental.serverComponentsExternalPackages array fixed the warning for us.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
question User is asking a question not related to a new feature or bug
Projects
None yet
Development

Successfully merging a pull request may close this issue.

10 participants