v8 Node + ESM + esbuild error

jd-carroll commented 4 months ago

Is there an existing issue for this?

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

How do you use Sentry?

Sentry Saas (sentry.io)

Which SDK are you using?

@sentry/aws-serverless

SDK Version

8.0.0

Framework Version

No response

Link to Sentry event

No response

SDK Setup

No response

Steps to Reproduce

For completeness I thought I would link this here too: https://github.com/open-telemetry/opentelemetry-js/issues/4691

When bundling @sentry/...@^8 in a "full ESM" mode, you will get (potentially) hundreds of errors along the lines of:

WARN  ▲ [WARNING] Constructing "ImportInTheMiddle" will crash at run-time because it's an import namespace object, not a constructor [call-import-namespace]
   ../../node_modules/@prisma/instrumentation/node_modules/@opentelemetry/instrumentation/build/esm/platform/node/instrumentation.js:273:30:
     273 │             var esmHook = new ImportInTheMiddle([
         ╵                               ~~~~~~~~~~~~~~~~~
 Consider changing "ImportInTheMiddle" to a default import instead:
   ../../node_modules/@prisma/instrumentation/node_modules/@opentelemetry/instrumentation/build/esm/platform/node/instrumentation.js:55:7:
     55 │ import * as ImportInTheMiddle from 'import-in-the-middle';
        │        ~~~~~~~~~~~~~~~~~~~~~~
        ╵        ImportInTheMiddle

I have not taken the time to identify how / when this error could be encountered, but can confirm that doing the following in a module environment will throw an error:

import * as ImportInTheMiddle from 'import-in-the-middle';
var esmHook = new ImportInTheMiddle( ... );

The instantiation would need to look like:

var esmHook = new ImportInTheMiddle.default.default( ... );

Would be nice to add some support for either of the following issues:

If for no other reason than eliminating unnecessary warnings during build.

Expected Result

NA

Actual Result

NA

mydea commented 4 months ago

Hello,

this is hopefully fixed by https://github.com/getsentry/sentry-javascript/pull/12017!

lforst commented 4 months ago

I think this will be an issue for all situations where esbuild is involved. @jd-carroll already you already did the right thing reporting this upstream! As soon as that fix lands in the opentelemetry packages we will bump the dep asap.

jd-carroll commented 3 months ago

@mydea @lforst This is 100% causing my lambdas to crash and makes using Sentry@v8 not possible for ESM.

Again, at runtime, the lambdas fail with:

That stacktrace corresponds to this code:

Could someone from @Sentry help identify what the path forward for ESM would be?

There seem to be a number of pertinent issues:

mydea commented 3 months ago

Hey,

this is most likely fixed by https://github.com/open-telemetry/opentelemetry-js/pull/4546 - we'll try to get this merged & released!

AbhiPrasad commented 3 months ago

@jd-carroll could you share the esbuild config you use?

jd-carroll commented 3 months ago

I’ll work on pulling together a small repro, but if you’re working on it yourself, make sure the generated file uses the mjs extension. (This is the recommended approach for ESM + lambda)

On Mon, May 27, 2024 at 9:48 AM Abhijeet Prasad @.***> wrote:

@jd-carroll https://github.com/jd-carroll could you share the esbuild config you use?

— Reply to this email directly, view it on GitHub https://github.com/getsentry/sentry-javascript/issues/12009#issuecomment-2133824195, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAOMNKHEOBALH4CPPS73KETZENPXRAVCNFSM6AAAAABHU5CNQWVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDCMZTHAZDIMJZGU . You are receiving this because you were mentioned.Message ID: @.***>

jd-carroll commented 3 months ago

Here is the raw config:

    // LambdaBundlerConfig
    const LambdaBundlerConfig: BuildOptions = {
      entryPoints: [LambdaEntryFile], // some/file.ts
      outfile: LambdaFile, // options.format === 'cjs' ? 'dist/lambdas/lambda.js' : 'dist/lambdas/lambda.mjs'
      bundle: true,
      sourcemap: 'linked',
      splitting: false,
      platform: 'node',
      mainFields: options.format === 'cjs' ? ['main'] : ['module', 'main'], // almost always ['module', 'main']
      external: options.external || [],
      target: options.target ?? (options.format === 'cjs' ? 'es5' : 'esnext'), // almost always esnext
      format: options.format, // almost always 'esm'
      banner: options.banner, // only used in dev
      minify: options.minify ?? true, // only false in dev
      metafile: true,

      plugins: [
        SentryBuildPlugin(definition, LambdaVersion, SentryEnabled),
        NotifyResultPlugin(),
        WriteEsbuildMetaPlugin(MetaFile),
        ValidateLambdaHandler(definition, LambdaFile, options.format),
        ...(options.plugins ?? [])
      ],
      inject: [SentryEnabled ? SentryInit : undefined, ...inject].filter(Boolean) as string[],
      define: {
        ...definedFlags
      }
    };

I'll also add that the SentryInit which is injected when SentryEnabled, is where the initial Sentry.init() call happens.

Otherwise, the lambda code already includes the Sentry wrapper for AWS lambda.

jd-carroll commented 3 months ago

NOTE: For all the esbuild'ers out there, here's a simple plugin to get things working that follows #12233

import fs from 'node:fs';
import path from 'node:path';
import type { Plugin } from 'esbuild';

export function FixImportInTheMiddlePlugin(enabled: boolean): Plugin {
  if (!enabled) {
    return {
      name: 'skip-fix-import-in-the-middle',
      setup() {}
    };
  }

  return {
    name: 'fix-import-in-the-middle',
    setup(build) {
      build.onLoad({ filter: /@opentelemetry\/instrumentation/ }, async (args) => {
        const extension = path.extname(args.path).slice(1);

        let text = await fs.promises.readFile(args.path, 'utf-8');
        if (text.includes('* as ImportInTheMiddle')) {
          text = text.replaceAll(/new\s+(ImportInTheMiddle)\(/gm, 'new $1.default(');
        }

        return {
          contents: text,
          loader: (extension === 'mjs' ? 'js' : extension) as 'ts' | 'js'
        };
      });
    }
  };
}

AbhiPrasad commented 3 months ago

https://github.com/DataDog/import-in-the-middle/pull/88 should fix this with import-in-the-middle, we also have to update OTEL but we'll take care of that too. Appreciate the patience in the meantime @jd-carroll!

AbhiPrasad commented 3 months ago

So this should be resolved with https://github.com/getsentry/sentry-javascript/releases/tag/8.8.0, but please reach out if anything looks off.

getsentry / sentry-javascript