Otherwise, you will delete the shapes and the property access will be slower.

True, but the memory use will be greater, and performance of getting channels shouldn't be much of a priority as it's intended for it to be acquired at the file top-level rather than dynamically. It's a situation of deciding which trade-offs to go for. 🤷🏻

Maybe @mcollina has some thoughts here? Not a clear win either way to me.

How frequently would a call to decRef be in a real APM? I feel deleting the key would have more prejudice than a little memory increase (assuming fewer calls)

In ideal conditions, basically never. But the whole reason for the weak behavior in the first place is to protect against abusing it with excessively dynamic use. If someone is continuously generating and subscribing to new, uniquely-named channels it will be constantly expanding the shape without ever cleaning it up. Though we technically already have that with GC if a channel gets collected without a subscriber ever being added. 🤷🏻

I think this is unrealistic. Technically, if someone is subscribing excessively to unique channels, they will only see a memory increase after tons of calls, right?

Also, might be good to include a benchmark to validate our thoughts

We have a test around that at the moment. https://github.com/nodejs/node/blob/main/test/parallel/test-diagnostics-channel-memory-leak.js

Without the delete that test will fail because the shape of channels keeps expanding and will get filled out with a bunch of empty WeakReference instances that the referenced object can get cleaned up but the container itself will not and those containers can add up quickly if people are, for example, generating channels uniquely named to ids of things. In my opinion that's bad practice, but there's not really any reasonable way to stop users from doing that so we opted for being the least damaging to the environment we can be if the API is used badly.

I think we have to use delete here to avoid a leak if we stick on an object. Maybe we should use a Map instead?

The lookup/remove performance is in my opinion not that important as the intended use is to create a Channel once during startup and use.

Yeah, might be worth considering. 🤔

Or maybe operation. The term trace is usually a tree of linked spans/operations and a single tracing channel forms one entry of a trace not the whole trace.

I don't think "span" or "operation" are great terms here either as there's zero agreement on terminology for what those units are called between APMs. It also means additional terminology that likely needs more explaining. This feature is not intended exclusively for APM and what can be considered a "trace" can be a single unit just as much as a collection of units. I think given the naming of TracingChannel it makes sense to call it a trace. If an expression of a collection of these units is described in this doc somewhere we could just call it a sub-trace. Many APMs think of it that way anyway, where there's not really a distinction between spans and traces, they're all just nested elements.

Yes, using (internal) terms from APM vendors will not help here.
I thought using Span would be quite good because it's also used in OpenTelemetry which is quite popular meanwhile and open.
At some other places you used single traceable action. Matches better but is quite long.

Anyhow, feel free to keep it as it is. APM aware people will be likely able to understand this and others are likely lost at more places.

The single traceable action terminology might be better here. It is long, but it's also unambiguous and not so much based on loaded terminology like "span" or whatever else an APM might know the concept as. What do you think of changing it to that?

sounds good.

Could we move this two lines into the Channel constructor?

Probably? I'll have to make sure the GC characteristics are still correct that way. I think it should be fine though.

There should be no difference. In the end you add two properties to the new channel instance. Not sure why we need two properties referring to the same WeakReference.

Ah, don't actually need _weak anymore. That's leftover from something I had before to hold channels open through TracingChannel as I was constructing them directly before, but I changed how that works so that's not relevant anymore.

Just noticed that this is not resulting in two properties on channel as it is channels[name]. I guess that is still needed to allow a setup where a consumer subscribes on a channel but doesn't keep a reference to the channel instance.

It can go back to how it was before where the WeakReference is stores directly in channels[name] rather than also needing to exist on the channel itself. That was just there because I had some weird weak reference forwarding thing for TracingChannel which turned out to be a bad idea so I deleted it. 😅

Not directly related to this PR but shouldn't we move the type check to the beginning of the function?

Is it still correct that ActiveChannel.hasSubscribers returns always true?
There can be a bound ALS but no subscriber.

I would think of a bound ALS as a type of subscriber, but maybe we need a discussion of what specifically hasSubscribers means? My intent for it was to decide if something needs to be published at all, which is still kind of the case but with a distinction between publish and runStores. Maybe we need another getter for if runStores needs to be called, which is any of subscribers or stores. 🤔

TracingChannel has no such API and in the traceXXX functions no check is done. Also your real world sample in loaders doesn't check this.

But I agree that from that point of view hasSubscribers does the right thing.

TracingChannel actually did have it originally, but I removed it in favour of doing something like tracingChannel.start.hasSubscribers instead.

I think we should document the sequence that first publish happens and then the store is entered.

Could do that. Can you think of any examples when that ordering would matter? Do we need to say anything beyond just swapping that and for a then?

It should be clear what one gets if als.getStore() is called inside the onMessage function. Same is valid for the transform function where sample relies on the fact that it happens before als.run() is called to get the parent.

I think it's important to tell users that publish and transform happen before.

Fair. I'll make some improvements to that soon. :)

Should we somehow handle a throwing transform function?

We could handle it similarly to subscribers where failures get deferred to the next tick so it can finish the loop.

Either this or silently discard the exception and skip this subscriber. But I think we should not swallow such exceptions.

Is there a reason to export TracingChannel?

Theoretically one could sub-class it. Not sure why they'd want to though. 🤷🏻

Similar as for Channel we export a factory method instead of telling users to use new TracingChannel().
If we don't want construction via new allowing sub-classing sounds wrong.
Well one could use it for instanceof.
ActiveChannel is not exported and this is good to my understanding.
Also Channel should be not exported in my opinion but that ship has sailed.

Channels will pass instanceof even when active due to the SymbolHasInstance method on Channel. Also, ideally I'd like to make the factory unnecessary. I have some ideas for sub-classing TracingChannel which I'd like to enable if at all possible.

Maybe remove the factory tracingChannel? I see no reason to support both variants.

Just went for that for consistency with Channel. We'll see how my sub-classing experiments go though. I'm not totally convinced of the utility of exposing both. I'll be taking another pass at cleaning stuff up soon, just busy with settling other work things before the holidays so might not get a chance to update this PR this week.

That's fine. My main point is that it is always easier to add an export compared to removing it - even if it is experimental.

Fair, I think I can just omit it then. It's easy to add later if I have a specific case for it.

if this is considered as a hot function it would maybe make sense to avoid the bind in case this._stores.size === 0

The inactive channel prototype handles that for the most part. A bit of a weird case with subscribers and stores competing for active status. Ideally this should be a no-op when there's no stores. 🤔

Not a noop, the function needs to be called. But this can be a simple Refect.apply instead of creating a bound function and call it.

I mean no-op in the form of doesn't need to check anything. Basically it should do what Channel.prototype.runStores does even if there are subscribers but no stores.

It's a chained promise, not the original one

That's an implementation detail which ideally shouldn't be visible to the user. Do you think this is really relevant?

For plain, native promises not. But there are still a lot frameworks which use thenables like bluebird. I have even seen thenables which are EventEmitters to allow caller to decide on awaiting or something else.

But maybe we should just disallow non native promises. Then it's don't care.

I assume this is restricted to native promises and wont work for there thenables like e.g. a bluebird promise.
Should we add some check regarding this to avoid a TypeError is thrown from here?

Could just wrap promise in a Promise.resolve(promise) if it's not already a native promise.

The problem with this is that it removes any API the original thenable had which likely breaks user code. So maybe best to restrict this to native promises. (refs: other comment above).

If we're going to document it anyway, I think I'd rather just document that it will convert thenables to native promises rather than documenting that it doesn't support thenables at all. 🤔

Maybe check that we really replace a function here

I guess this should be the same object instance. Similar a few lines below.

maybe assert return value and passes arguments

similar in other tests