Per-instance wrap

[Qard]

One of the challenges Node.js has faced with AsyncLocalStorage and the underlying async_hooks is that consumers of those APIs don't always agree on which path a given propagation should follow. For example, there's debate if it's correct to follow the path from where a promise is resolved or the path from where a then handler was attached. This path can be adjusted with AsyncContext.wrap however it currently only exists as a static impacting all stores. I would suggest also having a per-instance wrap method so any consumers that need to follow a different path from the default can graft the context graph only for their store and not break expectations of other stores by using the static version.

[legendecas]

FWIW, per-instance wrap can be built on top of existing methods as simple as:

function wrap(asyncContext, fn) {
  const value = asyncContext.get();
  return function(...args) {
    asyncContext.run(value, () => fn(...args));
  }
}

Would you mind sharing an example of how this would be helpful in practice?

[Qard]

The use case is being able to alter the graph shape for your own representation without altering it for everyone. A good example is when tracing the fs module. If you wrap a span around every fs function and then a fs.readFile(...) happens you'll see internally that it also does fs.open(...), fs.read(...), and fs.close(...). The internal behaviour might not be relevant so many APM products will choose to step over those internals by currently using AsyncResource to link the fs.readFile(...) callback to the point it was called, however this will alter the graph globally which can break other things. Often times APM products will break each other when used together because of destructively altering the global graph. If we could alter only the graph for our own store in a fairly standard way it would be a huge benefit to the stability of APM products.

I've actually introduced something alongside the Node.js TracingChannel feature recently to somewhat produce this behaviour except specifically bound to a diagnostics_channel, which can be seen in https://github.com/nodejs/node/blob/c23e772ccc19618493937fd2a6a7addf83267ade/doc/api/diagnostics_channel.md#channelbindstorestore-transform and https://github.com/nodejs/node/blob/c23e772ccc19618493937fd2a6a7addf83267ade/doc/api/diagnostics_channel.md#channelrunstorescontext-fn-thisarg-args. It does the same selective binding of a context, but delegated through diagnostics_channel. It would be ideal if this capable was supported directly in AsyncContext though as the code to produce this behaviour externally is somewhat more complicated and confusing than it actually should need to be.

[littledan]

We've added AsyncContext.Snapshot.wrap, so I think we can consider this issue resolved.

[Qard]

Snapshot is a container for the value of all stores though, so wouldn't that capture and restore the value of all stores? The purpose of this issue is to request the ability to do this for individual stores separately. In many cases we will want everyone to follow the same path, connection pools for example, but in some cases the correct path to follow depends on the use case and this is why a feel we need interfaces to control the flow per-store as needed.

[littledan]

Oh, right. Well, this would be quite a small amount of JS to implement on top of the APIs we have already, as explained above #25 (comment) , so I'm not really sure if we need it built-in. What more is needed?

[Qard]

It's a small amount of code on top of what is already there, but it's also I think a common enough need that making the DX of this common case just a little bit friendlier is worth the minimal additional code.