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(opencensus-shim) add require-in-the-middle hook to patch @opencensus/core #3809

Merged
merged 4 commits into from
May 31, 2023

Conversation

aabmass
Copy link
Member

@aabmass aabmass commented May 17, 2023

Which problem is this PR solving?

Part of #3749 [4 of 5]. Final state of the series of PRs: #3778

Short description of the changes

Adds a require-in-the-middle hook to patch @opencensus/core to return a ShimTracers. Includes a register script that can be used like node -r @opentelemetry/shim-opencensus/register ... to automatically install the hook at startup.

Type of change

Please delete options that are not relevant.

  • New feature (non-breaking change which adds functionality)

How Has This Been Tested?

Added tests in this PR. I also have an example in the next PR which I tested out to be working.

Checklist:

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

@codecov
Copy link

codecov bot commented May 17, 2023

Codecov Report

Merging #3809 (2902f05) into main (a8ac8ba) will decrease coverage by 0.02%.
The diff coverage is 93.33%.

❗ Current head 2902f05 differs from pull request most recent head cf27880. Consider uploading reports for the commit cf27880 to get more accurate results

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3809      +/-   ##
==========================================
- Coverage   92.95%   92.94%   -0.02%     
==========================================
  Files         297      299       +2     
  Lines        9060     9075      +15     
  Branches     1848     1851       +3     
==========================================
+ Hits         8422     8435      +13     
- Misses        638      640       +2     
Impacted Files Coverage Δ
...erimental/packages/shim-opencensus/src/register.ts 0.00% <0.00%> (ø)
experimental/packages/shim-opencensus/src/shim.ts 100.00% <100.00%> (ø)

... and 1 file with indirect coverage changes

@aabmass aabmass marked this pull request as ready for review May 17, 2023 02:30
@aabmass aabmass requested a review from a team May 17, 2023 02:30
@Flarna
Copy link
Member

Flarna commented May 17, 2023

If moving to @opentelemetry/instrumentation can't be done and RITM is used here it's most likely ok.

@opentelemetry/instrumentation has two main advantages:

One more RITM hook should be not that problematic (compared to >20 one got via autoinstrumentations in the past).

Missing ESM support is maybe also no issue because this shim is to get backward compatibility with a discontinued project where ESM is likely not a big topic anyway.

But we should document that ESM is not working for this shim.

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.

Implementation looks ok to me. Just some minor comments

experimental/CHANGELOG.md Show resolved Hide resolved
experimental/packages/shim-opencensus/package.json Outdated Show resolved Hide resolved
@dyladan
Copy link
Member

dyladan commented May 25, 2023

But we should document that ESM is not working for this shim.

I think we talked about it at the meeting last week but documenting here for clarity: opencensus doesn't support ESM so there should be no expectation it works with the shim

@aabmass aabmass requested a review from Flarna May 30, 2023 14:59
@pichlermarc pichlermarc merged commit 622955a into open-telemetry:main May 31, 2023
@aabmass aabmass deleted the oc-shim-4-rim branch May 31, 2023 14:23
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.

4 participants