Enable node.js stack locals capture #902

waltjones · 2020-11-04T15:21:31Z

Description of the change

Enables capture of local stack variables in Node.js. To enable, set locals: true in the config.

This feature uses the Node Inspector API to access debugger data at runtime. The Inspector API is a wrapper for the V8 Inspector interface. While the V8 interface relies on send() and receive() functions that can't be accessed from Node, the Node Inspector API provides access to these via the post() method on the Inspector Session instance. Using session.post(), all of the DevTools protocol can be accessed, including the Debugger and Runtime interfaces, which are used here.

The initial version of this feature supports both caught and uncaught exceptions and promise rejections. Nested exceptions and trace chains are supported.

The DevTools protocol allows retrieving nested variables to an arbitrary depth. However, the initial version of this feature only implements the equivalent of depth = 0. A future PR may enable arbitrary depth with a config option to set the depth.

Unexpanded objects are identified by their class, in keeping with the example in the Rollbar API docs. (https://explorer.docs.rollbar.com/#tag/Item)

Example:

trace: {
  frames: [
    {
      // ...
      locals: {
        foo: 'bar', // immediate string value
        obj: '<Foo object>' // instance of Foo
      }
    }
  ]
}

The feature is enabled for Node 10+. Node 8 doesn't implement the url property on the CallFrame object, and cannot reliably resolve to the correct locations in the stack. (There may be a workaround, but none that I know at this time.)

This PR adheres to the established style of Rollbar.js and uses var declarations, rather than let and const. ES5 style classes are used. I'm not sure whether session.post() supports async/await. Regardless, callbacks are used throughout as shared code that cannot be made async prevents the use of async/await in this code path.

Type of change

Bug fix (non-breaking change that fixes an issue)
New feature (non-breaking change that adds functionality)
Breaking change (fix or feature that would cause existing functionality to not work as expected)

Related issues

Fixes: ch74679

Development

Lint rules pass locally
The code changed/added as part of this pull request has been covered with tests
All tests related to the changed code pass in development

Code review

This pull request has a descriptive title and information useful to a reviewer. There may be a screenshot or screencast attached
"Ready for review" label attached to the PR and reviewers mentioned in a comment
Changes have been reviewed by at least one other engineer
Issue from task tracker has a link to this pull request

eudaimos

@waltjones

Overall I made comments/asked questions about the implementation and public api of Locals

Also wondering, should there be unit tests for the Locals API?
The tests I see seem to be routed through the rollbar SDK

eudaimos · 2020-11-24T01:49:54Z

src/server/locals.js

+      var key = params.data.description;
+      this.currentErrors.set(key, params);
+
+      if (this.currentErrors.size > 4) {


@waltjones what's the significance of the size > 4 - should this be maintained in a descriptive constant value?

Constant and explanatory comment added.

eudaimos · 2020-11-24T01:56:29Z

src/server/locals.js

+    return callback(e);
+  }
+
+  this.getLocalScopesForFrames(matchedFrames, function(err) {


is there a reason not to just pass callback as the second arg here?
as in:

this.getLocalScopesForFrames(matchedFrames, callback);

Good catch. Each of these cases have been condensed now.

eudaimos · 2020-11-24T01:58:58Z

src/server/locals.js

+  return matchedFrames;
+}
+
+Locals.prototype.firstFrame = function(localIndex, stackIndex) {


is there a need for this function to be attached to the Locals.prototype?

Seems better suited as a private local utility function

Non-interface functions are all private now.

eudaimos · 2020-11-24T02:00:38Z

src/server/locals.js

+  return !localIndex && !stackIndex;
+}
+
+Locals.prototype.matchedFrame = function(callFrame, stackLocation) {


same comment about this function as the above for firstFrame - this function isn't making use of this - does it need to be exported on the Locals.prototype?

eudaimos · 2020-11-24T02:02:06Z

src/server/locals.js

+}
+
+Locals.prototype.getLocalScopesForFrames = function(matchedFrames, callback) {
+  async.each(matchedFrames, this.getLocalScopeForFrame.bind(this), function (err) {


seems that you can pass callback as the thrid arg to async.each rather than having the wrapping function - is there a reason for the extra wrapping function?

seems it will be called no matter the outcome the way it's written

eudaimos · 2020-11-24T02:06:21Z

src/server/locals.js

+    callFrameColumn === position.column;
+}
+
+Locals.prototype.getLocalScopesForFrames = function(matchedFrames, callback) {


does this function need to be attached to the Locals.prototype and exported? I don't see it used outside of the mergeLocals body which does need to be exported (public).

As for the bind call to this, that could be done by mergeLocals when passing maybe? Just a thought on trying to expose less of these internal methods if they're not needed externally

eudaimos · 2020-11-24T02:12:49Z

src/server/locals.js

+  var _this = this;
+  for (var i = 0; i < scopes.length; i++) {
+    var scope = scopes[i]
+    if (scope.type === 'local') {


why not filter before the loop using a scopes.filter(scope => scope.type === 'local')

Updated to use find and expect one local scope.

eudaimos · 2020-11-24T02:13:45Z

src/server/locals.js

+  return value;
+}
+
+Locals.prototype.getObjectValue = function(local) {


does this need to be exported as attached to the Locals.prototype?

eudaimos · 2020-11-24T02:14:21Z

src/server/locals.js

+  }
+}
+
+Locals.prototype.getLocalValue = function(local) {


since the only ref to this is for the this.getObjectValue call, does this need to be exported on the Locals.prototype?

eudaimos · 2020-11-24T02:20:43Z

src/server/locals.js

+var inspector = require('inspector');
+var async = require('async');
+
+function Locals(config) {


defining classes like this the old school JS way has the problem of possible function call without instantiation, i.e. missing the new keyword by just calling Locals(…) as a function. To prevent this from doing harm, a pattern emerged for checking how a constructor is called like this:

function Locals(config) { if (!(this instanceof Locals)) { return new Locals(config); } // rest of constructor logic }

eudaimos

I never seem to get this right - requested changes are comments in previous "Comment" and submitting this now as a Review for the process to work

…vate functions

waltjones · 2020-12-02T18:47:55Z

Major updates are:

The inspector session is now a singleton at Locals.session. This makes more sense, since multiple Rollbar.js instances should not open multiple sessions. This has the side effect of greatly reducing the need to pass this around since the main need for it was when calling session.post().
Unit tests have been added, which allows exercising some conditions not produced when the actual inspector API is called.

christopherseaman · 2020-12-16T15:58:02Z

Update?

@eudaimos @waltjones

waltjones · 2020-12-16T16:09:40Z

Just waiting on approval or further feedback.

src/server/locals.js

waltjones added the Ready for review label Nov 4, 2020

waltjones force-pushed the wj-node-locals branch from 3547511 to b02bb69 Compare November 4, 2020 16:22

eudaimos reviewed Nov 24, 2020

View reviewed changes

eudaimos suggested changes Nov 24, 2020

View reviewed changes

waltjones added 3 commits December 2, 2020 10:28

feat: enable node.js stack locals capture

78704e9

fix: refactor to use singleton inspector session and non-exported pri…

1be9dea

…vate functions

test: add unit tests for Locals

52cc8c1

waltjones force-pushed the wj-node-locals branch from b02bb69 to 52cc8c1 Compare December 2, 2020 16:32

waltjones added 2 commits December 2, 2020 11:37

fix: constructor guard

40ea09f

fix: expect one or zero local scopes in scopeChain

79e2eb4

bishopb approved these changes Dec 18, 2020

View reviewed changes

src/server/locals.js Show resolved Hide resolved

src/server/locals.js Show resolved Hide resolved

src/server/locals.js Show resolved Hide resolved

fix: always return a map from currentLocalsMap

aaafe97

waltjones merged commit e04e7c5 into master Jan 4, 2021

waltjones mentioned this pull request Jan 12, 2021

Enable depth of traversal for node local vars #914

Merged

10 tasks

Nikamura mentioned this pull request Feb 2, 2021

Inspector is not available #921

Closed

dependabot bot mentioned this pull request Mar 18, 2021

Bump rollbar from 2.19.4 to 2.21.1 schalkneethling/vulnerability-alerts#106

Closed

renovate bot mentioned this pull request Jun 6, 2021

Update dependency rollbar to v2.26.4 spencermize/Veload#81

Open

1 task

mudetroit pushed a commit that referenced this pull request Mar 14, 2024

Enable node.js stack locals capture (#902)

d09e259

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enable node.js stack locals capture #902

Enable node.js stack locals capture #902

waltjones commented Nov 4, 2020 •

edited by bishopb

Loading

eudaimos left a comment

eudaimos Nov 24, 2020

waltjones Dec 2, 2020

eudaimos Nov 24, 2020

waltjones Dec 2, 2020

eudaimos Nov 24, 2020

waltjones Dec 2, 2020

eudaimos Nov 24, 2020

eudaimos Nov 24, 2020

eudaimos Nov 24, 2020

eudaimos Nov 24, 2020

waltjones Dec 2, 2020

eudaimos Nov 24, 2020

eudaimos Nov 24, 2020

eudaimos Nov 24, 2020

waltjones Dec 2, 2020

eudaimos left a comment

waltjones commented Dec 2, 2020

christopherseaman commented Dec 16, 2020

waltjones commented Dec 16, 2020

Enable node.js stack locals capture #902

Enable node.js stack locals capture #902

Conversation

waltjones commented Nov 4, 2020 • edited by bishopb Loading

Description of the change

Type of change

Related issues

Development

Code review

eudaimos left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eudaimos left a comment

Choose a reason for hiding this comment

waltjones commented Dec 2, 2020

christopherseaman commented Dec 16, 2020

waltjones commented Dec 16, 2020

waltjones commented Nov 4, 2020 •

edited by bishopb

Loading