Skip to content

Commit

Permalink
feat(node): Option to only wrap instrumented modules (#13139)
Browse files Browse the repository at this point in the history
Likely closes many issues but I don't want to auto-close anything
specific here. We should probably confirm the issues are closed
individually.

`import-in-the-middle` by default wraps every ES module with a wrapping
module that later allows it exports to be modified. This has issues
though because the wrapping output of `import-in-the-middle` is not
compatible with all modules.

To help work around this I [added a
feature](nodejs/import-in-the-middle#146) to
`import-in-the-middle` that allows us to only wrap modules that we
specifically need to instrument.

```ts
import * as Sentry from '@sentry/node';

Sentry.init({
  dsn: '__DSN__',
  registerEsmLoaderHooks: { onlyHookedModules: true },
});
```

So far I've only changed the express integration test to use this new
mode.
  • Loading branch information
timfish authored Sep 5, 2024
1 parent 80ec41a commit a69da1b
Show file tree
Hide file tree
Showing 6 changed files with 70 additions and 2 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -5,4 +5,5 @@ Sentry.init({
dsn: process.env.E2E_TEST_DSN,
tunnel: `http://localhost:3031/`, // proxy server
tracesSampleRate: 1,
registerEsmLoaderHooks: { onlyIncludeInstrumentedModules: true },
});
Original file line number Diff line number Diff line change
@@ -0,0 +1,21 @@
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');
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
// eslint-disable-next-line no-console
console.assert(true);
Original file line number Diff line number Diff line change
@@ -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);
});
});
23 changes: 22 additions & 1 deletion packages/node/src/sdk/initOtel.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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';
Expand All @@ -31,6 +32,26 @@ export function initOpenTelemetry(client: NodeClient): void {
client.traceProvider = provider;
}

type ImportInTheMiddleInitData = Pick<EsmLoaderHookOptions, 'include' | 'exclude'> & {
addHookMessagePort?: unknown;
};

interface RegisterOptions {
data?: ImportInTheMiddleInitData;
transferList?: unknown[];
}

function getRegisterOptions(esmHookConfig?: EsmLoaderHookOptions): RegisterOptions {
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
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);
Expand All @@ -44,7 +65,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);
Expand Down
13 changes: 12 additions & 1 deletion packages/node/src/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,18 @@ import type { NodeTransportOptions } from './transports';

export interface EsmLoaderHookOptions {
include?: Array<string | RegExp>;
exclude?: Array<string | RegExp>;
exclude?: Array<string | RegExp> /**
* 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 `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`.
*/;
onlyIncludeInstrumentedModules?: boolean;
}

export interface BaseNodeOptions {
Expand Down

0 comments on commit a69da1b

Please sign in to comment.