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(instrumentation-fetch): fetch(string, Request) silently drops request body #2411

Merged
merged 3 commits into from
Aug 14, 2021

Conversation

t2t2
Copy link
Contributor

@t2t2 t2t2 commented Aug 12, 2021

Which problem is this PR solving?

tl;dr: Fetch instrumentation drops request body when using Request object.

Currently fetch instrumentation forwards calls via 2 parameter signature regardless if 'fetch(Request)orfetch(url, object)` signature is being used:

const url = input instanceof Request ? input.url : input;

This however is causing a bug where passing request object as 2nd parameter drops request body: https://jsfiddle.net/t2t2/bc53reqf/

image

(confirmed with both Firefox 90.0.2 & Chrome 92.0.4515.131)

In quick research found that while Request.body should return a ReadableStream none of the browsers actually do it right now. fetch(url, object) is probably trying to find body from 2nd parameter as a normal object but isn't finding anything.

Short description of the changes

In case of fetch(Request) call original method with the same signature.

@t2t2 t2t2 requested a review from a team August 12, 2021 13:31
@codecov
Copy link

codecov bot commented Aug 12, 2021

Codecov Report

Merging #2411 (a6250b4) into main (90ea0fe) will increase coverage by 0.49%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main    #2411      +/-   ##
==========================================
+ Coverage   92.14%   92.64%   +0.49%     
==========================================
  Files         120      137      +17     
  Lines        3998     4975     +977     
  Branches      844     1048     +204     
==========================================
+ Hits         3684     4609     +925     
- Misses        314      366      +52     
Impacted Files Coverage Δ
...s/opentelemetry-instrumentation-fetch/src/fetch.ts 96.91% <100.00%> (ø)
.../opentelemetry-exporter-collector/src/transform.ts 88.69% <0.00%> (ø)
...ry-exporter-collector/src/CollectorExporterBase.ts 92.15% <0.00%> (ø)
packages/opentelemetry-sdk-trace-web/src/utils.ts 94.90% <0.00%> (ø)
...ry-context-zone-peer-dep/src/ZoneContextManager.ts 85.18% <0.00%> (ø)
...-sdk-trace-web/src/enums/PerformanceTimingNames.ts 100.00% <0.00%> (ø)
...ation-xml-http-request/src/enums/AttributeNames.ts 100.00% <0.00%> (ø)
...lemetry-exporter-collector/src/transformMetrics.ts 95.71% <0.00%> (ø)
...kages/opentelemetry-exporter-collector/src/util.ts 100.00% <0.00%> (ø)
...-instrumentation-fetch/src/enums/AttributeNames.ts 100.00% <0.00%> (ø)
... and 8 more

@dyladan dyladan added the bug Something isn't working label Aug 12, 2021
@dyladan dyladan changed the title fix(instrumentation-fetch): pass request object as the only parameter fix(instrumentation-fetch): fetch(string, Request) silently drops request body Aug 12, 2021
@vmarchaud vmarchaud merged commit 5b4ca1c into open-telemetry:main Aug 14, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants