From 624cdb6f87bb051cb279fa6c3b5e096604b395e0 Mon Sep 17 00:00:00 2001 From: Tim Fish Date: Fri, 3 Feb 2023 10:31:06 +0100 Subject: [PATCH 1/5] Better ESM matching --- .../LocalVariables/local-variables-caught.mjs | 36 +++++++++++++++++++ .../suites/public-api/LocalVariables/test.ts | 28 +++++++++++++++ .../node/src/integrations/localvariables.ts | 2 +- 3 files changed, 65 insertions(+), 1 deletion(-) create mode 100644 packages/node-integration-tests/suites/public-api/LocalVariables/local-variables-caught.mjs diff --git a/packages/node-integration-tests/suites/public-api/LocalVariables/local-variables-caught.mjs b/packages/node-integration-tests/suites/public-api/LocalVariables/local-variables-caught.mjs new file mode 100644 index 000000000000..175ff0702d30 --- /dev/null +++ b/packages/node-integration-tests/suites/public-api/LocalVariables/local-variables-caught.mjs @@ -0,0 +1,36 @@ +/* eslint-disable no-unused-vars */ +import * as Sentry from '@sentry/node'; + +Sentry.init({ + dsn: 'https://public@dsn.ingest.sentry.io/1337', + includeLocalVariables: true, + integrations: [new Sentry.Integrations.LocalVariables({ captureAllExceptions: true })], + beforeSend: event => { + // eslint-disable-next-line no-console + console.log(JSON.stringify(event)); + }, +}); + +class Some { + two(name) { + throw new Error('Enough!'); + } +} + +function one(name) { + const arr = [1, '2', null]; + const obj = { + name, + num: 5, + }; + + const ty = new Some(); + + ty.two(name); +} + +try { + one('some name'); +} catch (e) { + Sentry.captureException(e); +} diff --git a/packages/node-integration-tests/suites/public-api/LocalVariables/test.ts b/packages/node-integration-tests/suites/public-api/LocalVariables/test.ts index 649f7cc9ba06..5e849651d8c5 100644 --- a/packages/node-integration-tests/suites/public-api/LocalVariables/test.ts +++ b/packages/node-integration-tests/suites/public-api/LocalVariables/test.ts @@ -52,6 +52,34 @@ describe('LocalVariables integration', () => { }); }); + test('Should include local variables with ESM', done => { + expect.assertions(4); + + const testScriptPath = path.resolve(__dirname, 'local-variables-caught.mjs'); + + childProcess.exec(`node ${testScriptPath}`, { encoding: 'utf8' }, (_, stdout) => { + const event = JSON.parse(stdout) as Event; + + const frames = event.exception?.values?.[0].stacktrace?.frames || []; + const lastFrame = frames[frames.length - 1]; + + expect(lastFrame.function).toBe('Some.two'); + expect(lastFrame.vars).toEqual({ name: 'some name' }); + + const penultimateFrame = frames[frames.length - 2]; + + expect(penultimateFrame.function).toBe('one'); + expect(penultimateFrame.vars).toEqual({ + name: 'some name', + arr: [1, '2', null], + obj: { name: 'some name', num: 5 }, + ty: '', + }); + + done(); + }); + }); + test('Includes local variables for caught exceptions when enabled', done => { expect.assertions(4); diff --git a/packages/node/src/integrations/localvariables.ts b/packages/node/src/integrations/localvariables.ts index ac23e67af910..d8024b6c3322 100644 --- a/packages/node/src/integrations/localvariables.ts +++ b/packages/node/src/integrations/localvariables.ts @@ -237,7 +237,7 @@ export class LocalVariables implements Integration { const framePromises = callFrames.map(async ({ scopeChain, functionName, this: obj }) => { const localScope = scopeChain.find(scope => scope.type === 'local'); - const fn = obj.className === 'global' ? functionName : `${obj.className}.${functionName}`; + const fn = obj.className === 'global' || !obj.className ? functionName : `${obj.className}.${functionName}`; if (localScope?.object.objectId === undefined) { return { function: fn }; From 5e843301e1a2cfb6f3e2c8d4b5638288e04b109a Mon Sep 17 00:00:00 2001 From: Tim Fish Date: Fri, 3 Feb 2023 11:25:46 +0100 Subject: [PATCH 2/5] comment --- packages/node/src/integrations/localvariables.ts | 1 + 1 file changed, 1 insertion(+) diff --git a/packages/node/src/integrations/localvariables.ts b/packages/node/src/integrations/localvariables.ts index d8024b6c3322..2423cfa30c70 100644 --- a/packages/node/src/integrations/localvariables.ts +++ b/packages/node/src/integrations/localvariables.ts @@ -237,6 +237,7 @@ export class LocalVariables implements Integration { const framePromises = callFrames.map(async ({ scopeChain, functionName, this: obj }) => { const localScope = scopeChain.find(scope => scope.type === 'local'); + // obj.className is undefined in ESM modules const fn = obj.className === 'global' || !obj.className ? functionName : `${obj.className}.${functionName}`; if (localScope?.object.objectId === undefined) { From e325cc87adee736321204e2704dd9e659aad0e42 Mon Sep 17 00:00:00 2001 From: Tim Fish Date: Fri, 3 Feb 2023 11:37:59 +0100 Subject: [PATCH 3/5] Pass --experimental-modules in node v10 --- .../suites/public-api/LocalVariables/test.ts | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/packages/node-integration-tests/suites/public-api/LocalVariables/test.ts b/packages/node-integration-tests/suites/public-api/LocalVariables/test.ts index 5e849651d8c5..17948505164e 100644 --- a/packages/node-integration-tests/suites/public-api/LocalVariables/test.ts +++ b/packages/node-integration-tests/suites/public-api/LocalVariables/test.ts @@ -1,7 +1,10 @@ import type { Event } from '@sentry/node'; +import { parseSemver } from '@sentry/utils'; import * as childProcess from 'child_process'; import * as path from 'path'; +const nodeMajor = parseSemver(process.version).major || 1; + describe('LocalVariables integration', () => { test('Should not include local variables by default', done => { expect.assertions(2); @@ -56,8 +59,9 @@ describe('LocalVariables integration', () => { expect.assertions(4); const testScriptPath = path.resolve(__dirname, 'local-variables-caught.mjs'); + const arg = nodeMajor < 12 ? '--experimental-modules' : ''; - childProcess.exec(`node ${testScriptPath}`, { encoding: 'utf8' }, (_, stdout) => { + childProcess.exec(`node ${arg} ${testScriptPath}`, { encoding: 'utf8' }, (_, stdout) => { const event = JSON.parse(stdout) as Event; const frames = event.exception?.values?.[0].stacktrace?.frames || []; From cc88f8e8e090ce2703a4caf8beb4e87aa6ad3637 Mon Sep 17 00:00:00 2001 From: Tim Fish Date: Sun, 5 Feb 2023 15:36:19 +0100 Subject: [PATCH 4/5] Don't run ESM tests on node <= v10 --- .../suites/public-api/LocalVariables/test.ts | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/packages/node-integration-tests/suites/public-api/LocalVariables/test.ts b/packages/node-integration-tests/suites/public-api/LocalVariables/test.ts index 17948505164e..11f7d21ad485 100644 --- a/packages/node-integration-tests/suites/public-api/LocalVariables/test.ts +++ b/packages/node-integration-tests/suites/public-api/LocalVariables/test.ts @@ -5,6 +5,8 @@ import * as path from 'path'; const nodeMajor = parseSemver(process.version).major || 1; +const testIf = (condition: boolean, test: jest.It) => (condition ? test : test.skip); + describe('LocalVariables integration', () => { test('Should not include local variables by default', done => { expect.assertions(2); @@ -55,13 +57,12 @@ describe('LocalVariables integration', () => { }); }); - test('Should include local variables with ESM', done => { + testIf(nodeMajor > 10, test)('Should include local variables with ESM', done => { expect.assertions(4); const testScriptPath = path.resolve(__dirname, 'local-variables-caught.mjs'); - const arg = nodeMajor < 12 ? '--experimental-modules' : ''; - childProcess.exec(`node ${arg} ${testScriptPath}`, { encoding: 'utf8' }, (_, stdout) => { + childProcess.exec(`node ${testScriptPath}`, { encoding: 'utf8' }, (_, stdout) => { const event = JSON.parse(stdout) as Event; const frames = event.exception?.values?.[0].stacktrace?.frames || []; From 61975904974f0576d4f3b459ce9b09c98eba5e3a Mon Sep 17 00:00:00 2001 From: Tim Fish Date: Sun, 5 Feb 2023 16:16:39 +0100 Subject: [PATCH 5/5] Fix version detection --- .../suites/public-api/LocalVariables/test.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/packages/node-integration-tests/suites/public-api/LocalVariables/test.ts b/packages/node-integration-tests/suites/public-api/LocalVariables/test.ts index 11f7d21ad485..aae488d50475 100644 --- a/packages/node-integration-tests/suites/public-api/LocalVariables/test.ts +++ b/packages/node-integration-tests/suites/public-api/LocalVariables/test.ts @@ -3,9 +3,9 @@ import { parseSemver } from '@sentry/utils'; import * as childProcess from 'child_process'; import * as path from 'path'; -const nodeMajor = parseSemver(process.version).major || 1; +const nodeMajor = parseSemver(process.version.slice(1)).major || 1; -const testIf = (condition: boolean, test: jest.It) => (condition ? test : test.skip); +const testIf = (condition: boolean, t: jest.It) => (condition ? t : t.skip); describe('LocalVariables integration', () => { test('Should not include local variables by default', done => {