diagnostics_channel: improved subscribe/unsubscribe

[Qard]

This is an idea for a solution to #42170.

[Flarna]

I think we should add some note in the docs when to use channel.subscribe() and when to use dc.subscribe(). Or should we even mark channel.subscribe() as deprecated or remove it from public API?

Maybe add also some regression test for the linked GC problem?

[Qard]

I'm thinking deprecate channel.subscribe, but if we go forward with this idea I would like a proper deprecation cycle as we are using diagnostics_channel in production at Datadog. My thinking is doc-deprecate, get known users to update, then hopefully remove it in v19.

[bengl]

Should an equivalent dc.publish() be added, for symmetry/ergonomics?

[Qard]

I think the symmetry is actually the problem. The publishing end is expected to be used many times, thus the channel-as-an-object lookup optimization, while the subscribing end is expected to only be called once to register the callback and then just left as-is likely forever or at least not nearly as frequently changed.

The publishing side benefits from the object form while the subscriber side benefits from both the simplicity and the different ownership semantics of the separate top-level interface. Could perhaps be documented more clearly why the two are different though. 🤔

[Qard]

I've rebased and added a commit to mark the old subscribe/unsubscribe methods as deprecated in the docs.

@nodejs/diagnostics I'd appreciate some reviews. 😸

[tony-go]

LGTM 🎉

Just a comment.

[RafaelGSS]

I'm wondering whether is better to deprecate the entire Channel class and provide the hasSubscribers and publish directly as you did for subscribe/unsubscribe.

Anyway, it wouldn't block the PR. LGTM.

[legendecas]

I'm wondering whether is better to deprecate the entire Channel class and provide the hasSubscribers and publish directly as you did for subscribe/unsubscribe.

I believe this has been answered at #42714 (comment).

[Qard]

@nodejs/diagnostics Made some tiny docs fixes. Should be good to go now.

Could also consider changing the name of dc.channel(...) to something like dc.publisher(...) to make it more explicit that it's only for publishing. What do you all think?

[RafaelGSS]

@nodejs/diagnostics Made some tiny docs fixes. Should be good to go now.

Could also consider changing the name of dc.channel(...) to something like dc.publisher(...) to make it more explicit that it's only for publishing. What do you all think?

I'd say the dc.channel is better because it makes the name more flexible to future changes and it wouldn't require a deprecation.

[tony-go]

@nodejs/diagnostics Made some tiny docs fixes. Should be good to go now.

Could also consider changing the name of dc.channel(...) to something like dc.publisher(...) to make it more explicit that it's only for publishing. What do you all think?

WDYT about something like dc.select(...)? (Regarding what I read on doc: "This is the primary entry-point for anyone wanting to interact with a named channel")

[...] I'd say the dc.channel is better because it makes the name more flexible to future changes [...]

Didn't get you on this. Why does it make the name more flexible?

[Flarna]

Could also consider changing the name of dc.channel(...) to something like dc.publisher(...) to make it more explicit that it's only for publishing. What do you all think?

I'm fine with both variants.
But not sure if it is worth to do this breaking change. it's used e.g. in undici which is in turn used by node in fetch.
So I guess it would be only a doc deprecation but keep the old alias.

[Qard]

True. The publishing path is harder to fix the ecosystem than the subscribing path which there are not many users of yet.

In any case, I added a note before changing the wording from "This is the primary entry-point for anyone wanting to interact with a named channel" to suggest it's only meant for publishing. If we add other stuff later we can just update the docs to reflect that.

If everyone is good with the current state, this should be good to land. Just need to get the CI working properly, appears to be getting failures from the changes in #36510.