-
-
Notifications
You must be signed in to change notification settings - Fork 1.7k
Isaacs/jealous gx fix local variables 12588 #18245
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
base: develop
Are you sure you want to change the base?
Conversation
44f1258 to
a5170ec
Compare
size-limit report 📦
|
node-overhead report 🧳Note: This is a synthetic benchmark with a minimal express app and does not necessarily reflect the real-world performance impact in an application.
|
a5170ec to
4f88959
Compare
AbhiPrasad
left a comment
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.
Looks good! I wonder if the e2e test failures are related?
Let's also make sure we're following the commit guidelines here: https://github.com/getsentry/sentry-javascript/blob/develop/docs/commit-issue-pr-guidelines.md#commits
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
4f88959 to
fb3ba43
Compare
dev-packages/node-integration-tests/suites/public-api/LocalVariables/test.ts
Outdated
Show resolved
Hide resolved
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
fb3ba43 to
a7767a3
Compare
This is a fix commit for the test fragility, on top of JealousGx:fix/local-variables-12588, from PR #17545 which seems to have gone stale. Also, rebased onto develop branch, squashed, and updated to comply with commit message guidelines.
Close: #17545
Fix: #12588
CC: @JealousGx