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 #4176

Open
trentm opened this issue Aug 1, 2024 · 0 comments
Open

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

trentm opened this issue Aug 1, 2024 · 0 comments

Comments

@trentm
Copy link
Member

trentm commented Aug 1, 2024

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.

See equiv issue for the OTel distro here: elastic/elastic-otel-node#288

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