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: Replace require with import #4015

Closed
wants to merge 1 commit into from

Conversation

ddeboer
Copy link

@ddeboer ddeboer commented Jul 21, 2023

Which problem is this PR solving?

Modernise by replacing require with import.

This may also solve the following error when running tests that execute instrumented code
that has at least one exporter configured:

ReferenceError: You are trying to `import` a file after the Jest environment has been torn down.

Short description of the changes

Type of change

Please delete options that are not relevant.

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • This change requires a documentation update

How Has This Been Tested?

Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce. Please also list any relevant details for your test configuration

  • Test A

Checklist:

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

@ddeboer ddeboer requested a review from a team July 21, 2023 14:29
@linux-foundation-easycla
Copy link

linux-foundation-easycla bot commented Jul 21, 2023

CLA Signed

The committers listed above are authorized under a signed CLA.

  • ✅ login: ddeboer / name: David de Boer (30b2c5c)

Copy link
Member

@dyladan dyladan left a comment

Choose a reason for hiding this comment

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

This is done intentionally. The util is lazy loaded after application start because it requires the http module. If we require the http module, we then can't instrument it later. I believe we should be able to now for traces because of the proxy tracer which has been added to address this issue, but there still is no solution for that in metrics or logs. @pichlermarc you're more familiar with the exporters than me. Am I missing anything here?

@codecov
Copy link

codecov bot commented Jul 24, 2023

Codecov Report

Merging #4015 (207f991) into main (0f20b2a) will increase coverage by 0.01%.
The diff coverage is n/a.

❗ Current head 207f991 differs from pull request most recent head 30b2c5c. Consider uploading reports for the commit 30b2c5c to get more accurate results

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #4015      +/-   ##
==========================================
+ Coverage   92.30%   92.31%   +0.01%     
==========================================
  Files         321      321              
  Lines        9189     9189              
  Branches     1953     1953              
==========================================
+ Hits         8482     8483       +1     
+ Misses        707      706       -1     

see 1 file with indirect coverage changes

@Flarna
Copy link
Member

Flarna commented Jul 24, 2023

The ProxyTracerProvider doesn't solve the instrument after load problem. It solves the get tracer before TracerProvider was registered problem.

Not sure about the plans to publish an ESM module but if this is planned I guess this should be likely a dynamic import or so. But not sure if ESM and CJS can be in a single file for such cases.

@pichlermarc
Copy link
Member

This is done intentionally. The util is lazy loaded after application start because it requires the http module. If we require the http module, we then can't instrument it later. I believe we should be able to now for traces because of the proxy tracer which has been added to address this issue, but there still is no solution for that in metrics or logs. @pichlermarc you're more familiar with the exporters than me. Am I missing anything here?

The ProxyTracerProvider doesn't solve the instrument after load problem. It solves the get tracer before TracerProvider was registered problem.

exactly this. What it is trying to do is to defer requiring anything before instrumentations can wrap it, so the ProxyTracerProvider does not help in this case, unfortunately.

Not sure about the plans to publish an ESM module but if this is planned I guess this should be likely a dynamic import or so. But not sure if ESM and CJS can be in a single file for such cases.

Yep, I think we'll need to take care of this using multiple files. ESM publishing was already added in #3208 if I recall correctly.

@dyladan
Copy link
Member

dyladan commented Jul 25, 2023

This is done intentionally. The util is lazy loaded after application start because it requires the http module. If we require the http module, we then can't instrument it later. I believe we should be able to now for traces because of the proxy tracer which has been added to address this issue, but there still is no solution for that in metrics or logs. @pichlermarc you're more familiar with the exporters than me. Am I missing anything here?

The ProxyTracerProvider doesn't solve the instrument after load problem. It solves the get tracer before TracerProvider was registered problem.

exactly this. What it is trying to do is to defer requiring anything before instrumentations can wrap it, so the ProxyTracerProvider does not help in this case, unfortunately.

Bah yeah sorry.

Not sure about the plans to publish an ESM module but if this is planned I guess this should be likely a dynamic import or so. But not sure if ESM and CJS can be in a single file for such cases.

Yep, I think we'll need to take care of this using multiple files. ESM publishing was already added in #3208 if I recall correctly.

Our ESM publishing only works for bundlers which find the build/esm directory. Node will require us to rethink our distribution strategy. Now that ESM support is widespread I think it is well past time to do so.

@github-actions
Copy link

This PR is stale because it has been open 60 days with no activity. Remove stale label or comment or this will be closed in 14 days.

@github-actions github-actions bot added the stale label Sep 25, 2023
@github-actions
Copy link

github-actions bot commented Oct 9, 2023

This PR was closed because it has been stale for 14 days with no activity.

@github-actions github-actions bot closed this Oct 9, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants