feat(node): Local variables via async inspector in node 19+#9962
Merged
AbhiPrasad merged 15 commits intogetsentry:developfrom Jan 2, 2024
Merged
feat(node): Local variables via async inspector in node 19+#9962AbhiPrasad merged 15 commits intogetsentry:developfrom
AbhiPrasad merged 15 commits intogetsentry:developfrom
Conversation
AbhiPrasad
reviewed
Dec 21, 2023
anonrig
reviewed
Dec 21, 2023
packages/node/src/integrations/localvariables/localvariables-async.ts
Outdated
Show resolved
Hide resolved
packages/node/src/integrations/localvariables/localvariables-async.ts
Outdated
Show resolved
Hide resolved
packages/node/src/integrations/localvariables/localvariables-async.ts
Outdated
Show resolved
Hide resolved
packages/node/src/integrations/localvariables/localvariables-async.ts
Outdated
Show resolved
Hide resolved
packages/node/src/integrations/localvariables/localvariables-async.ts
Outdated
Show resolved
Hide resolved
packages/node/src/integrations/localvariables/localvariables-async.ts
Outdated
Show resolved
Hide resolved
packages/node/src/integrations/localvariables/localvariables-async.ts
Outdated
Show resolved
Hide resolved
…ctor' into feat/local-variables-async-inspector
anonrig
approved these changes
Dec 21, 2023
| } | ||
|
|
||
| /** | ||
| * @types/node doesn't have a `node:inspector/promises` module, maybe because it's still experimental? |
Contributor
There was a problem hiding this comment.
@types/node has it, but it is under inspector. Ref: https://github.com/DefinitelyTyped/DefinitelyTyped/blob/master/types/node/inspector.d.ts#L1775
Collaborator
Author
There was a problem hiding this comment.
Thats the sync interface with callbacks:
https://github.com/DefinitelyTyped/DefinitelyTyped/blob/a84a09dd3a6ce50a136a7c358c8581f46c812cf1/types/node/inspector.d.ts#L1818
The node:inspector/promises API returns promises:
https://nodejs.org/api/inspector.html#sessionpostmethod-params
…ctor' into feat/local-variables-async-inspector
aa026a4 to
8db6308
Compare
8db6308 to
a7f98f1
Compare
AbhiPrasad
approved these changes
Jan 2, 2024
anonrig
pushed a commit
that referenced
this pull request
Jan 3, 2024
This PR creates a new `LocalVariables` integration that uses the async inspector API. Rather than create a huge messy integration that supports both the sync (node 18) and async (node >= v19) APIs, I created a new integration and wrapped both the sync and async integrations with user facing integration that switches depending on node version. The async API doesn't require the stacking of callbacks that risks stack overflows and limits the number of frames we dare to evaluate. When we tried wrapping the sync API with promises, memory was leaked at an alarming rate! The inspector APIs are not available on all builds of Node so we have to lazily load it and catch any exceptions. I've had to use `dynamicRequire` because webpack picks up `import()` and reports missing dependency when bundling for older versions of node.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This PR creates a new
LocalVariablesintegration that uses the async inspector API.Rather than create a huge messy integration that supports both the sync (node 18) and async (node >= v19) APIs, I created a new integration and wrapped both the sync and async integrations with user facing integration that switches depending on node version.
The async API doesn't require the stacking of callbacks that risks stack overflows and limits the number of frames we dare to evaluate. When we tried wrapping the sync API with promises, memory was leaked at an alarming rate!
The inspector APIs are not available on all builds of Node so we have to lazily load it and catch any exceptions.
For the new integration I usedimportrather thanrequire. This async importing, coupled with the async API means that errors that occur before the import and debugger is configured will not have local variables attached... hence the addition of timeouts in the integration tests.I've had to use
dynamicRequirebecause webpack picks upimport()and reports missing dependency when bundling for older versions of node.