feat(replay): Add Request body with attachRawBodyFromRequest option
#18501
Conversation
| expect(actual).toBe(expected); | ||
| }); | ||
|
|
||
| describe('valid types of body (in Request)', () => { |
There was a problem hiding this comment.
Just a note to reviewers: Those tests are copied from above, where those different types are tested without the Request object.
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
| (request as any)[ORIGINAL_BODY] = init.body; | ||
| } | ||
|
|
||
| return request; |
There was a problem hiding this comment.
Bug: Request cloning doesn't preserve body tracking symbol
When a Request is created by cloning another Request (e.g., new Request(existingRequest) or new Request(existingRequest, { headers: {...} })), the ORIGINAL_BODY symbol is not copied to the new Request. The code only stores the body when init && 'body' in init && init.body != null, but when input is a Request object with the symbol already set and init doesn't have a body property, the new Request inherits the body stream but loses the symbol reference. This means body capture fails for cloned Requests.
logaretm
left a comment
logaretm
left a comment
There was a problem hiding this comment.
Nice one! Just one minor Q
|
|
||
| // Preserve the prototype | ||
| // eslint-disable-next-line @typescript-eslint/no-explicit-any, @typescript-eslint/no-unsafe-member-access | ||
| (GLOBAL_OBJ as any).Request.prototype = OriginalRequest.prototype; |
There was a problem hiding this comment.
Nice, I was wondering about instanceof checks.
| if (originalBody !== undefined) { | ||
| return originalBody; | ||
| } |
There was a problem hiding this comment.
q: It is possible that the user creates a req object with a stream body, should we explicitly return undefined here or is it ignored safely at the caller?
There was a problem hiding this comment.
Currently, we would also attach the stream. It's a very unlikely use case because it would mean that you create a ReadableStream yourself and attach it to a Request, which results in a double-nested readable stream 🤔
size-limit report 📦
|
packages/replay-internal/test/unit/coreHandlers/handleNetworkBreadcrumbs.test.ts
Show resolved
Hide resolved
| return async () => { | ||
| globalThis.Request = OriginalRequest; | ||
| }; | ||
| }); |
There was a problem hiding this comment.
Bug: Test cleanup doesn't reset module-level instrumentation flag
The beforeAll cleanup only restores globalThis.Request but doesn't reset the module-level _isRequestInstrumented flag. After the "with patching" tests run, _isRequestInstrumented remains true even after cleanup. If these tests run again in the same process (e.g., in watch mode), subsequent calls to _INTERNAL_instrumentRequestInterface() will return early without patching because the flag is still true, but globalThis.Request has been restored to the original. This causes the patching to silently not occur, making the "with patching" tests fail on re-runs.
node-overhead report 🧳Note: This is a synthetic benchmark with a minimal express app and does not necessarily reflect the real-world performance impact in an application.
|
Previously, we only got the body from the
fetchoptions. However, some people use theRequestobject which can also contain a body. As this body is aReadableStream, we could only read that async.With
attachRawBodyFromRequest, theRequestobject is patched to include the raw body as aSymbolso it can be read synchronously.Linear ticket (internal): https://linear.app/getsentry/issue/JS-1255/investigate-sdk-issues-causing-all-requests-to-default-to-get