Added diagnostics channels on fetch

[ida613]

We added five diagnostics channels on fetch to trace the same information as would tracingChannel.tracePromise. Some channels won't make perfect sense but we want to stay consistent with TracingChannel as we want to use it to trace fetch and maybe other functions when TracingChannel become available in the most commonly used versions of node.js. The descriptions of each channel and what gets published to them are detailed in DiagnosticsChannel.md

Use case: enable APM tools to trace fetch

[KhafraDev]

It deviates from the spec, we've been careful to maintain a nearly 1:1 implementation and I don't think that this PR adds enough value to do so, sorry. I also have other concerns if the other maintainers think that we should move forward on this.

[codecov-commenter]

Codecov Report

Patch coverage: 85.71% and project coverage change: -0.32% ⚠️

Comparison is base (5281fa8) 85.88% compared to head (1074439) 85.57%.
Report is 29 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2210      +/-   ##
==========================================
- Coverage   85.88%   85.57%   -0.32%     
==========================================
  Files          76       75       -1     
  Lines        6612     6655      +43     
==========================================
+ Hits         5679     5695      +16     
- Misses        933      960      +27     

Files Changed	Coverage Δ
lib/fetch/index.js	85.12% <85.71%> (-0.20%)	⬇️

... and 9 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[metcoder95]

What are you concerns @KhafraDev? Just out of curiosity 🙂

I enabled the CI to grasp the benchmarks, as I'm curious about the performance impact. The implementation produces roughly 14% reduction, which is quite significative.
I imagine this is due to diagnostic_channel implementation.

[mcollina]

It deviates from the spec, we've been careful to maintain a nearly 1:1 implementation and I don't think that this PR adds enough value to do so, sorry.

Why does it deviates from the spec? This is just information published on a node-only channel.

[KhafraDev]

the actual implementation deviates from the spec (adds innerFetch, adds a new deferred promise/context object, etc.)

[KhafraDev]

one of the biggest concerns is the end channel, which doesn't publish a message when the fetch ends, but is actually being done after a request has started:

function fetch (input, init = {}) {
  const context = { input, init }
  if (channels.start.hasSubscribers) channels.start.publish(context)
  const promise = innerFetch(input, init, context)
  if (channels.end.hasSubscribers) channels.end.publish(context)
  return promise
}

I don't see a way of having this channel without a) allocating another promise, or b) further deviating from the spec

[ida613]

@KhafraDev the end channel gets published to at the end of the synchronous part of fetch, I know it doesn't make the most sense but we are trying to keep consistent with tracingChannel.tracePromise

[mcollina]

the actual implementation deviates from the spec (adds innerFetch, adds a new deferred promise/context object, etc.)

I don't understand this, does the spec mandates there should not be another function inside? Seems a bit prescriptive.

[KhafraDev]

Yes it's prescriptive, but it makes maintaining fetch easier. For example, when the spec mentions "fetch", it will no longer be "fetch" in undici, but "innerFetch". Then there's also the added context argument which isn't required in the spec, but is now required in undici. Just things that will make maintaining it harder.

[ida613]

@KhafraDev thank you for bringing these concerns up! If it's any solace, all of this weirdness can be removed when tracing channel becomes available in the most commonly used versions of node.

[KhafraDev]

which is years away from being stable and available in all LTS versions

unless I'm missing something - are we talking about Channel or TracingChannel? If the former, all versions of node that fetch supports has it.

[ida613]

TracingChannel

[mcollina]

I don't think using TracingChannel adds anything to this topic, it will have the same identical issues that @KhafraDev has highlighted. The synchronous end event will be problematic anyway.

A solution to this issue is to avoid creating a new innerFetch function, but rather emit the start event synchronously at the beginning of the function, and emit the end event right before returning, on all paths. This solution is more verbose but equivalent in term of coding. Wdyt?

[ida613]

Using TracingChannel we can keep the function like this:

async function fetch (input, init = {}) {
    return tracingChannel.tracePromise(() => {
      // regular fetch function with regular p = createDeferredPromise()
    }
  }

which avoids defining an innerFetch, passing in an extra argument and creating a new createDeferredPromise function.
Thank you for your proposed solution! I'll discuss with my team and get back to you ^ ^

[mcollina]

That is identical to using innerFetch with the same problems spec-wise: you are creating an inner anonymous function within.

The solution is #2210 (comment).

[Qard]

A solution to this issue is to avoid creating a new innerFetch function, but rather emit the start event synchronously at the beginning of the function, and emit the end event right before returning, on all paths. This solution is more verbose but equivalent in term of coding. Wdyt?

The end event should match the point at which the function synchronously returns a value, which in this case is the promise. This means the correct place would actually be before the first await.

[mcollina]

@Qard there is no await.

[Qard]

Ah, then yes, the return would be the place for it. But then why is it marked as an async function? Does it need to be that way by the spec?

To ensure the end happens on all return branches, might be cleanest to wrap everything in a try/finally. 🤔

[KhafraDev]

Does it need to be that way by the spec?

it appears as though it shouldn't be async

[ida613]

Thank you so much for all the feedback! I'm putting this PR on hold for now and will work on it at a later date (probably within the next few months)

[mcollina]

Closing in favor of #2701