Skip to content

Commit

Permalink
feat(node)!: Remove fine grained registerEsmLoaderHooks option and …
Browse files Browse the repository at this point in the history
…`addOpenTelemetryInstrumentation()` (#14606)
  • Loading branch information
lforst authored Dec 19, 2024
1 parent 90fad5a commit e190793
Show file tree
Hide file tree
Showing 18 changed files with 11 additions and 179 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 @@ -13,7 +13,6 @@ Sentry.init({
release: '1.0',
autoSessionTracking: false,
transport: loggingTransport,
registerEsmLoaderHooks: { onlyIncludeInstrumentedModules: true },
});

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

conditionalTest({ min: 18 })('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: 0 additions & 2 deletions packages/astro/src/index.server.ts
Original file line number Diff line number Diff line change
Expand Up @@ -12,8 +12,6 @@ export {
addEventProcessor,
addIntegration,
// eslint-disable-next-line deprecation/deprecation
addOpenTelemetryInstrumentation,
// eslint-disable-next-line deprecation/deprecation
addRequestDataToEvent,
amqplibIntegration,
anrIntegration,
Expand Down
2 changes: 0 additions & 2 deletions packages/aws-serverless/src/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -119,8 +119,6 @@ export {
spanToTraceHeader,
spanToBaggageHeader,
trpcMiddleware,
// eslint-disable-next-line deprecation/deprecation
addOpenTelemetryInstrumentation,
zodErrorsIntegration,
profiler,
amqplibIntegration,
Expand Down
2 changes: 0 additions & 2 deletions packages/bun/src/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -141,8 +141,6 @@ export {
spanToTraceHeader,
spanToBaggageHeader,
trpcMiddleware,
// eslint-disable-next-line deprecation/deprecation
addOpenTelemetryInstrumentation,
zodErrorsIntegration,
profiler,
amqplibIntegration,
Expand Down
2 changes: 0 additions & 2 deletions packages/google-cloud-serverless/src/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -118,8 +118,6 @@ export {
spanToTraceHeader,
spanToBaggageHeader,
trpcMiddleware,
// eslint-disable-next-line deprecation/deprecation
addOpenTelemetryInstrumentation,
zodErrorsIntegration,
profiler,
amqplibIntegration,
Expand Down
2 changes: 0 additions & 2 deletions packages/node/src/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -60,8 +60,6 @@ export type { NodeOptions } from './types';
export { addRequestDataToEvent, DEFAULT_USER_INCLUDES, extractRequestData } from '@sentry/core';

export {
// eslint-disable-next-line deprecation/deprecation
addOpenTelemetryInstrumentation,
// These are custom variants that need to be used instead of the core one
// As they have slightly different implementations
continueTrace,
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 @@ -132,7 +132,7 @@ function _init(
}

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

setOpenTelemetryContextAsyncContextStrategy();
Expand Down
34 changes: 7 additions & 27 deletions packages/node/src/sdk/initOtel.ts
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,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 @@ -34,30 +33,8 @@ 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 {
// 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 @@ -68,8 +45,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));
moduleModule.register('import-in-the-middle/hook.mjs', importMetaUrl, {
data: { addHookMessagePort, include: [] },
transferList: [addHookMessagePort],
});
GLOBAL_OBJ._sentryEsmLoaderHookRegistered = true;
} catch (error) {
logger.warn('Failed to register ESM hook', error);
Expand All @@ -88,7 +69,6 @@ export function maybeInitializeEsmLoader(esmHookConfig?: EsmLoaderHookOptions):
interface NodePreloadOptions {
debug?: boolean;
integrations?: string[];
registerEsmLoaderHooks?: EsmLoaderHookOptions;
}

/**
Expand All @@ -105,7 +85,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 @@ -138,22 +101,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/] });
});
});
});
3 changes: 0 additions & 3 deletions packages/opentelemetry/src/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -58,8 +58,5 @@ export {

export { openTelemetrySetupCheck } from './utils/setupCheck';

// eslint-disable-next-line deprecation/deprecation
export { addOpenTelemetryInstrumentation } from './instrumentation';

// Legacy
export { getClient } from '@sentry/core';
15 changes: 0 additions & 15 deletions packages/opentelemetry/src/instrumentation.ts

This file was deleted.

2 changes: 0 additions & 2 deletions packages/remix/src/index.server.ts
Original file line number Diff line number Diff line change
Expand Up @@ -16,8 +16,6 @@ export {
addEventProcessor,
addIntegration,
// eslint-disable-next-line deprecation/deprecation
addOpenTelemetryInstrumentation,
// eslint-disable-next-line deprecation/deprecation
addRequestDataToEvent,
amqplibIntegration,
anrIntegration,
Expand Down
2 changes: 0 additions & 2 deletions packages/solidstart/src/server/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -8,8 +8,6 @@ export {
addEventProcessor,
addIntegration,
// eslint-disable-next-line deprecation/deprecation
addOpenTelemetryInstrumentation,
// eslint-disable-next-line deprecation/deprecation
addRequestDataToEvent,
amqplibIntegration,
anrIntegration,
Expand Down
2 changes: 0 additions & 2 deletions packages/sveltekit/src/server/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -8,8 +8,6 @@ export {
addEventProcessor,
addIntegration,
// eslint-disable-next-line deprecation/deprecation
addOpenTelemetryInstrumentation,
// eslint-disable-next-line deprecation/deprecation
addRequestDataToEvent,
amqplibIntegration,
anrIntegration,
Expand Down

0 comments on commit e190793

Please sign in to comment.