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(replay): Add experiment to capture request/response bodies #7589

Merged
merged 1 commit into from Mar 29, 2023

Conversation

mydea
Copy link
Member

@mydea mydea commented Mar 23, 2023

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 billyvg and Lms24 March 23, 2023 12:51
@mydea mydea self-assigned this Mar 23, 2023
@mydea mydea requested a review from ryan953 March 23, 2023 12:51

request?: {
size?: number;
body?: string;
Copy link
Member Author

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
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
Member Author

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
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
Copy link
Contributor

github-actions bot commented Mar 23, 2023

size-limit report 📦

Path Size
@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
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
Member Author

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!


request?: {
size?: number;
body?: string;
Copy link
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
Copy link
Member Author

mydea commented Mar 27, 2023

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.

@mydea
Copy link
Member Author

mydea commented Mar 27, 2023

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
Member

billyvg commented Mar 27, 2023

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

image

@mydea
Copy link
Member Author

mydea commented Mar 28, 2023

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!

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#7589

Fixes #46054
Copy link
Member

@billyvg billyvg left a comment

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!

packages/replay/src/coreHandlers/util/networkUtils.ts Outdated Show resolved Hide resolved
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 mydea merged commit 30f2c24 into develop Mar 29, 2023
48 of 49 checks passed
@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 free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants