doc, async_hooks: improve and add migration hints

[Flarna]

Add hints to migrate away from async hooks.
Change docs at various places to be more clear that resources are internals and may change at any time.

This might replace and/or extend #45335.

Main differences (besides the few additions):

• don't announce the We plan to remove this API
• don't refer to diagnostics_channel because I'm not aware of any async hooks use case which is solved by diagnostics_channel (well, we could create async.init, async.before, async.after and async.destroy channels to hide async hooks API but this would be of no real help)

I think it's easier to provide my view/input via a new PR. Anyhow, if people prefer the wording/message of #45335 I'm also fine and will close this one.

Refs: #45335

fyi @GeoffreyBooth @nodejs/diagnostics

[GeoffreyBooth]

• don’t refer to diagnostics_channel because I’m not aware of any async hooks use case which is solved by diagnostics_channel

Diagnostics Channel is itself experimental, so if it’s mentioned it needs to be in its own category as it’s a disfavored migration path as compared to the other options; but I wouldn’t exclude it entirely. It feels to me that the channels in https://nodejs.org/api/diagnostics_channel.html are defined as better handles for tracking async activity than the generic AsyncResource objects provided by async_hooks; as in, you could track inbound and outbound network requests via async_hooks but it’s a much better DX to do so via https://nodejs.org/api/diagnostics_channel.html#http. Diagnostics Channel feels very relevant to me as another way to achieve many of the use cases that people could use async_hooks for.

[Qard]

Not to get too far off-topic, but diagnostics_channel is getting marked stable in the next release. The PR landed the other day.

And yes, diagnostics_channel is intended as a better solution to many of the async_hooks uses for handle-tracking. There likely needs to be more events added to the specific places people are interested to know about though. We're still fairly light on diagnostics_channel events in core.

[Flarna]

It feels to me that the channels in https://nodejs.org/api/diagnostics_channel.html are defined as better handles for tracking async activity than the generic AsyncResource objects provided by async_hooks; as in, you could track inbound and outbound network requests via async_hooks but it’s a much better DX to do so via https://nodejs.org/api/diagnostics_channel.html#http. Diagnostics Channel feels very relevant to me as another way to achieve many of the use cases that people could use async_hooks for.

These channel are not to replace async_hooks. I doubt anyone uses/used async hooks to monitor a network request. These channels solve another problematic use case: monkey patching. Currently APMs (and maybe others) monkey path HTTP and/or net,... to get request start, end + meta data. via these channel the info is pushed to consumers.
They usually don't use async hooks and the the provided internal resource to get the data of an HTTP request. Async hooks are used to track the call tree down from the http request handler emitted by core to the user functions - even across async hops like nextTick, promises, disc I/O,...

Please believe me, the diagnostics channels are good and helpful but not to replace async hooks - except by creating async.init, async.before, async.after and async.destroy channels but they would then leak exactly the same data as async hooks now.

If some use case turns up where a DC channel solves an async hooks usecase I'm happy to extend the list.

[Flarna]

And yes, diagnostics_channel is intended as a better solution to many of the async_hooks uses for handle-tracking. There likely needs to be more events added to the specific places people are interested to know about though. We're still fairly light on diagnostics_channel events in core.

Once they are in place it's fine but they are not as of now. And we have to take care to add only channels which expose stable/public data. If we transport internal data through a stable API it's by no means better then now.

But yes, we tend to go off topic a bit.

[Qard]

Yes, agreed we should not publish public data with diagnostics_channel. I'm saying though that it is explicitly a goal of diagnostics_channel to replace many of the uses of async_hooks like tracking sockets and things like that which are currently done very unsafely though async_hooks and internal handles.

[GeoffreyBooth]

@Qard did you have any other notes for this? Is it okay to land?

And we can continue debating what text to put next to the status, if any, in #45335

[Trott]

@Flarna I made some style changes. Hopefully none of them are a problem, but PTAL. If this looks good to you, it's ready to land. Thanks!

[mcollina]

lgtm

[GeoffreyBooth]

I chatted with @mcollina and he mentioned that AsyncLocalStorage has much lower overhead than async_hooks, about 15% compared to 30%+ according to him; maybe this is worth mentioning? Ideally if we have a source we can cite.

Also per https://github.com/nodejs/node/blob/main/doc/api/diagnostics_channel.md, Diagnostics Channel is stable, or will be as soon as the docs on main get published to the website with the next release. So maybe Diagnostics Channel is worth including in the list of suggested other APIs?

I’m also fine with this landing as is, and/or these additions coming in a follow-up.

[Flarna]

I chatted with @mcollina and he mentioned that AsyncLocalStorage has much lower overhead than async_hooks, about 15% compared to 30%+ according to him; maybe this is worth mentioning? Ideally if we have a source we can cite.

AsyncLocalStorage uses async_hooks internally therefore such a statement would imply that AsyncLocalStorage needs a negative number of CPU cycles which is not true. AsyncLocalStorage is no magic thing, it doesn't use any internal thing (besides primordials) so it could be moved to a userland module from that point of view.
It is in core to avoid the need for users to use async hooks for context tracking - to have this use case covered by a stable API.

The performance difference mentioned by @mcollina are likely based on comparing "traditional" async hooks usage (init/before/after/destroy hooks) vs resource base async hooks usage (init hook + executionAsyncResource ).

@mcollina Please correct me if I'm wrong here

Besides that numbers heavily depend on the concrete application. So whenever we plan to publish something we should include a concrete benchmark application.

Also per https://github.com/nodejs/node/blob/main/doc/api/diagnostics_channel.md, Diagnostics Channel is stable, or will be as soon as the docs on main get published to the website with the next release. So maybe Diagnostics Channel is worth including in the list of suggested other APIs?

Diagnostics channel itself is an API to enable users to publish data, a somehow similar to EventEmitter. The presence of the API doesn't solve any async hooks use case. Target is to add concrete channels to solve concrete use case and once they exist add them here.

[mcollina]

The performance difference mentioned by @mcollina are likely based on comparing "traditional" async hooks usage (init/before/after/destroy hooks) vs resource base async hooks usage (init hook + executionAsyncResource ).

Yes, I oversimplified. AsyncLocalStorage is based on executionAsyncResource(), therefore the above should hold in principle. In all intent and purposes, migrating folks out of using the destroy hook will drastically reduce the overhead of promises, leading to faster Node.js applications.

[nodejs-github-bot]

Landed in 81ab00d

[Qard]

It's also worth mentioning that over 95% of all async_hooks usage remaining in the ecosystem that hasn't migrated to AsyncLocalStorage is using the destroy hook.

I did a mass code search the other day and the createHook function is almost universally used with all hooks except promiseResolve which is almost never used. I manually inspected the top 50 or so modules and only one of them was not an implementation of context storage which could be trivially ported to AsyncLocalStorage. It was doing handle-tracking for which we also have a replacement API. That tells me we are utterly failing at guiding the ecosystem to the new APIs.

[Flarna]

Migration means work and if you want/have to support old node versions you need to maintain two implementations.
e.g. opentelemetry has a context manager implementation for both and uses AsyncLocalStorage for node >=14.8.0 as this version added some important fixes (not sure if they ever arrived on 12). They removed 8, 10 and 12 support on june this year.

Node 8, 10 and 12 may look terrible old but they still run a lot in production environments next to newer node versions.

[Qard]

Yes, but if a company is running the old versions anyway, it shouldn't matter much if we remove a feature in a future major because they won't be upgrading anyway. Also, AsyncLocalStorage is supported in every supported version of Node.js at this point. There's no reason not to migrate at this point other than laziness. If you really need to support users that are too lazy to upgrade you can use async_hooks as a fallback until it eventually (maybe) disappears.

The issue is that users tend to get something to functional and then expect to never update it ever but that's not how maintenance works. You're going to have to make updates to deal with bugs and security issues, and just the same you're eventually going to have to make changes to keep in-line with modern practices. The same thing occurred with moving off the new Buffer constructor. We just need to put in the effort to promote that it's important for the ecosystem to migrate, and help with that process as much as possible.