Skip to content
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.

Sign up for GitHub

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

Jump to bottom

inspector: add Network.Initiator in inspector protocol #56805

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

inspector: add Network.Initiator in inspector protocol #56805

wants to merge 2 commits into from
+149 −55

Conversation

legendecas
Copy link
Member

Add initiator stack trace in inspector network events, reflecting
the location where the script created the request.

The http.client.request.created event is closer to where user code
creates the http request, and correctly reflects which script
initiated the request.

Refs: #53946

@nodejs-github-bot
Copy link
Collaborator

Review requested:

  • @nodejs/gyp

@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. lib / src Issues and PRs related to general changes in the lib or src directory. needs-ci PRs that need a full CI run. labels Jan 28, 2025
@legendecas legendecas added inspector Issues and PRs related to the V8 inspector protocol c++ Issues and PRs that require attention from people who are familiar with C++. lib / src Issues and PRs related to general changes in the lib or src directory. needs-ci PRs that need a full CI run. and removed c++ Issues and PRs that require attention from people who are familiar with C++. lib / src Issues and PRs related to general changes in the lib or src directory. needs-ci PRs that need a full CI run. labels Jan 28, 2025
@codecov Codecov
Copy link

codecov bot commented Jan 28, 2025 •
edited
Loading

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 89.17%. Comparing base (0aa5874) to head (314077c).
Report is 101 commits behind head on main.

Additional details and impacted files 
@@            Coverage Diff             @@
##             main   #56805      +/-   ##
==========================================
- Coverage   89.22%   89.17%   -0.05%     
==========================================
  Files         663      665       +2     
  Lines      191974   192581     +607     
  Branches    36916    37081     +165     
==========================================
+ Hits       171286   171734     +448     
- Misses      13558    13656      +98     
- Partials     7130     7191      +61
Files with missing lines Coverage Δ
lib/internal/inspector/network_http.js 94.69% <100.00%> (ø)
lib/internal/inspector/network_undici.js 93.61% <100.00%> (ø)
src/inspector/network_agent.cc 95.19% <100.00%> (+0.29%) ⬆️
src/inspector/network_inspector.cc 90.00% <100.00%> (ø)
src/inspector/network_inspector.h 100.00% <ø> (ø)
src/inspector_agent.cc 79.77% <100.00%> (+0.02%) ⬆️

... and 78 files with indirect coverage changes

@legendecas legendecas added the request-ci Add this label to start a Jenkins CI on a PR. label Jan 29, 2025
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Jan 29, 2025
@nodejs-github-bot

This comment was marked as outdated.

Sign in to view
@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/64820/

@legendecas
Copy link
Member Author

The CI failure on SmartOS seems related to the change, will take a look

@anonrig
Copy link
Member

anonrig commented Jan 31, 2025

cc @nodejs/platform-smartos

@legendecas
Copy link
Member Author

legendecas commented Jan 31, 2025 •
edited
Loading

It seems like the build dependencies are not respected while building on SmartOS. libnode depends on v8_snapshot, which depends on v8_base_without_compiler. The V8 inspector headers are generated in target v8_base_without_compiler. But on SmartOS, it is reporting that the generated v8 header inspector/Debugger.h is missing when building libnode's src/inspector_agent.cc.

@anonrig
Copy link
Member

anonrig commented Jan 31, 2025

It seems like the build dependencies are not respected while building on SmartOS. libnode depends on v8_snapshot, which depends on v8_base_without_compiler. The V8 inspector headers are generated in target v8_base_without_compiler. But on SmartOS, it is reporting that the generated v8 header inspector/Debugger.h is missing when building libnode's src/inspector_agent.cc.

@legendecas Can you open an issue about this?

@legendecas legendecas added the request-ci Add this label to start a Jenkins CI on a PR. label Feb 4, 2025
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Feb 4, 2025
@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/64982/

@legendecas legendecas force-pushed the inspector/network-initiator branch from f2e24d0 to 524bb64 Compare February 4, 2025 15:09
@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/64985/

@legendecas
src: add a hard dependency v8_inspector_headers
d0bf9dd 
A GYP hard dependency instructs GYP to not remove the dependency link
between two static library targets in its generated output. This
allows V8 dependents to include V8 inspector protocol headers.
@legendecas
inspector: add Network.Initiator in inspector protocol
314077c 
Add initiator stack trace in inspector network events, reflecting
the location where the script created the request.

The `http.client.request.created` event is closer to where user code
creates the http request, and correctly reflects which script
initiated the request.
@legendecas legendecas force-pushed the inspector/network-initiator branch from 524bb64 to 314077c Compare February 5, 2025 21:48
@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/65014/

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@anonrig anonrig anonrig left review comments

@jasnell jasnell jasnell approved these changes

Assignees
No one assigned
Labels
c++ Issues and PRs that require attention from people who are familiar with C++. inspector Issues and PRs related to the V8 inspector protocol lib / src Issues and PRs related to general changes in the lib or src directory. needs-ci PRs that need a full CI run.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@legendecas @nodejs-github-bot @anonrig @jasnell