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

[RUMF-824] support Request instances in tracing #684

Merged
merged 7 commits into from
Jan 21, 2021

Conversation

BenoitZugmeyer
Copy link
Member

Motivation

Support Request instances in tracing

fetch(
  new Request('http://localhost:3000', {
    headers: {
      'x-foo': 'bar',
    },
  })
)

Changes

Clone instance and add trace headers if the fetch init argument is a Request.

Testing

Unit


I have gone over the contributing documentation.

@BenoitZugmeyer BenoitZugmeyer requested a review from a team as a code owner January 19, 2021 10:05
@BenoitZugmeyer BenoitZugmeyer force-pushed the benoit/support-request-instance branch from a17434e to 75ad984 Compare January 19, 2021 10:07
@BenoitZugmeyer BenoitZugmeyer force-pushed the benoit/support-request-instance branch from 75ad984 to fb2165f Compare January 19, 2021 10:07
@codecov-io
Copy link

codecov-io commented Jan 19, 2021

Codecov Report

Merging #684 (86b0105) into master (5415a6f) will decrease coverage by 0.12%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #684      +/-   ##
==========================================
- Coverage   77.98%   77.85%   -0.13%     
==========================================
  Files          60       60              
  Lines        3284     3288       +4     
  Branches      714      715       +1     
==========================================
- Hits         2561     2560       -1     
- Misses        723      728       +5     
Impacted Files Coverage Δ
packages/core/src/browser/fetchProxy.ts 97.72% <100.00%> (ø)
packages/rum-core/src/domain/tracing/tracer.ts 100.00% <100.00%> (ø)
packages/rum-core/src/transport/batch.ts 68.42% <0.00%> (-10.53%) ⬇️
...ckages/core/src/domain/automaticErrorCollection.ts 98.41% <0.00%> (-1.59%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5415a6f...86b0105. Read the comment docs.

resolveWith and rejectWith don't need to be async, and they don't
actually return anything (`resolve` and `reject` are returning
'undefined')
Comment on lines 46 to 50
context.input.headers.forEach((value, key) => {
headers.push([key, value])
})
}
context.init.headers = headers.concat(objectEntries(tracingHeaders) as string[][])
Copy link
Contributor

@bcaudan bcaudan Jan 20, 2021

Choose a reason for hiding this comment

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

It seems a bit weird to me to not update input headers but to provide an init headers instead.
I am wondering if it could not be better to provide an updated input only:

context.input = new Request(request, {
  headers: headers.concat(objectEntries(tracingHeaders) as string[][])
})

no strong argument for one or the other though.
wdyt?

Copy link
Member Author

@BenoitZugmeyer BenoitZugmeyer Jan 21, 2021

Choose a reason for hiding this comment

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

Mmh it seems to be equivalent, since calling fetch(input, init) calls new Request(input, init) internally. After a few tests, I think that it should be fine. I only spoted an issue with streaming requests but it is likely to be a Chrome bug in the ongoing implementation:

const request = new Request('/post', {
  method: 'POST',
  body: new ReadableStream(),
  mode: 'cors'
});
// works:
await fetch(request, { mode: 'cors' })
// fails with: TypeError: Failed to construct 'Request': If request is made from ReadableStream, mode should be"same-origin" or "cors"
const requestCopy = new Request(request)
await fetch(requestCopy, { mode: 'cors' })

I think it's fine nonetheless, I updated the implementation, let me know if it looks better to you.

Copy link
Contributor

@bcaudan bcaudan left a comment

Choose a reason for hiding this comment

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

👍

@BenoitZugmeyer BenoitZugmeyer merged commit 9ea93ea into master Jan 21, 2021
@BenoitZugmeyer BenoitZugmeyer deleted the benoit/support-request-instance branch January 21, 2021 13:17
kcaffrey pushed a commit to WonderInventions/browser-sdk that referenced this pull request Jan 29, 2021
kcaffrey added a commit to WonderInventions/browser-sdk that referenced this pull request Jan 29, 2021
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.

3 participants