From 72d87d4c1ee651abe007d730736a3cb156234071 Mon Sep 17 00:00:00 2001 From: Tim Fish Date: Wed, 31 Jul 2024 16:06:46 +0200 Subject: [PATCH 1/7] feat(node): Option to only wrap instrumented modules --- packages/node/src/sdk/initOtel.ts | 28 +++++++++++++++++++++++++++- packages/node/src/types.ts | 12 +++++++++++- 2 files changed, 38 insertions(+), 2 deletions(-) diff --git a/packages/node/src/sdk/initOtel.ts b/packages/node/src/sdk/initOtel.ts index 03d8cea76fac..7203799d3207 100644 --- a/packages/node/src/sdk/initOtel.ts +++ b/packages/node/src/sdk/initOtel.ts @@ -10,6 +10,7 @@ import { import { SDK_VERSION } from '@sentry/core'; import { SentryPropagator, SentrySampler, SentrySpanProcessor } from '@sentry/opentelemetry'; import { GLOBAL_OBJ, consoleSandbox, logger } from '@sentry/utils'; +import { createAddHookMessageChannel } from 'import-in-the-middle'; import { getOpenTelemetryInstrumentationToPreload } from '../integrations/tracing'; import { SentryContextManager } from '../otel/contextManager'; @@ -31,6 +32,31 @@ export function initOpenTelemetry(client: NodeClient): void { client.traceProvider = provider; } +type ImportInTheMiddleInitData = Pick & { + addHookMessagePort?: MessagePort; +}; + +interface RegisterOptions { + data?: ImportInTheMiddleInitData; + transferList?: unknown[]; +} + +function getRegisterOptions(esmHookConfig?: EsmLoaderHookOptions): RegisterOptions { + // There was no specific config so empty options + if (!esmHookConfig) { + return {}; + } + + if (esmHookConfig.onlyHookedModules) { + const { addHookMessagePort } = createAddHookMessageChannel(); + // If the user supplied include, we need to use that as a starting point or use an empty array to ensure no modules + // are wrapped if they are not hooked + return { data: { addHookMessagePort, include: esmHookConfig.include || [] }, transferList: [addHookMessagePort] }; + } + + return { data: esmHookConfig }; +} + /** Initialize the ESM loader. */ export function maybeInitializeEsmLoader(esmHookConfig?: EsmLoaderHookOptions): void { const [nodeMajor = 0, nodeMinor = 0] = process.versions.node.split('.').map(Number); @@ -44,7 +70,7 @@ export function maybeInitializeEsmLoader(esmHookConfig?: EsmLoaderHookOptions): if (!GLOBAL_OBJ._sentryEsmLoaderHookRegistered && importMetaUrl) { try { // @ts-expect-error register is available in these versions - moduleModule.register('import-in-the-middle/hook.mjs', importMetaUrl, { data: esmHookConfig }); + moduleModule.register('import-in-the-middle/hook.mjs', importMetaUrl, getRegisterOptions(esmHookConfig)); GLOBAL_OBJ._sentryEsmLoaderHookRegistered = true; } catch (error) { logger.warn('Failed to register ESM hook', error); diff --git a/packages/node/src/types.ts b/packages/node/src/types.ts index 9604b31ddb22..03ff8ca15d71 100644 --- a/packages/node/src/types.ts +++ b/packages/node/src/types.ts @@ -6,7 +6,17 @@ import type { NodeTransportOptions } from './transports'; export interface EsmLoaderHookOptions { include?: Array; - exclude?: Array; + exclude?: Array /** + * When set to `true`, `import-in-the-middle` will only wrap ESM modules that are specifically hooked by OpenTelemetry + * instrumentations. This feature is useful to avoid issues where `import-in-the-middle` is not compatible with some + * of your dependencies. + * + * **Note**: This feature will only work if you `init` the SDK before the instrumented modules are loaded via the Node + * `--import` CLI flag or `init` is called before loading your app via async `import()`. + * + * Defaults to `false`. + */; + onlyHookedModules?: boolean; } export interface BaseNodeOptions { From 407643a049df765ff8323f49a39359096e55b534 Mon Sep 17 00:00:00 2001 From: Tim Fish Date: Wed, 31 Jul 2024 16:10:09 +0200 Subject: [PATCH 2/7] simplify a bit --- packages/node/src/sdk/initOtel.ts | 7 +------ 1 file changed, 1 insertion(+), 6 deletions(-) diff --git a/packages/node/src/sdk/initOtel.ts b/packages/node/src/sdk/initOtel.ts index 7203799d3207..b9e948c2f502 100644 --- a/packages/node/src/sdk/initOtel.ts +++ b/packages/node/src/sdk/initOtel.ts @@ -42,12 +42,7 @@ interface RegisterOptions { } function getRegisterOptions(esmHookConfig?: EsmLoaderHookOptions): RegisterOptions { - // There was no specific config so empty options - if (!esmHookConfig) { - return {}; - } - - if (esmHookConfig.onlyHookedModules) { + if (esmHookConfig?.onlyHookedModules) { const { addHookMessagePort } = createAddHookMessageChannel(); // If the user supplied include, we need to use that as a starting point or use an empty array to ensure no modules // are wrapped if they are not hooked From d3986c4633d050d716e9d4cc18856ed5c7dc5eab Mon Sep 17 00:00:00 2001 From: Tim Fish Date: Wed, 31 Jul 2024 16:12:18 +0200 Subject: [PATCH 3/7] Better jsdoc --- packages/node/src/types.ts | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/packages/node/src/types.ts b/packages/node/src/types.ts index 03ff8ca15d71..826e895b37be 100644 --- a/packages/node/src/types.ts +++ b/packages/node/src/types.ts @@ -7,12 +7,13 @@ import type { NodeTransportOptions } from './transports'; export interface EsmLoaderHookOptions { include?: Array; exclude?: Array /** - * When set to `true`, `import-in-the-middle` will only wrap ESM modules that are specifically hooked by OpenTelemetry - * instrumentations. This feature is useful to avoid issues where `import-in-the-middle` is not compatible with some - * of your dependencies. + * When set to `true`, `import-in-the-middle` will only wrap ESM modules that are specifically instrumented by + * OpenTelemetry plugins. This is useful to avoid issues where `import-in-the-middle` is not compatible with some of + * your dependencies. * - * **Note**: This feature will only work if you `init` the SDK before the instrumented modules are loaded via the Node - * `--import` CLI flag or `init` is called before loading your app via async `import()`. + * **Note**: This feature will only work if you `Sentry.init()` the SDK before the instrumented modules are loaded. + * This can be achieved via the Node `--import` CLI flag or by loading your app via async `import()` after calling + * `Sentry.init()`. * * Defaults to `false`. */; From d1d2f3fa42c6377270f2d9b66a9227bc930429b8 Mon Sep 17 00:00:00 2001 From: Tim Fish Date: Wed, 31 Jul 2024 17:04:33 +0200 Subject: [PATCH 4/7] Fix types --- packages/node/src/sdk/initOtel.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/node/src/sdk/initOtel.ts b/packages/node/src/sdk/initOtel.ts index b9e948c2f502..c526b56a9970 100644 --- a/packages/node/src/sdk/initOtel.ts +++ b/packages/node/src/sdk/initOtel.ts @@ -33,7 +33,7 @@ export function initOpenTelemetry(client: NodeClient): void { } type ImportInTheMiddleInitData = Pick & { - addHookMessagePort?: MessagePort; + addHookMessagePort?: unknown; }; interface RegisterOptions { From 8e2accbc6b3466b3d065d2f3e31dcef6a9b9f83e Mon Sep 17 00:00:00 2001 From: Tim Fish Date: Wed, 31 Jul 2024 18:20:59 +0200 Subject: [PATCH 5/7] Test express with `onlyHookedModules` --- .../test-applications/node-express-esm-loader/src/instrument.mjs | 1 + 1 file changed, 1 insertion(+) diff --git a/dev-packages/e2e-tests/test-applications/node-express-esm-loader/src/instrument.mjs b/dev-packages/e2e-tests/test-applications/node-express-esm-loader/src/instrument.mjs index 544c773e5e7c..72dd46f84215 100644 --- a/dev-packages/e2e-tests/test-applications/node-express-esm-loader/src/instrument.mjs +++ b/dev-packages/e2e-tests/test-applications/node-express-esm-loader/src/instrument.mjs @@ -5,4 +5,5 @@ Sentry.init({ dsn: process.env.E2E_TEST_DSN, tunnel: `http://localhost:3031/`, // proxy server tracesSampleRate: 1, + registerEsmLoaderHooks: { onlyHookedModules: true }, }); From f69e512442ed9a469c21333dd57e8bfaa2958d3b Mon Sep 17 00:00:00 2001 From: Tim Fish Date: Wed, 31 Jul 2024 21:26:50 +0200 Subject: [PATCH 6/7] Rename new config property and add integration test --- .../src/instrument.mjs | 2 +- .../suites/esm/import-in-the-middle/app.mjs | 22 +++++++++++++++++++ .../esm/import-in-the-middle/sub-module.mjs | 2 ++ .../suites/esm/import-in-the-middle/test.ts | 12 ++++++++++ packages/node/src/sdk/initOtel.ts | 2 +- packages/node/src/types.ts | 2 +- 6 files changed, 39 insertions(+), 3 deletions(-) create mode 100644 dev-packages/node-integration-tests/suites/esm/import-in-the-middle/app.mjs create mode 100644 dev-packages/node-integration-tests/suites/esm/import-in-the-middle/sub-module.mjs create mode 100644 dev-packages/node-integration-tests/suites/esm/import-in-the-middle/test.ts diff --git a/dev-packages/e2e-tests/test-applications/node-express-esm-loader/src/instrument.mjs b/dev-packages/e2e-tests/test-applications/node-express-esm-loader/src/instrument.mjs index 72dd46f84215..caaf73162ded 100644 --- a/dev-packages/e2e-tests/test-applications/node-express-esm-loader/src/instrument.mjs +++ b/dev-packages/e2e-tests/test-applications/node-express-esm-loader/src/instrument.mjs @@ -5,5 +5,5 @@ Sentry.init({ dsn: process.env.E2E_TEST_DSN, tunnel: `http://localhost:3031/`, // proxy server tracesSampleRate: 1, - registerEsmLoaderHooks: { onlyHookedModules: true }, + registerEsmLoaderHooks: { onlyIncludeInstrumentedModules: true }, }); diff --git a/dev-packages/node-integration-tests/suites/esm/import-in-the-middle/app.mjs b/dev-packages/node-integration-tests/suites/esm/import-in-the-middle/app.mjs new file mode 100644 index 000000000000..47c68f8748b1 --- /dev/null +++ b/dev-packages/node-integration-tests/suites/esm/import-in-the-middle/app.mjs @@ -0,0 +1,22 @@ +import { loggingTransport } from '@sentry-internal/node-integration-tests'; +import * as Sentry from '@sentry/node'; +import * as iitm from 'import-in-the-middle'; + +new iitm.Hook((_, name) => { + if(name !== 'http') { + throw new Error(`'http' should be the only hooked modules but we just hooked '${name}'`); + } +}) + +Sentry.init({ + dsn: 'https://public@dsn.ingest.sentry.io/1337', + release: '1.0', + autoSessionTracking: false, + transport: loggingTransport, + registerEsmLoaderHooks: { onlyIncludeInstrumentedModules: true }, +}); + +await import('./sub-module.mjs'); +await import('http'); +await import('os'); + diff --git a/dev-packages/node-integration-tests/suites/esm/import-in-the-middle/sub-module.mjs b/dev-packages/node-integration-tests/suites/esm/import-in-the-middle/sub-module.mjs new file mode 100644 index 000000000000..9940c57857eb --- /dev/null +++ b/dev-packages/node-integration-tests/suites/esm/import-in-the-middle/sub-module.mjs @@ -0,0 +1,2 @@ +// eslint-disable-next-line no-console +console.assert(true); diff --git a/dev-packages/node-integration-tests/suites/esm/import-in-the-middle/test.ts b/dev-packages/node-integration-tests/suites/esm/import-in-the-middle/test.ts new file mode 100644 index 000000000000..8b9e6e06202f --- /dev/null +++ b/dev-packages/node-integration-tests/suites/esm/import-in-the-middle/test.ts @@ -0,0 +1,12 @@ +import { conditionalTest } from '../../../utils'; +import { cleanupChildProcesses, createRunner } from '../../../utils/runner'; + +afterAll(() => { + cleanupChildProcesses(); +}); + +conditionalTest({ min: 18 })('import-in-the-middle', () => { + test('onlyIncludeInstrumentedModules', done => { + createRunner(__dirname, 'app.mjs').ensureNoErrorOutput().start(done); + }); +}); diff --git a/packages/node/src/sdk/initOtel.ts b/packages/node/src/sdk/initOtel.ts index c526b56a9970..37b94ebc439f 100644 --- a/packages/node/src/sdk/initOtel.ts +++ b/packages/node/src/sdk/initOtel.ts @@ -42,7 +42,7 @@ interface RegisterOptions { } function getRegisterOptions(esmHookConfig?: EsmLoaderHookOptions): RegisterOptions { - if (esmHookConfig?.onlyHookedModules) { + if (esmHookConfig?.onlyIncludeInstrumentedModules) { const { addHookMessagePort } = createAddHookMessageChannel(); // If the user supplied include, we need to use that as a starting point or use an empty array to ensure no modules // are wrapped if they are not hooked diff --git a/packages/node/src/types.ts b/packages/node/src/types.ts index 826e895b37be..aa9873e2da91 100644 --- a/packages/node/src/types.ts +++ b/packages/node/src/types.ts @@ -17,7 +17,7 @@ export interface EsmLoaderHookOptions { * * Defaults to `false`. */; - onlyHookedModules?: boolean; + onlyIncludeInstrumentedModules?: boolean; } export interface BaseNodeOptions { From edda5a37f7e8d50c4157aaf23c2c2a7d00fdd543 Mon Sep 17 00:00:00 2001 From: Tim Fish Date: Wed, 31 Jul 2024 21:47:28 +0200 Subject: [PATCH 7/7] lint --- .../suites/esm/import-in-the-middle/app.mjs | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/dev-packages/node-integration-tests/suites/esm/import-in-the-middle/app.mjs b/dev-packages/node-integration-tests/suites/esm/import-in-the-middle/app.mjs index 47c68f8748b1..6b20155aea38 100644 --- a/dev-packages/node-integration-tests/suites/esm/import-in-the-middle/app.mjs +++ b/dev-packages/node-integration-tests/suites/esm/import-in-the-middle/app.mjs @@ -3,10 +3,10 @@ import * as Sentry from '@sentry/node'; import * as iitm from 'import-in-the-middle'; new iitm.Hook((_, name) => { - if(name !== 'http') { + if (name !== 'http') { throw new Error(`'http' should be the only hooked modules but we just hooked '${name}'`); } -}) +}); Sentry.init({ dsn: 'https://public@dsn.ingest.sentry.io/1337', @@ -19,4 +19,3 @@ Sentry.init({ await import('./sub-module.mjs'); await import('http'); await import('os'); -