Adding diagnostics channels to Fetch #2701
Conversation
With the addition of the try block the indentations were necessary, excluding them the commit will result in lint errors. |
Yes, I can add the finally to the promise, though the initial start event is placed in the first try block |
|
Can you resolve the conflicts with |
|
@mcollina conflicts have been resolved 👍🏼 |
|
Since your work started, we have migrated the tests to |
|
My proposed suggestion would migrate the test from tap to node:test ;) |
|
@Uzlopak committed the changes, thanks for that! |
|
Interesting. The unit fests found a valid bug. |
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
|
Thanks for the great work and keeping up with us! |
@tsctx Which document are you referring to? |
|
@tsctx ptal
This PR changes around 80 lines of code in the actual algorithm. It doesn't seem this is making |
|
@jasnell can you take a quick look too? |
| @@ -114,6 +122,75 @@ if (undiciDebugLog.enabled || fetchDebuglog.enabled) { | |||
| isClientSet = true | |||
| } | |||
|
|
|||
| // Track fetch requests | |||
| if (fetchDebuglog.enabled && diagnosticsChannel.tracingChannel) { | |||
There was a problem hiding this comment.
Shouldn't this code be placed by users instead of having a helper inside undici core?
|
|
||
| // subscribersCheck will be called at the beginning of the fetch call | ||
| // and will check if we have subscribers | ||
| function subscribersCheck () { |
There was a problem hiding this comment.
AFAIK tracingChannel.hasSubscribers will do all these checks for you. No need to do it manually.
https://github.com/nodejs/node/blob/main/lib/diagnostics_channel.js#L280
cc: @Qard
There was a problem hiding this comment.
The tracingChannel.hasSubscribers getter is fairly new so this needs to continue doing things the old way for awhile.
|
It absolutely will make maintaining fetch harder. |
|
How? For the most part this is just an indentation level change. |
|
It's not the code itself, it's how intrusive it is. We are taking a web spec and shoving node-specific things into it, things that are not entirely compatible with the spec (I assume that's why you said "for the most part"). There are steps that are rewritten and things added to fetch itself. Every, every single time we have even slightly deviated from the spec it always comes back to bite us in the ass later on, even the smallest change to the spec makes maintaining it harder. I can find specific examples but I hope you'll trust me on that... |
|
@crysmags I think it could be implemented with fewer changes. In the way... tsctx@56c36f7 |
Documentation of how to use the added debuglog's. |
|
The |
|
Should we do a call to discuss this? |
|
I'm happy to get on a call with folks about it, if that is needed. |
|
Here is a doodle: https://doodle.com/meeting/participate/id/dwRDjnJb. |
|
Who is also invited? Am i also invited? |
|
Yes |
|
Is it intentional that the selected dates in the Doodle don't start until July? Seems like quite awhile to wait to make a decision on this. 🤔 |
|
I'm traveling the next 2 weeks. |
|
Fair enough. We can find time after your travels then. Safe travels! 👋🏻 |
|
What's happening with this? We still planning a meeting of some sort? @mcollina |
|
Any updates? I'm looking forward to this feature for network inspection fetch support. |
|
@KhafraDev can we do a meeting next week to figure out what can be done to unblock this? There are several companies asking me to make this happen. |
|
I don't have time - the semester is ramping up for me and finals are happening in a little over a month. There are still issues I have with this PR:
The further from the spec we deviate, and the more node-specific features we implement, the harder it becomes to maintain. It also becomes harder for other runtimes to maintain compatibility. Fortunately, undici (outside of the web specs) provides a fantastic api that fits these requirements, or at least is more open to changes that would satisfy them. |
This relates to...
This is a follow up to a previous PR [Added diagnostics channels on fetch] (#2210) , this includes [proposed changes] (#2210 (comment)) to emit the start event synchronously at the beginning of the function, and emit the end event right before returning, on all paths.
Rationale
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
Changes
Added diagnostics channel to fetch
Added tests to diagnostics channel for fetch
Status