Skip to content

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

Closed
pichlermarc wants to merge 10 commits intoopen-telemetry:mainfrom
dynatrace-oss-contrib:fix/http-dynamic-import-workaround
Closed

fix(otlp-exporter-base): use dynamic import instead of require in esm/esnext build#5220
pichlermarc wants to merge 10 commits intoopen-telemetry:mainfrom
dynatrace-oss-contrib:fix/http-dynamic-import-workaround

Conversation

@pichlermarc
Copy link
Copy Markdown
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?

@codecov
Copy link
Copy Markdown

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
Copy Markdown
Member

@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.

@pichlermarc
Copy link
Copy Markdown
Member Author

Closing this as there's no way I can make this work with the current tests.

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.

@raphael-theriault-swi - I like that thought. Getting rid of this lazy loading would be amazing.
I did carry this code forward from the old implementation when I re-implemented that code as I was assuming that it'd break @opentelemetry/instrumentation-http, but there were no tests to prove that it would actually break @opentelemetry/instrumentation-http.

It'd be interesting to have a sample app that shows that it works without lazy-loading and @opentelemetry/instrumentation-http registered after importing the exporter (also an example with a bundled app, where http might've been imported before through the exporter and is then used by end-user code).

If we'd have such an example that shows that it does not impact the instrumentation at all, I'd gladly throw this code. Having to lazy load happens to be a major pain to everyone involved, and also blocks features such as an HTTP agent override (where importing from the HTTP module is required),

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