inspector: add inspector console support for native Console #7988

alexkozy · 2016-08-05T21:18:17Z

Checklist

make -j4 test (UNIX), or vcbuild test nosign (Windows) passes
commit message follows commit guidelines

Affected core subsystem(s)

console

Description of change

When node is running with --inspect flag, default console.log,
console.warn and other methods call inspector console methods in
addition to current behaviour (dump formatted message to stderr
and stdout). Inspector console methods forward message to DevTools and
show its in DevTools Console with DevTools formatters. Inspector console
methods not presented on Node console will be added into it.

Only own methods on global.console object will be changed while
debugging session. User still able to redefine it, use console.Console
or change original methods on Console.prototype.

alexkozy · 2016-08-05T21:23:50Z

/cc @ofrobots @eugeneo

jasnell · 2016-08-05T21:24:03Z

@bnoordhuis @ofrobots @trevnorris ...

eugeneo · 2016-08-05T21:36:10Z

src/node.cc

@@ -3299,6 +3338,15 @@ void SetupProcessObject(Environment* env,
  env->SetMethod(process, "_setupPromises", SetupPromises);
  env->SetMethod(process, "_setupDomainUse", SetupDomainUse);

+#if HAVE_INSPECTOR
+  // process.inspector
+  if (use_inspector) {


I would like to suggest moving this block (and 2 functions above) into inspector_agent.{h,cc} - this way there will be less conditional compilation to maintain.

alexkozy · 2016-08-05T22:07:55Z

/cc @dgozman

mscdex · 2016-08-05T22:54:12Z

Wouldn't it be possible to instead store copies of the original console functions, which would allow us to avoid having to modify console.js at all? Having to make those console.js changes just for inspector makes me cringe.

alexkozy · 2016-08-06T02:01:15Z

I addressed your comments and moved all changes into inspector_agent.cc and bootstrap_node.js.
Please take another look!

dgozman · 2016-08-06T02:48:04Z

src/inspector_agent.cc

+  }
+
+  v8::Local<v8::Array> array = v8::Array::New(env->isolate());
+  for (int i = 0; i < args.Length(); ++i) {


Let's check that arguments are functions here.

alexkozy · 2016-08-06T21:10:13Z

Addressed all comments, please take another look!

bnoordhuis · 2016-08-07T12:05:43Z

lib/internal/bootstrap_node.js

+    if (process.inspector) {
+      inspectorConsole = global.console;
+      wrapConsoleCall = process.inspector.wrapConsoleCall;
+      delete process.inspector.sequentally;


Still a s/sequentally/sequentially/ typo here.

bnoordhuis · 2016-08-07T12:10:19Z

src/inspector_agent.cc

+  Environment* env = Environment::GetCurrent(args);
+
+  if (args.Length() != 3 || !args[0]->IsFunction()
+      || !args[1]->IsFunction() || !args[2]->IsObject()) {


Nit: operator should go on previous line.

alexkozy · 2016-08-07T17:31:48Z

addressed comments, please take another look.

bnoordhuis · 2016-08-07T20:27:44Z

src/inspector_agent.cc

+  }
+
+  node_method.As<v8::Function>()->Call(context, info.Holder(), call_args.size(),
+      call_args.data());


Nit: arguments should line up.

bnoordhuis · 2016-08-07T20:31:10Z

LGTM with a suggestion and a nit. CI: https://ci.nodejs.org/job/node-test-pull-request/3559/

alexkozy · 2016-08-07T23:23:44Z

Thanks! All done. It looks like freebsd test failure doesn't relate to this pull request.

jasnell · 2016-08-08T14:51:36Z

LGTM

ofrobots · 2016-08-08T15:14:32Z

lib/internal/bootstrap_node.js

+    if (process.inspector) {
+      inspectorConsole = global.console;
+      wrapConsoleCall = process.inspector.wrapConsoleCall;
+      delete process.inspector.wrapConsoleCall;


I would prefer if inspector wasn't a public property on process (except during startup). Could you get away with deleting process.inspector here?

I can remove process.inspector here. Done.

alexkozy · 2016-08-08T16:25:48Z

All done. Please take another look!

ofrobots · 2016-08-08T22:44:57Z

LGTM. New CI: https://ci.nodejs.org/job/node-test-pull-request/3580/.

trevnorris · 2016-08-09T16:40:37Z

lib/internal/bootstrap_node.js

+      if (!inspectorConsole.hasOwnProperty(key))
+        continue;
+      // If node console has the same method as inspector console,
+      // then wrap this two methods into one. Native wrapper will preserve


grammar nit: "then wrap these two methods..."

Also, "will preserve the original stack"

alexkozy · 2016-08-09T16:51:06Z

Thanks for review.
Fixed grammar.

jasnell · 2016-08-09T16:55:02Z

src/inspector_agent.cc

+
+  if (args.Length() != 3 || !args[0]->IsFunction() ||
+      !args[1]->IsFunction() || !args[2]->IsObject()) {
+    return env->ThrowError("Bad argument.");


Would prefer the error message to be a bit more descriptive

jasnell · 2016-08-09T16:55:57Z

LGTM with a nit

alexkozy · 2016-08-09T17:04:45Z

Done.

ofrobots · 2016-08-10T23:22:53Z

One more CI: https://ci.nodejs.org/job/node-test-pull-request/3616/

alexkozy · 2016-08-11T00:04:19Z

This time it was failed on https://ci.nodejs.org/job/node-compile-windows/3631/label=win-vs2015/console .
I compare output with previous successful build and it looks like difference at the end of output:
c:\workspace\node-compile-windows\label\win-vs2015>exit 0
FATAL: java.io.IOException: Connection aborted: org.jenkinsci.remoting.nio.NioChannelHub$MonoNioTransport@6034828e[name=test-rackspace-win2012r2-x64-6] ...

It looks like it doesn't relate to my change, does it?

ofrobots · 2016-08-11T00:08:40Z

That seems to be infrastructure related. Windows did pass on the previous build. I think this is good to land. I will do so tomorrow.

When node is running with --inspect flag, default console.log, console.warn and other methods call inspector console methods in addition to current behaviour (dump formatted message to stderr and stdout). Inspector console methods forward message to DevTools and show its in DevTools Console with DevTools formatters. Inspector console methods not presented on Node console will be added into it. Only own methods on global.console object will be changed while debugging session. User still able to redefine it, use console.Console or change original methods on Console.prototype.

alexkozy · 2016-08-12T00:35:41Z

I've rebased this pull request to resolve conflicts.

ofrobots · 2016-08-12T00:52:21Z

Thanks, landed as f6070a1.

When node is running with --inspect flag, default console.log, console.warn and other methods call inspector console methods in addition to current behaviour (dump formatted message to stderr and stdout). Inspector console methods forward message to DevTools and show up in DevTools Console with DevTools formatters. Inspector console methods not present on Node console will be added into it. Only own methods on global.console object will be changed while in a debugging session. User are still able to redefine it, use console.Console or change original methods on Console.prototype. PR-URL: #7988 Reviewed-By: bnoordhuis - Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: jasnell - James M Snell <jasnell@gmail.com> Reviewed-By: ofrobots - Ali Ijaz Sheikh <ofrobots@google.com>

ofrobots · 2016-08-12T00:56:24Z

Looks great, btw:

// ...
http.get('http://www.google.com/index.html', (res) => {
  console.log(res);
  res.resume();
});

When node is running with --inspect flag, default console.log, console.warn and other methods call inspector console methods in addition to current behaviour (dump formatted message to stderr and stdout). Inspector console methods forward message to DevTools and show up in DevTools Console with DevTools formatters. Inspector console methods not present on Node console will be added into it. Only own methods on global.console object will be changed while in a debugging session. User are still able to redefine it, use console.Console or change original methods on Console.prototype. PR-URL: #7988 Reviewed-By: bnoordhuis - Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: jasnell - James M Snell <jasnell@gmail.com> Reviewed-By: ofrobots - Ali Ijaz Sheikh <ofrobots@google.com>

jasnell added the semver-minor PRs that contain new features and should be released in the next minor version. label Aug 5, 2016

eugeneo reviewed Aug 5, 2016
View reviewed changes

dgozman reviewed Aug 6, 2016
View reviewed changes

bnoordhuis reviewed Aug 7, 2016
View reviewed changes

ofrobots reviewed Aug 8, 2016
View reviewed changes

eugeneo mentioned this pull request Aug 8, 2016

node --inspect: dev tools console not working #7891

Closed

trevnorris reviewed Aug 9, 2016
View reviewed changes

jasnell reviewed Aug 9, 2016
View reviewed changes

ofrobots closed this Aug 12, 2016

cjihrig mentioned this pull request Aug 15, 2016

v6.4.0 Proposal #8070

Merged

MylesBorins added the dont-land-on-v4.x label Sep 9, 2016

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

inspector: add inspector console support for native Console #7988

inspector: add inspector console support for native Console #7988

alexkozy commented Aug 5, 2016

alexkozy commented Aug 5, 2016

jasnell commented Aug 5, 2016

eugeneo Aug 5, 2016

alexkozy commented Aug 5, 2016

mscdex commented Aug 5, 2016

alexkozy commented Aug 6, 2016

dgozman Aug 6, 2016

alexkozy commented Aug 6, 2016

bnoordhuis Aug 7, 2016

bnoordhuis Aug 7, 2016

alexkozy commented Aug 7, 2016

bnoordhuis Aug 7, 2016

bnoordhuis commented Aug 7, 2016

alexkozy commented Aug 7, 2016

jasnell commented Aug 8, 2016

ofrobots Aug 8, 2016

alexkozy Aug 8, 2016

alexkozy commented Aug 8, 2016

ofrobots commented Aug 8, 2016

trevnorris Aug 9, 2016

trevnorris Aug 9, 2016

alexkozy commented Aug 9, 2016

jasnell Aug 9, 2016

jasnell commented Aug 9, 2016

alexkozy commented Aug 9, 2016

ofrobots commented Aug 10, 2016

alexkozy commented Aug 11, 2016

ofrobots commented Aug 11, 2016

alexkozy commented Aug 12, 2016

ofrobots commented Aug 12, 2016

ofrobots commented Aug 12, 2016

inspector: add inspector console support for native Console #7988

inspector: add inspector console support for native Console #7988

Conversation

alexkozy commented Aug 5, 2016

Checklist

Affected core subsystem(s)

Description of change

alexkozy commented Aug 5, 2016

jasnell commented Aug 5, 2016

Choose a reason for hiding this comment

alexkozy commented Aug 5, 2016

mscdex commented Aug 5, 2016

alexkozy commented Aug 6, 2016

Choose a reason for hiding this comment

alexkozy commented Aug 6, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexkozy commented Aug 7, 2016

Choose a reason for hiding this comment

bnoordhuis commented Aug 7, 2016

alexkozy commented Aug 7, 2016

jasnell commented Aug 8, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexkozy commented Aug 8, 2016

ofrobots commented Aug 8, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexkozy commented Aug 9, 2016

Choose a reason for hiding this comment

jasnell commented Aug 9, 2016

alexkozy commented Aug 9, 2016

ofrobots commented Aug 10, 2016

alexkozy commented Aug 11, 2016

ofrobots commented Aug 11, 2016

alexkozy commented Aug 12, 2016

ofrobots commented Aug 12, 2016

ofrobots commented Aug 12, 2016