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

feat: ESM support for instrumentation #3698

Merged
merged 46 commits into from
May 15, 2023

Conversation

JamieDanielson
Copy link
Member

@JamieDanielson JamieDanielson commented Mar 22, 2023

Co-authored-by: Purvi Kanal <purvikanal@honeycomb.io>

Which problem is this PR solving?

Updates / Closes #1946

Short description of the changes

ECMAScript Modules (ESM) have become the standard module system for Node.js, but CommonJS (CJS) is also still used widely, so we need a way to enable instrumentation for both module systems. This is built upon / supersedes the work of previous PRs and discussions (#2640, #2763, #2846) .

  • Add import-in-the-middle (IITM) as another hook, to be used instead of require-in-the-middle (RITM) when the argument is passed to use the new hook --experimental-loader=@opentelemetry/instrumentation/hook.mjs (alternative option includes the ability to use --experimental-loader=import-in-the-middle/hook.mjs with an added dependency on import-in-the-middle)
  • Add test for ESM instrumentation that is run with npm run test:esm and as part of npm run test
  • Modify wrapping and unwrapping functionality to handle Proxy objects from IITM
  • Add example app esm-http-ts built in TypeScript that compiles to ESM and shows http instrumentation working
  • Adjust http instrumentation to get process.version to prevent undefined version for ESM.

Type of change

  • New feature (non-breaking change which adds functionality)
  • This change requires a documentation update

How Has This Been Tested?

  • Run the new esm-http-ts example app to see http-instrumented span and manual span
  • Unit test(s)
  • This is now being used in eve-val/eve-roster, a project using Yarn PNP. Here is where it uses the loader, and here is where it's using a tarball of this package. Note this requires loader chaining to work with Yarn PNP and this fix for loading, which is currently only available in Node19, but should be being backported to Node18. These loader-specific issues should not be a concern in regular npm, pnpm, or yarn legacy mode.

eve-using-esm-honeycomb

Checklist:

  • Followed the style guidelines of this project
  • Unit tests have been added
  • Documentation has been updated

JamieDanielson and others added 6 commits March 21, 2023 10:48
also, `this` doesnt work outside scope with esm
iitm added to ritm singleton
tests stubbed to keep current behavior

Co-authored-by: Purvi Kanal <kanal.purvi@gmail.com>
doesn't currently work with iitm register
@codecov
Copy link

codecov bot commented Mar 22, 2023

Codecov Report

Merging #3698 (d9f329b) into main (bba09c0) will increase coverage by 0.02%.
The diff coverage is 100.00%.

❗ Current head d9f329b differs from pull request most recent head 0167a10. Consider uploading reports for the commit 0167a10 to get more accurate results

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3698      +/-   ##
==========================================
+ Coverage   93.19%   93.22%   +0.02%     
==========================================
  Files         298      293       -5     
  Lines        9057     8883     -174     
  Branches     1864     1825      -39     
==========================================
- Hits         8441     8281     -160     
+ Misses        616      602      -14     
Impacted Files Coverage Δ
...ges/opentelemetry-instrumentation-http/src/http.ts 93.31% <100.00%> (+0.02%) ⬆️

... and 6 files with indirect coverage changes

@weyert
Copy link
Contributor

weyert commented Mar 30, 2023

Thank you for working on this! :)

@gabrielemontano
Copy link

@JamieDanielson my personal superheroine!!!

@JamieDanielson JamieDanielson changed the title WIP feat: ESM support for instrumentation feat: ESM support for instrumentation Apr 4, 2023
@JamieDanielson JamieDanielson marked this pull request as ready for review April 4, 2023 18:13
@JamieDanielson JamieDanielson requested a review from a team April 4, 2023 18:13
@lizthegrey
Copy link
Member

omg, wahoo, y'all got it over the line @JamieDanielson and @pkanal!!!

@JamieDanielson
Copy link
Member Author

@JamieDanielson my personal superheroine!!!

That's very kind but I definitely cannot take all this credit. It's taken a lot of work from a lot of people, and this latest iteration has a ton of work done by my colleague Purvi (@pkanal) - it just only allows one name on the PR in GH 😃

@vmarchaud
Copy link
Member

I'll also re-link the comment that Qard made over my PR #2846 (comment) which is still relevant to this one

pkanal added 2 commits May 9, 2023 16:17
- pins versions in example app
- node 20 support disclaimer
- re-export hook.mjs file functions 😍
@JamieDanielson
Copy link
Member Author

I don't have the ability to rerun workflows but I think the current status check failures may be related to some service issues GitHub is having today, as I've had similar-looking fails on other workflows that resolved when rerunning.

@Flarna
Copy link
Member

Flarna commented May 11, 2023

@JamieDanielson seems my PR created a conflict in readme.

Please update the limitations accordingly because once this PR is merge one limitation is gone (besides for node 20) 🎉

Copy link
Member

@pichlermarc pichlermarc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, thank you for putting in all the work. 🙂
As this is quite a significant change, I'll wait for another approval before merging this.

As the ESM module loader from NodeJS is experimental, so is our support for it. Feel free to provide feedback or report issues about it.

**Note**: ESM Instrumentation is not yet supported for Node 20.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please update the limitations section below which tells ECMA script modules (using import) is not supported as of now.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what does this mean? How can one use ESM without import?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It means that the limitations section requires changes in this PR because it removes one of them.
Before this PR ESM/import was not supported and only CJS/require worked.
Th limitation with bundlers stays.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

huh, super, thanks for the clarification :) a panic mode was already about to arise :D

@pichlermarc pichlermarc merged commit 422a36a into open-telemetry:main May 15, 2023
@MikeGoldsmith MikeGoldsmith deleted the esm-support branch May 15, 2023 12:22
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

Successfully merging this pull request may close these issues.

Question: Plans for Node.js ESM Module Support?