GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — lib: add diagnostics_channel events to module loading by Qard · Pull Request #44340 · nodejs/node

Conversation

@Qard

@Qard Qard commented Aug 22, 2022

Copy link
Copy Markdown
Member

Adding module.cjs.load and module.esm.load diagnostics_channel events so APMs don't need to patch the module system to know when a particular module has been loaded.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
dygabo, trevnorris, and astuyve reacted with thumbs up emoji bricss and trevnorris reacted with eyes emoji
All reactions
  • 👍 3 reactions
  • 👀 2 reactions
@Qard Qard added module Issues and PRs related to the module subsystem. semver-minor PRs that contain new features and should be released in the next minor version. esm Issues and PRs related to the ECMAScript Modules implementation. loaders Issues and PRs related to ES module loaders. diagnostics_channel Issues and PRs related to the diagnostics_channel module. labels Aug 22, 2022
@nodejs-github-bot

nodejs-github-bot commented Aug 22, 2022

Copy link
Copy Markdown
Collaborator

Review requested:

  • @nodejs/modules
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@nodejs-github-bot nodejs-github-bot added the needs-ci PRs that need a full CI run. label Aug 22, 2022
simon-id
simon-id reviewed Aug 22, 2022
View reviewed changes
Comment thread lib/internal/modules/cjs/loader.js Resolved

Uh oh!

There was an error while loading. Please reload this page.
@bmeck

bmeck commented Aug 22, 2022 via email

Copy link
Copy Markdown
Member
I'd be more comfortable not sending something that will be directly patched as it won't work for ESM
…
On Mon, Aug 22, 2022, 7:39 AM Stephen Belanger ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In lib/internal/modules/cjs/loader.js <#44340 (comment)>: > @@ -797,6 +800,7 @@ Module._load = function(request, parent, isMain) { throw new ERR_UNKNOWN_BUILTIN_MODULE(request); } + onModuleLoad.publish(module); The hasSubscribers check is only relevant if you're building input objects. If you just pass stuff in as-is it will be a no-op when there's no subscribers. Do we want to match anyway though just for consistency? — Reply to this email directly, view it on GitHub <#44340 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AABZJI2NJF3NWVW462NJJ3TV2NYJNANCNFSM57HMHFCQ> . You are receiving this because you are on a team that was mentioned.Message ID: ***@***.***>
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@GeoffreyBooth

GeoffreyBooth commented Aug 22, 2022

Copy link
Copy Markdown
Member

Since the ESM loader can also load CommonJS, should the event perhaps just be "module load" without the specific of whether it's ESM or CommonJS? I assume there's a payload with details like what module was loaded (?) so that could be where we specify which loader was used, which format the file was recognized as, etc.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
GeoffreyBooth
GeoffreyBooth reviewed Aug 22, 2022
View reviewed changes
Comment thread test/parallel/test-diagnostics-channel-module-esm-load.js Outdated

@GeoffreyBooth GeoffreyBooth Aug 22, 2022

Copy link
Copy Markdown
Member

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.

Can you write a test in ESM (in an .mjs file) to confirm that works too? What about static import statements?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@Qard Qard Aug 23, 2022

Copy link
Copy Markdown
Member Author

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.

Not sure how that would work. Wouldn't the module graph already be resolved/loaded by the time any code runs to actually register a subscriber? Maybe it could work in a loader?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@GeoffreyBooth GeoffreyBooth Aug 23, 2022

Copy link
Copy Markdown
Member

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.

That's why I asked 😄

We should add a test that shows expected esm behavior.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@simon-id

simon-id commented Aug 23, 2022

Copy link
Copy Markdown
Contributor

Don't forget to add it to the docs here: https://nodejs.org/api/diagnostics_channel.html#built-in-channels
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@rochdev

rochdev commented Aug 23, 2022

Copy link
Copy Markdown
Contributor

Could more events be added? There are use cases where having just one event is not enough, for example capturing how long each require call took which would require 2 events surrounding the function. Also including more data than just the module would be helpful, for example the path to the entry point, whether it's a core module, the base path, etc. Basically most of what require-in-the-middle has been doing historically, except the version since that's specific to package managers. Basically I think there could be 3 events: 1 when require is called, 1 when it finishes, and 1 when the module is actually loaded with module resolution information included as well.
dygabo reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@trevnorris

trevnorris commented Aug 25, 2022

Copy link
Copy Markdown
Contributor

@Qard Thanks for doing this. I just want to let you know that this would cover all the use cases for the data we've been collecting internally.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
jasnell
jasnell previously approved these changes Aug 28, 2022
View reviewed changes
@rochdev rochdev mentioned this pull request Sep 21, 2022
fs plugin support DataDog/dd-trace-js#2395
Closed
@trevnorris

trevnorris commented Sep 27, 2022

Copy link
Copy Markdown
Contributor

@Qard Would you mind fixing the lint error so all the tests can pass?
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@Qard

Qard commented Sep 27, 2022

Copy link
Copy Markdown
Member Author

There's some more changes I've been meaning to make. I'll try to get to that after NodeConf EU. Just had a very busy few weeks. 😅
trevnorris reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@GeoffreyBooth

GeoffreyBooth commented Sep 28, 2022

Copy link
Copy Markdown
Member

There’s some more changes I’ve been meaning to make. I’ll try to get to that after NodeConf EU. Just had a very busy few weeks. 😅

Thanks. Let’s also please not land this until all open questions are resolved.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@Qard

Qard commented Oct 18, 2022 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member Author

Going to redo this on #44943 after it lands.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@GeoffreyBooth

GeoffreyBooth commented Oct 18, 2022

Copy link
Copy Markdown
Member

Going to redo this on #44943 after it lands.

Thanks. Will that other PR enable things like I mentioned in #44340 (comment), where we could have more generic events like “module load” and then additional properties for extra data like module specifier, module system (CommonJS or ESM), resolved path, etc.?
Qard reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@Trott
Trott force-pushed the main branch from 2d76238 to ca3ed36 Compare November 12, 2022 01:49
@Qard
Qard force-pushed the diagnostics-channel-for-module-loads branch 5 times, most recently from 92affe4 to 99f0481 Compare December 15, 2022 06:16
@Qard

Qard commented Dec 15, 2022

Copy link
Copy Markdown
Member Author

I've reimplemented this on top of #44943. Ignore the first commit, that's inherited from that PR. I'll rebase after that lands to fix this PR. :)
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@GeoffreyBooth

GeoffreyBooth commented Dec 15, 2022

Copy link
Copy Markdown
Member

FYI there's a lot of refactoring in progress for module loading, including starting to merge together the CJS and ESM loaders. I would wait until the dust settles on that work before landing this. I would also suggest you make the events less tied to subsystem: like module resolution, module loading; whether it was from require or import, etc.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@Qard
Qard force-pushed the diagnostics-channel-for-module-loads branch from 99f0481 to c565e0e Compare December 15, 2022 06:32
@RafaelGSS RafaelGSS added the commit-queue PRs queued for automated landing through the Commit Queue. label Jun 20, 2024
@nodejs-github-bot nodejs-github-bot added commit-queue-failed PRs whose Commit Queue landing failed and need manual intervention before retrying. and removed commit-queue PRs queued for automated landing through the Commit Queue. labels Jun 20, 2024
@nodejs-github-bot

nodejs-github-bot commented Jun 20, 2024

Copy link
Copy Markdown
Collaborator
Commit Queue failed
- Loading data for nodejs/node/pull/44340
✔  Done loading data for nodejs/node/pull/44340
----------------------------------- PR info ------------------------------------
Title      lib: add diagnostics_channel events to module loading (#44340)
Author     Stephen Belanger  (@Qard)
Branch     Qard:diagnostics-channel-for-module-loads -> nodejs:main
Labels     module, semver-minor, esm, author ready, needs-ci, loaders, diagnostics_channel, commit-queue-squash
Commits    1
 - lib: add diagnostics_channel events to module loading
Committers 1
 - RafaelGSS 
PR-URL: https://github.com/nodejs/node/pull/44340
Reviewed-By: Geoffrey Booth 
Reviewed-By: Santiago Gimeno 
Reviewed-By: Rafael Gonzaga 
Reviewed-By: James M Snell 
Reviewed-By: Gerhard Stöbich 
------------------------------ Generated metadata ------------------------------
PR-URL: https://github.com/nodejs/node/pull/44340
Reviewed-By: Geoffrey Booth 
Reviewed-By: Santiago Gimeno 
Reviewed-By: Rafael Gonzaga 
Reviewed-By: James M Snell 
Reviewed-By: Gerhard Stöbich 
--------------------------------------------------------------------------------
   ⚠  Commits were pushed since the last approving review:
   ⚠  - lib: add diagnostics_channel events to module loading
   ℹ  This PR was created on Mon, 22 Aug 2022 11:57:11 GMT
   ✔  Approvals: 5
   ✔  - Geoffrey Booth (@GeoffreyBooth) (TSC): https://github.com/nodejs/node/pull/44340#pullrequestreview-2126574661
   ✔  - Santiago Gimeno (@santigimeno): https://github.com/nodejs/node/pull/44340#pullrequestreview-2126639621
   ✔  - Rafael Gonzaga (@RafaelGSS) (TSC): https://github.com/nodejs/node/pull/44340#pullrequestreview-2126661939
   ✔  - James M Snell (@jasnell) (TSC): https://github.com/nodejs/node/pull/44340#pullrequestreview-2128947843
   ✔  - Gerhard Stöbich (@Flarna): https://github.com/nodejs/node/pull/44340#pullrequestreview-2129682835
   ✔  Last GitHub CI successful
   ℹ  Last Full PR CI on 2024-06-20T17:43:17Z: https://ci.nodejs.org/job/node-test-pull-request/59906/
- Querying data for job/node-test-pull-request/59906/
   ✔  Last Jenkins CI successful
--------------------------------------------------------------------------------
   ✔  Aborted `git node land` session in /home/runner/work/node/node/.ncu
https://github.com/nodejs/node/actions/runs/9605921698
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@RafaelGSS RafaelGSS removed the commit-queue-failed PRs whose Commit Queue landing failed and need manual intervention before retrying. label Jun 21, 2024
RafaelGSS added a commit that referenced this pull request Jun 21, 2024
@RafaelGSS @Qard
lib: add diagnostics_channel events to module loading
fbdfe93 
This commit adds a tracing channel for module loading
through `import()` and `require()`.

Co-Authored-By: Stephen Belanger <[email protected]>
PR-URL: #44340
Reviewed-By: Geoffrey Booth <[email protected]>
Reviewed-By: Santiago Gimeno <[email protected]>
Reviewed-By: Rafael Gonzaga <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Gerhard Stöbich <[email protected]>
@RafaelGSS

RafaelGSS commented Jun 21, 2024

Copy link
Copy Markdown
Member

Landed in fbdfe93
Qard reacted with hooray emoji
All reactions
  • 🎉 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@RafaelGSS RafaelGSS closed this Jun 21, 2024
targos pushed a commit that referenced this pull request Jun 21, 2024
@RafaelGSS @Qard @targos
lib: add diagnostics_channel events to module loading
e95af74 
This commit adds a tracing channel for module loading
through `import()` and `require()`.

Co-Authored-By: Stephen Belanger <[email protected]>
PR-URL: #44340
Reviewed-By: Geoffrey Booth <[email protected]>
Reviewed-By: Santiago Gimeno <[email protected]>
Reviewed-By: Rafael Gonzaga <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Gerhard Stöbich <[email protected]>
targos added a commit that referenced this pull request Jun 25, 2024
@targos
2024-06-26, Version 22.4.0 (Current)
8efb279 
Notable changes:

deps,lib,src:
  * (SEMVER-MINOR) add experimental web storage (Colin Ihrig) #52435
doc:
  * move `node --run` stability to rc (Yagiz Nizipli) #53433
  * mark WebSocket as stable (Matthew Aitken) #53352
  * mark --heap-prof and related flags stable (Joyee Cheung) #53343
  * mark --cpu-prof and related flags stable (Joyee Cheung) #53343
  * doc-only deprecate OpenSSL engine-based APIs (Richard Lau) #53329
inspector:
  * fix disable async hooks on Debugger.setAsyncCallStackDepth (Joyee Cheung) #53473
lib:
  * (SEMVER-MINOR) add diagnostics_channel events to module loading (RafaelGSS) #44340
util:
  * (SEMVER-MINOR) support `--no-` for argument with boolean type for parseArgs (Zhenwei Jin) #53107

PR-URL: #53583
@targos targos mentioned this pull request Jun 25, 2024
v22.4.0 release proposal #53583
Merged
targos added a commit that referenced this pull request Jun 26, 2024
@targos
2024-06-26, Version 22.4.0 (Current)
e7e7e93 
Notable changes:

deps,lib,src:
  * (SEMVER-MINOR) add experimental web storage (Colin Ihrig) #52435
doc:
  * move `node --run` stability to rc (Yagiz Nizipli) #53433
  * mark WebSocket as stable (Matthew Aitken) #53352
  * mark --heap-prof and related flags stable (Joyee Cheung) #53343
  * mark --cpu-prof and related flags stable (Joyee Cheung) #53343
  * doc-only deprecate OpenSSL engine-based APIs (Richard Lau) #53329
inspector:
  * fix disable async hooks on Debugger.setAsyncCallStackDepth (Joyee Cheung) #53473
lib:
  * (SEMVER-MINOR) add diagnostics_channel events to module loading (RafaelGSS) #44340
util:
  * (SEMVER-MINOR) support `--no-` for argument with boolean type for parseArgs (Zhenwei Jin) #53107

PR-URL: #53583
targos added a commit that referenced this pull request Jul 1, 2024
@targos
2024-07-02, Version 22.4.0 (Current)
7e13cc5 
Notable changes:

deps,lib,src:
  * (SEMVER-MINOR) add experimental web storage (Colin Ihrig) #52435
doc:
  * move `node --run` stability to rc (Yagiz Nizipli) #53433
  * mark WebSocket as stable (Matthew Aitken) #53352
  * mark --heap-prof and related flags stable (Joyee Cheung) #53343
  * mark --cpu-prof and related flags stable (Joyee Cheung) #53343
  * doc-only deprecate OpenSSL engine-based APIs (Richard Lau) #53329
inspector:
  * fix disable async hooks on Debugger.setAsyncCallStackDepth (Joyee Cheung) #53473
lib:
  * (SEMVER-MINOR) add diagnostics_channel events to module loading (RafaelGSS) #44340
util:
  * (SEMVER-MINOR) support `--no-` for argument with boolean type for parseArgs (Zhenwei Jin) #53107

PR-URL: #53583
targos added a commit that referenced this pull request Jul 2, 2024
@targos
2024-07-02, Version 22.4.0 (Current)
056f055 
Notable changes:

deps,lib,src:
  * (SEMVER-MINOR) add experimental web storage (Colin Ihrig) #52435
doc:
  * move `node --run` stability to rc (Yagiz Nizipli) #53433
  * mark WebSocket as stable (Matthew Aitken) #53352
  * mark --heap-prof and related flags stable (Joyee Cheung) #53343
  * mark --cpu-prof and related flags stable (Joyee Cheung) #53343
  * doc-only deprecate OpenSSL engine-based APIs (Richard Lau) #53329
inspector:
  * fix disable async hooks on Debugger.setAsyncCallStackDepth (Joyee Cheung) #53473
lib:
  * (SEMVER-MINOR) add diagnostics_channel events to module loading (RafaelGSS) #44340
util:
  * (SEMVER-MINOR) support `--no-` for argument with boolean type for parseArgs (Zhenwei Jin) #53107

PR-URL: #53583
@Karliz24 Karliz24 mentioned this pull request Jul 9, 2024
## 2024-07-02, Version 22.4.0 (Current), @targos Karliz24/Marley-#16
Open
@targos targos added the dont-land-on-v20.x label Sep 21, 2024
@targos targos mentioned this pull request Oct 10, 2024
Wrong error annotation when commonjs requires an ES module #55350
Open
@RafaelGSS RafaelGSS mentioned this pull request Oct 31, 2024
module: fix error reporting #55561
Closed
@joyeecheung

joyeecheung commented Feb 6, 2025

Copy link
Copy Markdown
Member

Tentatively backport in #56927 to reduce the amount of conflicts.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@joyeecheung joyeecheung added backport-open-v20.x and removed dont-land-on-v20.x labels Feb 6, 2025
joyeecheung pushed a commit to joyeecheung/node that referenced this pull request Feb 6, 2025
@RafaelGSS @Qard @joyeecheung
lib: add diagnostics_channel events to module loading
2eb0cb2 
This commit adds a tracing channel for module loading
through `import()` and `require()`.

Co-Authored-By: Stephen Belanger <[email protected]>
PR-URL: nodejs#44340
Reviewed-By: Geoffrey Booth <[email protected]>
Reviewed-By: Santiago Gimeno <[email protected]>
Reviewed-By: Rafael Gonzaga <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Gerhard Stöbich <[email protected]>
@joyeecheung joyeecheung added dont-land-on-v20.x and removed backport-open-v20.x labels Feb 6, 2025
@joyeecheung

joyeecheung commented Feb 6, 2025

Copy link
Copy Markdown
Member

Backing it out from #56927 to avoid introducing surface for regressions.
simon-id reacted with eyes emoji
All reactions
  • 👀 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@ljharb ljharb ljharb left review comments
@mcollina mcollina mcollina left review comments
@RafaelGSS RafaelGSS RafaelGSS approved these changes
@jasnell jasnell jasnell approved these changes
@GeoffreyBooth GeoffreyBooth GeoffreyBooth approved these changes
@santigimeno santigimeno santigimeno approved these changes
@Flarna Flarna Flarna approved these changes
+2 more reviewers
@tlhunter tlhunter tlhunter left review comments
@simon-id simon-id simon-id left review comments
Reviewers whose approvals may not affect merge requirements

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

author ready PRs with CI started, the required approvals, and no outstanding review comments. commit-queue-squash PRs the Commit Queue should land as one squashed commit. diagnostics_channel Issues and PRs related to the diagnostics_channel module. esm Issues and PRs related to the ECMAScript Modules implementation. loaders Issues and PRs related to ES module loaders. module Issues and PRs related to the module subsystem. needs-ci PRs that need a full CI run. semver-minor PRs that contain new features and should be released in the next minor version.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

17 participants

@Qard @nodejs-github-bot @bmeck @GeoffreyBooth @simon-id @rochdev @trevnorris @aduh95 @RafaelGSS @Flarna @joyeecheung @ljharb @mcollina @jasnell @tlhunter @santigimeno @targos
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.