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

feat(node): Option to only wrap instrumented modules #13139

Merged
merged 7 commits into from
Sep 5, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
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;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

m: Are we sure we want to put this option in here? The API feels a bit "clunky" to me 🤔 also, does this not conflict with include/exclude options, somehow (or do they play together?)?

What about making this a top-level option? Esp. if we think this may become a default at some point...? (no strong feelings, just want us to consider this!)

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

does this not conflict with include/exclude options, somehow (or do they play together?)?

include/exclude were always mutually exclusive because It doesn't make much sense to use both. You could potentially enable this new onlyIncludeInstrumentedModules option while still supplying include and this would only include instrumented modules, plus whatever the user supplied. This might be useful if a user is adding an instrumentation later.

Are we sure we want to put this option in here? What about making this a top-level option? Esp. if we think this may become a default at some point...?

Yep valid points. I don't feel that strongly either way.

}

export interface BaseNodeOptions {
Expand Down
Loading