fix(instr-fetch): avoid unwrap fetch API

[david-luna]

Which problem is this PR solving?

When fetch adn XHR instrumentations are unwrapping the browser APIs they may also remove any other patch added by another instrumentation. This PR makes the fetch instrumentation to avoid the unwrap and check on the enabled state of the instrumentation to decide to generate telemetry or not

Refs: open-telemetry/opentelemetry-browser#204

Short description of the changes

Type of change

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

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

• ran unit tests

Checklist:

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

[codecov]

Codecov Report

❌ Patch coverage is 58.33333% with 5 lines in your changes missing coverage. Please review.
✅ Project coverage is 95.72%. Comparing base (2da8d39) to head (29ded9a).
⚠️ Report is 2 commits behind head on main.

Files with missing lines	Patch %	Lines
...s/opentelemetry-instrumentation-fetch/src/fetch.ts	58.33%	5 Missing ⚠️

❌ Your patch status has failed because the patch coverage (58.33%) is below the target coverage (80.00%). You can increase the patch coverage or adjust the target coverage.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #6575      +/-   ##
==========================================
- Coverage   95.76%   95.72%   -0.04%     
==========================================
  Files         375      375              
  Lines       12761    12769       +8     
  Branches     3025     3028       +3     
==========================================
+ Hits        12220    12223       +3     
- Misses        541      546       +5     

Files with missing lines	Coverage Δ
...s/opentelemetry-instrumentation-fetch/src/fetch.ts	84.38% <58.33%> (-1.64%)	⬇️

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[overbalance]

Is this guard still needed if there are two instances of fetch instrumentation loaded? I think it will double wrap and produce double the data if that happens

[overbalance]

Also, do we care if another piece of code on the page manages to unwrap fetch? We could re-apply the patch here if so

[david-luna]

good questions :)

Is this guard still needed if there are two instances of fetch instrumentation loaded? I think it will double wrap and produce double the data if that happens

the isWrapped function only tells if something has wrapped the method 1st. I could be the same instrumentation or another one. I'm not sure if there is a way to distinguish. So unwrapping is potentially breaking another instrumentation that wraps fetch.

Also, do we care if another piece of code on the page manages to unwrap fetch? We could re-apply the patch here if so

What comes to mind is another agent or library patching the function. If it uses shimmer I think we are okay but if not we are not 100% sure that the wrap was removed.

If the following code executes after enabling the instrumentation

const nativeFetch = globalThis.fetch; // this is the wrapped function
globalThis.fetch = function (...args) {
  console.log('Fetch');
  return nativeFetch.apply(this, args);
}

now the global fetch function does not have the shimmer props (_wrapped, __original, __unwrap) but its still calling our wrapped transitively.

I'm thinking that maybe we should patch in a way that it cannot be undone and use the instrumentation state/config to control the behavior (which this PR does except it can be unpatched) and also that we cannot protect the users for their wrong doing like creating 2 instances of the same instrumentation.

[overbalance]

There's a small race condition here if calls are in flight because of the setTimeout above. Probably rare 🤷

[david-luna]

IIUC this is already happening. The 1st that comes to mind is to reuse _clearResources private method which has a check for pending tasks... or just let the resources be cleared when called by the setTimeout callback.

Also it seems clearTimingResources has no default value which becomes undefined (falsy). So if not configured otherwise the instrumentation accumulates resources and I worry that this might lead to memory issues for web apps running for a long period of time. 🤔