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

fix(otlp-exporter-base): use dynamic import instead of require in esm/esnext build #5220

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

Conversation

pichlermarc
Copy link
Member

@pichlermarc pichlermarc commented Nov 29, 2024

Which problem is this PR solving?

When bundling with rollup, the compiled files from the ESM build are used. When the bundle is run, require is not defined because it's ESM - also the file it tries to require is not there since it's a bundle now.

This is the case because we have a workaround in the HttpExporterTransport to lazy load a file that requires http until we actually export. We need this workaround to ensure that the @opentelemetry/instrumentation-http package can properly wrap everything - I'm not sure exactly how much of that is actually needed so I'll follow up with some more investigation after we get this fix in.

This PR changes the require() to an await import(), which compiles to

  • require() wrapped in a Promise.resolve() for CJS
  • import() for esm/esnext

These types of problems are actually prime candidates to test when we start working on

While this PR does not add any additional tests, it will unblock bundling with rollup for now. We should, however, get started on #4744 eventually to have this tested in an automated manner.

Type of change

  • Bug fix (non-breaking change which fixes an issue)

How Has This Been Tested?

Copy link

codecov bot commented Nov 29, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 94.59%. Comparing base (484af40) to head (0199730).

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #5220      +/-   ##
==========================================
- Coverage   94.60%   94.59%   -0.02%     
==========================================
  Files         315      315              
  Lines        8011     8011              
  Branches     1617     1617              
==========================================
- Hits         7579     7578       -1     
- Misses        432      433       +1     
Files with missing lines Coverage Δ
...rter-base/src/transport/http-exporter-transport.ts 93.75% <100.00%> (ø)

... and 1 file with indirect coverage changes

@pichlermarc pichlermarc added bug Something isn't working pkg:otlp-exporter-base labels Dec 2, 2024
@pichlermarc pichlermarc marked this pull request as ready for review December 6, 2024 14:18
@pichlermarc pichlermarc requested a review from a team as a code owner December 6, 2024 14:18
Copy link
Contributor

@raphael-theriault-swi raphael-theriault-swi left a comment

Choose a reason for hiding this comment

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

In my experience importing a core module before hooking doesn't actually cause any issues and it gets instrumented fine, we use fs before anything else to load a config file and it's never been a problem.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working pkg:otlp-exporter-base
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants