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(@opentelemetry-instrumentation-fetch): support reading response body from the hook applyCustomAttributesOnSpan #2497

Conversation

echoontheway
Copy link
Contributor

@echoontheway echoontheway commented Sep 24, 2021

opentelemetry-instrumentation-fetch support reading response body from the hook applyCustomAttributesOnSpan

Which problem is this PR solving?

applyCustomAttributesOnSpan's callback param response has been used, so that we can't get response body.
image
https://developer.mozilla.org/en-US/docs/Web/API/Fetch_API/Using_Fetch
UT with current code:
e31094cd-71ef-4e96-8803-dfeff524634d
Detail error is as follows,
2ce9e044-c8e8-4a5c-babe-3fc5521828f2

Short description of the changes

  1. return the response.clone() for applyCustomAttributesOnSpan, so that we can read the body from the cloned repsonse.
  2. add corresponding unit test

@echoontheway echoontheway requested a review from a team as a code owner September 24, 2021 04:54
@codecov
Copy link

codecov bot commented Sep 24, 2021

Codecov Report

Merging #2497 (84a9bfd) into main (d3f5163) will increase coverage by 0.00%.
The diff coverage is n/a.

@@           Coverage Diff           @@
##             main    #2497   +/-   ##
=======================================
  Coverage   93.23%   93.23%           
=======================================
  Files         137      137           
  Lines        5043     5044    +1     
  Branches     1067     1067           
=======================================
+ Hits         4702     4703    +1     
  Misses        341      341           
Impacted Files Coverage Δ
...s/opentelemetry-instrumentation-fetch/src/fetch.ts 96.98% <0.00%> (+0.01%) ⬆️

@johnbley
Copy link
Member

Hi! Thanks for contributing! Could you first provide a unit test showing the problem (i.e., one that fails without your code changes and passes with them)?

@echoontheway echoontheway force-pushed the feature/echo_optimizeFetchInstrument branch from 6f57be3 to 3fe7175 Compare September 26, 2021 09:50
…body from the hook applyCustomAttributesOnSpan
@echoontheway echoontheway force-pushed the feature/echo_optimizeFetchInstrument branch from 3fe7175 to 7f90bf4 Compare September 26, 2021 10:04
@echoontheway echoontheway changed the title feat(@opentelemetry-instrumentation-fetch): support get response data… feat(@opentelemetry-instrumentation-fetch): support reading response body from the hook applyCustomAttributesOnSpan Sep 26, 2021
@echoontheway
Copy link
Contributor Author

Hi! Thanks for contributing! Could you first provide a unit test showing the problem (i.e., one that fails without your code changes and passes with them)?

done~~

@vmarchaud vmarchaud added the bug Something isn't working label Oct 9, 2021
@vmarchaud vmarchaud merged commit faca317 into open-telemetry:main Oct 9, 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.

None yet

4 participants