Skip to content

feat: Override import-in-the-middle code to only instrument specified modules #12167

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.

Sign up for GitHub

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

Closed
wants to merge 1 commit into from
Closed

feat: Override import-in-the-middle code to only instrument specified modules #12167

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
1 change: 1 addition & 0 deletions packages/node/src/nodeVersion.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,3 +2,4 @@ import { parseSemver } from '@sentry/utils';

export const NODE_VERSION = parseSemver(process.versions.node) as { major: number; minor: number; patch: number };
export const NODE_MAJOR = NODE_VERSION.major;
export const NODE_MINOR = NODE_VERSION.minor;
51 changes: 22 additions & 29 deletions packages/node/src/sdk/init.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,6 @@ import {
import { openTelemetrySetupCheck, setOpenTelemetryContextAsyncContextStrategy } from '@sentry/opentelemetry';
import type { Client, Integration, Options } from '@sentry/types';
import {
GLOBAL_OBJ,
consoleSandbox,
dropUndefinedKeys,
logger,
Expand All @@ -26,7 +25,6 @@ import { consoleIntegration } from '../integrations/console';
import { nodeContextIntegration } from '../integrations/context';
import { contextLinesIntegration } from '../integrations/contextlines';

import moduleModule from 'module';
import { httpIntegration } from '../integrations/http';
import { localVariablesIntegration } from '../integrations/local-variables';
import { modulesIntegration } from '../integrations/modules';
Expand All @@ -41,6 +39,7 @@ import { isCjs } from '../utils/commonjs';
import { defaultStackParser, getSentryRelease } from './api';
import { NodeClient } from './client';
import { initOpenTelemetry } from './initOtel';
import { registerEsmModuleHooks } from './register-hooks';

function getCjsOnlyIntegrations(): Integration[] {
return isCjs() ? [modulesIntegration()] : [];
Expand Down Expand Up @@ -92,8 +91,6 @@ function shouldAddPerformanceIntegrations(options: Options): boolean {
return options.enableTracing || options.tracesSampleRate != null || 'tracesSampler' in options;
}

declare const __IMPORT_META_URL_REPLACEMENT__: string;

/**
* Initialize Sentry for Node.
*/
Expand Down Expand Up @@ -130,31 +127,27 @@ function _init(
}

if (!isCjs()) {
const [nodeMajor, nodeMinor] = process.versions.node.split('.').map(Number);

// Register hook was added in v20.6.0 and v18.19.0
if (nodeMajor >= 22 || (nodeMajor === 20 && nodeMinor >= 6) || (nodeMajor === 18 && nodeMinor >= 19)) {
// We need to work around using import.meta.url directly because jest complains about it.
const importMetaUrl =
typeof __IMPORT_META_URL_REPLACEMENT__ !== 'undefined' ? __IMPORT_META_URL_REPLACEMENT__ : undefined;

if (!GLOBAL_OBJ._sentryEsmLoaderHookRegistered && importMetaUrl) {
try {
// @ts-expect-error register is available in these versions
moduleModule.register('@opentelemetry/instrumentation/hook.mjs', importMetaUrl);
GLOBAL_OBJ._sentryEsmLoaderHookRegistered = true;
} catch (error) {
logger.warn('Failed to register ESM hook', error);
}
}
} else {
consoleSandbox(() => {
// eslint-disable-next-line no-console
console.warn(
'[Sentry] You are using Node.js in ESM mode ("import syntax"). The Sentry Node.js SDK is not compatible with ESM in Node.js versions before 18.19.0 or before 20.6.0. Please either build your application with CommonJS ("require() syntax"), or use version 7.x of the Sentry Node.js SDK.',
);
});
}
registerEsmModuleHooks([
Copy link
Member

Choose a reason for hiding this comment

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

Hmm, will this prevent other instrumentation from working, if people add their own OTEL instrumentation?

'http',
'node:http',
'https',
'node:https',
'connect',
'express',
'fastify',
'graphql',
'@hapi/hapi',
'ioredis',
'koa',
'mongodb',
'mongoose',
'mysql',
'mysql2',
'pg',
'@nestjs/core',
'prisma',
...(options._experiments?.modulesToInstrument || []),
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@mydea I added this so that users can include other modules

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Another more complex approach might be to catch the file-not-found errors and revert to the default resolve/load.

Copy link
Member

Choose a reason for hiding this comment

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

catch the file-not-found errors and revert to the default resolve/load.

I assume this is pretty expensive right?

Copy link
Collaborator Author

@timfish timfish May 22, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Oh, it looks like it's not possible to fix this way. The errors are coming from the load hook while it's fetching files to collect exports.

Looking through the code, import-in-the-middle simply doesn't support re-exporting exports from external modules (ie. export * from 'some-external-module'). It doesn't appear to support resolving anything that isn't relative to the current file and it needs significant work to fix this. For example, to fix this for mono-repositories too, it will need to traverse up though multiple the node_modules directories searching for the module 🙁

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Although having written that... I'm going to try a couple more things...

Copy link
Collaborator Author

@timfish timfish May 22, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Ok, so it's possible to call resolve from within load to find modules that cannot be found relative to the current file. I suspect this is how it can be fixed in import-in-the-middle so I'll look into doing a PR there.

I could create a hacky PR similar the this PR but using what I've learnt above but it doesn't fix all the issues and it's almost too hacky to consider actually publishing with the SDK. Once I've got an import-in-the-middle PR up, this might change.

The issue with the @discord packages at least is that every package has a version export which all get combined via export * from and results in:
image

]);
}

setOpenTelemetryContextAsyncContextStrategy();
Expand Down
73 changes: 73 additions & 0 deletions packages/node/src/sdk/register-hooks.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,73 @@
import * as mod from 'node:module';
import { GLOBAL_OBJ, consoleSandbox, logger } from '@sentry/utils';
import { NODE_MAJOR, NODE_MINOR } from '../nodeVersion';

declare const __IMPORT_META_URL_REPLACEMENT__: string;

/**
* Registers hooks for ESM modules.
*
* The first hook overrides the source code of 'import-in-the-middle/hook.mjs' so it only instruments pre-specified modules.
*/
export function registerEsmModuleHooks(modulesToInstrument: string[]): void {
// Register hook was added in v20.6.0 and v18.19.0
if (NODE_MAJOR >= 22 || (NODE_MAJOR === 20 && NODE_MINOR >= 6) || (NODE_MAJOR === 18 && NODE_MINOR >= 19)) {
// We need to work around using import.meta.url directly because jest complains about it.
const importMetaUrl =
typeof __IMPORT_META_URL_REPLACEMENT__ !== 'undefined' ? __IMPORT_META_URL_REPLACEMENT__ : undefined;

if (!importMetaUrl || GLOBAL_OBJ._sentryEsmLoaderHookRegistered) {
return;
}

try {
const iitmOverrideCode = `
import { createHook } from "./hook.js";

const { load, resolve: resolveIITM, getFormat, getSource } = createHook(import.meta);

const modulesToInstrument = ${JSON.stringify(modulesToInstrument)};

export async function resolve(specifier, context, nextResolve) {
if (modulesToInstrument.includes(specifier)) {
return resolveIITM(specifier, context, nextResolve);
}

return nextResolve(specifier, context);
}

export { load, getFormat, getSource };`;

// This loader overrides the source code of 'import-in-the-middle/hook.mjs' with
// the above code. This code ensures that only the specified modules are proxied.
// @ts-expect-error register is available in these versions
mod.register(
new URL(
`data:application/javascript,
export async function load(url, context, nextLoad) {
if (url.endsWith('import-in-the-middle/hook.mjs')) {
const result = await nextLoad(url, context);
return {...result, source: '${iitmOverrideCode}' };
}
return nextLoad(url, context);
}`,
),
importMetaUrl,
);

// @ts-expect-error register is available in these versions
mod.register('@opentelemetry/instrumentation/hook.mjs', importMetaUrl);

GLOBAL_OBJ._sentryEsmLoaderHookRegistered = true;
} catch (error) {
logger.warn('Failed to register ESM hook', error);
}
} else {
consoleSandbox(() => {
// eslint-disable-next-line no-console
console.warn(
'[Sentry] You are using Node.js in ESM mode ("import syntax"). The Sentry Node.js SDK is not compatible with ESM in Node.js versions before 18.19.0 or before 20.6.0. Please either build your application with CommonJS ("require() syntax"), or use version 7.x of the Sentry Node.js SDK.',
);
});
}
}
Loading