feat(replay): Capture request & response headers #7816
Conversation
size-limit report 📦
|
| await page.evaluate(() => { | ||
| /* eslint-disable */ | ||
| const xhr = new XMLHttpRequest(); | ||
|
|
||
| xhr.open('GET', 'http://localhost:7654/foo'); | ||
| xhr.send(); | ||
|
|
||
| xhr.addEventListener('readystatechange', function () { | ||
| if (xhr.readyState === 4) { | ||
| // @ts-ignore Sentry is a global | ||
| setTimeout(() => Sentry.captureException('test error', 0)); | ||
| } | ||
| }); | ||
| /* eslint-enable */ | ||
| }); |
There was a problem hiding this comment.
(Not particular relevant for this test but more for all tests)
Good idea to use page.evaluate instead of subject.ts files! Out of curiosity - did this reduce flakiness or improve other aspects of the tests?
There was a problem hiding this comment.
it's IMHO just easier to read for such small things, as you see right in the test what's happening. Def. not for all cases, but here it's fine I'd say!
| requestHeaders: [...defaultHeaders, ...requestHeaders.map(header => header.toLowerCase())], | ||
| responseHeaders: [...defaultHeaders, ...responseHeaders.map(header => header.toLowerCase())], |
There was a problem hiding this comment.
I'm just wondering (disregard if you already talked about this with the replay team) if there'd be a reason for users to actively disable sending the default headers?
There was a problem hiding this comment.
I think it's fine for now to always include these headers.
There was a problem hiding this comment.
We can always adjust this later IMHO and reduce the defaults, but I think these should be pretty safe for all use cases?
| Object.keys(headers).forEach(key => { | ||
| const normalizedKey = key.toLowerCase(); | ||
| if (allowedHeaders.includes(normalizedKey)) { | ||
| filteredHeaders[normalizedKey] = headers[key]; | ||
| } | ||
| }); |
There was a problem hiding this comment.
l: Seems to me like we're doing the same thing here as in XHR but a little differently. Wdyt about either using reduce in both cases or forEach? Or could we even use getAllowedHeaders for both? Might make things a little more unified overall. But logaf-l so feel free to leave it as is!
| requestHeaders: [...defaultHeaders, ...requestHeaders.map(header => header.toLowerCase())], | ||
| responseHeaders: [...defaultHeaders, ...responseHeaders.map(header => header.toLowerCase())], |
There was a problem hiding this comment.
I think it's fine for now to always include these headers.
mydea
force-pushed
the
fn/replay-network-req-headers
branch
from
642c4a0 to
8afccf9
Compare
mydea
force-pushed
the
fn/replay-network-req-headers
branch
from
8afccf9 to
d805b1a
Compare
We will capture the following headers by default:
Additional headers can be opted in via
_experiments.captureRequestHeadersand_experiments.captureResponseHeaders, which take an array of strings as arguments.While at this, I also refactored the integration tests to be grouped by test type - this grew a bit out of hand for the network stuff. Now we have a single test file for request/response bodies, headers, sizes.
Closes #7495