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

fix(profiling): Don't put require, __filename and __dirname on global object #14470

Open
wants to merge 32 commits into
base: develop
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
32 commits
Select commit Hold shift + click to select a range
7b07ab9
ref(profiling) test local require
JonasBa Nov 25, 2024
a473e0f
test(profiling) throw after each possible culprit
JonasBa Nov 28, 2024
a3a20d1
test(profiling) format
JonasBa Nov 29, 2024
d4505a4
test runtime
JonasBa Nov 30, 2024
98e4eec
test node
JonasBa Nov 30, 2024
4626a4e
test profiling
JonasBa Nov 30, 2024
df0d758
fix(profiling) shim __dirname for esm
JonasBa Dec 2, 2024
cd974e4
test run
JonasBa Dec 2, 2024
6ac71f9
fix formatting
JonasBa Dec 2, 2024
79314fe
set type module before running test
JonasBa Dec 2, 2024
083594e
try running directly
JonasBa Dec 2, 2024
de1a11f
define main and module
JonasBa Dec 2, 2024
0e3c15c
separate step
JonasBa Dec 2, 2024
e79958b
print file
JonasBa Dec 3, 2024
c78ad0b
Merge branch 'develop' into jb/profiling/local-require
JonasBa Dec 3, 2024
629bedd
format
JonasBa Dec 3, 2024
54d2383
Merge branch 'develop' into jb/profiling/local-require
JonasBa Dec 4, 2024
fd618e9
Merge branch 'develop' into jb/profiling/local-require
JonasBa Dec 4, 2024
31653f3
massively lost
lforst Dec 12, 2024
3c5b4ff
somewhat wild guess
lforst Dec 12, 2024
8275abb
pls
lforst Dec 12, 2024
98effdf
fml
lforst Dec 12, 2024
f5d0c49
idk man
lforst Dec 13, 2024
6c2ba5d
skip
lforst Dec 13, 2024
4d9c906
Try some stuff
lforst Dec 16, 2024
e643537
try and simplify
lforst Jan 8, 2025
66f6a82
replace import.meta.url
lforst Jan 8, 2025
a0fe858
woops
lforst Jan 8, 2025
331f955
don't change how we detect commonjs
lforst Jan 8, 2025
43fcb0f
Pin esbuild to version we know that works
lforst Jan 8, 2025
18f044a
rm lockfile
lforst Jan 8, 2025
4026445
maybe
lforst Jan 8, 2025
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
3 changes: 2 additions & 1 deletion .github/workflows/build.yml
Original file line number Diff line number Diff line change
Expand Up @@ -1205,7 +1205,8 @@ jobs:
- name: Run E2E test
working-directory: dev-packages/e2e-tests/test-applications/${{ matrix.test-application }}
timeout-minutes: 10
run: xvfb-run --auto-servernum --server-args="-screen 0 1280x960x24" -- yarn test:assert
run: |
xvfb-run --auto-servernum --server-args="-screen 0 1280x960x24" -- yarn test:assert

job_required_jobs_passed:
name: All required jobs passed or were skipped
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -4,16 +4,17 @@
// only runs integration and unit tests, this change would be missed and could end up in a release.
// Therefor, once all binaries are precompiled in CI and tests pass, run esbuild with bundle:true
// which will copy all binaries to the outfile folder and throw if any of them are missing.
import esbuild from 'esbuild';
import esbuild from "esbuild";

console.log('Running build using esbuild version', esbuild.version);
console.log("Running build using esbuild version", esbuild.version);

esbuild.buildSync({
platform: 'node',
entryPoints: ['./index.ts'],
outdir: './dist',
target: 'esnext',
format: 'cjs',
bundle: true,
loader: { '.node': 'copy' },
platform: "node",
entryPoints: ["./index.ts"],
outfile: "./dist/cjs/index.js",
target: "esnext",
format: "cjs",
bundle: true,
loader: { ".node": "copy" },
external: ["@sentry/node", "@sentry/profiling-node"],
});
Original file line number Diff line number Diff line change
Expand Up @@ -4,26 +4,17 @@
// only runs integration and unit tests, this change would be missed and could end up in a release.
// Therefor, once all binaries are precompiled in CI and tests pass, run esbuild with bundle:true
// which will copy all binaries to the outfile folder and throw if any of them are missing.
import esbuild from 'esbuild';
import esbuild from "esbuild";

console.log('Running build using esbuild version', esbuild.version);
console.log("Running build using esbuild version", esbuild.version);

esbuild.buildSync({
platform: 'node',
entryPoints: ['./index.ts'],
outfile: './dist/index.shimmed.mjs',
target: 'esnext',
format: 'esm',
bundle: true,
loader: { '.node': 'copy' },
banner: {
js: `
import { dirname } from 'node:path';
import { fileURLToPath } from 'node:url';
import { createRequire } from 'node:module';
const require = createRequire(import.meta.url);
const __filename = fileURLToPath(import.meta.url);
const __dirname = dirname(__filename);
`,
},
platform: "node",
entryPoints: ["./index.ts"],
outfile: "./dist/esm/index.mjs",
target: "esnext",
format: "esm",
bundle: true,
loader: { ".node": "copy" },
external: ["@sentry/node", "@sentry/profiling-node"],
});
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
const Sentry = require('@sentry/node');
const { nodeProfilingIntegration } = require('@sentry/profiling-node');
import * as Sentry from '@sentry/node';
import { nodeProfilingIntegration } from '@sentry/profiling-node';

const wait = (ms: number) => new Promise(resolve => setTimeout(resolve, ms));

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -4,8 +4,8 @@
"private": true,
"scripts": {
"typecheck": "tsc --noEmit",
"build": "node build.mjs && node build.shimmed.mjs",
"test": "node dist/index.js && node --experimental-require-module dist/index.js && node dist/index.shimmed.mjs",
"build": "node build-cjs.mjs && node build-esm.mjs",
"test": "node dist/cjs/index.js && node --experimental-require-module dist/cjs/index.js && node dist/esm/index.mjs",
"clean": "npx rimraf node_modules dist",
"test:electron": "$(pnpm bin)/electron-rebuild && playwright test",
"test:build": "pnpm run typecheck && pnpm run build",
Expand All @@ -17,9 +17,9 @@
"@sentry/electron": "latest || *",
"@sentry/node": "latest || *",
"@sentry/profiling-node": "latest || *",
"electron": "^33.2.0"
"electron": "^33.2.0",
"esbuild": "0.20.0"
},
"devDependencies": {},
"volta": {
"extends": "../../package.json"
},
Expand Down
1 change: 1 addition & 0 deletions packages/profiling-node/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@
"author": "Sentry",
"license": "MIT",
"main": "lib/cjs/index.js",
"module": "lib/esm/index.js",
"types": "lib/types/index.d.ts",
"exports": {
"./package.json": "./package.json",
Expand Down
51 changes: 11 additions & 40 deletions packages/profiling-node/rollup.npm.config.mjs
Original file line number Diff line number Diff line change
@@ -1,49 +1,20 @@
import commonjs from '@rollup/plugin-commonjs';
import replace from '@rollup/plugin-replace';
import { makeBaseNPMConfig, makeNPMConfigVariants } from '@sentry-internal/rollup-utils';

export const ESMShim = `
import cjsUrl from 'node:url';
import cjsPath from 'node:path';
import cjsModule from 'node:module';

if(typeof __filename === 'undefined'){
globalThis.__filename = cjsUrl.fileURLToPath(import.meta.url);
}

if(typeof __dirname === 'undefined'){
globalThis.__dirname = cjsPath.dirname(__filename);
}

if(typeof require === 'undefined'){
globalThis.require = cjsModule.createRequire(import.meta.url);
}
`;

function makeESMShimPlugin(shim) {
return {
transform(code) {
const SHIM_REGEXP = /\/\/ #START_SENTRY_ESM_SHIM[\s\S]*?\/\/ #END_SENTRY_ESM_SHIM/;
return code.replace(SHIM_REGEXP, shim);
},
};
}

const variants = makeNPMConfigVariants(
export default makeNPMConfigVariants(
makeBaseNPMConfig({
packageSpecificConfig: {
output: { dir: 'lib', preserveModules: false },
plugins: [commonjs()],
plugins: [
commonjs(),
replace({
preventAssignment: false,
values: {
__IMPORT_META_URL_REPLACEMENT__: 'import.meta.url',
},
}),
],
},
}),
);

for (const variant of variants) {
if (variant.output.format === 'esm') {
variant.plugins.push(makeESMShimPlugin(ESMShim));
} else {
// Remove the ESM shim comment
variant.plugins.push(makeESMShimPlugin(''));
}
}

export default variants;
86 changes: 48 additions & 38 deletions packages/profiling-node/src/cpu_profiler.ts
Original file line number Diff line number Diff line change
@@ -1,6 +1,9 @@
import { createRequire } from 'node:module';
import { arch as _arch, platform as _platform } from 'node:os';
import { join, resolve } from 'node:path';
import { dirname } from 'node:path';
import { env, versions } from 'node:process';
import { fileURLToPath, pathToFileURL } from 'node:url';
import { threadId } from 'node:worker_threads';
import { familySync } from 'detect-libc';
import { getAbi } from 'node-abi';
Expand All @@ -15,84 +18,89 @@ import type {
} from './types';
import type { ProfileFormat } from './types';

// #START_SENTRY_ESM_SHIM
// When building for ESM, we shim require to use createRequire and __dirname.
// We need to do this because .node extensions in esm are not supported.
// The comment below this line exists as a placeholder for where to insert the shim.
// #END_SENTRY_ESM_SHIM
declare const __IMPORT_META_URL_REPLACEMENT__: string;

const stdlib = familySync();
const platform = process.env['BUILD_PLATFORM'] || _platform();
const arch = process.env['BUILD_ARCH'] || _arch();
const abi = getAbi(versions.node, 'node');
const identifier = [platform, arch, stdlib, abi].filter(c => c !== undefined && c !== null).join('-');

const built_from_source_path = resolve(__dirname, '..', `./sentry_cpu_profiler-${identifier}`);

/**
* Imports cpp bindings based on the current platform and architecture.
*/
// eslint-disable-next-line complexity
export function importCppBindingsModule(): PrivateV8CpuProfilerBindings {
// We need to work around using import.meta.url directly with __IMPORT_META_URL_REPLACEMENT__ because jest complains about it.
const importMetaUrl =
typeof __IMPORT_META_URL_REPLACEMENT__ !== 'undefined'
? // This case is always hit when the SDK is built
__IMPORT_META_URL_REPLACEMENT__
: // This case is hit when the tests are run
pathToFileURL(__filename).href;

const createdRequire = createRequire(importMetaUrl);
const esmCompatibleDirname = dirname(fileURLToPath(importMetaUrl));

// If a binary path is specified, use that.
if (env['SENTRY_PROFILER_BINARY_PATH']) {
const envPath = env['SENTRY_PROFILER_BINARY_PATH'];
return require(envPath);
return createdRequire(envPath);
}

// If a user specifies a different binary dir, they are in control of the binaries being moved there
if (env['SENTRY_PROFILER_BINARY_DIR']) {
const binaryPath = join(resolve(env['SENTRY_PROFILER_BINARY_DIR']), `sentry_cpu_profiler-${identifier}`);
return require(`${binaryPath}.node`);
return createdRequire(`${binaryPath}.node`);
}

// We need the fallthrough so that in the end, we can fallback to the dynamic require.
// This is for cases where precompiled binaries were not provided, but may have been compiled from source.
if (platform === 'darwin') {
if (arch === 'x64') {
if (abi === '93') {
return require('../sentry_cpu_profiler-darwin-x64-93.node');
return createdRequire('../sentry_cpu_profiler-darwin-x64-93.node');
}
if (abi === '108') {
return require('../sentry_cpu_profiler-darwin-x64-108.node');
return createdRequire('../sentry_cpu_profiler-darwin-x64-108.node');
}
if (abi === '115') {
return require('../sentry_cpu_profiler-darwin-x64-115.node');
return createdRequire('../sentry_cpu_profiler-darwin-x64-115.node');
}
if (abi === '127') {
return require('../sentry_cpu_profiler-darwin-x64-127.node');
return createdRequire('../sentry_cpu_profiler-darwin-x64-127.node');
}
}

if (arch === 'arm64') {
if (abi === '93') {
return require('../sentry_cpu_profiler-darwin-arm64-93.node');
return createdRequire('../sentry_cpu_profiler-darwin-arm64-93.node');
}
if (abi === '108') {
return require('../sentry_cpu_profiler-darwin-arm64-108.node');
return createdRequire('../sentry_cpu_profiler-darwin-arm64-108.node');
}
if (abi === '115') {
return require('../sentry_cpu_profiler-darwin-arm64-115.node');
return createdRequire('../sentry_cpu_profiler-darwin-arm64-115.node');
}
if (abi === '127') {
return require('../sentry_cpu_profiler-darwin-arm64-127.node');
return createdRequire('../sentry_cpu_profiler-darwin-arm64-127.node');
}
}
}

if (platform === 'win32') {
if (arch === 'x64') {
if (abi === '93') {
return require('../sentry_cpu_profiler-win32-x64-93.node');
return createdRequire('../sentry_cpu_profiler-win32-x64-93.node');
}
if (abi === '108') {
return require('../sentry_cpu_profiler-win32-x64-108.node');
return createdRequire('../sentry_cpu_profiler-win32-x64-108.node');
}
if (abi === '115') {
return require('../sentry_cpu_profiler-win32-x64-115.node');
return createdRequire('../sentry_cpu_profiler-win32-x64-115.node');
}
if (abi === '127') {
return require('../sentry_cpu_profiler-win32-x64-127.node');
return createdRequire('../sentry_cpu_profiler-win32-x64-127.node');
}
}
}
Expand All @@ -101,66 +109,68 @@ export function importCppBindingsModule(): PrivateV8CpuProfilerBindings {
if (arch === 'x64') {
if (stdlib === 'musl') {
if (abi === '93') {
return require('../sentry_cpu_profiler-linux-x64-musl-93.node');
return createdRequire('../sentry_cpu_profiler-linux-x64-musl-93.node');
}
if (abi === '108') {
return require('../sentry_cpu_profiler-linux-x64-musl-108.node');
return createdRequire('../sentry_cpu_profiler-linux-x64-musl-108.node');
}
if (abi === '115') {
return require('../sentry_cpu_profiler-linux-x64-musl-115.node');
return createdRequire('../sentry_cpu_profiler-linux-x64-musl-115.node');
}
if (abi === '127') {
return require('../sentry_cpu_profiler-linux-x64-musl-127.node');
return createdRequire('../sentry_cpu_profiler-linux-x64-musl-127.node');
}
}
if (stdlib === 'glibc') {
if (abi === '93') {
return require('../sentry_cpu_profiler-linux-x64-glibc-93.node');
return createdRequire('../sentry_cpu_profiler-linux-x64-glibc-93.node');
}
if (abi === '108') {
return require('../sentry_cpu_profiler-linux-x64-glibc-108.node');
return createdRequire('../sentry_cpu_profiler-linux-x64-glibc-108.node');
}
if (abi === '115') {
return require('../sentry_cpu_profiler-linux-x64-glibc-115.node');
return createdRequire('../sentry_cpu_profiler-linux-x64-glibc-115.node');
}
if (abi === '127') {
return require('../sentry_cpu_profiler-linux-x64-glibc-127.node');
return createdRequire('../sentry_cpu_profiler-linux-x64-glibc-127.node');
}
}
}
if (arch === 'arm64') {
if (stdlib === 'musl') {
if (abi === '93') {
return require('../sentry_cpu_profiler-linux-arm64-musl-93.node');
return createdRequire('../sentry_cpu_profiler-linux-arm64-musl-93.node');
}
if (abi === '108') {
return require('../sentry_cpu_profiler-linux-arm64-musl-108.node');
return createdRequire('../sentry_cpu_profiler-linux-arm64-musl-108.node');
}
if (abi === '115') {
return require('../sentry_cpu_profiler-linux-arm64-musl-115.node');
return createdRequire('../sentry_cpu_profiler-linux-arm64-musl-115.node');
}
if (abi === '127') {
return require('../sentry_cpu_profiler-linux-arm64-musl-127.node');
return createdRequire('../sentry_cpu_profiler-linux-arm64-musl-127.node');
}
}

if (stdlib === 'glibc') {
if (abi === '93') {
return require('../sentry_cpu_profiler-linux-arm64-glibc-93.node');
return createdRequire('../sentry_cpu_profiler-linux-arm64-glibc-93.node');
}
if (abi === '108') {
return require('../sentry_cpu_profiler-linux-arm64-glibc-108.node');
return createdRequire('../sentry_cpu_profiler-linux-arm64-glibc-108.node');
}
if (abi === '115') {
return require('../sentry_cpu_profiler-linux-arm64-glibc-115.node');
return createdRequire('../sentry_cpu_profiler-linux-arm64-glibc-115.node');
}
if (abi === '127') {
return require('../sentry_cpu_profiler-linux-arm64-glibc-127.node');
return createdRequire('../sentry_cpu_profiler-linux-arm64-glibc-127.node');
}
}
}
}
return require(`${built_from_source_path}.node`);

const built_from_source_path = resolve(esmCompatibleDirname, '..', `sentry_cpu_profiler-${identifier}`);
return createdRequire(`${built_from_source_path}.node`);
}

const PrivateCpuProfilerBindings: PrivateV8CpuProfilerBindings = importCppBindingsModule();
Expand Down
1 change: 0 additions & 1 deletion packages/profiling-node/tsconfig.json
Original file line number Diff line number Diff line change
Expand Up @@ -8,4 +8,3 @@
},
"include": ["src/**/*"]
}

Loading