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.

Not a feedback on this PR, but I don't think node.async_hooks are good candidates for the default set of events to enable. They are a bit too low level. Higher level tracing of at the APIs level would be a lot more useful to most users. Since trace_events is experimental, I expect we should be able to change this in the future.

I think it is for backward compatibility, ultimately I think --trace-events-enabled should be removed. A default set of trace_events doesn't really make sense to me.

Yes, it's for backwards compatibility only. Once this lands I plan to open a PR deprecating --trace-events-enabled and removing it from the node --help output. It will still be there for legacy support and can eventually be removed.

sgtm

At some point we may also want to expose the list of static categories that can be enabled by the user to facilitate interactive tool writers to expose them in the UI. With that in mind, do we want name this Tracing.prototype.enabled_categories so that it is more easily distinguishable from trace_events.categories?

well... we can expose the list of categories that we explicitly support, but the list is not entirely static.

re: categories... there is no trace_events.categories.

const trace_events = require('trace_events');
trace_events.getEnabledCategories(); // returns the string listing of enabled categories

const tracing = trace_events.createTracing({ categories: ['foo', 'bar'] });
tracing.categories  // the string listing of categories for this tracing object

Yes, there is no API for that today, but we would want one the future.

To give some more context, this is what one can do in Chrome today:

I would imagine that people would want to build tracing tools that let users select from a list of categories of events they can trace. It would be good if such tools can obtain the predefined list of categories programmatically.

It should be relatively trivial to add this with a separate PR once the trace_events module lands.

I'm not truly fond of returning a comma-separated list of things. Can we return an Array? Do we expect this API to be sitting in hot paths where doing that would be too expensive?

The main reason I went with the comma separated list is that it uses the same format at that accepted by the --trace-event-categories flag. A user can easily console.log() this then copy-and-paste it in to a script using that flag. The values are purely informational otherwise and it is exceedingly unlikely to end up in a hot path. At most, a user will log the data out and, in so doing, would have to convert it to a string anyway. I could provide an second property that gives an Array.

Do we also want to mention somewhere that the traced categories will get disabled if the Tracing object gets garbage collected. I wonder if this is going to be a gotcha for people – they might enable tracing then forget to keep a reference to the object long term?

Likely a good idea.

I don't really like that the categories are disabled if the Tracing object is garbage collected. We don't remove hooks in async_hooks just because the hooks object is removed. We don't stop a setTimeout just because the timer object is removed. I think it is perfectly natural to do:

trace_events.createTracing({ categories: ['node.http'] }).enable();

without ever disabling it again.

In that case, however, it would be trivial to put this call into a context where it is never garbage collected, then you'd accomplish the same goal.

In that case, however, it would be trivial to put this call into a context where it is never garbage collected, then you'd accomplish the same goal.

I'm not really buying that as a good argument. You could say that about a lot of APIs we have. As a general rule, I don't think garbage collection should have side-effects.

Could we turn this discussion around, where you argue why it should have this side effect?

I do think letting users lose a reference to hooks is less bad as this is. I would note that the hooks objects has no chance of being collected anyway, as it's typically referenced by the closures that are passed to the hook.

I agree that this matches the behavior of async_hooks.

setTimeout  is something that resolve itself. It does not leave any resource occupied after it's done, so it's not related.
I do agree that unused fd should be automatically closed.

I keep disagreeing with this approach, however let's land this and iterate separately, as it would need to be changed in async_hooks as well.

This central question: "is cleanup directly observable by the users?", is why I find calling tracer.disable() on GC a really bad idea.

I find it as bad as allocating resources that could not be collected in the future.

setTimeout is something that resolve itself. It does not leave any resource occupied after it's done, so it's not related.

setInterval then.

setInterval() is also a bit of a special case here because it's not going to be gc'd at all while the timer is running and the behavior with it is well established and expected with it by this point. If setInterval() were introduced today I'd be much more likely to advocate for more closely following the browser implementation that returns a number identifying the timer rather than an object, allowing clearInterval() to be called from any context. I'd also prefer to have an API that lets me discover which intervals are active. That ship has sailed for that particular API, however.

Another possible approach on this is to encourage folks to proactively disable the handles rather than wait for gc the same way we do with the new FileHandle class in fs.promises. Essentially, emit a warning if the categories are disabled implicitly on gc rather than explicitly by calling the disable() method. It would be much more likely to encourage better API use while still cleaning up after ourselves.

If setInterval() were introduced today I'd be much more likely to advocate for more closely following the browser implementation that returns a number identifying the timer rather than an object, allowing clearInterval() to be called from any context.

What do we mean by context, maybe that is what I don't understand? How do you know what number to use?

Other examples:

• child = child_process.spawn() could call child.kill().
• server = http.createServer() could call server.close().
• request = http.request() could call request.end().
• stream = new zlib.Deflate() could call stream.close().
• stream = new crypto.createHash() could call stream.end().

For child_process.spawn(), yes it could, but again, the ship has long sailed for that particular API and the current behavior is what is expected. I actually wouldn't mind an optional setting that would allow the spawned process to be torn down if the child handle was gc'd.

For the other examples: server won't gc if it's an active handle. By the time it gc's, there should not be anything left to clean up and calling close() certainly would not be the correct choice. Generally likewise for request, and the two stream examples so I don't see those as being comparable.

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.