feat: Add loader file to node-based SDKs to support ESM monkeypatching #11338

lforst · 2024-03-28T14:38:02Z

Adds a loader script that can be used like node --loader=@sentry/[sdk-of-your-choice] your-script.js to allow for monkey-patching modules even in ESM.

Note

Currently, this is broken for upstream reasons. See #11338 (comment)

lforst · 2024-03-28T14:41:18Z

I am having a problem where this loader simply doesn't work with our SDK. I am doing the following but no spans are created for incoming requests (in fact otel doesn't seem to path the http module):

// test.mjs
import * as Sentry from '@sentry/node';

Sentry.init({
  dsn: 'https://7cea2b6e298f4fcc86bb28d22ceaeac4@o447951.ingest.sentry.io/4505391490007040',
  tracesSampleRate: 1,
  debug: true,
});

import('node:http').then(http => {
  const PORT = 3000;

  const server = http.createServer((req, res) => {
    res.writeHead(200, { 'Content-Type': 'text/html' });
    res.end('<html>response</html>');
  });

  server.listen(PORT, () => {
    console.log(`Server is running on http://localhost:${PORT}`);
  });
});

node --import=@sentry/node/register test.mjs

If I do

node --loader=@sentry/node/register test.mjs

it crashes with

(node:615243) ExperimentalWarning: `--experimental-loader` may be removed in the future; instead use `register()`:
--import 'data:text/javascript,import { register } from "node:module"; import { pathToFileURL } from "node:url"; register("%40sentry/node/register", pathToFileURL("./"));'
(Use `node --trace-warnings ...` to show where the warning was created)
file:///workspaces/sentry-nextjs-vercel-testproject/node_modules/@sentry/opentelemetry/esm/index.js:9
import { SamplingDecision } from '@opentelemetry/sdk-trace-base';
         ^^^^^^^^^^^^^^^^
SyntaxError: The requested module '@opentelemetry/sdk-trace-base' does not provide an export named 'SamplingDecision'
    at ModuleJob._instantiate (node:internal/modules/esm/module_job:123:21)
    at async ModuleJob.run (node:internal/modules/esm/module_job:191:5)
    at async ModuleLoader.import (node:internal/modules/esm/loader:336:24)
    at async loadESM (node:internal/process/esm_loader:34:7)
    at async handleMainPromise (node:internal/modules/run_main:106:12)

codecov · 2024-03-28T14:44:24Z

Bundle Report

Changes will decrease total bundle size by 381.72kB ⬇️

Bundle name	Size	Change
@sentry/types-cjs	35 bytes	0 bytes
@sentry/types-esm	35 bytes	0 bytes
@sentry/utils-cjs	178.75kB	0 bytes
@sentry/node-cjs	336.97kB	0 bytes
@sentry/utils-esm	174.17kB	0 bytes
@sentry-internal/replay-cjs	306.35kB	0 bytes
@sentry/core-cjs	240.44kB	0 bytes
@sentry/core-esm	236.82kB	0 bytes
@sentry-internal/integration-shims-cjs	3.65kB	0 bytes
@sentry-internal/replay-esm	306.46kB	0 bytes
@sentry/aws-serverless-cjs	14.62kB	0 bytes
@sentry-internal/replay-canvas-cjs	29.51kB	0 bytes
@sentry-internal/node-integration-tests-cjs	1.04kB	0 bytes
@sentry-internal/node-integration-tests-esm	888 bytes	0 bytes
@sentry-internal/replay-canvas-esm	29.43kB	0 bytes
@sentry/bun-cjs	13.5kB	0 bytes
@sentry/google-cloud-serverless-esm	19.16kB	0 bytes
@sentry/browser-esm	104.53kB	13 bytes ⬆️
@sentry/browser-cjs	107.36kB	44 bytes ⬆️
@sentry/bun-esm	10.05kB	0 bytes
@sentry/google-cloud-serverless-cjs	23.0kB	0 bytes
@sentry/wasm-cjs	5.2kB	0 bytes
@sentry/node-esm	333.56kB	0 bytes
@sentry/vue-cjs	20.19kB	0 bytes
@sentry/svelte-cjs	13.84kB	0 bytes
@sentry/astro-cjs	27.13kB	0 bytes
@sentry/wasm-esm	4.85kB	0 bytes
@sentry/astro-esm	23.39kB	0 bytes
@sentry/react-cjs	45.04kB	0 bytes
@sentry/vue-esm	18.85kB	0 bytes
@sentry/svelte-esm	12.72kB	0 bytes
@sentry/sveltekit-cjs	69.31kB	0 bytes
@sentry/sveltekit-esm	61.08kB	0 bytes
@sentry/remix-esm	48.23kB	0 bytes
@sentry/react-esm	41.18kB	0 bytes
@sentry/remix-cjs	53.62kB	0 bytes
@sentry/gatsby-cjs	905 bytes	0 bytes
@sentry/nextjs-cjs	20.52kB	0 bytes
@sentry/nextjs-esm	150.44kB	137.91kB ⬆️
@sentry/profiling-node-cjs	25.5kB	0 bytes
@sentry/profiling-node-esm	25.52kB	0 bytes
@sentry/vercel-edge-cjs	(removed)	18.23kB ⬇️
@sentry-internal/integration-shims-esm	(removed)	2.99kB ⬇️
@sentry-internal/feedback-cjs	(removed)	65.56kB ⬇️
@sentry/vercel-edge-esm	(removed)	16.13kB ⬇️
@sentry/gatsby-esm	(removed)	385 bytes ⬇️
@sentry/opentelemetry-cjs	(removed)	68.45kB ⬇️
@sentry-internal/tracing-cjs	(removed)	108.01kB ⬇️
@sentry-internal/feedback-esm	(removed)	65.28kB ⬇️
@sentry/opentelemetry-esm	(removed)	67.4kB ⬇️
@sentry-internal/tracing-esm	(removed)	107.26kB ⬇️

lforst · 2024-03-28T15:05:50Z

Talked with @timfish (🙏).

So apparently, @opentelemetry/instrumentation + ESM + our loader (or even @opentelemetry/instrumentation/hook.mjs) is broken on certain Node.js versions (like 18.19.0, 19, 20, 21), because import-in-the-middle only added support for these Node.js versions in version >=1.7.2, however import-in-the-middle@>=1.7.2 breaks @opentelemetry/instrumentation's instrumentation.

All of the above is outlined and tracked here: open-telemetry/opentelemetry-js#4547 (comment)

dev-packages/e2e-tests/test-applications/esm-loader-node-express-app/event-proxy-server.ts

timfish

--loader is deprecated and marked for removal at some point in the future so we should be targeting --import.

https://nodejs.org/api/cli.html#--experimental-loadermodule

This flag is discouraged and may be removed in a future version of Node.js. Please use --import with register() instead.

The only difference is that the code in the register export should pass the loader to register. Then it can be used in the same way but wth --import:

import { register } from 'node:module';
register('../build/loader-hook.mjs', import.meta.url););

Docs are here:
https://nodejs.org/api/module.html#enabling

lforst · 2024-04-02T09:43:37Z

@timfish This is funny. I tried to use register but it was only added in Node 18.19.0, which is incompatible with the currently used version of import-in-the-middle. 😭 😂

lforst · 2024-04-02T09:48:59Z

It looks like we need import.meta.url, otherwise node throws with

[WebServer] 
node:internal/process/esm_loader:40
      internalBinding('errors').triggerUncaughtException(
                                ^

[WebServer] TypeError [ERR_INVALID_URL]: Invalid URL
    at new NodeError (node:internal/errors:405:5)
    at new URL (node:internal/url:676:13)
    at getPackageScopeConfig (node:internal/modules/esm/package_config:29:24)
    at packageResolve (node:internal/modules/esm/resolve:870:25)
    at moduleResolve (node:internal/modules/esm/resolve:973:20)
    at defaultResolve (node:internal/modules/esm/resolve:1193:11)
    at nextResolve (node:internal/modules/esm/hooks:864:28)
    at Hooks.resolve (node:internal/modules/esm/hooks:302:30)
    at ModuleLoader.resolve (node:internal/modules/esm/loader:365:35)
    at ModuleLoader.getModuleJob (node:internal/modules/esm/loader:249:38) {
  input: './package.json',
  code: 'ERR_INVALID_URL'
}

I don't fully understand yet why, but I assume it doesn't know how to resolve relative imports without a base url.

timfish

👍

We might also want to export the loader hook too, to support the < Node v18 case?

lforst · 2024-04-02T13:48:06Z

We might also want to export the loader hook too, to support the < Node v18 case?

Fair point, it would need to be in a separate hook because simply importing register from module will crash on Node < 18.

timfish · 2024-04-02T19:35:38Z

it would need to be in a separate hook because simply importing register from module will crash on Node < 18.

Yeah, there would need to be another export. Maybe @sentry/node/hook?

lforst · 2024-04-03T11:01:37Z

I changed it so we have two files. Users would have to do the following:

Node <=18.18.2: node --experimental-loader=@sentry/node/hook app.js
Node >=18.19.0: node --import=@sentry/node/register app.js

lforst · 2024-04-03T11:47:20Z

An important todo for us here is gonna be documenting this properly when import-in-the-middle and @opentelemetry/instrumentation is fixed.

wojtekmaj · 2024-04-08T21:38:54Z

Perhaps we could rename hook to loader and register to import? I forgot these names in the middle of writing this comment. 😅 Or is there a naming convention I'm not aware of?

lforst · 2024-04-09T07:20:23Z

@wojtekmaj That's a very fair suggestion. Otel and import-in-the-middle call the hook "hook", so that's why we named it like that. It's like a semi-convention. But I see the appeal of naming them loader and import.

wojtekmaj · 2024-04-09T07:24:06Z

https://github.com/tapjs/tsimp uses what I proposed and I love I never need to dig through the docs to configure it properly :)

lforst · 2024-04-09T07:32:49Z

Ok that looks actually pretty clean. Let me change this :D

lforst · 2024-04-09T07:41:08Z

@wojtekmaj What do you think about import-hook and loader-hook? I am worried that just @sentry/node/import is not descriptive enough..

wojtekmaj · 2024-04-09T07:43:12Z

Makes sense to me! I'd go for a more descriptive version if you think it's gonna be used as one-time thing you put in your scripts section, and a shorter version if you expect people to type in commands in their terminals more often.

getsentry#11338)

feat: Add loader file to node-based SDKs to support ESM

57f837b

lint

a9fc29c

lforst added 2 commits March 28, 2024 15:39

Add E2E test

5a137d8

rename

39e2c59

github-advanced-security bot found potential problems Mar 28, 2024

View reviewed changes

dev-packages/e2e-tests/test-applications/esm-loader-node-express-app/event-proxy-server.ts Dismissed Show dismissed Hide dismissed

lforst added 2 commits March 28, 2024 15:56

better deps

1f37050

Add disable test to ci

a5ae9a1

lforst requested review from timfish, mydea and AbhiPrasad April 2, 2024 07:20

timfish requested changes Apr 2, 2024

View reviewed changes

Use register

9236a80

lforst requested a review from timfish April 2, 2024 10:07

timfish approved these changes Apr 2, 2024

View reviewed changes

lforst added 2 commits April 3, 2024 09:37

Add secondary hook to support the --loader usecase

ab9ddc8

Merge remote-tracking branch 'origin/develop' into lforst-loader-file

21f47fb

Merge branch 'develop' into lforst-loader-file

82d178b

lforst changed the title ~~feat: Add loader file to node-based SDKs to support ESM~~ feat: Add loader file to node-based SDKs to support ESM monkeypatching Apr 3, 2024

lforst merged commit 4085728 into develop Apr 3, 2024
94 checks passed

lforst deleted the lforst-loader-file branch April 3, 2024 11:36

lforst mentioned this pull request Apr 3, 2024

OpenTelemetry Instrumentation Issues #11070

Closed

lforst mentioned this pull request Apr 9, 2024

feat: Rename ESM loader hooks to import and loader #11498

Merged

cadesalaberry pushed a commit to cadesalaberry/sentry-javascript that referenced this pull request Apr 19, 2024

feat: Add loader file to node-based SDKs to support ESM monkeypatching (

d685991

getsentry#11338)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Add loader file to node-based SDKs to support ESM monkeypatching #11338

feat: Add loader file to node-based SDKs to support ESM monkeypatching #11338

lforst commented Mar 28, 2024 •

edited

Loading

lforst commented Mar 28, 2024 •

edited

Loading

codecov bot commented Mar 28, 2024 •

edited

Loading

lforst commented Mar 28, 2024 •

edited

Loading

timfish left a comment •

edited

Loading

lforst commented Apr 2, 2024

lforst commented Apr 2, 2024 •

edited

Loading

timfish left a comment

lforst commented Apr 2, 2024

timfish commented Apr 2, 2024

lforst commented Apr 3, 2024

lforst commented Apr 3, 2024

wojtekmaj commented Apr 8, 2024

lforst commented Apr 9, 2024

wojtekmaj commented Apr 9, 2024

lforst commented Apr 9, 2024

lforst commented Apr 9, 2024

wojtekmaj commented Apr 9, 2024 •

edited

Loading

feat: Add loader file to node-based SDKs to support ESM monkeypatching #11338

feat: Add loader file to node-based SDKs to support ESM monkeypatching #11338

Conversation

lforst commented Mar 28, 2024 • edited Loading

Note

lforst commented Mar 28, 2024 • edited Loading

codecov bot commented Mar 28, 2024 • edited Loading

Bundle Report

lforst commented Mar 28, 2024 • edited Loading

timfish left a comment • edited Loading

Choose a reason for hiding this comment

lforst commented Apr 2, 2024

lforst commented Apr 2, 2024 • edited Loading

timfish left a comment

Choose a reason for hiding this comment

lforst commented Apr 2, 2024

timfish commented Apr 2, 2024

lforst commented Apr 3, 2024

lforst commented Apr 3, 2024

wojtekmaj commented Apr 8, 2024

lforst commented Apr 9, 2024

wojtekmaj commented Apr 9, 2024

lforst commented Apr 9, 2024

lforst commented Apr 9, 2024

wojtekmaj commented Apr 9, 2024 • edited Loading

lforst commented Mar 28, 2024 •

edited

Loading

lforst commented Mar 28, 2024 •

edited

Loading

codecov bot commented Mar 28, 2024 •

edited

Loading

lforst commented Mar 28, 2024 •

edited

Loading

timfish left a comment •

edited

Loading

lforst commented Apr 2, 2024 •

edited

Loading

wojtekmaj commented Apr 9, 2024 •

edited

Loading