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.

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

use IITM wrap-only-hooked support for ESM #288

Open
trentm opened this issue Jul 16, 2024 · 0 comments
Open

use IITM wrap-only-hooked support for ESM #288

trentm opened this issue Jul 16, 2024 · 0 comments

Comments

@trentm
Copy link
Member

trentm commented Jul 16, 2024

IITM v1.9.0 added the ability to specify includes/excludes to the module.register(...) call. See:
https://github.com/nodejs/import-in-the-middle/pull/124/files#diff-b335630551682c19a781afebcf4d07bf978fb1f8ac04c6bf87428ed5106870f5R56-R71

I'm curious if this could be used to avoid IITM wrapping of most loaded modules, but passing in a hardcoded list of includes that matches all the possible instrumentations included in the distro. This would need to be careful to not break future extensions usage, current programmatic usage of the ElasticNodeSDK that adds additional instrumentations, etc.

It may be that the IITM wrapping of un-instrumented modules isn't a real perf concern. Need to measure.


Update: IITM v1.11.0 includes support for only wrapping modules that are to be Hooked: https://github.com/nodejs/import-in-the-middle?tab=readme-ov-file#only-intercepting-hooked-modules
This avoid possible (even inevitable) issues with the best-effort wrapping that IITM does. If we only wrap the modules that we are going to hook for instrumentation (a known set that we test against), then we can be sure IITM isn't going to cause issues with other modules loaded in the user's app.

A possible benefit is a perf benefit as well, mentioned above. "Possible" because I haven't measured to see if this is at all significant overhead.

  • Changing to wrap-only-hooked requires using module.register(...) so the registerOptions can be passed through. This means moving away from recommending --experimental-loader=.../hook.mjs at all for ESM support.
  • Full usage of this IITM support means using top-level await, which means we now should recommend using --import ... over --require ... for starting the agent. (The race that motivated the await waitForAllMessagesAcknowledged() is, IIUC, theoretical, so using --require ... will probably still work FWIW.)
  • This basically also means that we say ESM support is unsupported in earlier versions of Node.js that don'e have module.register(...) and --import .... The required min Node.js versions for ESM support would then be: ^18.19.0 || >=20.6.0.
@trentm trentm changed the title investigate using include/exclude IITM option to improve ESM instrumentation perf use IITM wrap-only-hooked support Aug 1, 2024
@trentm trentm changed the title use IITM wrap-only-hooked support use IITM wrap-only-hooked support for ESM Aug 1, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

1 participant