Skip to content

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

tmm1
Copy link

@tmm1 tmm1 commented Apr 21, 2025

Adds diagnostics similar to http/undici, to enable integration into network inspector protocol

cc #53946 (comment) @cola119

@nodejs-github-bot
Copy link
Collaborator

Review requested:

  • @nodejs/http2
  • @nodejs/net

@nodejs-github-bot nodejs-github-bot added http2 Issues or PRs related to the http2 subsystem. needs-ci PRs that need a full CI run. labels Apr 21, 2025
Copy link
Member

@RaisinTen RaisinTen left a 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.

Copy link

codecov bot commented Apr 21, 2025

Codecov Report

Attention: Patch coverage is 35.48387% with 20 lines in your changes missing coverage. Please review.

Project coverage is 90.26%. Comparing base (25842c5) to head (5e15a34).
Report is 4 commits behind head on main.

Files with missing lines Patch % Lines
lib/internal/http2/core.js 35.48% 18 Missing and 2 partials ⚠️
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     
Files with missing lines Coverage Δ
lib/internal/http2/core.js 95.10% <35.48%> (-0.39%) ⬇️

... and 412 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@Flarna
Copy link
Member

Flarna commented Apr 23, 2025

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

Copy link
Member

@Qard Qard left a 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.

@tmm1
Copy link
Author

tmm1 commented Apr 25, 2025

In researching this further, I discovered significant prior art by the nodesource team:

nodesource/nsolid@6350b95
nodesource/nsolid@63a9b6f

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
http2 Issues or PRs related to the http2 subsystem. needs-ci PRs that need a full CI run.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants