Skip to content

fix(instrumentation-fetch): do not modify the returned type of fetch#6521

Merged
pichlermarc merged 6 commits intoopen-telemetry:mainfrom
dynatrace-oss-contrib:fix-fetch-promise
Mar 25, 2026
Merged

fix(instrumentation-fetch): do not modify the returned type of fetch#6521
pichlermarc merged 6 commits intoopen-telemetry:mainfrom
dynatrace-oss-contrib:fix-fetch-promise

Conversation

@dyladan
Copy link
Copy Markdown
Member

@dyladan dyladan commented Mar 23, 2026

fetch returns a native promise. If we wrap that promise, any code that checks if a promise is native will fail. Notably this happens in the case of WebAssembly.compileStreaming

fetch returns a Promise<Response>. When we wrap fetch we also wrap the Response. It used to be wrapped in a new Response object. Recently in #6343 a Proxy was introduced so that certain readonly properties could be retained.

This fix works by removing the Proxy and returning the original Response. The downside is that if the user cancels the request reader with reader.cancel(), our clone will not be cancelled and the clone will continue to read until the underlying stream is closed (most likely by the server finishing the response). AbortController.abort() still works because it aborts the underlying fetch, closing both the original and clone streams immediately, which is the typical way to abort a fetch anyway.

Fixes #6518

fetch returns a native promise. If we wrap that promise, any code that
checks if a promise is native will fail. Notably this happens in the
case of WebAssembly.compileStreaming
@dyladan dyladan requested review from a team as code owners March 23, 2026 18:00
@codecov
Copy link
Copy Markdown

codecov Bot commented Mar 23, 2026

Codecov Report

❌ Patch coverage is 56.25000% with 7 lines in your changes missing coverage. Please review.
✅ Project coverage is 95.67%. Comparing base (b5a6b12) to head (a491fc0).
⚠️ Report is 3 commits behind head on main.

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

❌ Your patch status has failed because the patch coverage (56.25%) 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    #6521      +/-   ##
==========================================
- Coverage   95.73%   95.67%   -0.07%     
==========================================
  Files         364      364              
  Lines       12104    12069      -35     
  Branches     2887     2876      -11     
==========================================
- Hits        11588    11547      -41     
- Misses        516      522       +6     
Files with missing lines Coverage Δ
...s/opentelemetry-instrumentation-fetch/src/fetch.ts 85.84% <56.25%> (-4.20%) ⬇️
🚀 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.

@dyladan dyladan marked this pull request as draft March 23, 2026 18:07
@dyladan dyladan marked this pull request as ready for review March 23, 2026 18:49
throw error;
}

return new Promise((resolve, reject) => {
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

new Promise not required because context.with returns its callback return value synchronously, which is already a Promise

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.

LGTM, personally got no objection to the cancellation stuff. I'd still like a 2nd review from someone else with more familiarity with this code is possible though.

Comment thread experimental/packages/opentelemetry-instrumentation-fetch/src/fetch.ts Outdated
Comment thread experimental/packages/opentelemetry-instrumentation-fetch/src/fetch.ts Outdated
@dyladan
Copy link
Copy Markdown
Member Author

dyladan commented Mar 25, 2026

@overbalance updated the PR. PTAL

Copy link
Copy Markdown
Contributor

@overbalance overbalance left a comment

Choose a reason for hiding this comment

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

A runtime guard on error.status is probably overkill. Looks good!

@pichlermarc pichlermarc added this pull request to the merge queue Mar 25, 2026
Merged via the queue into open-telemetry:main with commit c846919 Mar 25, 2026
26 of 27 checks passed
@pichlermarc pichlermarc deleted the fix-fetch-promise branch March 25, 2026 13:47
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.

bug(instrumentation-fetch): WebAssembly.compileStreaming failed: TypeError

4 participants