http2: add diagnostics_channel for client session and streams#57955
http2: add diagnostics_channel for client session and streams#57955tmm1 wants to merge 1 commit intonodejs:mainfrom
Conversation
|
Review requested:
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #57955 +/- ##
==========================================
- Coverage 92.26% 90.26% -2.00%
==========================================
Files 325 630 +305
Lines 126673 186155 +59482
Branches 20783 36478 +15695
==========================================
+ Hits 116869 168035 +51166
- Misses 9576 11000 +1424
- Partials 228 7120 +6892
🚀 New features to boost your workflow:
|
|
I think the new diagnostics channels should be mentioned in doc somewhere including stability level and version when they were added. e.g. see https://nodejs.org/docs/latest/api/diagnostics_channel.html#built-in-channels |
Qard
left a comment
There was a problem hiding this comment.
As already stated, needs docs.
Also, might be better to put the created channel publishes in the ClientHttp2Stream and ClientHttp2Session constructors. Looks like some other logic is not being included in that window.
|
In researching this further, I discovered significant prior art by the nodesource team: nodesource/nsolid@6350b95 They are amenable to having this work upstreamed, so I am going to revamp my PR to pull in those commits and flesh it out with docs and tests. |
|
As discussed privately with Aman, I'll send separate PRs for this with tests and docs. Starting with #58246. |
|
Next one: #58292 |
|
Another one #58306 |
Adds diagnostics similar to http/undici, to enable integration into network inspector protocol
cc #53946 (comment) @cola119