Initial commit to prioritize capturing expressions by DominicKramer · Pull Request #162 · googleapis/cloud-debug-nodejs

DominicKramer · 2016-10-20T22:54:39Z

In particular, now if a captured value has a length attribute,
the value of that attribute is shown in the status field.

Also, the length of a watched expression is now bounded by
config.maxDataSize instead of config.maxProperties.

Unit tests still need to be addressed, and more fine grained
control of the total length of the data captured needs to be
added.

In particular, now if a captured value has a `length` attribute, the value of that attribute is shown in the status field. Also, the length of a watched expression is now bounded by `config.maxDataSize` instead of `config.maxProperties`. Unit tests still need to be addressed, and more fine grained control of the total length of the data captured needs to be added.

DominicKramer · 2016-10-20T22:57:07Z

@GoogleCloudPlatform/node-team I'm still working on this, but I opened this pull request to get early feedback so that if my changes will have a negative impact that I am not aware of, I can fix it sooner rather than later.

In particular, if an evaluated expression has more than `config.capture.maxDataSize` elements, then only the first `config.capture.maxDataSize` elements will be listed and the new message will be set as the status of the data in the debugger.

lib/state.js

        };
      } else {
-        evaluated = that.resolveVariable_(expression, result.mirror);
+        evaluated = that.resolveVariable_(expression, result.mirror, true);


Now the `mirror` object returned from V8 no longer has an `isEvaluated` attribute attached to it. Instead, whether or not a variable is evaluated is passed to the `resolveMirror_` method and all associated methods.

Updated the unit tests so that they verify that evaluated objects and arrays do not have their lengths limited.

Now evaluated expressions are processed before stack frames.

Unit tests were added to verify that the length of evaluated expressions are limited by the `maxDataSize` configuration option.

An array 'var A = [1, 2, 3]' has members for the attributes '1', '2', and '3' in version 0.12 but has members for the attributes '1', '2', and '3' as well as 'length' for the other versions of Node.

The test needed to be updated because the addition of the new data limit message in `state.js` caused the local variable in the test to have its variable table index be increased from 6 to 7 since the variable table now includes the new data limit message.

…amer/cloud-debug-nodejs into feature/prioritize-capture-exp

DominicKramer · 2016-11-02T20:40:46Z

PTAL

lib/state.js

 * @param {Object} value A v8 debugger representation of a variable value.
 */
-StateResolver.prototype.resolveVariable_ = function(name, value) {
+StateResolver.prototype.resolveVariable_ = function(name, value, isEvaluated) {


lib/state.js

+      that.resolveMirror_(that.rawVariableTable_[index], isEvaluated);
    index++;
+
+    if (!noLimit && that.totalSize_ >= that.config_.capture.maxDataSize) {


lib/state.js

      data.status = MESSAGE_TABLE[OBJECT_LIMIT_MESSAGE_INDEX].status;
    }
-
+    else if (isEvaluated && maxBuffer > 0 && maxBuffer < numKeys) {


This makes the logic more clear.

The value of `maxDataSize` is not used to determine if an object or array should have its contents truncated. Instead, an evaluated expression is either entirely displayed or not entirely displayed dependent on whether or not all collected data is beyond `maxDataSize`.

cristiancavalli

Generally looks good to me - a few questions though:

lib/state.js

    data.varTableIndex = this.getVariableIndex_(value);
    var maxProps = this.config_.capture.maxProperties;
-    if (maxProps && maxProps < Object.keys(value.value()).length) {
+    var numKeys = Object.keys(value.value()).length;


lib/state.js

-    if (maxProps && maxProps < Object.keys(value.value()).length) {
+    var numKeys = Object.keys(value.value()).length;
+
+    if (!isEvaluated && maxProps && maxProps < numKeys) {


lib/state.js

  // I'm going with the former as that's what util.inspect does.
  var that = this;
+
  var keys = Object.keys(mirror.value());


lib/state.js

-  var members = this.getMirrorProperties_(mirror).map(
-      this.resolveMirrorProperty_.bind(this));
+StateResolver.prototype.resolveMirrorFast_ = function(mirror, isEvaluated) {
+  var resMirrorProp = this.resolveMirrorProperty_.bind(this);


test/test-v8debugapi.js

+            config.capture.maxProperties = oldMaxProps;
+            done();
+          });
+          process.nextTick(function() {foo(2);});


test/test-v8debugapi.js

+            config.capture.maxProperties = oldMaxProps;
+            done();
+          });
+          process.nextTick(function() {foo(2);});


test/test-v8debugapi.js

+            config.capture.maxProperties = oldMaxProps;
+            done();
+          });
+          process.nextTick(function() {foo(2);});


test/test-v8debugapi.js

+            config.capture.maxProperties = oldMaxProps;
+            done();
+          });
+          process.nextTick(function() {foo(2);});


lib/state.js

@@ -349,7 +364,7 @@ StateResolver.prototype.extractArgumentsList_ = function (frame) {
 StateResolver.prototype.resolveArgumentList_ = function(args) {
  var resolveVariable = this.resolveVariable_.bind(this);


ofrobots

LGTM.

googlebot added the cla: yes This human has signed the Contributor License Agreement. label Oct 20, 2016

matthewloring reviewed Oct 22, 2016

View reviewed changes

lib/state.js

};

} else {

evaluated = that.resolveVariable_(expression, result.mirror);

evaluated = that.resolveVariable_(expression, result.mirror, true);

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

DominicKramer added 11 commits October 28, 2016 16:20

Merge branch 'master' into feature/prioritize-capture-exp

b4f7f82

[SQUASH] Remove isEvaluated prop from mirror objs

3cad9bb

Now the `mirror` object returned from V8 no longer has an `isEvaluated` attribute attached to it. Instead, whether or not a variable is evaluated is passed to the `resolveMirror_` method and all associated methods.

[SQUASH] Update unit tests for evaluated exps

e32587f

Updated the unit tests so that they verify that evaluated objects and arrays do not have their lengths limited.

Merge branch 'master' into feature/prioritize-capture-exp

43f9631

[SQUASH] Prioritize evaluated expressions

211a1d1

Now evaluated expressions are processed before stack frames.

[SQUASH] Add unit tests for limiting exp size

a2f2280

Unit tests were added to verify that the length of evaluated expressions are limited by the `maxDataSize` configuration option.

[SQUASH] Fix a unit test to verify lengths of arrs

efbdcaa

An array 'var A = [1, 2, 3]' has members for the attributes '1', '2', and '3' in version 0.12 but has members for the attributes '1', '2', and '3' as well as 'length' for the other versions of Node.

[SQUASH] Fix a unit test to verify lengths of arrs

6d98e73

An array 'var A = [1, 2, 3]' has members for the attributes '1', '2', and '3' in version 0.12 but has members for the attributes '1', '2', and '3' as well as 'length' for the other versions of Node.

Merge branch 'feature/prioritize-capture-exp' of github.com:DominicKr…

77283e5

…amer/cloud-debug-nodejs into feature/prioritize-capture-exp

Merge branch 'master' into feature/prioritize-capture-exp

dc0af83

ofrobots suggested changes Nov 3, 2016

View reviewed changes

DominicKramer added 3 commits November 8, 2016 15:55

Fix to use a var name haveLimit over noLimit

d9402e6

This makes the logic more clear.

Add JSDocs

366c3ef

cristiancavalli suggested changes Nov 10, 2016

View reviewed changes

ofrobots approved these changes Nov 10, 2016

View reviewed changes

cristiancavalli approved these changes Nov 10, 2016

View reviewed changes

DominicKramer merged commit d370e20 into googleapis:master Nov 10, 2016

DominicKramer deleted the feature/prioritize-capture-exp branch November 10, 2016 22:13

		@@ -349,7 +364,7 @@ StateResolver.prototype.extractArgumentsList_ = function (frame) {
		StateResolver.prototype.resolveArgumentList_ = function(args) {
		var resolveVariable = this.resolveVariable_.bind(this);

Conversation

DominicKramer commented Oct 20, 2016

Uh oh!

DominicKramer commented Oct 20, 2016

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

DominicKramer commented Nov 2, 2016

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

cristiancavalli left a comment

Choose a reason for hiding this comment

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

ofrobots left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers