"NodeTracing" domain introduction #20608

eugeneo · 2018-05-08T17:57:09Z

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
tests and/or benchmarks are included
commit message follows commit guidelines

This pull request includes:

Infrastructure for implementing Node.js-specific inspector protocol extensions.
Implementation of the "NodeTracing" inspector domain, enabling the users to access tracing data through the inspector protocol.
Backport of the V8 patch that makes it easier to reuse the Trace serialization code.

CC: @nodejs/trace-events @nodejs/v8-inspector @nodejs/diagnostics

a1ph · 2018-05-08T20:25:47Z

src/inspector/node_protocol.pdl

+      # total size.
+      optional number value
+
+  # Contains an bucket of collected trace events. When tracing is stopped collected events will be


Can you omit mention of when events start streaming. This way we can later add a mode when events start streaming after start, not stop.

I just want to clarify: with this channel, are the events buffered in Node.js until a given point and then delivered or are they streamed continuously as they are emitted? The latter would be far more useful in the long run. If it's the former then it will be good to know :-)

@a1ph Did that.

@jasnell Right now events are sent out when trace buffer is flushed, same as writing the trace to the file. This is done to minimize performance impact (and also not to brake the trace to file). Some events will force flush, such as connecting/disconnecting an inspector session (multiple sessions are supported) or using JS API to tweak enabled categories.

Just thinking out loud... I wonder if it would make sense to support some signal to force flushing on demand.

a1ph · 2018-05-08T20:26:11Z

src/inspector/node_protocol.pdl

+      optional number value
+
+  # Contains an bucket of collected trace events. When tracing is stopped collected events will be
+  # send as a sequence of dataCollected events followed by tracingComplete event.


s/send/sent/

Done (I moved the comment up to the stop method)

a1ph · 2018-05-08T20:28:07Z

src/inspector/node_protocol.pdl

+      array of string includedCategories
+
+  # Stop trace events collection.
+  command end


I'd rather name the pair start/stop instead of start/end

This is a copy of the Chrome protocol - https://chromedevtools.github.io/devtools-protocol/tot/Tracing - I don't mind changing this for Node.js but I am not sure how far we want to diverge.

@a1ph Renamed it.

a1ph · 2018-05-08T20:29:12Z

src/inspector/node_protocol.pdl

+    parameters
+      # A number in range [0..1] that indicates the used size of event buffer as a fraction of its
+      # total size.
+      optional number percentFull


if the number is in [0..1] I'd not call it percent.

Currently this structure is not used, I removed it altogether.

a1ph · 2018-05-08T20:56:07Z

src/tracing/agent.cc

-    RestartTracing();
+  if (categories.empty())
+    return;
+  std::set<std::string> categories_set;


Out of curiosity, why it's not an unordered_set ?

Actually Chrome tracing treats categories as sort of command list rather than a set. This allows you to use wildcards, e.g.
"-*,devtools.timeline" will disable all default categories and then enable just devtools.timeline. Not sure if you want to support that here though.

An unordered_set would be fine here, I think. And yeah, treating these as a command set would be difficult, especially given the trace_events JS API model

@a1ph Node does not have a notion of "default categories" at this time and that command list is not implemented. I suppose, if it is decided to implement something similar, parsing will be done in the protocol agent so this implementation will still have this set.

a1ph · 2018-05-08T21:07:50Z

src/inspector/tracing_agent.cc

+DispatchResponse TracingAgent::getCategories(
+    std::unique_ptr<protocol::Array<String>>* categories) {
+  *categories = Array<String>::create();
+  categories->get()->addItem("node");


Is it a temp stub implementation? The actual list is dynamic and should be pulled from TracingController. Right?

This is a hardcoded list that should ultimately be coming from the TracingController or some other place.

Currently these are adhoc in core. We have macros for defining them, and once we have the js emit function, the category could be somewhat arbitrary. We'll need to make sure we have a good centralized place to define these.

There's one missing: `node.fs.sync'

@jasnell added the category.

Created nodejs/diagnostics#191 to track this as a follow-on feature.

jasnell · 2018-05-09T02:39:23Z

src/inspector/node_string.h

@@ -0,0 +1,79 @@
+// Bridges V8 Inspector generated code with the std::string used by the Node
+// Comnpare to V8 counterpart - deps/v8/src/inspector/string-util.h


s/Comnpare/Compare

jasnell · 2018-05-09T02:47:55Z

Looks very good on first read through. I'm not going to sign off yet as I need to run it and step through it a bit first. Would love to get @addaleax and @bnoordhuis to take a look also.

Also /cc @nodejs/diagnostics

eugeneo · 2018-05-09T18:44:36Z

Added a commit that addresses the review comments.

ofrobots · 2018-05-10T19:03:24Z

src/inspector/tracing_agent.cc

+DispatchResponse TracingAgent::getCategories(
+    std::unique_ptr<protocol::Array<String>>* categories) {
+  *categories = Array<String>::create();
+  categories->get()->addItem("node");


Created nodejs/diagnostics#191 to track this as a follow-on feature.

ofrobots · 2018-05-10T19:13:37Z

src/tracing/node_trace_buffer.h

@@ -51,7 +50,7 @@ class InternalTraceBuffer {

 class NodeTraceBuffer : public TraceBuffer {
 public:
-  NodeTraceBuffer(size_t max_chunks, NodeTraceWriter* trace_writer,
+  NodeTraceBuffer(size_t max_chunks, Agent* agent,


style nit: this should fit on one line now, right?

eugeneo · 2018-05-11T20:44:32Z

CI: https://ci.nodejs.org/job/node-test-pull-request/14817/

It has genuine Windows failures, I'll work on them.

eugeneo · 2018-05-13T18:31:24Z

CI: https://ci.nodejs.org/job/node-test-pull-request/14844/

eugeneo · 2018-05-14T17:44:01Z

CI is green. I would like to wait 48 hours for additional comments.

addaleax · 2018-05-14T18:19:44Z

src/inspector_agent.h

-  Persistent<v8::Function> enable_async_hook_function_;
-  Persistent<v8::Function> disable_async_hook_function_;
+  v8::Persistent<v8::Function> enable_async_hook_function_;
+  v8::Persistent<v8::Function> disable_async_hook_function_;


Why this change? node::Persistent has the advantage of cleaning up automatically when it is destroyed, but I don’t see any Reset() calls being added here…?

Fixed. Thanks! I did not realize this was something other then v8::Persistent, I now made it explicit by using the node namespace.

addaleax · 2018-05-14T18:21:12Z

src/tracing/agent.cc

+};
+
+const std::set<std::string> flatten(
+    const std::unordered_map<int, std::set<std::string>> map) {


Style nits: const on a return value doesn’t change anything, so you can omit it. The parameter should probably be a const reference?

eugeneo · 2018-05-15T18:53:45Z

@addaleax I addressed your comments. Please, take another look.

addaleax · 2018-05-15T19:58:17Z

@eugeneo Can’t really review this fully, but I’m happy as far as code style is concerned. :)

mcollina

rubberstamp LGTM.

Considering the amount of code being added, it seems to be pretty low on the number of tests added.

eugeneo · 2018-05-16T16:55:40Z

@addaleax Thanks for the code style review.
@mcollina Thanks. Testing mostly relies on existing tests, i.e. nothing should be broken.

I rebased and straighten the history. New CI: https://ci.nodejs.org/job/node-test-pull-request/14909/

eugeneo · 2018-05-16T17:53:19Z

Looks like macOS bot ran out of disk space... @nodejs/build

eugeneo · 2018-05-17T18:31:58Z

CI: https://ci.nodejs.org/job/node-test-pull-request/14933/
parallel/test-http2-client-upload-reject failures are the only ones, do not seem caused by this CL, happen for other changes as well.

eugeneo · 2018-05-17T20:24:15Z

Landed as 47bdc71

Original commit message: Custom tag for the traceEvents array This API will be used by Node.js to provide output compatible with Chrome devtools. Cq-Include-Trybots: luci.chromium.try:linux_chromium_rel_ng Change-Id: I265495f8af39bfc78d7fdbe43ac308f0920e817d Reviewed-on: https://chromium-review.googlesource.com/1044491 Reviewed-by: Aleksey Kozyatinskiy <kozyatinskiy@chromium.org> Reviewed-by: Ulan Degenbaev <ulan@chromium.org> Commit-Queue: Eugene Ostroukhov <eostroukhov@chromium.org> Cr-Commit-Position: refs/heads/master@{#53041} PR-URL: #20608 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Ali Ijaz Sheikh <ofrobots@google.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com>

This change adds a new inspector domain for receiving Node tracing data. 1. Node.js now can extend Inspector protocol with new domains with the API defined in the src/inspector/node_protocol.pdl. 2. Plumbing code will be generated at the build time. /json/protocol HTTP endpoint returns both V8 and Node.js inspector protocol. 3. "NodeTracing" domain was introduced. It is based on the Chrome "Tracing" domain. PR-URL: #20608 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Ali Ijaz Sheikh <ofrobots@google.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com>

richardlau · 2018-05-22T21:27:39Z

Doing a quick search, looks like std::make_unique isn't C++11 compatible? That's the version we target, right?

We target C++14:

node/common.gypi

Line 324 in 23a56e0

'cflags_cc': [ '-fno-rtti', '-fno-exceptions', '-std=gnu++1y' ],

Original commit message: Custom tag for the traceEvents array This API will be used by Node.js to provide output compatible with Chrome devtools. Cq-Include-Trybots: luci.chromium.try:linux_chromium_rel_ng Change-Id: I265495f8af39bfc78d7fdbe43ac308f0920e817d Reviewed-on: https://chromium-review.googlesource.com/1044491 Reviewed-by: Aleksey Kozyatinskiy <kozyatinskiy@chromium.org> Reviewed-by: Ulan Degenbaev <ulan@chromium.org> Commit-Queue: Eugene Ostroukhov <eostroukhov@chromium.org> Cr-Commit-Position: refs/heads/master@{nodejs#53041} PR-URL: nodejs#20608 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Ali Ijaz Sheikh <ofrobots@google.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com>

Original commit message: Custom tag for the traceEvents array This API will be used by Node.js to provide output compatible with Chrome devtools. Cq-Include-Trybots: luci.chromium.try:linux_chromium_rel_ng Change-Id: I265495f8af39bfc78d7fdbe43ac308f0920e817d Reviewed-on: https://chromium-review.googlesource.com/1044491 Reviewed-by: Aleksey Kozyatinskiy <kozyatinskiy@chromium.org> Reviewed-by: Ulan Degenbaev <ulan@chromium.org> Commit-Queue: Eugene Ostroukhov <eostroukhov@chromium.org> Cr-Commit-Position: refs/heads/master@{#53041} PR-URL: #20608 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Ali Ijaz Sheikh <ofrobots@google.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com>

MylesBorins · 2018-08-07T15:38:41Z

Should this be backported to v8.x-staging? If yes please follow the guide and raise a backport PR, if not let me know or add the dont-land-on label.

jasnell · 2018-08-07T15:40:53Z

Please don't backport yet. There are some stability issues in the tracing code that need to be worked out fully before backporting.

eugeneo self-assigned this May 8, 2018

eugeneo requested a review from ofrobots May 8, 2018 17:57

nodejs-github-bot added the lib / src Issues and PRs related to general changes in the lib or src directory. label May 8, 2018

a1ph reviewed May 8, 2018

View reviewed changes

jasnell reviewed May 9, 2018

View reviewed changes

jasnell requested review from bnoordhuis and addaleax May 9, 2018 02:48

jasnell approved these changes May 9, 2018

View reviewed changes

ofrobots approved these changes May 10, 2018

View reviewed changes

addaleax reviewed May 14, 2018

View reviewed changes

mcollina approved these changes May 16, 2018

View reviewed changes

eugeneo closed this May 17, 2018

eugeneo merged commit 47bdc71 into nodejs:master May 17, 2018

eugeneo deleted the node_dispatcher branch May 17, 2018 20:24

addaleax mentioned this pull request May 22, 2018

v10.2.0 proposal #20724

Merged

MylesBorins mentioned this pull request Jun 6, 2018

v10.4.0 proposal #21167

Merged

MylesBorins added the backport-requested-v8.x label Aug 7, 2018

mcollina mentioned this pull request Sep 12, 2018

Support Network Inspection nodejs/diagnostics#75

Open

		@@ -0,0 +1,79 @@
		// Bridges V8 Inspector generated code with the std::string used by the Node
		// Comnpare to V8 counterpart - deps/v8/src/inspector/string-util.h

"NodeTracing" domain introduction #20608

"NodeTracing" domain introduction #20608

Conversation

eugeneo commented May 8, 2018

Checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jasnell May 9, 2018 • edited Loading

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jasnell commented May 9, 2018

eugeneo commented May 9, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eugeneo commented May 11, 2018

eugeneo commented May 13, 2018

eugeneo commented May 14, 2018

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

eugeneo commented May 15, 2018

addaleax commented May 15, 2018

mcollina left a comment

Choose a reason for hiding this comment

eugeneo commented May 16, 2018

eugeneo commented May 16, 2018 • edited Loading

eugeneo commented May 17, 2018

eugeneo commented May 17, 2018

richardlau commented May 22, 2018

MylesBorins commented Aug 7, 2018

jasnell commented Aug 7, 2018

jasnell May 9, 2018 •

edited

Loading

eugeneo commented May 16, 2018 •

edited

Loading