-
-
Notifications
You must be signed in to change notification settings - Fork 2
feat: Capture thread state from AsyncLocalStorage store
#24
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
bd16bcd to
b642be1
Compare
80402df to
1ee6ed1
Compare
AsyncLocalStorage store
e623ef6 to
936d588
Compare
1bd5e78 to
9046e07
Compare
63a93d1 to
1eccc68
Compare
c26eb88 to
0937b72
Compare
0937b72 to
b3c5cd6
Compare
|
@sentry review |
| poll_state}; | ||
| }, | ||
| thread_isolate)); | ||
| std::cref(thread_info.async_store))); |
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.
so this is being accessed in a lock, but it's async right? Will the async operation be safe? Should we just copy the reference to thread_info.async_store?
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.
Unfortunately the v8 objects are mostly not copyable. Here we are passing the reference which is the potentially risky thing.
The lock is held until all the futures have completed so this reference can never be changed from any of our code. This is however just a reference to an object on the javascript heap. Because we hold it as a v8::Global<v8::Value>, this retains a reference which will stop it getting GC'd.
It could go out of scope as the isolate terminates which I've tried to protect against here:
https://github.com/getsentry/sentry-javascript-node-native-stacktrace/pull/24/files#diff-469028a692a3e0cf7037091c4846a3fbd09baf3f789e51d7b4271b5ef911f64bR188
|
Before we merge this PR I want to see if the changes are sufficient to add scope capture in the |
15aa515 to
b98a775
Compare
e49f7a2 to
eafa21b
Compare
|
This has now been tested working with our integration capturing scope! |
eventLoopBlockIntegrationcaptures events with missing scope sentry-javascript#17887This PR retains the original polled state feature so that we still support debug images and session termination on older versions of Node.
For Node.js v24 (and v22 with
--experimental-async-context-frameflag) this PR adds support for fetching state from anAsyncLocalStorageinstance from the native side. This allows us to fetch the current scopes from the Open Telemetry context object.With this we will be able to include all scope data and trace context!
Results in the following output:
{ "0": { "frames": [ { "function": "from", "filename": "node:buffer", "lineno": 304, "colno": 28 }, { "function": "pbkdf2Sync", "filename": "node:internal/crypto/pbkdf2", "lineno": 79, "colno": 17 }, { "function": "longWork", "filename": "/Users/tim/Documents/Repos/node-native-stacktrace/test/long-work.js", "lineno": 6, "colno": 25 }, { "function": "?", "filename": "file:///Users/tim/Documents/Repos/node-native-stacktrace/test/async-storage.mjs", "lineno": 22, "colno": 7 }, ], "asyncState": { "traceId": "trace-5" } } }