Sentry Node.js ESM + OTEL instrumentation blockers - Part 2

AbhiPrasad commented 3 months ago

With the release of v8 of the Sentry SDK, the Node SDK now relies on OpenTelemetry. OpenTelemetry instrumentation does have some problems though (particularly with ESM because of import-in-the-middle), so this issue aims at documented these gaps.

Part 1: https://github.com/getsentry/sentry-javascript/issues/12242

With 8.8.0 we released all the fixes for Part 1, but there are a new set of bugs for us to release fixes for. This is tracked in this issue.

### import-in-the-middle PRs to be merged
- [ ] https://github.com/DataDog/import-in-the-middle/pull/92
- [ ] https://github.com/DataDog/import-in-the-middle/pull/93
- [ ] https://github.com/DataDog/import-in-the-middle/pull/96
- [ ] https://github.com/DataDog/import-in-the-middle/pull/100
- [ ] https://github.com/DataDog/import-in-the-middle/pull/103
- [ ] https://github.com/DataDog/import-in-the-middle/pull/104
- [ ] https://github.com/DataDog/import-in-the-middle/pull/106
- [ ] https://github.com/DataDog/import-in-the-middle/pull/108
- [ ] https://github.com/DataDog/import-in-the-middle/pull/109
- [ ] https://github.com/DataDog/import-in-the-middle/pull/114
- [ ] https://github.com/DataDog/import-in-the-middle/pull/115

### Sentry issues
- [ ] https://github.com/getsentry/sentry-javascript/issues/12325
- [ ] https://github.com/getsentry/sentry-javascript/issues/12357
- [ ] https://github.com/getsentry/sentry-javascript/issues/12414
- [ ] https://github.com/getsentry/sentry-javascript/issues/12422
- [ ] https://github.com/getsentry/sentry-javascript/issues/12480
- [ ] https://github.com/getsentry/sentry-javascript/issues/12490
- [ ] https://github.com/getsentry/sentry-javascript/issues/12622

Crashes with Nuxt + Nitro

Error: The requested module 'vue' does not provide an export named 'computed'

Issue: https://github.com/getsentry/sentry-javascript/issues/12490

timfish commented 3 months ago

The above PRs have been merged and we're just waiting for these changes to make it through all the required release processes.

Until then I've made a patch which can be used with patch-package so the combination of these changes can be tested: import-in-the-middle+1.8.0.patch

AnthonyDugarte commented 3 months ago

@timfish these warns are polluting the local env logs when booting up the apps, it'd be nice if they can be hidden via a debug env variable

edit: warn message spammed on app startup example

Error: 'import-in-the-middle' failed to wrap 'file:///path-to-app/src/services/index.ts'
    at load (/path-to-monorepo/node_modules/.pnpm/import-in-the-middle@1.8.0_patch_hash=yuhjsw3albhdp7qjccbkymjzsy/node_modules/import-in-the-middle/hook.js:306:21)
    at async nextLoad (node:internal/modules/esm/hooks:866:22)
    at async Hooks.load (node:internal/modules/esm/hooks:449:20)
    at async handleMessage (node:internal/modules/esm/worker:196:18) {
  cause: [Error: ENOENT: no such file or directory, open 'path-to-app/src/services/person.service.js'] {
    errno: -2,
    code: 'ENOENT',
    syscall: 'open',
    path: 'path-to-app/src/services/person.service.js'
  }
}

bubooon commented 3 months ago

Hey! I see the fix was already released for import-in-the-middle but it seems to be not related to the nuxt-nitro issue. I think my notes here #12490 could shine some light on the issue to help you fix or at least reproduce it.

MeCapron commented 3 months ago

Hello!

I have this issue on my side :

https://adexos.sentry.io/issues/5559252313/?project=4507505661837312&query=is%3Aunresolved+issue.priority%3A%5Bhigh%2C+medium%5D&referrer=issue-stream&statsPeriod=14d&stream_index=1

It seems to be greatly related to the issues seen here.

However, I could not find one that looks like mine. I'm using Sentry 8.13.0

I'm copying the test for further research :

require is not defined in ES module scope, you can use import instead
This file is being treated as an ES module because it has a '.js' file extension and '/project/package.json' contains "type": "module". To treat it as a CommonJS script, rename it ...

This issue only comes since the installation of Sentry, when the app is built (using Remix with Vite).

Have you any idea?

timfish commented 3 months ago

Hi @MeCapron,

This looks like a different issue and is very strange since all this code is CommonJs!

Could you open a new issue with a minimal reproduction so I can debug what's going on?

AbhiPrasad commented 2 months ago

With the newest release of import-in-the-middle v1.9.0 this should be fixed.

If you upgrade to a fresh install of the latest version of the Node SDK it should use import-in-the-middle@1.9.0 by default.

There are some follow up issues though - I'll create a new ESM blockers issue (Part 3 😅) that we can use to track this!

We need to:

Merge in https://github.com/nodejs/import-in-the-middle/pull/115 which should fix https://github.com/getsentry/sentry-javascript/issues/12622
Further investigate https://github.com/getsentry/sentry-javascript/issues/12414 which should we are asking about here: https://github.com/nodejs/loaders/pull/198#discussion_r1664546188
Figure out why https://github.com/getsentry/sentry-javascript/issues/12490 is continuing to regress

AbhiPrasad commented 2 months ago

Follow up issue: https://github.com/getsentry/sentry-javascript/issues/12806

getsentry / sentry-javascript