async stack tracking should not be enabled by default

[ofrobots]

#13870 adds support for async stack tracking whenever the inspector agent is enabled. Async stack traces are awesome for debugging, but doing this by automatically when the inspector agent is active has a lot of problems:

Some problematic scenarios I can think of:

• User wants to attach DevTools in order to profile their app. Enabling async stack traces add overhead that was not present before the profiler was attached. The profile would not be representative of the original program.
• User wants to take a heap profile and uses the inspector module to take the heap profile. Async stack tracking gets enabled as a side-effect – something that the user did not want (the user is not 'single step debugging').
• User wants to use the inspector module to build a passive, in-production debugger. User is not interested in the overhead of async stack tracking.

Async stack tracking should have been an opt in. Whether or not the inspector agent is enabled conflates too many use-cases.

Apologies that I didn't catch this issue earlier.

/cc @nodejs/v8-inspector @bajtos @nodejs/diagnostics

[bajtos]

@ofrobots good points. IIRC, DevTools have a checkbox to enable/disable async stack traces in the debugger. Under the hood, DevTools send Debugger.setAsyncCallStackDepth with a non-zero value (I think it's always 10) to enable async stack traces in V8 and zero to disable them. When the async call stack depth is 0, the V8 AsyncTask* API calls are effectively no-ops, at least it was this way the last time I checked V8 sources.

So I think the only overhead of enabling async stack traces lies in the overhead of Node.js's async_hooks machinery, which is still likely to change the performance profile of the inspected app as you pointed out.

I think it would be best if Node.js could intercept Debugger.setAsyncCallStackDepth commands send by inspector clients (e.g. DevTools front-end) and enable/disable the async_hook depending on whether the front-end requested async stack traces or not.

Thoughts?

[AndreasMadsen]

I think it would be best if Node.js could intercept Debugger.setAsyncCallStackDepth commands send by inspector clients (e.g. DevTools front-end) and enable/disable the async_hook depending on whether the front-end requested async stack traces or not.

That would be amazing. But we need to read the asyncCallStackDepth on startup otherwise we won't have the full context. Is that possible?

[TimothyGu]

Interception is totally possible. Reading it at startup isn't as big of a concern, since right now we allow enabling inspection during runtime and tasks started before starting inspection are not tracked anyway.

[refack]

As I mentioned in IRC I would consider this semver-minor.

/cc @jkrems, @roblourens, @billti, @prigara, @ulitink, @segrey

[prigara]

@refack Thanks for the heads up.
Our current plan is to keep async stack traces enabled by default. Right now WebStorm doesn’t support heap profiling with the new protocol, so it shouldn’t be affected by the issues mentioned in the first comment.
Are there any other known issues related to having async stack tracking enabled?

[ofrobots]

@prigara The OP mentions a few cases other than heap profiler (e.g. CPU profiling, passive debugging).

IMO, it is not an option to keep by-default stack tracking. I would like to get this fixed before we reach LTS for Node 8.x. I'm looking into intercepting Debugger.setAsyncCallStackDepth as a mechanism to enable async tracking. Perhaps we can add an additional command line flag that allows folks to enable async tracking on process startup.

[ofrobots]

Interception of the Debugger.setAsyncCallStackDepth message is somewhat complicated as it has to be implemented in C++.

With --inspect-brk, the message arrives on the wire way too early (before JavaScript has bootstrapped). This means that the interception function cannot be JavaScript. Having to parse JSON on the C++ side in the inspector Agent is starting to look very clunky.

[refack]

Are there any other known issues related to having async stack tracking enabled?

@prigara with the hooks enabled, the debugger steps into the hooks scripts for all of the invocations (before and after async operations), and when stepping through the hook invocation script makes the async stack suddenly appear

---

@ofrobots I might be wrong, but I think that if the inspector Agent will send Debugger.setAsyncCallStackDepth only after the client has attached, it might be good enough.

[ofrobots]

PR: #16260