trace_events: adds a new trace_events api #19803

jasnell · 2018-04-04T17:45:37Z

Removes the requirement to use --trace-events-enabled to enable trace events. Tracing is enabled automatically if there are any enabled categories.

Adds a new trace_events module with an API for enabling/disabling trace events at runtime without a command line flag.

const trace_events = require('trace_events');
const categories = [ 'node.perf', 'node.async_hooks' ];
const tracing = trace_events.createTracing({ categories });
tracing.enable();
// do stuff
tracing.disable();

Multiple Tracing objects may exist and be enabled at any point in time. The enabled trace event categories is the union of all enabled Tracing objects and the --trace-event-categories flag.

Alternative to: #19233

ping @ofrobots @AndreasMadsen @mcollina @nodejs/diagnostics

Checklist

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

jasnell · 2018-04-04T17:49:30Z

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

AndreasMadsen · 2018-04-04T17:51:00Z

lib/trace_events.js

this.enabled should be enough

AndreasMadsen · 2018-04-04T17:51:58Z

lib/trace_events.js

Why Boolean(this[kEnabled]),couldn't you just initialize this[kEnabled] = false; in the constructor?

No particular reason, either works

No particular reason, either works

I would prefer the one that doesn't involve type conversion. I think V8 will also prefer the monomorphism.

AndreasMadsen · 2018-04-04T17:53:02Z

lib/trace_events.js

I think categories should be mandatory. Logging all this data is never a good idea.

devsnek · 2018-04-04T17:55:04Z

once again a small reminder about whether we want to namespace builtins, not blocking this or anything, it looks pretty good 👍

Qard · 2018-04-04T18:14:45Z

Any particular reason for trace_events.createTracing({ categories: [ 'node.perf', 'node.async_hooks' ] }) versus just trace_events.createTracing([ 'node.perf', 'node.async_hooks' ])?

jasnell · 2018-04-04T18:23:27Z

@Qard ... to account for the possibility of new options in the future.

jasnell · 2018-04-04T18:57:41Z

Failures in CI are unrelated.

Qard · 2018-04-04T18:59:27Z

Could you not just do trace_events.createTracing(categories, options) if that need comes up in the future?

jasnell · 2018-04-04T19:03:49Z

Yes, but this API is also consistent with the styling of the async_hooks API (e.g. async_hooks.createHook({ ... }). Consistency of similarly purposed APIs is a good thing :-)

Trott · 2018-04-04T19:29:24Z

doc/api/errors.md

Optional suggestion:

Simplify this to something like:

The trace_events module cannot be loaded because Node.js was compiled without Trace Events.

...or:

The trace_events module could not be loaded because Node.js was compiled with V8 platform disabled.

This will allow for the possibility now or in the future that there are other reasons Node.js might be compiled without Trace Events.

(Totally non-blocking. Just a suggestion.)

Trott · 2018-04-04T19:31:03Z

doc/api/tracing.md

Alternate suggestion:

Tracing can be enabled with the --trace-event-categories command-line flag
or by using the trace_events module.

Trott · 2018-04-04T19:32:45Z

doc/api/tracing.md

Typo: enableed -> enabled

Trott · 2018-04-04T19:33:09Z

doc/api/tracing.md

Nit: listing -> list

Nit: currently enabled -> currently-enabled

Trott · 2018-04-04T19:33:48Z

doc/api/tracing.md

Same: currently enabled -> currently-enabled

Trott · 2018-04-04T19:37:16Z

doc/api/tracing.md

Nit: Get rid of everything from For example to this line, as well as the Will emit line below and use this instead:

Given the file `test.js` below, the command `node --trace-event-categories node.perf test.js` will print `'node.async_hooks,node.perf'` to the console.

AndreasMadsen · 2018-04-04T19:39:07Z

Yes, but this API is also consistent with the styling of the async_hooks API (e.g. async_hooks.createHook({ ... }). Consistency of similarly purposed APIs is a good thing :-)

I don't really hold any strong opinion. But the reason why it is an object in async_hooks is that all parameters are optional. That is not the case here.

jasnell · 2018-04-04T19:42:05Z

I don't really hold any strong opinion. But the reason why it is an object in async_hooks is that all parameters are optional. That is not the case here.

Yep, I get that, but I still have a strong preference towards keeping the arguments as consistent as possible.

Qard · 2018-04-04T20:22:54Z

I don't really agree with the "consistency" argument here--the purpose of async_hooks is not that similar, and the purpose of the arguments are entirely different. I don't really have a strong opinion about it though. 🤷‍♂️

mcollina · 2018-04-04T21:16:46Z

I think there is a package-lock file committed by accident.

Side note, as this is an experimental module, I don't think this is semver-major. The major reason this is semver-major is that adding this will shadow my trace_events placeholder on npm.

Can you add setting the destination file via the JS API?

jasnell · 2018-04-04T21:25:24Z

I think there is a package-lock file committed by accident.

Ugh.

Side note, as this is an experimental module, I don't think this is semver-major. The major reason this is semver-major is that adding this will shadow my trace_events placeholder on npm.

Well, adding a new top level module is always semver-major by default. The @nodejs/tsc can decide otherwise, however.

Can you add setting the destination file via the JS API?

In a separate PR, yes. Once this lands.

jasnell · 2018-04-04T22:02:15Z

Errant package-lock change removed.

vsemozhetbyt · 2018-04-04T22:17:12Z

doc/api/tracing.md

It seems Value: part is unconventional: other properties in the docs mostly have just a type.

vsemozhetbyt · 2018-04-04T22:22:06Z

doc/api/tracing.md

vsemozhetbyt · 2018-04-04T22:27:29Z

doc/api/tracing.md

Nit: maybe it is worth to unify padding spaces in array literals inside this doc: some have them, some have not.

vsemozhetbyt · 2018-04-04T22:44:01Z

doc/api/tracing.md

Maybe it is worth to add this type in customTypesMap and format it here, like this:

in type-parser.js:

- 'tls.TLSSocket': 'tls.html#tls_class_tls_tlssocket', - + 'tls.TLSSocket': 'tls.html#tls_class_tls_tlssocket', + + 'Tracing': 'tracing.html#tracing_tracing_object', +

in tracing.md:

-* Returns: `Tracing` object. +* Returns: {Tracing}

jasnell · 2018-04-05T00:30:03Z

@vsemozhetbyt ... updated! :-)

mcollina

LGTM

jasnell · 2018-04-10T20:34:21Z

I've updated the PR to emit a process warning if the Tracing object is gc'd while still enabled. This is specifically to discourage users from allowing references to get lost and to help identify buggy code. The Tracing object is still disabled on gc but the best practice would be to hold a reference to prevent gc and to explicitly call disable() before allowing the object to gc. There is existing precedence for this with the new FileHandle object that is used in the fs.promises API.

jasnell · 2018-04-11T14:47:07Z

@nodejs/diagnostics @nodejs/tsc ... I'd appreciate a few more looks at this and some additional points of view on the one item that is holding this up... namely whether gc of an enabled Tracing object should automatically disable it.

bnoordhuis · 2018-04-11T20:12:15Z

doc/api/errors.md

"with V8 platform disabled" is cryptic. I'd say something like "with ./configure --without-v8-platform".

Maybe "with the --without-v8-platform flag"?

bnoordhuis · 2018-04-11T20:16:20Z

doc/api/tracing.md

'not enabled', no dash, or just 'disabled.'

disabled is the better of those two options IMO

bnoordhuis · 2018-04-11T20:24:28Z

src/node.cc

Method could be const, ditto on line 353.

bnoordhuis · 2018-04-11T20:35:36Z

src/node_trace_events.cc

Don't .As<String>() here:

The constructor takes a Local<Value>

It looks wrong because there's nothing that guarantees it's really a string.

bnoordhuis · 2018-04-11T20:50:34Z

src/tracing/agent.cc

CHECK_EQ(0, uv_thread_create(...)); - that's the most common form in src/ for such checks.

bnoordhuis · 2018-04-11T21:25:57Z

src/tracing/agent.cc

These methods look unnecessary. I can't really tell why CategorySet is exposed to JS because the core of its logic is just this and that could be done by a single binding method:

Utf8Value categories(args.GetIsolate(), args[0]); v8_platform.GetTracingAgent()->Enable(categories);

And Enable() can be condensed to just this, it looks like:

// split categories by comma, then: TraceConfig* trace_config = new TraceConfig(); for (auto category : categories) trace_config->AddIncludedCategory(category.c_str()); tracing_controller_->StartTracing(config);

If CategorySet exists to have some weak GC object, eh, there's probably better ways to achieve that. I see it's been discussed at some length already. :-)

The reason CategorySet exists is specifically to have some weak GC object that can be used with the theoretical goal of being able to manage multiple trace event targets and possibly other hypothetical use cases later. I'm personally not convinced it's needed and would prefer the trace_events.enable()/ trace_events.disable() approach with the internal ref counting. It would make this much simpler as you suggest.

~~To explain a bit more... the simplified versions you suggest are definitely possible. However,~~

1) there is a general concern over multiple modules stepping on each other if they are enabling/disabling categories which is why the code uses a crude form of reference counting using an std::multiset as opposed to using AddIncludedCategory directly. We should likely keep that in place.

2) CategorySet exists specifically to have a JS that can be used. Right now enabling and disabling the categories is the only use for it. There has been some discussion, however, about allowing JS code to specify multiple trace event destinations (e.g. one Tracing object could direct the trace events it cares about to one file, while another directs trace events it wants to another file). This is a purely hypothetical use case at this point but it's been discussed.

~~I've got zero problems with simplifying this by eliminating CategorySet but we need to resolve the API question... do we, or we do not, want the JS-accessible Tracing object?~~

Reading the comment again I realize I misread your original point. The internal CategorySet is there largely to eventually allow for more Tracing object specific configuration stuff such as multiple outputs. You're absolutely right that it can be eliminated if all we're going to do is enable/disable categories.

jasnell · 2018-04-13T23:06:11Z

Thanks for the comments @bnoordhuis ... I think I got everything...

@nodejs/tsc ... still looking for additional review on this. There is a conflict of opinion about whether the Tracing object should disable itself on gc. We should try to resolve that before landing this.

Trott · 2018-04-13T23:57:14Z

doc/api/errors.md

These two sentences should (IMO) be replaced with a single short sentence:

`trace_events.createTracing()` requires at least one trace event category.
^{...or if you must use passive voice, something more like:
`trace_events.createTracing()` must be called with at least one trace event category.}

jasnell · 2018-04-14T21:01:47Z

After talking with @mcollina about it, in order to get this unblocked, I'm removing the disable on gc and will maintain an internal Set of enabled Tracing object references to ensure that they cannot be gc'd while they are enabled. A new getEnabledTracingObjects() method will allow inspection of what Tracing objects are in the Set (and will allow recovery of any lost references).

Removes the requirement to use `--trace-events-enabled` to enable trace events. Tracing is enabled automatically if there are any enabled categories. Adds a new `trace_events` module with an API for enabling/disabling trace events at runtime without a command line flag. ```js const trace_events = require('trace_events'); const categories = [ 'node.perf', 'node.async_hooks' ]; const tracing = trace_events.createTracing({ categories }); tracing.enable(); // do stuff tracing.disable(); ``` Multiple `Tracing` objects may exist and be enabled at any point in time. The enabled trace event categories is the union of all enabled `Tracing` objects and the `--trace-event-categories` flag.

jasnell · 2018-04-14T21:09:29Z

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

ofrobots

LGTM if getEnabledTracingObjects is omitted for now, but I am open to counter arguments.

ofrobots · 2018-04-16T16:39:09Z

doc/api/tracing.md

+Tracing can be enabled with the `--trace-event-categories` command-line flag
+or by using the `trace_events` module.

 The set of categories for which traces are recorded can be specified using the


minor nit: elide these two paragraphs:

Tracing can be enabled with the --trace-event-categories command-line flag
or by using the trace_events module. The --trace-event-categories flag accepts
accepts a list of comma separated category..

How come? They seem useful.

I agree with @ofrobots and think the current formulation as separate paragraphs is confusing. They are written as if they are about two different flags but they're about the same flag. Put the information in one place.

Also, the way @ofrobots re-wrote the second sentence (to be less wordy and to eliminate the passive voice) is much more readable IMO.

(Although get rid of the accepts accepts in the second sentence. One accepts is enough. 😄)

(Also: comma separated -> comma-separated.)

If you want to keep them as separate paragraphs on the grounds that one is about enabling the feature and the other is about specifying categories, rewrite the second one so that it doesn't sound like it's introducing a new flag when it's not. Maybe this?

Categories for which traces are recorded are specified as one or more names after the --trace-event-categories flag.

ofrobots · 2018-04-16T16:50:04Z

src/node_trace_events.cc

+  }
+
+  bool enabled_ = false;
+  std::set<std::string> categories_;


nit: make this a const as well.

ofrobots · 2018-04-16T17:01:59Z

lib/trace_events.js

+  return new Tracing(options.categories);
+}
+
+function getEnabledTracingObjects() {


The danger with this API is that others can get a handle to my tracing object and mutate. I think the danger you are guarding against here is that people might lose a handle to their Tracing object, and this provides a way to recover. I am not convinced that this will be a real problem that users of trace_events are likely to run into. And even if they do, it is going to be really easy to fix their code.

IMO, we don't need this API. If there is real need for this API that cannot be addressed by a fix in user code, then lets hear that reason from users and then add this

The concern here is that we're building a potential memory leak... giving users the ability to allocate objects that cannot be recovered and gc'd. If we don't have deactivation on cleanup, then I think we at least need to have a mechanism like this because the failure might be entirely silent and go unnoticed by the users.

One thing we could do to make it nosier is a warning if the set of enabled Tracing objects becomes too large but that's only a partial mitigation.

The concern here is that we're building a potential memory leak.

I think it would be unlikely for people to enable the same category multiple times – without disabling the previously enabled tracing object. So, the only case where the leak is possible is when users are trying out various different permutations of the categories – but never disabling them. I think this is unlikely, and a warning for (say >10) tracing objects would probably be enough to guard against this danger.

The proposed API here is more dangerous than the theoretical memory leak IMO.

I'll remove the function and add the error. I'm not entirely happy with it but I'd like to get this unblocked.

ofrobots · 2018-04-16T17:10:50Z

test/parallel/test-trace-events-api.js

+const enabledCategories = isChild ? 'foo' : undefined;
+
+assert.strictEqual(getEnabledCategories(), enabledCategories);
+[1, 'foo', true, false, null, undefined].forEach((i) => {


nit: might be good to deal with this (i.e. better error message). It might be worth noting that non-string values get coerced to a string in the docs. Okay, if it gets done in a follow-on PR.

> t.createTracing({categories: [Symbol("foo")]}) TypeError: Cannot convert a Symbol value to a string at new Tracing (trace_events.js:25:21) at Object.createTracing (trace_events.js:75:10

jasnell · 2018-04-16T17:18:19Z

~~This is ready to go, just waiting on additional @nodejs/tsc review since it is semver-major due to the addition of the new top level module.~~ hah, just saw that @ofrobots' reviewed :-) thank you!

jasnell · 2018-04-16T19:03:49Z

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

jasnell · 2018-04-16T22:38:32Z

CI is green. There are three TSC approvals. If there are no objections I will get this landed tomorrow.

@nodejs/tsc ... let me know if there are any objections to going ahead and pulling this in to 10.0.0

Removes the requirement to use `--trace-events-enabled` to enable trace events. Tracing is enabled automatically if there are any enabled categories. Adds a new `trace_events` module with an API for enabling/disabling trace events at runtime without a command line flag. ```js const trace_events = require('trace_events'); const categories = [ 'node.perf', 'node.async_hooks' ]; const tracing = trace_events.createTracing({ categories }); tracing.enable(); // do stuff tracing.disable(); ``` Multiple `Tracing` objects may exist and be enabled at any point in time. The enabled trace event categories is the union of all enabled `Tracing` objects and the `--trace-event-categories` flag. PR-URL: #19803 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Ali Ijaz Sheikh <ofrobots@google.com> Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com>

jasnell · 2018-04-17T16:09:37Z

Landed in 5c27e44

Removes the requirement to use `--trace-events-enabled` to enable trace events. Tracing is enabled automatically if there are any enabled categories. Adds a new `trace_events` module with an API for enabling/disabling trace events at runtime without a command line flag. ```js const trace_events = require('trace_events'); const categories = [ 'node.perf', 'node.async_hooks' ]; const tracing = trace_events.createTracing({ categories }); tracing.enable(); // do stuff tracing.disable(); ``` Multiple `Tracing` objects may exist and be enabled at any point in time. The enabled trace event categories is the union of all enabled `Tracing` objects and the `--trace-event-categories` flag. PR-URL: #19803 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Ali Ijaz Sheikh <ofrobots@google.com> Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com>

jasnell added semver-major PRs that contain breaking changes and should be released in the next major version. trace_events Issues and PRs related to V8, Node.js core, and userspace code trace events. labels Apr 4, 2018

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

jasnell mentioned this pull request Apr 4, 2018

trace_events: enable trace events implicitly, add module #19233

Closed

4 tasks

AndreasMadsen reviewed Apr 4, 2018

View reviewed changes

lib/trace_events.js Outdated

Copy link

Member

AndreasMadsen Apr 4, 2018 •

edited

Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this.enabled should be enough

jasnell reacted with thumbs up emoji

AndreasMadsen reviewed Apr 4, 2018

View reviewed changes

Trott reviewed Apr 4, 2018

View reviewed changes

doc/api/tracing.md Outdated

Copy link

Member

Trott Apr 4, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Typo: enableed -> enabled

Trott reviewed Apr 4, 2018

View reviewed changes

doc/api/tracing.md Outdated

Copy link

Member

Trott Apr 4, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same: currently enabled -> currently-enabled

Trott reviewed Apr 4, 2018

View reviewed changes

jasnell force-pushed the trace-events-api branch from 5d242ba to 3ae19e2 Compare April 4, 2018 19:48

jasnell force-pushed the trace-events-api branch from 3ae19e2 to eea7510 Compare April 4, 2018 22:01

vsemozhetbyt reviewed Apr 4, 2018

View reviewed changes

mcollina approved these changes Apr 10, 2018

View reviewed changes

jasnell force-pushed the trace-events-api branch 2 times, most recently from c6e111f to 2ade138 Compare April 10, 2018 20:32

jasnell added the tsc-review label Apr 11, 2018

bnoordhuis reviewed Apr 11, 2018

View reviewed changes

jasnell force-pushed the trace-events-api branch from 2ade138 to db09864 Compare April 13, 2018 21:18

Trott reviewed Apr 13, 2018

View reviewed changes

vsemozhetbyt mentioned this pull request Apr 14, 2018

doc: add missing type=misc top comments #20022

Closed

3 tasks

jasnell removed the tsc-review label Apr 14, 2018

jasnell force-pushed the trace-events-api branch from 4cbccaf to 0c7fd73 Compare April 14, 2018 21:08

ofrobots approved these changes Apr 16, 2018

View reviewed changes

[Squash] address feedback[Squash] address feedback

7be9b8d

jasnell mentioned this pull request Apr 16, 2018

Node.js Foundation Technical Steering Committee (TSC) Meeting 2018-04-18 nodejs/TSC#520

Closed

fhinkel approved these changes Apr 16, 2018

View reviewed changes

[Squash] doc nit

4876121

vsemozhetbyt mentioned this pull request Apr 17, 2018

doc: update trace_event doc in tracing.md #20092

Closed

3 tasks

jasnell closed this Apr 17, 2018

tony-go mentioned this pull request Jun 2, 2022

trace_events: mark it as stable #43294

Open

Uh oh!

trace_events: adds a new trace_events api #19803

trace_events: adds a new trace_events api #19803

Uh oh!

Conversation

jasnell commented Apr 4, 2018

Checklist

Uh oh!

jasnell commented Apr 4, 2018

Uh oh!

AndreasMadsen Apr 4, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

devsnek commented Apr 4, 2018

Uh oh!

Qard commented Apr 4, 2018

Uh oh!

jasnell commented Apr 4, 2018

Uh oh!

jasnell commented Apr 4, 2018

Uh oh!

Qard commented Apr 4, 2018

Uh oh!

jasnell commented Apr 4, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AndreasMadsen commented Apr 4, 2018

Uh oh!

jasnell commented Apr 4, 2018

Uh oh!

Qard commented Apr 4, 2018

Uh oh!

mcollina commented Apr 4, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jasnell commented Apr 4, 2018

Uh oh!

jasnell commented Apr 4, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vsemozhetbyt Apr 4, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vsemozhetbyt Apr 4, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jasnell commented Apr 5, 2018

Uh oh!

mcollina left a comment

Choose a reason for hiding this comment

Uh oh!

jasnell commented Apr 10, 2018

Uh oh!

AndreasMadsen Apr 4, 2018 •

edited

Loading

mcollina commented Apr 4, 2018 •

edited

Loading

vsemozhetbyt Apr 4, 2018 •

edited

Loading

vsemozhetbyt Apr 4, 2018 •

edited

Loading

jasnell Apr 11, 2018 •

edited

Loading

Trott Apr 13, 2018 •

edited

Loading