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

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

Closed
15 of 18 tasks
AbhiPrasad opened this issue Jun 13, 2024 · 7 comments
Closed
15 of 18 tasks

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

AbhiPrasad opened this issue Jun 13, 2024 · 7 comments
Assignees
Labels
Package: node Issues related to the Sentry Node SDK Package-Meta: OpenTelemetry

Comments

@AbhiPrasad
Copy link
Member

AbhiPrasad commented Jun 13, 2024

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: #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.

Sentry issues

  1. Package-Meta: OpenTelemetry Package: node Type: Bug
    timfish
  2. Package-Meta: OpenTelemetry Package: node Type: Bug
    timfish
  3. Package-Meta: OpenTelemetry Package: node Type: Bug
    timfish
  4. Package-Meta: OpenTelemetry Package: node Type: Bug
    timfish
  5. Package-Meta: OpenTelemetry Package: node Type: Bug
    timfish
  6. Package-Meta: OpenTelemetry Package: node Type: Bug
    timfish
  7. Package-Meta: OpenTelemetry Package: node Type: Bug
    timfish

1. Issue with prisma library

Issue: #12325

Repro: #12325 (comment)

IITM issue: nodejs/import-in-the-middle#95
Fix: #12325

2. using --import ./instrument.mjs and tsx fails

Issue: #12357
TSX issue: privatenumber/tsx#571

3. Issue with openai library

Issue: #12414

Repro: NatoBoram/bug-report-sentry#9

IITM issue: nodejs/import-in-the-middle#102
Fix: nodejs/import-in-the-middle#103

4. import-in-the-middle does not support import attributes

Issue: #12422

IITM issue: nodejs/import-in-the-middle#105
Fix: nodejs/import-in-the-middle#104

5. Does not work with ts-node

Issue: #12480

Crashes with Nuxt + Nitro

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

Issue: #12490

@timfish
Copy link
Collaborator

timfish commented Jun 15, 2024

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

AnthonyDugarte commented Jun 18, 2024

@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/[email protected]_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
Copy link

bubooon commented Jun 29, 2024

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

MeCapron commented Jul 1, 2024

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

image

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

timfish commented Jul 2, 2024

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

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 [email protected] 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:

  1. Merge in fix: Skip wrapping CJS modules which result in invalid identifiers nodejs/import-in-the-middle#115 which should fix . in exports property of CJS file breaks dynamic imports #12622
  2. Further investigate SDK fails in ESM mode in combination with openai #12414 which should we are asking about here: doc: add synchronous hooks proposal nodejs/loaders#198 (comment)
  3. Figure out why Error: The requested module 'vue' does not provide an export named 'computed' #12490 is continuing to regress

@AbhiPrasad
Copy link
Member Author

Follow up issue: #12806

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Package: node Issues related to the Sentry Node SDK Package-Meta: OpenTelemetry
Projects
Archived in project
Development

No branches or pull requests

6 participants