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

[AbhiPrasad]

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
Part 2: #12485

With 8.15.0 + [email protected] we released most of the fixes for Part 2, 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

Beta Give feedback

1. fix: Skip wrapping CJS modules which result in invalid identifiers nodejs/import-in-the-middle#115
   diag-agenda +1
2. fix: Don't wrap native modules nodejs/import-in-the-middle#142
   +0
3. fix: Use correct format when resolving exports from sub-modules nodejs/import-in-the-middle#140
   +0
4. fix: Use correct format when resolving exports from relative paths nodejs/import-in-the-middle#145
   +0
5. feat: Optionally only wrap modules hooked in --import nodejs/import-in-the-middle#146
   +0
6. feat: Allow regex for include and exclude options nodejs/import-in-the-middle#148
   +0

Sentry issues

Beta Give feedback

1. SDK fails in ESM mode in combination with openai #12414
   Package-Meta: OpenTelemetry Package: node Type: Bug +3
   
2. . in exports property of CJS file breaks dynamic imports #12622
   Package-Meta: OpenTelemetry Package: node Type: Bug +3
   
3. Unexpected closing brace error from import-in-the-middle #12807
   Package-Meta: OpenTelemetry Package: node Type: Bug +3
   
4. @sentry/astro breaks drizzle-orm in Astro #12912
   Package: astro +1
   

1. openai shims causes issues

Issue: #12414

IITM issue: nodejs/import-in-the-middle#38
Asking about this to Node.js loaders team: nodejs/loaders#198 (comment)

2. CJS identifiers that don't work in ESM break

Issue: #12622

IITM issue: nodejs/import-in-the-middle#94
Fix: nodejs/import-in-the-middle#115

3. Unexpected closing brace error from import-in-the-middle (affects discordjs)

Issue: #12807

Repro: https://github.com/bubooon/sentry-nuxt-nitro-example

4. No spans are created for Node http.get calls in ESM mode

Issue: open-telemetry/opentelemetry-js#4857

Repro: https://github.com/Lms24/otel-node-http-get-esm-reproduction

[RIP21]

Unsure if it's related to the import-in-the-middle but I have problems with drizzle-orm imports.

Like that

import { eq, ilike, or } from "drizzle-orm";
         ^^
SyntaxError: The requested module 'drizzle-orm' does not provide an export named 'eq'
    at ModuleJob._instantiate (node:internal/modules/esm/module_job:134:21)
    at async ModuleJob.run (node:internal/modules/esm/module_job:217:5)
    at async ModuleLoader.import (node:internal/modules/esm/loader:316:24)
    at async asyncRunEntryPointWithESMLoader (node:internal/modules/run_main:123:5)

Node.js v20.13.1

This is how I'm running my node:

 node --import @sentry/node/preload --import @swc-node/register/esm-register ./src/main.ts

[timfish]

@RIP21 this issue tracked here:

• @sentry/astro breaks drizzle-orm in Astro #12912

[timfish]

I'm hoping that this import-in-the-middle PR should finally close a lot of these issues:

• feat: Optionally only wrap modules hooked in --import nodejs/import-in-the-middle#146

For a bit of context... import-in-the-middle and instrumentation of ESM in Node is fundamentally broken in a couple of ways. Since ES modules are immutable, import-in-the-middle creates wrapper modules for every module which allows for changes to be made after they are loaded. Unfortunately, wrapper modules do not allow import-in-the-middle to fully represent what a regular module looks like to those consuming it.

The above PR should allow us to signal to import-in-the-middle to only wrap the libraries that we intend to later instrument. While this doesn't fix the fundamental issues surrounding instrumenting ESM, it significantly reduces the number of libraries that import-in-the-middle needs to be compatible with.

[AbhiPrasad]

We've released https://github.com/getsentry/sentry-javascript/releases/tag/8.29.0 which includes #13139 as part of this PR.

As such I think we can close this issue.

@timfish mind helping follow up on the linked issues?

[timfish]

Yep and I think many of the issues are assigned to me which should help!