feat(node): Fix local variables capturing for out-of-app frames #17545
Conversation
…etsentry#12588) This commit addresses 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. This was fixed 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. This has been corrected by adding a client to the test setup. Additionally, this commit adds tests for the `localVariablesAsyncIntegration` to ensure it correctly handles the `includeOutOfAppFrames` option.
…local variables integration
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
… in local variables integration
![seer-by-sentry[bot]](https://avatars.githubusercontent.com/in/801464?s=60&v=4){kind=small}
| return { | ||
| name: INTEGRATION_NAME, |
There was a problem hiding this comment.
Potential bug: Changing setupOnce to async violates the integration interface contract. The framework calls it synchronously, reintroducing a race condition where events are silently ignored during initialization.
-
Description: The
Integrationinterface definessetupOnceas a synchronous method, but it was changed to beasync. The integration framework calls this method withoutawait, causing the initialization logic inside to run in the background. While this async operation is pending, ashouldProcessEventflag remainsfalse. Consequently, any events received byprocessEventduring this startup window are silently ignored instead of being processed. This reintroduces the exact race condition the change was intended to fix, causing the integration to silently fail to capture variables for an indeterminate period. -
Suggested fix: Revert
setupOnceto a synchronous function to adhere to the interface contract. If asynchronous initialization is required, consider an alternative approach, such as queuing events within the integration until the async setup is complete, rather than making thesetupOncehook itself asynchronous.
severity: 0.85, confidence: 0.98
Did we get this right? 👍 / 👎 to inform future reviews.
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. This commit fixes the race condition by: - Making `setupOnce` synchronous to adhere to the interface contract. - Moving the asynchronous initialization logic to a separate `setup` function. - Making `processEvent` asynchronous and awaiting the result of the `setup` function, ensuring that the integration is fully initialized before processing any events. - Updating the tests to correctly `await` the `processEvent` method.
mydea
changed the title
There was a problem hiding this comment.
Thank you for the PR @JealousGx!
This needs to be a feat(node) because we are adding a new includeOutOfAppFrames. Only feedback is around testing strategy, implementation looks good!
| // // TEST-ONLY: allow tests to access the cache | ||
|
|
||
| import type { LRUMap } from '@sentry/core'; | ||
| import type { FrameVariables } from './common'; |
There was a problem hiding this comment.
I would prefer if we didn't do this (and not add unit tests). Instead I'd like us to add tests under our node integration tests to validate this: https://github.com/getsentry/sentry-javascript/tree/develop/dev-packages/node-integration-tests/suites/public-api/LocalVariables
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
JealousGx
changed the title
rodolfoBee
requested review from
a team and
Lms24
and removed request for
a team
| /* eslint-disable no-unused-vars */ | ||
|
|
||
| const Sentry = require('@sentry/node'); | ||
| // const { loggingTransport } = require('@sentry-internal/node-integration-tests'); is throwing error that package not found, so using relative path |
There was a problem hiding this comment.
This shouldn't be happening, are you running yarn build in the root of the repo? You might have to run yarn clean and delete node modules to get stuff in a good state.
There was a problem hiding this comment.
Well, unfortunately, it's happening even after clean installation and building.
The steps I took:
- Delete root node_modules directory
yarnyarn cleanyarn build- Make sure to import
loggingTransportfrom@sentry-internal/node-integration-tests DEBUG=true yarn workspace @sentry-internal/node-integration-tests test -- LocalVariables
You will get this error:
Error: Cannot find module '/Users/my-pc/sentry-javascript/node_modules/@sentry-internal/node-integration-tests/build/cjs/index.js'. Please verify that the package.json has a valid "main" entry
at tryPackage (node:internal/modules/cjs/loader:515:19)
at Function._findPath (node:internal/modules/cjs/loader:800:18)
at Function._resolveFilename (node:internal/modules/cjs/loader:1391:27)
at defaultResolveImpl (node:internal/modules/cjs/loader:1061:19)
at resolveForCJSWithHooks (node:internal/modules/cjs/loader:1066:22)
at Function._load (node:internal/modules/cjs/loader:1215:37)
at TracingChannel.traceSync (node:diagnostics_channel:322:14)
at wrapModuleLoad (node:internal/modules/cjs/loader:235:24)
at Module.require (node:internal/modules/cjs/loader:1491:12)
at require (node:internal/modules/helpers:135:16) {
code: 'MODULE_NOT_FOUND',
path: '/Users/my-pc/sentry-javascript/node_modules/@sentry-internal/node-integration-tests/package.json',
requestPath: '@sentry-internal/node-integration-tests'
}
There was a problem hiding this comment.
I wonder if the issue is yarn workspace, if you cd into the directory and run the tests there directly do you get the same issues? I'm unfortunately not able to reproduce.
There was a problem hiding this comment.
Alright yeah. My bad. I was building in the root directory. I should've run the build command in the node-integration-tests directory as well. Working fine now. Let's wait for @timfish for their reply to the last comment now.
| const Sentry = require('@sentry/node'); | ||
| // const { loggingTransport } = require('@sentry-internal/node-integration-tests'); is throwing error that package not found, so using relative path | ||
| const { loggingTransport } = require('../../../src/index.ts'); | ||
|
|
||
| // make sure to create the following file with the following content: | ||
| // function out_of_app_function() { | ||
| // const outOfAppVar = 'out of app value'; | ||
| // throw new Error('out-of-app error'); | ||
| // } | ||
|
|
||
| // module.exports = { out_of_app_function }; |
There was a problem hiding this comment.
This is far too brittle, but not sure how to best do this. Maybe @timfish you have some ideas?
There was a problem hiding this comment.
We can either do this or use beforeSend method in the init to modify the frames and include the necessary details. If there are other ways to achieve this, we could try them as well.
…esolve package not found error
This commit addresses an issue where local variables were not being captured for out-of-app frames.
The
localVariablesSyncIntegrationhad a race condition where it would process events before the debugger session was fully initialized. This was fixed by awaiting the session creation insetupOnce.Additionally, this PR adds tests for the
localVariablesAsyncIntegrationto ensure it correctly handles theincludeOutOfAppFramesoption.--
Before submitting a pull request, please take a look at our
Contributing guidelines and verify:
yarn lint) & (yarn test): The tests failError: Failed to resolve entry for package "@sentry/browser". The package may have incorrect main/module/exports specified in its package.json.