diagnostics_channel: add iterator support to tracing channel

[rochdev]

Most tracing channel use cases can already be handled by a combination of traceSync, tracePromise and traceCallback. However, functions returning an iterator need the iterator to also be instrumented. This is because for the operation executed by the function to actually be done the iterator needs to reach its done state, and to collect the result the chunks from each iteration must be available.

This PR adds support for a new traceIterator function that wraps a function returning an iterator so that both the function and every iterator iterations are traced.

Open question:

• Should traceIterator just wrap iterators and not functions? This would deviate from other traceX methods but would remove the need to handle sync and async with the same function.
  • Is it even desirable to handle only one type of return value at a time? The current API is often unusable because of this and the underlying code must be duplicated instead. Maybe it would be better to have tracing methods be able to handle more not less?
  • For example, should tracePromise also handle synchronous return values similar to the new #traceMaybePromise? Right now it converts to a promise, which may incorrectly change the return type.

[codecov]

Codecov Report

❌ Patch coverage is 91.25000% with 7 lines in your changes missing coverage. Please review.
✅ Project coverage is 89.65%. Comparing base (8c32389) to head (46b889a).
⚠️ Report is 239 commits behind head on main.

Files with missing lines	Patch %	Lines
lib/diagnostics_channel.js	91.25%	7 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #61686      +/-   ##
==========================================
- Coverage   89.75%   89.65%   -0.10%     
==========================================
  Files         674      676       +2     
  Lines      204394   206626    +2232     
  Branches    39278    39560     +282     
==========================================
+ Hits       183448   185248    +1800     
- Misses      13238    13492     +254     
- Partials     7708     7886     +178     

Files with missing lines	Coverage Δ
lib/diagnostics_channel.js	97.95% <91.25%> (-1.15%)	⬇️

... and 219 files with indirect coverage changes

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[Qard]

Thanks for addressing this! I've left some feedback. I think we may want to consider a deeper refactor of TracingChannel. It's still not marked stable, so I think it may be a good idea to steer it in a more flexible direction where we can build a wider variety of specializations more flexibly. 🤔

[Qard]

No, because thenables. That's why it does the PromiseResolve(...) currently, though that isn't actually quite right either. What we should be doing is using promise.then(resolve, reject) after rather than PromisePrototypeThen(result, resolve, reject) as that assumes it's a native promise, thus the PromiseResolve(...) to convert it into one if it is not one already.

[rochdev]

What about functions that can return both though? Sometimes the caller handles that, and changing the return type may be unexpected. In some cases we end up having to manually do a traceSync and handle the promise. Relying on duck-typing might be more flexible no? It just feels weird to have this TracingChannel in Node when in at least half of cases we need to do everything manually because it's too rigid. I guess an alternative would be to keep the individual specialized ones and add a traceAny or similar that handles all cases automatically in a less strict way. It could even have an optional position parameter so that it can also do callbacks (so many cases where functions can be all 3 at the same time).

[Qard]

Not a huge fan of magically deriving these from the main channels. Could we perhaps implement this traceIterator functionality as an entirely other class which we could simply construct with two separate TracingChannel instances for the overall execution and the per-yield execution?

I've been thinking we might want to split up TracingChannel into specializations anyway, something like: SyncTracingChannel, CallbackTracingChannel, PromiseTracingChannel, and a new IteratorTracingChannel?

[rochdev]

Not a huge fan of magically deriving these from the main channels. Could we perhaps implement this traceIterator functionality as an entirely other class which we could simply construct with two separate TracingChannel instances for the overall execution and the per-yield execution?

I agree and I'm also not a fan, but it felt like the simplest approach.

Are you saying to do instead something like this?

const ctx = {}
const iter = traceSync(fn, ctx)
return traceIterator(iter, ctx)

I thought about that, but I wasn't sure since it would be the first time that TracingChannel instruments anything that is not a function (I mean it still patches the functions on the iterator, but that's more magic than usual). It also makes it impossible to reimplement differently later, although in practice I'm not sure that would happen. And it would also potentially be inconsistent across implementors. One could use :yield, another could use :next, etc, and thenTracingChannel would lose the consistency it was built for, and subscribers could not be built generically.

Worth noting: I'm adding the same functionality to Orchestrion-JS right now and it's unclear whether the FunctionType should be Iterator or not. If this is split, then I guess it would probably be a separate IteratorType?

I've been thinking we might want to split up TracingChannel into specializations anyway, something like: SyncTracingChannel, CallbackTracingChannel, PromiseTracingChannel, and a new IteratorTracingChannel?

I prefer the one class because we often have to mix functionalities or use the underlying channels directly for example when functions accept a callback but can also return a promise or a sync value.