New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Trace event support for Node.js #9304

Closed
wants to merge 22 commits into
base: master
from

Conversation

@matthewloring
Contributor

matthewloring commented Oct 26, 2016

Checklist
  • make -j8 test (UNIX), or vcbuild test nosign (Windows) passes
  • documentation is changed or added
  • commit message follows commit guidelines
Affected core subsystem(s)

src

Description of change

Background: nodejs/diagnostics#30, nodejs/diagnostics#53

This PR adds support for trace-event tracing to Node.js. It provides a mechanism to centralize tracing information generated by V8, Node core, and userspace code. The PR contains a few components:

  • A trace writer responsible for serializing traces and cycling the output files so that no individual file becomes to large.
  • A buffer for aggregating traces to allow for batched flushes.
  • An agent which initializes the tracing controller and ensures that trace serialization is done on a separate thread.
  • A set of macros for generating trace events.

Generating Trace Events

This PR introduces macros for recording trace events. Synchronous operations can be traced by wrapping them with the appropriate macros:

TRACE_EVENT_BEGIN0("MY_CATEGORY", "SomethingCostly")
doSomethingCostly()
TRACE_EVENT_END0("MY_CATEGORY", "SomethingCostly")

Asynchronous operations can be traced using:

void StartAsyncOperation(int operation_id, ...) {
  TRACE_EVENT_NESTABLE_ASYNC_BEGIN0("category", "operation", operation_id);
  ...
}
void EndAsyncOperation(int operation_id, ...) {
  TRACE_EVENT_NESTABLE_ASYNC_END0("category", "operation", operation_id);
  ...
}

An example of async traces can be seen below. The example was generated by integrating the above macros into Async-Wrap.

Additional examples as well as a complete list of trace macros can be found in src/tracing/trace_event.h. This PR does not introduce any JS interface for generating events or tracing of Node core with the hope that the community can determine how the instrumentation of core should be approached and how this functionality can be exposed to end users.

Viewing Trace Events

The current trace serialization outputs traces in a format that can be visualized by navigating to chrome://tracing in Chrome and loading a trace output file.

w85xe3osv1u

We can zoom in on this visualization to view VM events alongside the opening and closing of TCP connections:

dsksrjkirjkirjkk69xc63ny39g3a3zhbdtmw4aaaaaelftksuqmcc

Usage

Tracing can be enabled by running:

node --trace-events-enabled server.js

By default, this will record all events in the "v8" and "node" categories. To trace other categories or ignore either of these categories, use the --trace-event-categories flag:

node --trace-events-enabled --trace-event-categories v8,custom-category server.js

This PR is the result of a lot of hard work by @misterpoe, @kjin, @lpy, @fmeawad, and @ofrobots.

Thanks for the review!

/cc @nodejs/diagnostics

@AndreasMadsen

This comment has been minimized.

Show comment
Hide comment
@AndreasMadsen

AndreasMadsen Oct 27, 2016

Member

Awesome

An example of async traces can be seen below. The example was generated by integrating the above macros into Async-Wrap.

Should we consider doing this by default?

Member

AndreasMadsen commented Oct 27, 2016

Awesome

An example of async traces can be seen below. The example was generated by integrating the above macros into Async-Wrap.

Should we consider doing this by default?

@matthewloring

This comment has been minimized.

Show comment
Hide comment
@matthewloring

matthewloring Oct 27, 2016

Contributor

@AndreasMadsen I think that could be a good idea and it did generate useful information but I want to keep this PR focused on the core tracing system. I am hoping that instrumentation to generate trace events can be left to a follow on.

Contributor

matthewloring commented Oct 27, 2016

@AndreasMadsen I think that could be a good idea and it did generate useful information but I want to keep this PR focused on the core tracing system. I am hoping that instrumentation to generate trace events can be left to a follow on.

@cjihrig

This comment has been minimized.

Show comment
Hide comment
@cjihrig

cjihrig Oct 27, 2016

Contributor

Is it possible to add docs and tests?

Contributor

cjihrig commented Oct 27, 2016

Is it possible to add docs and tests?

@matthewloring

This comment has been minimized.

Show comment
Hide comment
@matthewloring

matthewloring Oct 27, 2016

Contributor

@cjihrig Working on tests now. Are the debugger docs the best place to put doc changes or should they go somewhere else?

Contributor

matthewloring commented Oct 27, 2016

@cjihrig Working on tests now. Are the debugger docs the best place to put doc changes or should they go somewhere else?

@cjihrig

This comment has been minimized.

Show comment
Hide comment
@cjihrig

cjihrig Oct 27, 2016

Contributor

Good question. Do you think there will be enough content to warrant a separate Tracing documentation page? If not, then the Debugger docs or even guides might be an appropriate place.

Others can weigh in here too.

Contributor

cjihrig commented Oct 27, 2016

Good question. Do you think there will be enough content to warrant a separate Tracing documentation page? If not, then the Debugger docs or even guides might be an appropriate place.

Others can weigh in here too.

@sam-github

This comment has been minimized.

Show comment
Hide comment
@sam-github

sam-github Oct 27, 2016

Member

In the example above, trace config doesn't look complex enough to warrant a JSON config file. It can be difficult to arrange for files on disk to appear when wanting to configure node behaviour.

I suggest the example JSON config file would be better expressed as

node --trace-include=v8,custom_category --trace-exclude=node ...
Member

sam-github commented Oct 27, 2016

In the example above, trace config doesn't look complex enough to warrant a JSON config file. It can be difficult to arrange for files on disk to appear when wanting to configure node behaviour.

I suggest the example JSON config file would be better expressed as

node --trace-include=v8,custom_category --trace-exclude=node ...
@matthewloring

This comment has been minimized.

Show comment
Hide comment
@matthewloring

matthewloring Oct 27, 2016

Contributor

@cjihrig I've added an end to end test and put some documentation in the debugger docs. Happy to move it if there is a better place.

@sam-github I've updated the description to list the full set of supported configuration options. Though I would anticipate included and excluded categories to be the options relevant to node, V8 does provide additional configuration.

Contributor

matthewloring commented Oct 27, 2016

@cjihrig I've added an end to end test and put some documentation in the debugger docs. Happy to move it if there is a better place.

@sam-github I've updated the description to list the full set of supported configuration options. Though I would anticipate included and excluded categories to be the options relevant to node, V8 does provide additional configuration.

Show outdated Hide outdated test/parallel/test-trace-event.js
@@ -0,0 +1,29 @@
'use strict';

This comment has been minimized.

@cjihrig

cjihrig Oct 27, 2016

Contributor

Can you include ../common

@cjihrig

cjihrig Oct 27, 2016

Contributor

Can you include ../common

This comment has been minimized.

@matthewloring

matthewloring Oct 27, 2016

Contributor

Done.

@matthewloring

matthewloring Oct 27, 2016

Contributor

Done.

Show outdated Hide outdated test/parallel/test-trace-event.js
const cp = require('child_process');
const CODE = 'for (var i = 0; i < 100000; i++) { \"test\" + i }';
const FILE_NAME = 'node_trace.1.log';

This comment has been minimized.

@cjihrig

cjihrig Oct 27, 2016

Contributor

The file should probably be written to common.tmpDir.

@cjihrig

cjihrig Oct 27, 2016

Contributor

The file should probably be written to common.tmpDir.

This comment has been minimized.

@matthewloring

matthewloring Oct 27, 2016

Contributor

Done.

@matthewloring

matthewloring Oct 27, 2016

Contributor

Done.

Show outdated Hide outdated test/parallel/test-trace-event.js
const CODE = 'for (var i = 0; i < 100000; i++) { \"test\" + i }';
const FILE_NAME = 'node_trace.1.log';
fs.access(FILE_NAME, (err) => {

This comment has been minimized.

@cjihrig

cjihrig Oct 27, 2016

Contributor

You can use common.fileExists() to avoid a level of indentation.

@cjihrig

cjihrig Oct 27, 2016

Contributor

You can use common.fileExists() to avoid a level of indentation.

This comment has been minimized.

@matthewloring

matthewloring Oct 27, 2016

Contributor

Done.

@matthewloring

matthewloring Oct 27, 2016

Contributor

Done.

Show outdated Hide outdated test/parallel/test-trace-event.js
const proc = cp.spawn(process.execPath,
[ '--enable-tracing', '-e', CODE ]);
proc.once('exit', () => {

This comment has been minimized.

@cjihrig

cjihrig Oct 27, 2016

Contributor

Please add common.mustCall() around the callback.

@cjihrig

cjihrig Oct 27, 2016

Contributor

Please add common.mustCall() around the callback.

This comment has been minimized.

@matthewloring

matthewloring Oct 27, 2016

Contributor

Done.

@matthewloring

matthewloring Oct 27, 2016

Contributor

Done.

@sam-github

This comment has been minimized.

Show comment
Hide comment
@sam-github

sam-github Oct 28, 2016

Member

@matthewloring I still think the configuration should be via CLI options. JSON should be avoided unless necessary, where necessary means "complex data structures", which this doesn't have, its a bunch of boolean or string flags. Its not even nice for dev use... typing CLI options is easy, seeing CLI docs in the help output is easy, having to write your options into a file for a one off trace run is not easy. Sure, do it for permanent config like .eslintrc, but ephemeral config like tracing? Doesn't make sense to me.

I have tooling that allows node monitoring and tracing to be enabled in production, and I've been looking forward to the v8 trace lib getting integrated, pretty excited about this.

Restarting node with new CLI options is easy, having to write my config into filesystem (somewhere, where?) is not convenient. There isn't even a way to guarantee its cleaned up afterwards. I can do it (well, assuming I'm not running in docker with a read-only FS I can do it), but its not a good way.

Member

sam-github commented Oct 28, 2016

@matthewloring I still think the configuration should be via CLI options. JSON should be avoided unless necessary, where necessary means "complex data structures", which this doesn't have, its a bunch of boolean or string flags. Its not even nice for dev use... typing CLI options is easy, seeing CLI docs in the help output is easy, having to write your options into a file for a one off trace run is not easy. Sure, do it for permanent config like .eslintrc, but ephemeral config like tracing? Doesn't make sense to me.

I have tooling that allows node monitoring and tracing to be enabled in production, and I've been looking forward to the v8 trace lib getting integrated, pretty excited about this.

Restarting node with new CLI options is easy, having to write my config into filesystem (somewhere, where?) is not convenient. There isn't even a way to guarantee its cleaned up afterwards. I can do it (well, assuming I'm not running in docker with a read-only FS I can do it), but its not a good way.

Show outdated Hide outdated test/parallel/test-trace-event.js
proc.once('exit', common.mustCall(() => {
assert(common.fileExists(FILE_NAME));
fs.readFile(FILE_NAME, (err, data) => {
fs.unlinkSync(FILE_NAME);

This comment has been minimized.

@Trott

Trott Oct 28, 2016

Member

Tiniest of nits that can totally be ignored: No need to clean up after the test in the temp directory. Each test that needs to use the temp directory runs common.refreshTmpDir() which will clean up anything there.

@Trott

Trott Oct 28, 2016

Member

Tiniest of nits that can totally be ignored: No need to clean up after the test in the temp directory. Each test that needs to use the temp directory runs common.refreshTmpDir() which will clean up anything there.

This comment has been minimized.

@matthewloring

matthewloring Oct 28, 2016

Contributor

Fixed, thanks.

@matthewloring

matthewloring Oct 28, 2016

Contributor

Fixed, thanks.

@matthewloring

This comment has been minimized.

Show comment
Hide comment
@matthewloring

matthewloring Oct 28, 2016

Contributor

@sam-github That makes a lot of sense. I'm happy to introduce command line flags for included/excluded categories and get rid of the json configuration. I'm going to give it a little time before changing this in case anyone has other ideas on configuration approaches.

Contributor

matthewloring commented Oct 28, 2016

@sam-github That makes a lot of sense. I'm happy to introduce command line flags for included/excluded categories and get rid of the json configuration. I'm going to give it a little time before changing this in case anyone has other ideas on configuration approaches.

@Fishrock123

This comment has been minimized.

Show comment
Hide comment
@Fishrock123

Fishrock123 Oct 28, 2016

Member

Does this have any performance impact when off?

Member

Fishrock123 commented Oct 28, 2016

Does this have any performance impact when off?

@fmeawad

This comment has been minimized.

Show comment
Hide comment
@fmeawad

fmeawad Oct 28, 2016

@Fishrock123 The performance impact is minimal specially when off

For every call site, we check if the category is enabled or disabled the first time the call site was visited, and this value is cached so that we do not have to do the expensive category check again.

If tracing was off, then it was enabled: all cached references are updated. And we do that to avoid doing the category check every time we visit the call site.

But that limits the ability to change the enabled categories when tracing is already on. Instead, we need to stop change, then start again.

That being said, trace-events should not be used to track high frequency event*, sampling and runtime stats should be used instead (both are getting standardized in tracing).
*events in tight loops.

fmeawad commented Oct 28, 2016

@Fishrock123 The performance impact is minimal specially when off

For every call site, we check if the category is enabled or disabled the first time the call site was visited, and this value is cached so that we do not have to do the expensive category check again.

If tracing was off, then it was enabled: all cached references are updated. And we do that to avoid doing the category check every time we visit the call site.

But that limits the ability to change the enabled categories when tracing is already on. Instead, we need to stop change, then start again.

That being said, trace-events should not be used to track high frequency event*, sampling and runtime stats should be used instead (both are getting standardized in tracing).
*events in tight loops.

@trevnorris

This comment has been minimized.

Show comment
Hide comment
@trevnorris

trevnorris Oct 30, 2016

Contributor

@fmeawad Ignoring the tight loop scenario, a single http request can make 12-18 async calls. A high speed proxy can process upwards of 30k reqs/sec. Which results in 360k-540k async calls/sec. This ignores sync calls that would also be traced (e.g. writes to a socket that are immediately flushed to the kernel). At that volume, what performance impact would we expect to see with this both enabled and disabled?

Contributor

trevnorris commented Oct 30, 2016

@fmeawad Ignoring the tight loop scenario, a single http request can make 12-18 async calls. A high speed proxy can process upwards of 30k reqs/sec. Which results in 360k-540k async calls/sec. This ignores sync calls that would also be traced (e.g. writes to a socket that are immediately flushed to the kernel). At that volume, what performance impact would we expect to see with this both enabled and disabled?

@trevnorris

This comment has been minimized.

Show comment
Hide comment
@trevnorris

trevnorris Oct 30, 2016

Contributor

On the side, this PR doesn't replace async hooks in any way. The two are complimentary.

Contributor

trevnorris commented Oct 30, 2016

On the side, this PR doesn't replace async hooks in any way. The two are complimentary.

Show outdated Hide outdated doc/api/debugger.md
`--trace-config` flag:
```txt
node --enable-trace --trace-config=trace-config.json server.js

This comment has been minimized.

@jasongin

jasongin Oct 31, 2016

Contributor

--enable-tracing

@jasongin

jasongin Oct 31, 2016

Contributor

--enable-tracing

This comment has been minimized.

@matthewloring

matthewloring Oct 31, 2016

Contributor

Thanks, fixed.

@matthewloring

matthewloring Oct 31, 2016

Contributor

Thanks, fixed.

Show outdated Hide outdated doc/api/debugger.md
node --enable-trace --trace-config=trace-config.json server.js
```
where `trace-config.json` may override any of the default configuration values:

This comment has been minimized.

@jasongin

jasongin Oct 31, 2016

Contributor

More clarification would be helpful here:

  • Are the values below actually the defaults, or just examples?
  • What are the valid values for record_mode?
  • What do each of those boolean flags mean?
  • Are there any other known categories for the node/v8 engine? (I realize user code can add arbitrary tracing categories.)
@jasongin

jasongin Oct 31, 2016

Contributor

More clarification would be helpful here:

  • Are the values below actually the defaults, or just examples?
  • What are the valid values for record_mode?
  • What do each of those boolean flags mean?
  • Are there any other known categories for the node/v8 engine? (I realize user code can add arbitrary tracing categories.)

This comment has been minimized.

@matthewloring

matthewloring Oct 31, 2016

Contributor

Configuration has been simplified to just specify enabled categories through a command line flag. Other configuration options are not useful at this time (only one record_mode supported by the buffer, systrace not being used by node, etc). We are working on getting a list of categories compiled here: https://github.com/v8/v8/wiki/Tracing%20V8. Currently node does not introduce any categories of its own. The node category introduced by this PR is just a placeholder and can be updated when the community decides how/where trace points should be added to core.

@matthewloring

matthewloring Oct 31, 2016

Contributor

Configuration has been simplified to just specify enabled categories through a command line flag. Other configuration options are not useful at this time (only one record_mode supported by the buffer, systrace not being used by node, etc). We are working on getting a list of categories compiled here: https://github.com/v8/v8/wiki/Tracing%20V8. Currently node does not introduce any categories of its own. The node category introduced by this PR is just a placeholder and can be updated when the community decides how/where trace points should be added to core.

Show outdated Hide outdated doc/api/debugger.md
"enable_systrace": true,
"enable_argument_filter": true,
"included_categories": [ "v8", "node" ],
"excluded_categories": [ ]

This comment has been minimized.

@jasongin

jasongin Oct 31, 2016

Contributor

Does excluding categories actually do anything? As far as I can see, TraceConfig::IsCategoryGroupEnabled() ignores that excluded category list, and the list is not checked anywhere else either.

@jasongin

jasongin Oct 31, 2016

Contributor

Does excluding categories actually do anything? As far as I can see, TraceConfig::IsCategoryGroupEnabled() ignores that excluded category list, and the list is not checked anywhere else either.

This comment has been minimized.

@matthewloring

matthewloring Oct 31, 2016

Contributor

This is used by Chrome since they specify enabled categories using regular expressions which can be refined by excluded categories. The node/v8 category parser doesn't support regexes so this isn't needed at this time.

@matthewloring

matthewloring Oct 31, 2016

Contributor

This is used by Chrome since they specify enabled categories using regular expressions which can be refined by excluded categories. The node/v8 category parser doesn't support regexes so this isn't needed at this time.

Show outdated Hide outdated src/tracing/agent.cc
std::string str((std::istreambuf_iterator<char>(fin)),
std::istreambuf_iterator<char>());
TraceConfigParser::FillTraceConfig(env->isolate(), trace_config,
str.c_str());

This comment has been minimized.

@jasongin

jasongin Oct 31, 2016

Contributor

It looks like the above code will crash if the trace config file is not found or has invalid contents. Is that acceptable? Or should there be a more helpful error message?

@jasongin

jasongin Oct 31, 2016

Contributor

It looks like the above code will crash if the trace config file is not found or has invalid contents. Is that acceptable? Or should there be a more helpful error message?

This comment has been minimized.

@matthewloring

matthewloring Oct 31, 2016

Contributor

Removed.

@matthewloring

matthewloring Oct 31, 2016

Contributor

Removed.

@fmeawad

This comment has been minimized.

Show comment
Hide comment
@fmeawad

fmeawad Oct 31, 2016

@trevnorris According to my math, If you are using a single thread for that, an async call can take as low as 1.5us, a trace event can take up to 4us, therefore I do not recommend using trace events for that. The off overhead is going to be high as well, it will be ~10% here.

But it seems this is an easy experiment to conduct, and that would be my recommendation here.

fmeawad commented Oct 31, 2016

@trevnorris According to my math, If you are using a single thread for that, an async call can take as low as 1.5us, a trace event can take up to 4us, therefore I do not recommend using trace events for that. The off overhead is going to be high as well, it will be ~10% here.

But it seems this is an easy experiment to conduct, and that would be my recommendation here.

@matthewloring

This comment has been minimized.

Show comment
Hide comment
@matthewloring

matthewloring Oct 31, 2016

Contributor

@trevnorris Just for a data point, adding the trace events used to generate the graphs above to async-wrap (but with tracing disabled) caused no change in request latency and ~2% reduction in requests per second for a server with no logic:

var server = http.createServer(function(req, res) {
  res.end('Hello world');
});

The % performance impact would be even lower if the server did anything before responding. We will definitely need to be careful about how many trace points are added and where they go.

Contributor

matthewloring commented Oct 31, 2016

@trevnorris Just for a data point, adding the trace events used to generate the graphs above to async-wrap (but with tracing disabled) caused no change in request latency and ~2% reduction in requests per second for a server with no logic:

var server = http.createServer(function(req, res) {
  res.end('Hello world');
});

The % performance impact would be even lower if the server did anything before responding. We will definitely need to be careful about how many trace points are added and where they go.

@joshgav joshgav referenced this pull request Oct 31, 2016

Merged

doc: update Diag WG info #9329

2 of 2 tasks complete
Show outdated Hide outdated src/tracing/trace_config_parser.cc
bool TraceConfigParser::GetBoolean(Isolate* isolate, Local<Context> context,
Local<Object> object, const char* property) {
Local<Value> value = GetValue(isolate, context, object, property);
if (value->IsNumber()) {

This comment has been minimized.

@jasongin

jasongin Oct 31, 2016

Contributor

Should this be:

if (value->IsNumber() || value->IsBoolean())

I'm not certain IsNumber() is false for a boolean literal, but I'm assuming that's the case.

@jasongin

jasongin Oct 31, 2016

Contributor

Should this be:

if (value->IsNumber() || value->IsBoolean())

I'm not certain IsNumber() is false for a boolean literal, but I'm assuming that's the case.

This comment has been minimized.

@matthewloring

matthewloring Oct 31, 2016

Contributor

Removed.

@matthewloring

matthewloring Oct 31, 2016

Contributor

Removed.

Show outdated Hide outdated src/tracing/node_trace_writer.cc
Mutex::ScopedLock stream_scoped_lock(stream_mutex_);
if (total_traces_ >= kTracesPerFile) {
total_traces_ = 0;
// Destroying the member JSONTraceWriter object appends "]"\n"" to

This comment has been minimized.

@jasongin

jasongin Oct 31, 2016

Contributor

"]}" to be more precise. (And no \n.)

@jasongin

jasongin Oct 31, 2016

Contributor

"]}" to be more precise. (And no \n.)

This comment has been minimized.

@matthewloring

matthewloring Oct 31, 2016

Contributor

Fixed, thanks!

@matthewloring

matthewloring Oct 31, 2016

Contributor

Fixed, thanks!

@matthewloring

This comment has been minimized.

Show comment
Hide comment
@matthewloring

matthewloring Oct 31, 2016

Contributor

@sam-github I've updated the category configuration to use the --enabled-categories command line flag.

@jasongin Thank you for the review. This round of comments should all be addressed by the last commit.

Contributor

matthewloring commented Oct 31, 2016

@sam-github I've updated the category configuration to use the --enabled-categories command line flag.

@jasongin Thank you for the review. This round of comments should all be addressed by the last commit.

Show outdated Hide outdated doc/api/debugger.md
Node.js application.
The set of categories for which traces are recorded can be specified using the
`--enabled-categories` flag followed by a list of comma separated category names.

This comment has been minimized.

@jasongin

jasongin Oct 31, 2016

Contributor

Probably the categories command-line option should have the word 'tracing' in it somehow, so that it's more obviously associated with tracing. Maybe --tracing-categories?

And is there any way to help avoid confusion with the other command-line options starting with --trace-, which are actually about stack traces?

@jasongin

jasongin Oct 31, 2016

Contributor

Probably the categories command-line option should have the word 'tracing' in it somehow, so that it's more obviously associated with tracing. Maybe --tracing-categories?

And is there any way to help avoid confusion with the other command-line options starting with --trace-, which are actually about stack traces?

This comment has been minimized.

@matthewloring

matthewloring Oct 31, 2016

Contributor

Hmm, that's a good point. Maybe --enable-trace-events and --trace-event-categories?

@matthewloring

matthewloring Oct 31, 2016

Contributor

Hmm, that's a good point. Maybe --enable-trace-events and --trace-event-categories?

This comment has been minimized.

@mhdawson

mhdawson Nov 22, 2016

Member

Would it make sense to have them all start with --trace-event so its clear they go together. For example:

--trace-events-enabled
--trace-event-categories

Not sure how this fits with the way we use enabled/disabled for existing options but having all options related to trace-events start with the same prefix might make it easier to know they are related.

@mhdawson

mhdawson Nov 22, 2016

Member

Would it make sense to have them all start with --trace-event so its clear they go together. For example:

--trace-events-enabled
--trace-event-categories

Not sure how this fits with the way we use enabled/disabled for existing options but having all options related to trace-events start with the same prefix might make it easier to know they are related.

This comment has been minimized.

@matthewloring

matthewloring Nov 30, 2016

Contributor

Updated the flags.

@matthewloring

matthewloring Nov 30, 2016

Contributor

Updated the flags.

@trevnorris

This comment has been minimized.

Show comment
Hide comment
@trevnorris

trevnorris Oct 31, 2016

Contributor

@matthewloring one area i've almost broken my back over with async hooks is that there is no overhead when not in use. the user voluntarily enabling them is a different story, but IMO a feature shouldn't impose overhead on applications that aren't using it. from what you say, it sounds like node will incur overhead for every trace event placed in core, whether they're used or not. can I get clarification that is actually the case?

Contributor

trevnorris commented Oct 31, 2016

@matthewloring one area i've almost broken my back over with async hooks is that there is no overhead when not in use. the user voluntarily enabling them is a different story, but IMO a feature shouldn't impose overhead on applications that aren't using it. from what you say, it sounds like node will incur overhead for every trace event placed in core, whether they're used or not. can I get clarification that is actually the case?

@matthewloring

This comment has been minimized.

Show comment
Hide comment
@matthewloring

matthewloring Nov 1, 2016

Contributor

@trevnorris This trace system has been designed to incur as little overhead as possible, especially when not enabled, and is already widely used in V8 and Chrome. Once categories have been cached, it should boil down to a single (easily predicted) compare and branch.

Contributor

matthewloring commented Nov 1, 2016

@trevnorris This trace system has been designed to incur as little overhead as possible, especially when not enabled, and is already widely used in V8 and Chrome. Once categories have been cached, it should boil down to a single (easily predicted) compare and branch.

@matthewloring

This comment has been minimized.

Show comment
Hide comment
@matthewloring

matthewloring Nov 9, 2016

Contributor

@bnoordhuis @joshgav @mhdawson Could I get a review on this from you as well?

Contributor

matthewloring commented Nov 9, 2016

@bnoordhuis @joshgav @mhdawson Could I get a review on this from you as well?

Show outdated Hide outdated test/parallel/test-trace-event.js
const assert = require('assert');
const cp = require('child_process');
const common = require('../common.js');

This comment has been minimized.

@richardlau

richardlau Nov 9, 2016

Member

For tests, I believe common is supposed to be the first line (after 'use strict'), see nodejs/node#7786.

@richardlau

richardlau Nov 9, 2016

Member

For tests, I believe common is supposed to be the first line (after 'use strict'), see nodejs/node#7786.

This comment has been minimized.

@matthewloring

matthewloring Nov 10, 2016

Contributor

Fixed.

@matthewloring

matthewloring Nov 10, 2016

Contributor

Fixed.

@richardlau

This comment has been minimized.

Show comment
Hide comment
@richardlau

richardlau Nov 10, 2016

Member

Shouldn't the new command line options be added to the node --help output and doc/api/cli.md?

Member

richardlau commented Nov 10, 2016

Shouldn't the new command line options be added to the node --help output and doc/api/cli.md?

@matthewloring

This comment has been minimized.

Show comment
Hide comment
@matthewloring

matthewloring Nov 10, 2016

Contributor

@richardlau I've added the command line options to the --help output.

Contributor

matthewloring commented Nov 10, 2016

@richardlau I've added the command line options to the --help output.

@richardlau

This comment has been minimized.

Show comment
Hide comment
@richardlau

richardlau Nov 10, 2016

Member

@matthewloring Thanks. Please could you also update doc/api/cli.md?

Member

richardlau commented Nov 10, 2016

@matthewloring Thanks. Please could you also update doc/api/cli.md?

@bnoordhuis

This comment has been minimized.

Show comment
Hide comment
@bnoordhuis

bnoordhuis Nov 10, 2016

Member

@matthewloring I don't have time to do an in-depth review right now, maybe next week.

Member

bnoordhuis commented Nov 10, 2016

@matthewloring I don't have time to do an in-depth review right now, maybe next week.

richardlau added a commit to richardlau/node-1 that referenced this pull request Jan 21, 2017

build: add tracing/trace_event.h to tarballs
node.h was modifed by #9304 to include tracing/trace_event.h but
tracing/trace_event.h was not added to the headers installed by
tools/install.py.

@richardlau richardlau referenced this pull request Jan 21, 2017

Closed

build: add tracing/trace_event.h to tarballs #10929

2 of 2 tasks complete
@targos

This comment has been minimized.

Show comment
Hide comment
@targos

targos Jan 28, 2017

Member

Is there a plan to backport this to release branches?

Member

targos commented Jan 28, 2017

Is there a plan to backport this to release branches?

@evanlucas

This comment has been minimized.

Show comment
Hide comment
@evanlucas

evanlucas Jan 31, 2017

Member

@matthewloring This isn't landing cleanly on v7.x. Mind submitting a backport PR?

Member

evanlucas commented Jan 31, 2017

@matthewloring This isn't landing cleanly on v7.x. Mind submitting a backport PR?

@richardlau

This comment has been minimized.

Show comment
Hide comment
@richardlau

richardlau Jan 31, 2017

Member

If this is backported please also include #10959.

Member

richardlau commented Jan 31, 2017

If this is backported please also include #10959.

@matthewloring

This comment has been minimized.

Show comment
Hide comment
@matthewloring

matthewloring Feb 1, 2017

Contributor

@evanlucas I opened a backport PR here: #11106.

Contributor

matthewloring commented Feb 1, 2017

@evanlucas I opened a backport PR here: #11106.

joshgav added a commit to matthewloring/node that referenced this pull request Feb 28, 2017

src: add tracing controller
This commit adds support for trace-event tracing to Node.js. It provides
a mechanism to centralize tracing information generated by V8, Node
core, and userspace code. It includes:

 - A trace writer responsible for serializing traces and cycling the
   output files so that no individual file becomes to large.
 - A buffer for aggregating traces to allow for batched flushes.
 - An agent which initializes the tracing controller and ensures that
   trace serialization is done on a separate thread.
 - A set of macros for generating trace events.
 - Tests and documentation.

Author: Raymond Kang <raymondksi@gmail.com>
Author: Kelvin Jin <kelvinjin@google.com>
Author: Matthew Loring <mattloring@google.com>
Author: Jason Ginchereau <jasongin@microsoft.com>

PR-URL: nodejs#9304
Backport PR-URL: nodejs#11106
Reviewed-By: Josh Gavant <josh.gavant@outlook.com>

@Trott Trott referenced this pull request Mar 3, 2017

Closed

test: increase strictness for test-trace-event #11065

2 of 2 tasks complete
@Trott

This comment has been minimized.

Show comment
Hide comment
@Trott

Trott Mar 3, 2017

Member

Adding possibly-gratuitous but hopefully-not-harmful do-not-land labels for 4.x and 6.x. /cc @MylesBorins (3 of 3)

Member

Trott commented Mar 3, 2017

Adding possibly-gratuitous but hopefully-not-harmful do-not-land labels for 4.x and 6.x. /cc @MylesBorins (3 of 3)

@jasnell

This comment has been minimized.

Show comment
Hide comment
@jasnell

jasnell Mar 3, 2017

Member

ping @nodejs/lts ... do we want this in 4 and 6?
/cc @nodejs/ctc ... thoughts?

Member

jasnell commented Mar 3, 2017

ping @nodejs/lts ... do we want this in 4 and 6?
/cc @nodejs/ctc ... thoughts?

@matthewloring

This comment has been minimized.

Show comment
Hide comment
@matthewloring

matthewloring Mar 3, 2017

Contributor

The versions of V8 in 4 and 6 do not have the APIs that this change depends on.

Contributor

matthewloring commented Mar 3, 2017

The versions of V8 in 4 and 6 do not have the APIs that this change depends on.

@jasnell jasnell referenced this pull request Apr 4, 2017

Closed

8.0.0 Release Proposal #12220

@jasnell jasnell referenced this pull request Apr 25, 2017

Closed

Collaborator nominations #12646

@watilde watilde referenced this pull request Apr 30, 2017

Closed

tools: ignore node_trace.*.log #12754

2 of 2 tasks complete

watilde added a commit to watilde/node that referenced this pull request May 1, 2017

tools: ignore node_trace.*.log
`node_trace.*.log` files are generated by `NodeTraceWriter`.

Refs: nodejs#9304

watilde added a commit that referenced this pull request May 2, 2017

tools: ignore node_trace.*.log
`node_trace.*.log` files are generated by `NodeTraceWriter`.

Refs: #9304
PR-URL: #12754
Reviewed-By: Сковорода Никита Андреевич <chalkerx@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>

anchnk added a commit to anchnk/node that referenced this pull request May 6, 2017

tools: ignore node_trace.*.log
`node_trace.*.log` files are generated by `NodeTraceWriter`.

Refs: nodejs#9304
PR-URL: nodejs#12754
Reviewed-By: Сковорода Никита Андреевич <chalkerx@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>

gibfahn added a commit that referenced this pull request Jun 18, 2017

tools: ignore node_trace.*.log
`node_trace.*.log` files are generated by `NodeTraceWriter`.

Refs: #9304
PR-URL: #12754
Reviewed-By: Сковорода Никита Андреевич <chalkerx@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>

gibfahn added a commit that referenced this pull request Jun 20, 2017

tools: ignore node_trace.*.log
`node_trace.*.log` files are generated by `NodeTraceWriter`.

Refs: #9304
PR-URL: #12754
Reviewed-By: Сковорода Никита Андреевич <chalkerx@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>

MylesBorins added a commit that referenced this pull request Jul 11, 2017

tools: ignore node_trace.*.log
`node_trace.*.log` files are generated by `NodeTraceWriter`.

Refs: #9304
PR-URL: #12754
Reviewed-By: Сковорода Никита Андреевич <chalkerx@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment