feat(node): Add `LocalVariables` integration to capture local variables to stack frames #6478

timfish · 2022-12-09T00:04:03Z

Takes the demo by @kamilogorek and adds:

Caching with LRU + hash so that the correct previously encountered exception can be looked up
Adds local variables for as many frame as possible
Fixes a couple of minor issues on older versions of node

It looks like the error.stack is available through the inspector error data.description. This is parsed and used to create a unique hash which is used later to lookup vars for stack frames when the event processor is run.

Applying vars to frames is best effort since the function names from the inspector don't always match those from the stack trace. The most obvious place this happens it anonymous functions. For example the name of anonymous function passed to setTimeout shows as an empty string from the inspector and Timeout._onTimeout in the stack trace.

What's left:

Add e2e Tests
Looks like it'll also just work for reason: 'promiseRejection'
Could probably do with better comparison for anonymous functions

AbhiPrasad · 2022-12-09T13:48:07Z

Do we have an idea of the performance impact here?

timfish · 2022-12-09T14:44:57Z

I haven't measured anything yet but I suspect it's Not That Bad™️.

I've had to cache the promise(s) looking up the stack vars because I found that the event processor was called before all the inspector async calls returned. This suggests that the debug pause isn't that long. I guessed it would block until our async function completed but it doesn't.

I'm wondering whether the inspector API might be a suitable alternative to hooking error and unhandledRection since the stack frames look better than what we can get from parsing error.stack, especially when it comes to async frames.

One caveat: I've only tested a single recent version of node!

…-javascript into feat/stack-locals

timfish · 2022-12-12T15:05:58Z

This is now failing the AWS Lambda serverless build. I had assumed that dynamicRequire would negate all bundler issues.

What's the safest way to only load this integration in node.js >= v14 while not tripping up Rollup or other bundlers?

AbhiPrasad · 2022-12-12T15:17:54Z

What's the safest way to only load this integration in node.js >= v14 while not tripping up Rollup or other bundlers?

Can we just always import this integration but then gate the functionality by a runtime node version check?

timfish · 2022-12-12T15:40:24Z

Can we just always import this integration but then gate the functionality by a runtime node version check?

My concern was ending up with code containing require('inspector') which would fail on runtimes that don't have that module. We would need to gate that import or gate importing the entire integration to avoid that. I've just checked and node.js v8 does support it (although it's experimental) so it shouldn't be an issue with the node.

However, will require('inspector') result in an error on AWS lambda?

packages/node/src/types.ts

AbhiPrasad · 2022-12-12T16:45:01Z

However, will require('inspector') result in an error on AWS lambda?

It shouldn't since it's running NodeJS, so the APIs should all be there, and we're not worried about non-node platforms for now. If the import exists since Node 8, we should be good to go.

…-javascript into feat/stack-locals

packages/node-integration-tests/suites/public-api/LocalVariables/test.ts

+
+    const testScriptPath = path.resolve(__dirname, 'no-local-variables.js');
+
+    childProcess.exec(`node ${testScriptPath}`, { encoding: 'utf8' }, (_, stdout) => {


packages/node-integration-tests/suites/public-api/LocalVariables/test.ts

+
+    const testScriptPath = path.resolve(__dirname, 'local-variables.js');
+
+    childProcess.exec(`node ${testScriptPath}`, { encoding: 'utf8' }, (_, stdout) => {


timfish · 2022-12-13T14:38:58Z

This is ready for a first pass!

AbhiPrasad

1st pass. Would be nice to get some unit tests for the event manipulation and _handlePaused func. Thanks for taking a look Tim!

packages/node/src/integrations/localvariables.ts

smeubank · 2022-12-15T15:33:36Z

the option is off by default correct? Did you already start looking at adding it to docs for node?

timfish · 2022-12-15T15:50:40Z

Yes, this is disabled by default and enabled by options._experiments.includeStackLocals.

It's in _experiments because if this becomes enabled by default, it'll be disabled by removing the integration rather than having it's own specific option.

Did you already start looking at adding it to docs for node?

No but I will do!

AbhiPrasad

Let's - excited to see this out there.

We also need a docs PR for this!

AbhiPrasad · 2023-01-09T09:17:26Z

packages/node/src/integrations/localvariables.ts

+    this.on('Debugger.paused', onPause);
+    this.post('Debugger.enable');
+    // We only want to pause on uncaught exceptions
+    this.post('Debugger.setPauseOnExceptions', { state: 'uncaught' });


Does this mean we only add stack locals for uncaught exceptions?

What do you think about making this configurable then?

Yes, it's probably worth adding an options object as the first constructor argument so we can also add more options in the future without a breaking change.

Let's add that now, and then merge this in? We can keep the options object empty for now.

AbhiPrasad · 2023-01-09T09:17:39Z

packages/node/src/integrations/localvariables.ts

+    stackParser: StackParser,
+    { params: { reason, data, callFrames } }: InspectorNotification<PausedExceptionEvent>,
+  ): Promise<void> {
+    if (reason !== 'exception' && reason !== 'promiseRejection') {


Is there a list of all the reasons somewhere?

https://chromedevtools.github.io/devtools-protocol/v8/Debugger/#event-paused

Allowed Values: ambiguous, assert, CSPViolation, debugCommand, DOM, EventListener, exception, instrumentation, OOM, other, promiseRejection, XHR

timfish · 2023-01-09T11:50:45Z

Added an empty options object.

Docs PR next!

Even though this could be pretty useful, local variables may contain sensitive information and sentry seems to be planning to add this to the list of default integrations. See getsentry/sentry-javascript#6478

timfish added 6 commits December 7, 2022 21:19

Add untested integration

381748a

Add LocalVariables integration

47bf353

Fix dynamic loading of integration

f89101d

Lint

5c03a9d

Improve

9a8f0dc

Better defaultIntegrations

726e874

AbhiPrasad self-requested a review December 9, 2022 13:48

timfish added 3 commits December 12, 2022 13:12

Merge remote-tracking branch 'upstream/master' into feat/stack-locals

4c2b05d

Merge branch 'feat/stack-locals' of https://github.com/timfish/sentry…

8635c2d

…-javascript into feat/stack-locals

Merge remote-tracking branch 'upstream/master' into feat/stack-locals

815a1d6

Dont check for node version since v8 supports inspector

d39fe92

Simplify a bit more

65ea442

timfish commented Dec 12, 2022

View reviewed changes

packages/node/src/types.ts Outdated Show resolved Hide resolved

timfish added 2 commits December 12, 2022 17:22

Merge branch 'feat/stack-locals' of https://github.com/timfish/sentry…

5d698a2

…-javascript into feat/stack-locals

Add some tests

5e0174b

timfish force-pushed the feat/stack-locals branch from 86be9bc to 5e0174b Compare December 12, 2022 19:24

github-advanced-security bot found potential problems Dec 12, 2022

View reviewed changes

timfish added 2 commits December 13, 2022 13:11

fix for older node versions where we get a __proto__ array property

c0f47b6

Move option to _experiments

b847980

timfish marked this pull request as ready for review December 13, 2022 13:40

AbhiPrasad reviewed Dec 13, 2022

View reviewed changes

timfish added 2 commits December 14, 2022 14:50

changes from review

d402354

Minor changes

c842fb3

timfish added 2 commits December 15, 2022 15:23

Add unit tests

93cb95e

remove lint disable

e3d11f3

Remove nullable stackParser property

e2b1f71

I miss test lint warnings

0b7fd39

timfish force-pushed the feat/stack-locals branch from d0180fc to 0b7fd39 Compare December 15, 2022 19:08

Merge remote-tracking branch 'upstream/master' into feat/stack-locals

d5d4700

timfish self-assigned this Dec 20, 2022

AbhiPrasad approved these changes Jan 9, 2023

View reviewed changes

timfish added 2 commits January 9, 2023 11:20

Add empty options object as first constructor param

2c144d0

jsdocs

bd351cc

AbhiPrasad merged commit 57c7e7b into getsentry:master Jan 9, 2023

timfish deleted the feat/stack-locals branch January 9, 2023 12:49

zolbooo mentioned this pull request Jan 13, 2023

Error [ERR_INSPECTOR_NOT_AVAILABLE]: Inspector is not available when using vercel/pkg #6769

Closed

3 tasks

shouze mentioned this pull request May 2, 2023

Errors with ky / globalThis.fetch when trying to upgrade @sentry/nextjs > 7.29.0: typeerror, cannot read properties of undefined (reading 'bind') #7070

Closed

3 tasks


		const testScriptPath = path.resolve(__dirname, 'no-local-variables.js');

		childProcess.exec(`node ${testScriptPath}`, { encoding: 'utf8' }, (_, stdout) => {

Uh oh!

feat(node): Add LocalVariables integration to capture local variables to stack frames #6478

feat(node): Add LocalVariables integration to capture local variables to stack frames #6478

Uh oh!

Conversation

timfish commented Dec 9, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

AbhiPrasad commented Dec 9, 2022

Uh oh!

timfish commented Dec 9, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

timfish commented Dec 12, 2022

Uh oh!

AbhiPrasad commented Dec 12, 2022

Uh oh!

timfish commented Dec 12, 2022

Uh oh!

Uh oh!

AbhiPrasad commented Dec 12, 2022

Uh oh!

Check warning

Check warning

timfish commented Dec 13, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

AbhiPrasad left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

smeubank commented Dec 15, 2022

Uh oh!

timfish commented Dec 15, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

AbhiPrasad left a comment

Choose a reason for hiding this comment

Uh oh!

AbhiPrasad Jan 9, 2023

Choose a reason for hiding this comment

Uh oh!

timfish Jan 9, 2023

Choose a reason for hiding this comment

Uh oh!

AbhiPrasad Jan 9, 2023

Choose a reason for hiding this comment

Uh oh!

AbhiPrasad Jan 9, 2023

Choose a reason for hiding this comment

Uh oh!

kamilogorek Jan 9, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

timfish commented Jan 9, 2023

Uh oh!

Uh oh!

feat(node): Add `LocalVariables` integration to capture local variables to stack frames #6478

feat(node): Add `LocalVariables` integration to capture local variables to stack frames #6478

timfish commented Dec 9, 2022 •

edited

Loading

timfish commented Dec 9, 2022 •

edited

Loading

timfish commented Dec 13, 2022 •

edited

Loading

timfish commented Dec 15, 2022 •

edited

Loading

kamilogorek Jan 9, 2023 •

edited

Loading