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! |
|
@Uzlopak @KhafraDev ptal |
lib/core/diagnostics.js
Outdated
| @@ -114,6 +122,75 @@ if (undiciDebugLog.enabled || fetchDebuglog.enabled) { | |||
| isClientSet = true | |||
| } | |||
|
|
|||
| // Track fetch requests | |||
| if (fetchDebuglog.enabled) { | |||
There was a problem hiding this comment.
This needs to check for the existence of tracing channel
|
I promise, i will have a look at it tonight (german time). Sorry for the delays. I was the last three weeks busy with taking care of administrative stuff. |
|
We have made additional updates to the start/end/error publish events, we are now calling runStores with these events. We have extracted all the diagnostic channel events code to a function which wraps the body of fetch. |
|
@Uzlopak @KhafraDev Any other concerns with the PR? |
|
tests seems to be failing, plus there is a conflict now |
crysmags
force-pushed
the
fetch_channel_tracing
branch
from
6d298a9
to
01cd0f6
Compare
|
@Uzlopak ping |
|
Sry for the mess. Accidently closed the PR with my fat fingers. My approval is just a rubberstamp, as a sign that i am not oposing this PR. |
|
Documentation has not been updated and does not match current behavior. |
There was a problem hiding this comment.
Seconding @tsctx, it seems we expanded the debuglog but we haven't document that, can we expand the documentation?
| @@ -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) | |||
crysmags
force-pushed
the
fetch_channel_tracing
branch
from
1c1ac66
to
b9dc1ff
Compare
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