Skip to content

feat(replay): Add experiment to capture request/response bodies - #7589

Merged
mydea merged 1 commit into
developfrom
fn/replay-request-bodies
Mar 29, 2023
Merged

feat(replay): Add experiment to capture request/response bodies#7589
mydea merged 1 commit into
developfrom
fn/replay-request-bodies

Conversation

@mydea

@mydeamydea commented Mar 23, 2023

Copy link
Copy Markdown
Member

This adds a new experiment: captureNetworkBodies that, when enabled, will attach the request/response body to the replay breadcrumb.

In the process of doing this I moved the network breadcrumb stuff around a bit, IMHO it is a bit clearer now how things work together.

The responding breadcrumbs will look like this:

data: {method: 'POST',statusCode: 200,request: {size: 3,body: 'my body'},response: {size: 11,body: {data:3}},},

Note that the response.size and request.size will be sent anyhow, even without the experiment.
This removes the (shortly available) requestBodySize and responseBodySize fields.

ref #7103

@mydea
mydea requested review from Lms24 and billyvgMarch 23, 2023 12:51
@mydeamydea self-assigned this Mar 23, 2023
@mydea
mydea requested a review from ryan953March 23, 2023 12:51
Comment threadpackages/replay/src/coreHandlers/util/networkUtils.ts Outdated
Comment threadpackages/replay/src/types.ts Outdated

request?: {
size?: number;
body?: string;

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

Q @billyvg & @ryan953: Is it OK to just always pass this as string, or should we pass a POJO if it is JSON?

String pro:

  • It is easier
  • It is less error prone
  • Less bytes on the client

POJO pro:

  • Easier to differentiate what is JSON vs. what is a string (for UI)
  • I guess PII stripping would work differently, and not redact the whole body but only certain fields?

Maybe we can also do the string->POJO conversion server side, to get both benefits in one package...? cc @cmanallen as well

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Whatever makes network load better is my preference. Since we only show one response at a time parsing & client memory should be fine.

As a stringy value we can do try { JSON.parse() } and hope for the best.

Headers could guide this too, but any 3rd-party or custom mime type would annoying to deal with, so we'd probably just ignore headers for this transform anyway.

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

sounds good!
We'll add headers in a follow up PR, we can still look into using them if we want to/need to.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

@mydea Let's go with POJO - I think redacting the whole body is a worse experience than individual fields.

@github-actions

github-actionsBot commented Mar 23, 2023

Copy link
Copy Markdown
Contributor

size-limit report 📦

PathSize
@sentry/browser - ES5 CDN Bundle (gzipped + minified)20.61 KB (-0.01% 🔽)
@sentry/browser - ES5 CDN Bundle (minified)64.39 KB (0%)
@sentry/browser - ES6 CDN Bundle (gzipped + minified)19.16 KB (-0.01% 🔽)
@sentry/browser - ES6 CDN Bundle (minified)56.77 KB (0%)
@sentry/browser - Webpack (gzipped + minified)21.62 KB (0%)
@sentry/browser - Webpack (minified)72.03 KB (0%)
@sentry/react - Webpack (gzipped + minified)21.64 KB (0%)
@sentry/nextjs Client - Webpack (gzipped + minified)51.99 KB (+0.02% 🔺)
@sentry/browser + @sentry/tracing - ES5 CDN Bundle (gzipped + minified)28.15 KB (+0.02% 🔺)
@sentry/browser + @sentry/tracing - ES6 CDN Bundle (gzipped + minified)26.35 KB (+0.03% 🔺)
@sentry/replay ES6 CDN Bundle (gzipped + minified)44.74 KB (+0.6% 🔺)
@sentry/replay - Webpack (gzipped + minified)38.86 KB (+0.83% 🔺)
@sentry/browser + @sentry/tracing + @sentry/replay - ES6 CDN Bundle (gzipped + minified)63.4 KB (+0.45% 🔺)
@sentry/browser + @sentry/replay - ES6 CDN Bundle (gzipped + minified)56.48 KB (+0.47% 🔺)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Is this redundant with captureBodies test?

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

this tests that without the experiment we do not capture the body!

Comment threadpackages/replay/src/types.ts Outdated

request?: {
size?: number;
body?: string;

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

@mydea Let's go with POJO - I think redacting the whole body is a worse experience than individual fields.

@mydea
mydeaforce-pushed the fn/replay-request-bodies branch from 2cbc520 to 218c30cCompareMarch 27, 2023 07:28
@mydea

Copy link
Copy Markdown
MemberAuthor

I updated this:

  • bodies are now sent as POJO if JSON-deserializable, else as text
  • I put in a random limit of 50k characters, when this is exceeded the body is not added. Happy to adjust this however way we think makes sense, but I'd say it is good to have some limit in place there.

Comment threadpackages/replay/src/coreHandlers/util/fetchUtils.ts Outdated
Comment threadpackages/replay/src/coreHandlers/util/fetchUtils.ts Outdated
@mydea
mydeaforce-pushed the fn/replay-request-bodies branch from 053b301 to 0a81c64CompareMarch 27, 2023 15:38
@mydea

Copy link
Copy Markdown
MemberAuthor

Update:

  • Added hard limit of 300k bytes - when this is exceeded, no body will be sent, instead we add e.g.:
response: {size: 400_000,_meta: {errors: ['MAX_BODY_SIZE_EXCEEDED']}}

@billyvg

Copy link
Copy Markdown
Member

Does the build usually take this long? Looks like the test timed out.

image

@mydea

Copy link
Copy Markdown
MemberAuthor

Does the build usually take this long? Looks like the test timed out.

image

I think github was still acting up a bit, there have been a few super slow builds. Hopefully resolved by now!

@mydea
mydeaforce-pushed the fn/replay-request-bodies branch from 0a81c64 to 1b83e65CompareMarch 28, 2023 08:14
billyvg pushed a commit to getsentry/sentry that referenced this pull request Mar 28, 2023
New 'Network details' panel. Figma per
[figma](https://www.figma.com/file/SVyNvm8p17xGfIC7r7l2YS/Specs%3A-Network-Request-Details?node-id=4%3A7443&t=VjNMXfMJQKHfj0ks-0)
specs
There are a few states so observe in the screen shots, and some followup
tasks
Note that:
- You can only open the details panel for xhr & fetch network types
- The data is a string that's run through the pii scrubber, we try to
parse it with `JSON.parse`, but if that fails you'll just get a string
| State | Img |
| --- | --- |
| Click a network row to open the Details. Notice that the row you
clicked is Bold. The lower area with the data is scrollable, strings or
objects will display |
![SCR-20230323-km4](https://user-images.githubusercontent.com/187460/227372507-b093de12-66df-4383-a46f-650a488c7821.png)
|
| The X button will close the panel |
![SCR-20230323-kmb](https://user-images.githubusercontent.com/187460/227372508-18954b0a-c593-4aa8-9eca-49a99f191685.png)
|
| Click+drag (don't click on the tabs or the X button) to make the panel
larger/smaller |
![SCR-20230323-kmd](https://user-images.githubusercontent.com/187460/227372509-a3318199-01ef-486d-a00b-ece5008ca465.png)
|
Related to getsentry/sentry-javascript#7589Fixes#46054

@billyvgbillyvg left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Tested this with the new UI and it's working great!

Comment threadpackages/replay/src/coreHandlers/util/networkUtils.ts Outdated
billyvg added a commit to getsentry/sentry that referenced this pull request Mar 28, 2023
We moved the size field in the sdk in getsentry/sentry-javascript#7589
Note that this field is for XHR/fetch and reflects the decoded body size whereas the other `data.size` fields are *transfer* size (i.e. over the network). We may want to consider changing to show decoded body size for all, though that was only recently added to the SDK.
@mydea
mydeaforce-pushed the fn/replay-request-bodies branch from 1b83e65 to 496fdbcCompareMarch 28, 2023 14:57
@mydea
mydea merged commit 30f2c24 into developMar 29, 2023
@mydea
mydea deleted the fn/replay-request-bodies branch March 29, 2023 07:07
billyvg added a commit to getsentry/sentry that referenced this pull request Mar 30, 2023
We moved the size field in the sdk in
getsentry/sentry-javascript#7589
Note that this field is for XHR/fetch and reflects the decoded body size
whereas the other `data.size` fields are *transfer* size (i.e. over the
network). We may want to consider changing to show decoded body size for
all, though that was only recently added to the SDK.
Sign up for freeto 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

@mydea@billyvg@ryan953