-
Notifications
You must be signed in to change notification settings - Fork 503
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Adding diagnostics channels to Fetch #2701
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you please undo the indent changes please
With the addition of the try block the indentations were necessary, excluding them the commit will result in lint errors. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I missed that.. why is the extra try block needed? Wouldn't it be possible to add a .finally() to the p.promise instead?
Yes, I can add the finally to the promise, though the initial start event is placed in the first try block |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good job. Some nits.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm
Can you resolve the conflicts with |
@mcollina conflicts have been resolved 👍🏼 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm
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! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm
cc @metcoder95
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minor detail, and LGTM 👍
Thanks for the great work and keeping up with us! |
@@ -135,114 +191,115 @@ function fetch (input, init = undefined) { | |||
try { | |||
requestObject = new Request(input, init) | |||
} catch (e) { | |||
p.reject(e) | |||
return p.promise | |||
return Promise.reject(e) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
1 microtask delayed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@tsctx I am not sure what you mean by this, can you elaborate?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
new Promise((_, reject) => reject(v))
is rejected synchronously, while Promise.reject(v)
is rejected asynchronously (after one microtask). It must be rejected synchronously as per the spec.
1c1ac66
to
b9dc1ff
Compare
looking forward to this :) |
@tsctx ptal |
Please answer this. #2701 (comment) |
This looks cool, should we not maybe prefix the new channel with Also looking at the code it looks like we don't capture the start of fetching when it actually occurs? As in we perform all the trace calls when the promise is settled. |
@tsctx Which document are you referring to? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I won't block but I am a huge -1 on this. "Node-ifying" fetch will turn it into one of the other half-dozen implementations node has that aren't really maintainable or maintained.
@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? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks reasonable to me
@@ -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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
if (hasSubscribers) { | ||
const context = { req, input, init, result: null, error: null } | ||
|
||
return channels.start.runStores(context, () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't it run traceSync
or tracePromise
instead?
cc: @Qard
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. |
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