-
-
Couldn't load subscription status.
- Fork 1.7k
feat(node): Fix local variables capturing for out-of-app frames #17545
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
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
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
... in local variables integration
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
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.