Skip to content

Commit a7767a3

Browse files
JealousGxisaacs
authored andcommitted
feat(node): Fix local variables capturing for out-of-app frames (#18245)
Address an issue where local variables were not being captured for out-of-app frames, even when the `includeOutOfAppFrames` option was enabled. The `localVariablesSyncIntegration` had a race condition where it would process events before the debugger session was fully initialized. Fix this by awaiting the session creation in `setupOnce`. The tests for this integration were failing because they were not setting up a Sentry client, which is required for the integration to be enabled. Correct by adding a client to the test setup. Additionally, add tests for the `localVariablesAsyncIntegration` to ensure it correctly handles the `includeOutOfAppFrames` option. The `LocalVariables` integrations `setupOnce` method was `async`, which violates the `Integration` interface. This caused a race condition where events could be processed before the integration was fully initialized, leading to missed local variables. Fix the race condition by: - Make `setupOnce` synchronous to adhere to the interface contract - Move the asynchronous initialization logic to a separate `setup` function - Make `processEvent` asynchronous and await the result of the `setup` function, so the integration is fully initialized before processing any events - Update tests to correctly `await` the `processEvent` method Fixes GH-12588 Fixes GH-17545
1 parent 93faf61 commit a7767a3

File tree

6 files changed

+253
-109
lines changed

6 files changed

+253
-109
lines changed
Lines changed: 28 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,28 @@
1+
/* eslint-disable no-unused-vars */
2+
3+
const Sentry = require('@sentry/node');
4+
const { loggingTransport } = require('@sentry-internal/node-integration-tests');
5+
6+
const externalFunctionFile = require.resolve('./node_modules/out-of-app-function.js');
7+
8+
const { out_of_app_function } = require(externalFunctionFile);
9+
10+
function in_app_function() {
11+
const inAppVar = 'in app value';
12+
out_of_app_function(`${inAppVar} modified value`);
13+
}
14+
15+
Sentry.init({
16+
dsn: 'https://public@dsn.ingest.sentry.io/1337',
17+
transport: loggingTransport,
18+
includeLocalVariables: true,
19+
});
20+
21+
setTimeout(async () => {
22+
try {
23+
in_app_function();
24+
} catch (e) {
25+
Sentry.captureException(e);
26+
await Sentry.flush();
27+
}
28+
}, 500);
Lines changed: 33 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,33 @@
1+
/* eslint-disable no-unused-vars */
2+
3+
const Sentry = require('@sentry/node');
4+
const { loggingTransport } = require('@sentry-internal/node-integration-tests');
5+
6+
const externalFunctionFile = require.resolve('./node_modules/out-of-app-function.js');
7+
8+
const { out_of_app_function } = require(externalFunctionFile);
9+
10+
Sentry.init({
11+
dsn: 'https://public@dsn.ingest.sentry.io/1337',
12+
transport: loggingTransport,
13+
includeLocalVariables: true,
14+
integrations: [
15+
Sentry.localVariablesIntegration({
16+
includeOutOfAppFrames: true,
17+
}),
18+
],
19+
});
20+
21+
function in_app_function() {
22+
const inAppVar = 'in app value';
23+
out_of_app_function(`${inAppVar} modified value`);
24+
}
25+
26+
setTimeout(async () => {
27+
try {
28+
in_app_function();
29+
} catch (e) {
30+
Sentry.captureException(e);
31+
await Sentry.flush();
32+
}
33+
}, 500);

dev-packages/node-integration-tests/suites/public-api/LocalVariables/test.ts

Lines changed: 72 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,6 @@
1+
import { mkdirSync, rmdirSync, unlinkSync, writeFileSync } from 'fs';
12
import * as path from 'path';
2-
import { afterAll, describe, expect, test } from 'vitest';
3+
import { afterAll, beforeAll, describe, expect, test } from 'vitest';
34
import { conditionalTest } from '../../../utils';
45
import { cleanupChildProcesses, createRunner } from '../../../utils/runner';
56

@@ -39,8 +40,35 @@ const EXPECTED_LOCAL_VARIABLES_EVENT = {
3940
};
4041

4142
describe('LocalVariables integration', () => {
43+
const nodeModules = `${__dirname}/node_modules`;
44+
const externalModule = `${nodeModules}//out-of-app-function.js`;
45+
function cleanupExternalModuleFile() {
46+
try {
47+
unlinkSync(externalModule);
48+
// eslint-disable-next-line no-empty
49+
} catch {}
50+
try {
51+
rmdirSync(nodeModules);
52+
// eslint-disable-next-line no-empty
53+
} catch {}
54+
}
55+
56+
beforeAll(() => {
57+
cleanupExternalModuleFile();
58+
mkdirSync(nodeModules, { recursive: true });
59+
writeFileSync(
60+
externalModule,
61+
`
62+
function out_of_app_function(passedArg) {
63+
const outOfAppVar = "out of app value " + passedArg.substring(13);
64+
throw new Error("out-of-app error");
65+
}
66+
module.exports = { out_of_app_function };`,
67+
);
68+
});
4269
afterAll(() => {
4370
cleanupChildProcesses();
71+
cleanupExternalModuleFile();
4472
});
4573

4674
test('Should not include local variables by default', async () => {
@@ -127,4 +155,47 @@ describe('LocalVariables integration', () => {
127155
.start()
128156
.completed();
129157
});
158+
159+
test('adds local variables to out of app frames when includeOutOfAppFrames is true', async () => {
160+
await createRunner(__dirname, 'local-variables-out-of-app.js')
161+
.expect({
162+
event: event => {
163+
const frames = event.exception?.values?.[0]?.stacktrace?.frames || [];
164+
165+
const inAppFrame = frames.find(frame => frame.function === 'in_app_function');
166+
const outOfAppFrame = frames.find(frame => frame.function === 'out_of_app_function');
167+
168+
expect(inAppFrame?.vars).toEqual({ inAppVar: 'in app value' });
169+
expect(inAppFrame?.in_app).toEqual(true);
170+
171+
expect(outOfAppFrame?.vars).toEqual({
172+
outOfAppVar: 'out of app value modified value',
173+
passedArg: 'in app value modified value',
174+
});
175+
expect(outOfAppFrame?.in_app).toEqual(false);
176+
},
177+
})
178+
.start()
179+
.completed();
180+
});
181+
182+
test('does not add local variables to out of app frames by default', async () => {
183+
await createRunner(__dirname, 'local-variables-out-of-app-default.js')
184+
.expect({
185+
event: event => {
186+
const frames = event.exception?.values?.[0]?.stacktrace?.frames || [];
187+
188+
const inAppFrame = frames.find(frame => frame.function === 'in_app_function');
189+
const outOfAppFrame = frames.find(frame => frame.function === 'out_of_app_function');
190+
191+
expect(inAppFrame?.vars).toEqual({ inAppVar: 'in app value' });
192+
expect(inAppFrame?.in_app).toEqual(true);
193+
194+
expect(outOfAppFrame?.vars).toBeUndefined();
195+
expect(outOfAppFrame?.in_app).toEqual(false);
196+
},
197+
})
198+
.start()
199+
.completed();
200+
});
130201
});

packages/node-core/src/integrations/local-variables/common.ts

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -99,6 +99,12 @@ export interface LocalVariablesIntegrationOptions {
9999
* Maximum number of exceptions to capture local variables for per second before rate limiting is triggered.
100100
*/
101101
maxExceptionsPerSecond?: number;
102+
/**
103+
* When true, local variables will be captured for all frames, including those that are not in_app.
104+
*
105+
* Defaults to `false`.
106+
*/
107+
includeOutOfAppFrames?: boolean;
102108
}
103109

104110
export interface LocalVariablesWorkerArgs extends LocalVariablesIntegrationOptions {

packages/node-core/src/integrations/local-variables/local-variables-async.ts

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -39,8 +39,8 @@ export const localVariablesAsyncIntegration = defineIntegration(((
3939
if (
4040
// We need to have vars to add
4141
frameLocalVariables.vars === undefined ||
42-
// We're not interested in frames that are not in_app because the vars are not relevant
43-
frame.in_app === false ||
42+
// Only skip out-of-app frames if includeOutOfAppFrames is not true
43+
(frame.in_app === false && integrationOptions.includeOutOfAppFrames !== true) ||
4444
// The function names need to match
4545
!functionNamesMatch(frame.function, frameLocalVariables.function)
4646
) {

0 commit comments

Comments
 (0)