Skip to content

Commit

Permalink
feat(node)!: Remove fine grained registerEsmLoaderHooks (#15002)
Browse files Browse the repository at this point in the history
  • Loading branch information
lforst authored Jan 14, 2025
1 parent 2f302d7 commit aaf7d53
Show file tree
Hide file tree
Showing 8 changed files with 11 additions and 146 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -5,5 +5,4 @@ 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
Expand Up @@ -12,7 +12,6 @@ Sentry.init({
dsn: 'https://[email protected]/1337',
release: '1.0',
transport: loggingTransport,
registerEsmLoaderHooks: { onlyIncludeInstrumentedModules: true },
});

await import('./sub-module.mjs');
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ afterAll(() => {
});

describe('import-in-the-middle', () => {
test('onlyIncludeInstrumentedModules', () => {
test('should only instrument modules that we have instrumentation for', () => {
const result = spawnSync('node', [join(__dirname, 'app.mjs')], { encoding: 'utf-8' });
expect(result.stderr).not.toMatch('should be the only hooked modules but we just hooked');
});
Expand Down
2 changes: 1 addition & 1 deletion packages/node/src/sdk/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -131,7 +131,7 @@ function _init(
}

if (!isCjs() && options.registerEsmLoaderHooks !== false) {
maybeInitializeEsmLoader(options.registerEsmLoaderHooks === true ? undefined : options.registerEsmLoaderHooks);
maybeInitializeEsmLoader();
}

setOpenTelemetryContextAsyncContextStrategy();
Expand Down
35 changes: 7 additions & 28 deletions packages/node/src/sdk/initOtel.ts
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,6 @@ import { createAddHookMessageChannel } from 'import-in-the-middle';
import { DEBUG_BUILD } from '../debug-build';
import { getOpenTelemetryInstrumentationToPreload } from '../integrations/tracing';
import { SentryContextManager } from '../otel/contextManager';
import type { EsmLoaderHookOptions } from '../types';
import { isCjs } from '../utils/commonjs';
import type { NodeClient } from './client';

Expand All @@ -40,30 +39,8 @@ export function initOpenTelemetry(client: NodeClient, options: AdditionalOpenTel
client.traceProvider = provider;
}

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

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

function getRegisterOptions(esmHookConfig?: EsmLoaderHookOptions): RegisterOptions {
// TODO(v9): Make onlyIncludeInstrumentedModules: true the default behavior.
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
// eslint-disable-next-line deprecation/deprecation
return { data: { addHookMessagePort, include: esmHookConfig.include || [] }, transferList: [addHookMessagePort] };
}

return { data: esmHookConfig };
}

/** Initialize the ESM loader. */
export function maybeInitializeEsmLoader(esmHookConfig?: EsmLoaderHookOptions): void {
export function maybeInitializeEsmLoader(): void {
const [nodeMajor = 0, nodeMinor = 0] = process.versions.node.split('.').map(Number);

// Register hook was added in v20.6.0 and v18.19.0
Expand All @@ -74,9 +51,12 @@ export function maybeInitializeEsmLoader(esmHookConfig?: EsmLoaderHookOptions):

if (!GLOBAL_OBJ._sentryEsmLoaderHookRegistered && importMetaUrl) {
try {
const { addHookMessagePort } = createAddHookMessageChannel();
// @ts-expect-error register is available in these versions
moduleModule.register('import-in-the-middle/hook.mjs', importMetaUrl, getRegisterOptions(esmHookConfig));
GLOBAL_OBJ._sentryEsmLoaderHookRegistered = true;
moduleModule.register('import-in-the-middle/hook.mjs', importMetaUrl, {
data: { addHookMessagePort, include: [] },
transferList: [addHookMessagePort],
});
} catch (error) {
logger.warn('Failed to register ESM hook', error);
}
Expand All @@ -94,7 +74,6 @@ export function maybeInitializeEsmLoader(esmHookConfig?: EsmLoaderHookOptions):
interface NodePreloadOptions {
debug?: boolean;
integrations?: string[];
registerEsmLoaderHooks?: EsmLoaderHookOptions;
}

/**
Expand All @@ -111,7 +90,7 @@ export function preloadOpenTelemetry(options: NodePreloadOptions = {}): void {
}

if (!isCjs()) {
maybeInitializeEsmLoader(options.registerEsmLoaderHooks);
maybeInitializeEsmLoader();
}

// These are all integrations that we need to pre-load to ensure they are set up before any other code runs
Expand Down
52 changes: 1 addition & 51 deletions packages/node/src/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -5,43 +5,6 @@ import type { ClientOptions, Options, SamplingContext, Scope, Span, TracePropaga

import type { NodeTransportOptions } from './transports';

/**
* Note: In the next major version of the Sentry SDK this interface will be removed and the SDK will by default only wrap
* ESM modules that are required to be wrapped by OpenTelemetry Instrumentation.
*/
export interface EsmLoaderHookOptions {
/**
* Provide a list of modules to wrap with `import-in-the-middle`.
*
* @deprecated It is recommended to use `onlyIncludeInstrumentedModules: true` instead of manually defining modules to include and exclude.
*/
include?: Array<string | RegExp>;

/**
* Provide a list of modules to prevent them from being wrapped with `import-in-the-middle`.
*
* @deprecated It is recommended to use `onlyIncludeInstrumentedModules: true` instead of manually defining modules to include and exclude.
*/
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`.
*
* Note: In the next major version of the Sentry SDK this option will be removed and the SDK will by default only wrap
* ESM modules that are required to be wrapped by OpenTelemetry Instrumentation.
*/
// TODO(v9): Make `onlyIncludeInstrumentedModules: true` the default behavior.
onlyIncludeInstrumentedModules?: boolean;
}

export interface BaseNodeOptions {
/**
* List of strings/regex controlling to which outgoing requests
Expand Down Expand Up @@ -143,22 +106,9 @@ export interface BaseNodeOptions {
* with certain libraries. If you run into problems running your app with this enabled,
* please raise an issue in https://github.com/getsentry/sentry-javascript.
*
* You can optionally exclude specific modules or only include specific modules from being instrumented by providing
* an object with `include` or `exclude` properties.
*
* ```js
* registerEsmLoaderHooks: {
* exclude: ['openai'],
* }
* ```
*
* Defaults to `true`.
*
* Note: In the next major version of the SDK, the possibility to provide fine-grained control will be removed from this option.
* This means that it will only be possible to pass `true` or `false`. The default value will continue to be `true`.
*/
// TODO(v9): Only accept true | false | undefined.
registerEsmLoaderHooks?: boolean | EsmLoaderHookOptions;
registerEsmLoaderHooks?: boolean;

/**
* Configures in which interval client reports will be flushed. Defaults to `60_000` (milliseconds).
Expand Down
23 changes: 0 additions & 23 deletions packages/nuxt/src/server/sdk.ts
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,6 @@ import type { SentryNuxtServerOptions } from '../common/types';
export function init(options: SentryNuxtServerOptions): Client | undefined {
const sentryOptions = {
...options,
registerEsmLoaderHooks: mergeRegisterEsmLoaderHooks(options),
defaultIntegrations: getNuxtDefaultIntegrations(options),
};

Expand Down Expand Up @@ -92,28 +91,6 @@ function getNuxtDefaultIntegrations(options: NodeOptions): Integration[] {
];
}

/**
* Adds /vue/ to the registerEsmLoaderHooks options and merges it with the old values in the array if one is defined.
* If the registerEsmLoaderHooks option is already a boolean, nothing is changed.
*
* Only exported for Testing purposes.
*/
export function mergeRegisterEsmLoaderHooks(
options: SentryNuxtServerOptions,
): SentryNuxtServerOptions['registerEsmLoaderHooks'] {
if (typeof options.registerEsmLoaderHooks === 'object' && options.registerEsmLoaderHooks !== null) {
return {
// eslint-disable-next-line deprecation/deprecation
exclude: Array.isArray(options.registerEsmLoaderHooks.exclude)
? // eslint-disable-next-line deprecation/deprecation
[...options.registerEsmLoaderHooks.exclude, /vue/]
: // eslint-disable-next-line deprecation/deprecation
options.registerEsmLoaderHooks.exclude ?? [/vue/],
};
}
return options.registerEsmLoaderHooks ?? { exclude: [/vue/] };
}

/**
* Flushes pending Sentry events with a 2-second timeout and in a way that cannot create unhandled promise rejections.
*/
Expand Down
41 changes: 1 addition & 40 deletions packages/nuxt/test/server/sdk.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -5,9 +5,8 @@ import { Scope } from '@sentry/node';
import { getGlobalScope } from '@sentry/node';
import { SDK_VERSION } from '@sentry/node';
import { beforeEach, describe, expect, it, vi } from 'vitest';
import type { SentryNuxtServerOptions } from '../../src/common/types';
import { init } from '../../src/server';
import { clientSourceMapErrorFilter, mergeRegisterEsmLoaderHooks } from '../../src/server/sdk';
import { clientSourceMapErrorFilter } from '../../src/server/sdk';

const nodeInit = vi.spyOn(SentryNode, 'init');

Expand Down Expand Up @@ -163,42 +162,4 @@ describe('Nuxt Server SDK', () => {
});
});
});

describe('mergeRegisterEsmLoaderHooks', () => {
it('merges exclude array when registerEsmLoaderHooks is an object with an exclude array', () => {
const options: SentryNuxtServerOptions = {
registerEsmLoaderHooks: { exclude: [/test/] },
};
const result = mergeRegisterEsmLoaderHooks(options);
expect(result).toEqual({ exclude: [/test/, /vue/] });
});

it('sets exclude array when registerEsmLoaderHooks is an object without an exclude array', () => {
const options: SentryNuxtServerOptions = {
registerEsmLoaderHooks: {},
};
const result = mergeRegisterEsmLoaderHooks(options);
expect(result).toEqual({ exclude: [/vue/] });
});

it('returns boolean when registerEsmLoaderHooks is a boolean', () => {
const options1: SentryNuxtServerOptions = {
registerEsmLoaderHooks: true,
};
const result1 = mergeRegisterEsmLoaderHooks(options1);
expect(result1).toBe(true);

const options2: SentryNuxtServerOptions = {
registerEsmLoaderHooks: false,
};
const result2 = mergeRegisterEsmLoaderHooks(options2);
expect(result2).toBe(false);
});

it('sets exclude array when registerEsmLoaderHooks is undefined', () => {
const options: SentryNuxtServerOptions = {};
const result = mergeRegisterEsmLoaderHooks(options);
expect(result).toEqual({ exclude: [/vue/] });
});
});
});

0 comments on commit aaf7d53

Please sign in to comment.