-
-
Notifications
You must be signed in to change notification settings - Fork 31.4k
http2: add diagnostics_channel for client session and streams #57955
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
base: main
Are you sure you want to change the base?
Conversation
Review requested:
|
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 to see you contributing to Node.js Aman :)
The new diagnostic channels need to be documented and this also needs some tests.
You can see #55622 for reference.
Codecov ReportAttention: 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 |
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.
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. |
Adds diagnostics similar to http/undici, to enable integration into network inspector protocol
cc #53946 (comment) @cola119