diagnostics_channel: add storage channel by Qard · Pull Request #44894 · nodejs/node · GitHub

A storage channel integrates between diagnostics_channel and AsyncLocalStorage for the publisher to define a scope in which a store should run and what data to provide in the scope when running.

I'll write some proper docs for this after some discussion first to verify the current design satisfies those of us that need this.

cc @nodejs/diagnostics @jasnell

Review requested:

@nodejs/http
@nodejs/net

diagnosticChannel.bindStore reads to me that the diagnostic channel can only be bound to one single AsyncLocalStorage at one time. I believe this is not the intention, right?

No, the intent is the reverse, that the store is being bound to the storage channel.

I don't understand why it's called the storage channel.

It's called StorageChannel mainly because it uses two channels as delimiters marking where a storage context should be run. I'm open to suggestions for better names though. Still doing a bunch of iterating though so could change completely. 😅

@Qard Is it intended that you created the branch here and in #44943 on nodejs/node instead on your private fork?

if there is the need for the undocumented _enter/_exit here it's likely that they are also useful at other places outside of core.
Is there a reason to keep them internal?

I want to avoid exposing those if at all possible because it's easy to miss one side of it and screw things up. Best to wait and see if a specific use-case comes up and consider exposing it then. I'm not convinced we actually need it though.

Maybe change the usage in http to use run?

That adds an extra closure though, which I'd prefer to avoid when we can. The intent is for internals to use the lower-level interface directly rather than creating closures. It's more error-prone, but core is a surface area of code we control so we can easily ensure it's used correctly. Exposed to the ecosystem though we have less ability to guide away from using it wrong.

It took me a while to fully understand what this does and is used for. So I think docs need some more work and maybe a better sample. But as usual in this ALS topics that's not easy.

I'm also not sure if this is really needed. If e.g. http issues enter/exit events via existing channels the same can be reached by calling enter/exit on your store in your callback. StorageChannel seems to be quite specific for a single use case to get something on ALS. Is it intended to add more such classes/APIs if similar use cases pop up?

The actual requirement here for DC is that the lib in question (http here) is reliable calling enter/exit (or start/end).

Nope. Not intended. Brand new computer and forgot to set up my new Node.js checkout properly. 😅

The purpose of a custom API for this is to have a standard interface that other environments can implement to handle the request wrapping concept in a standard way. Some FaaS environments want this.

A storage channel integrates between diagnostics_channel and AsyncLocalStorage for the publisher to define a scope in which a store should run and what data to provide in the scope when running.

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

Can you document somewhere that there is an http.server channel?

Can you expand this test a bit more with an actual use case for this API?

It's called StorageChannel mainly because it uses two channels as delimiters marking where a storage context should be run.

Aren't this exactly the same delimiters as for the sync part of the tracing channel proposed in #44943?

I think it's hard for a library developer to choose between StorageChannel, Channel and Tracing Channel.

Yes, this could be built on top of the start and end events of TracingChannel. We do need a version of that explicitly for binding storage scopes though. The general interface of this can remain the same while being rebuilt on top of TracingChannel later though, so I think it's not important that one lands before the other.

I was thinking though that it might be worth inverting the relation here to instead put an interface on AsyncLocalStorage to bind an instance to a TracingChannel. I feel like that might make the relationship clearer. I think I will start another PR for that and see how that goes. I'll link that here when it's ready.

We do need a version of that explicitly for binding storage scopes though

Why is this needed inside core/dc? In think the actual need is to get consistent, synchronous start/end notification. The actual enter/exit call to AsyncLocalStore (or whatever else is needed) can happen inside the user callback. Sure, having it in core makes it easier but effective the binding to AsyncLocalStore is just one specific use case. There are other topics usually happening in these notifications like:

collect wall clock time (which one is up to user, e.g. Date.now(), performance.now(), something synced with backend,...)
collect CPU times
instrument/bind EventEmitters and/or install listeners
collect data describing the operation
extract/inject data needed for linking operations (e.g. W3C tracecontext)
...

A library author can't (and should not) know what users do with the data provided. They can just give notification that some operation started/ended along with the metadata.
The usecase to extract/inject something like W3C tracecontext headers is already a bit problematic once you consider that there might be more subscribers and all of them tinker with the same underlying single data set.

It needs to be in core because there is no way to do this without enterWith which is unsafe and thus we would like to discourage use. We intend for AsyncLocalStorage to exist in runtimes other than Node.js though, so it needs a consistent and safe solution, which is what this feature is meant to solve.

I doubt that we can get rid of enter/exit in AsyncLocalStorage - except by adding something similar on diagnostics channel which makes it just more complicated as it adds an indirection. Currently enter/exit on StorageChannel are internal but above you mentioned that it may be needed to expose them.

OpenTelemetry has currently no enter/exit in their context manager API but there are request for it since quite a while.
Looking into e.g. .NET which has AsyncLocal longer then node - it allows to set a value which is mostly the same as our enter.

I've created #45277 as an alternate, hopefully improved implementation of this concept. Please have a look!

Qard added async_hooks Issues and PRs related to the async hooks subsystem. diagnostics_channel Issues and PRs related to diagnostics channel labels Oct 4, 2022

nodejs-github-bot added http Issues or PRs related to the http subsystem. needs-ci PRs that need a full CI run. labels Oct 4, 2022

tony-go reviewed Oct 5, 2022

View reviewed changes

Comment thread lib/_http_server.js Outdated

legendecas reviewed Oct 5, 2022

View reviewed changes

mcollina requested changes Oct 6, 2022

View reviewed changes

Comment thread lib/_http_server.js Outdated

Comment thread lib/diagnostics_channel.js Outdated

Qard force-pushed the diagnostics-channel-storage branch from b2b69ef to 1a233aa Compare October 7, 2022 21:59

Qard mentioned this pull request Oct 10, 2022

diagnostics_channel: add tracing channel #44943

Merged

Flarna reviewed Oct 12, 2022

View reviewed changes

Qard force-pushed the diagnostics-channel-storage branch 2 times, most recently from 3b1d61c to b601ed5 Compare October 18, 2022 00:06

Qard marked this pull request as ready for review October 18, 2022 03:02

Qard added the request-ci Add this label to start a Jenkins CI on a PR. label Oct 18, 2022

diagnostics_channel: add storage channel

c6d01f6

A storage channel integrates between diagnostics_channel and AsyncLocalStorage for the publisher to define a scope in which a store should run and what data to provide in the scope when running.

Qard force-pushed the diagnostics-channel-storage branch from b601ed5 to c6d01f6 Compare October 18, 2022 03:12

github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Oct 18, 2022

mcollina requested changes Oct 18, 2022

View reviewed changes

Qard mentioned this pull request Nov 2, 2022

async_hooks: AsyncLocalStorage to diagnostics_channel integration #45277

Closed

Trott force-pushed the main branch from 2d76238 to ca3ed36 Compare November 12, 2022 01:49

Qard closed this Nov 18, 2022

Flarna deleted the diagnostics-channel-storage branch October 30, 2023 12:08

+                assert.strictEqual(store.getStore(), undefined);
+                get(`http://localhost:${port}`, (res) => res.resume());
+                assert.strictEqual(store.getStore(), undefined);
+              });

                 return channel.hasSubscribers;
               }
+              function bindStore(name, store, build = (v) => v) {

                 }
+                if (storageChannel.hasSubscribers) {
+                  storageChannel._enter({

               });
               const dc = require('diagnostics_channel');
+              const storageChannel = dc.storageChannel('http.server');

diagnostics_channel: add storage channel by Qard · Pull Request #44894 · nodejs/node · GitHub

Uh oh!

Uh oh!

diagnostics_channel: add storage channel - #44894

diagnostics_channel: add storage channel#44894Qard wants to merge 1 commit intomainnodejs/node:mainfrom diagnostics-channel-storagenodejs/node:diagnostics-channel-storageCopy head branch name to clipboard

Conversation

Qard commented Oct 4, 2022

Uh oh!

nodejs-github-bot commented Oct 4, 2022

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mcollina left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Qard commented Oct 6, 2022

Uh oh!

Flarna commented Oct 12, 2022

Uh oh!

Uh oh!

Uh oh!

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!

Uh oh!

Uh oh!

Flarna commented Oct 12, 2022

Uh oh!

Qard commented Oct 12, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nodejs-github-bot commented Oct 18, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Flarna commented Oct 18, 2022

Uh oh!

Qard commented Oct 18, 2022

Uh oh!

Flarna commented Oct 19, 2022

Uh oh!

Qard commented Oct 20, 2022

Uh oh!

Flarna commented Oct 20, 2022

Uh oh!

Qard commented Nov 2, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

diagnostics_channel: add storage channel#44894
Qard wants to merge 1 commit into
mainfrom
diagnostics-channel-storage

Qard commented Oct 12, 2022 •
edited
Loading