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.

Sign up for GitHub

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

Jump to bottom

feat(browser): Allow to capture request payload/responses #7287

Closed
wants to merge 5 commits into from
Closed

feat(browser): Allow to capture request payload/responses #7287

wants to merge 5 commits into from

Conversation

mydea
Copy link
Member

@mydea mydea commented Feb 27, 2023

This adds a new opt-in config for the Breadcrumbs integrations to also capture request payload/responses:

Sentry.init({
  integrations: [
    new Breadcrumbs({ 
      captureResponsePayload: true,
      captureRequestPayload: true 
    })
  ],
});

If these are set, we add request_payload and/or response_payload fields to the xhr/fetch breadcrumbs. An example breadcrumb would then be:

 {
    category: 'fetch',
    data: {
      method: 'POST',
      request_payload: 'body sent',
      response_payload: '{"testApi":"OK"}',
      status_code: 200,
      url: 'http://localhost:7654/foo',
    },
    timestamp: 1234,
    type: 'http',
  }

For replays, we also pass these through as requestPayload / responsePayload (note that we seem to be using camelCase in replay breadcrumbs, snake_case in the general breadcrumbs).

Some notes on this/things to consider:

  • The UI probably needs to be updated to actually show this data somehow.
  • These fields may contain PII. I guess the general breadcrumbs PII scrubbing should happen for them?
  • The fetch handler has to be async for this (as we need to await response.text()). However as long as this is not enabled it shouldn't actually change the timing.

Closes #7103

@mydea mydea self-assigned this Feb 27, 2023
billyvg
billyvg reviewed Feb 27, 2023
View reviewed changes
packages/browser/src/integrations/breadcrumbs.ts
status_code,
};

if (options.captureRequestPayload && body) {
Copy link
Member

Choose a reason for hiding this comment

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

We should probably capture falsy body values, no?

Copy link
Member Author

Choose a reason for hiding this comment

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

Good question, wasn't so sure about that. the response body seemed to be '' when "empty", the request body undefined. We can also just pass this through as-is and just not care what it is! WDYT?

packages/browser/src/integrations/breadcrumbs.ts
data: {
...handlerData.fetchData,
status_code: handlerData.response.status,
if (options.captureRequestPayload && handlerData.args[1] && handlerData.args[1].body) {
Copy link
Member

Choose a reason for hiding this comment

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

Same question here irt falsy body

packages/browser/src/integrations/breadcrumbs.ts
Comment on lines +294 to +295
if (text) {
handlerData.fetchData.response_payload = text;
Copy link
Member

Choose a reason for hiding this comment

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

And here for falsy values

@github-actions
Copy link
Contributor

Replay SDK metrics 🚀

    Plain +Sentry +Replay
Revision Value Value Diff Ratio Value Diff Ratio
LCP This PR 3324324 66.52 ms 89.29 ms +22.76 ms +34.22 % 123.88 ms +57.35 ms +86.21 %
Previous 4b95c04 74.67 ms 100.14 ms +25.47 ms +34.11 % 132.23 ms +57.56 ms +77.09 %
CLS This PR 3324324 0.06 ms 0.06 ms -0.00 ms -0.03 % 0.06 ms -0.00 ms -0.28 %
Previous 4b95c04 0.06 ms 0.06 ms -0.00 ms -0.79 % 0.06 ms -0.00 ms -0.47 %
CPU This PR 3324324 12.80 % 12.97 % +0.17 pp +1.32 % 18.20 % +5.40 pp +42.20 %
Previous 4b95c04 12.32 % 12.48 % +0.16 pp +1.28 % 20.27 % +7.94 pp +64.46 %
JS heap avg This PR 3324324 1.94 MB 1.99 MB +48.55 kB +2.50 % 2.87 MB +931.61 kB +47.95 %
Previous 4b95c04 1.94 MB 1.99 MB +43.74 kB +2.25 % 2.86 MB +920.88 kB +47.38 %
JS heap max This PR 3324324 2.3 MB 2.57 MB +265.6 kB +11.53 % 3.35 MB +1.05 MB +45.40 %
Previous 4b95c04 2.3 MB 2.55 MB +249.86 kB +10.85 % 3.35 MB +1.05 MB +45.58 %
netTx This PR 3324324 0 B 0 B 0 B n/a 2.23 kB +2.23 kB n/a
Previous 4b95c04 0 B 0 B 0 B n/a 2.21 kB +2.21 kB n/a
netRx This PR 3324324 0 B 0 B 0 B n/a 41 B +41 B n/a
Previous 4b95c04 0 B 0 B 0 B n/a 41 B +41 B n/a
netCount This PR 3324324 0 0 0 n/a 1 +1 n/a
Previous 4b95c04 0 0 0 n/a 1 +1 n/a
netTime This PR 3324324 0.00 ms 0.00 ms 0.00 ms n/a 121.25 ms +121.25 ms n/a
Previous 4b95c04 0.00 ms 0.00 ms 0.00 ms n/a 90.32 ms +90.32 ms n/a

Previous results on branch: develop
RevisionLCPCLSCPUJS heap avgJS heap maxnetTxnetRxnetCountnetTime
4b95c04+57.56 ms-0.00 ms+7.94 pp+920.88 kB+1.05 MB+2.21 kB+41 B+1+90.32 ms
e60cd02+56.25 ms-0.00 ms+6.32 pp+927.44 kB+1.06 MB+2.21 kB+41 B+1+117.55 ms
e25c067+48.34 ms+0.00 ms+5.59 pp+926.37 kB+1.05 MB+2.22 kB+41 B+1+65.23 ms
b1b249b+43.88 ms+0.00 ms+4.80 pp+937.99 kB+1.05 MB+2.22 kB+41 B+1+111.56 ms
12e34d4+28.57 ms+0.00 ms+5.77 pp+930.12 kB+1.04 MB+2.26 kB+41 B+1+109.67 ms
c46c56c+65.45 ms-0.00 ms+5.38 pp+930.26 kB+1.07 MB+2.21 kB+41 B+1+91.29 ms
7f4c4ec+56.64 ms-0.00 ms+5.57 pp+927.42 kB+1.06 MB+2.21 kB+41 B+1+110.83 ms
00d2360+55.18 ms+0.00 ms+2.23 pp+934.14 kB+1.05 MB+2.22 kB+41 B+1+71.65 ms
*) pp - percentage points - an absolute difference between two percentages.
Last updated: Tue, 28 Feb 2023 00:27:39 GMT

@github-actions
Copy link
Contributor

size-limit report 📦

Path Size
@sentry/browser - ES5 CDN Bundle (gzipped + minified) 20.91 KB (+3.96% 🔺)
@sentry/browser - ES5 CDN Bundle (minified) 64.51 KB (+3.24% 🔺)
@sentry/browser - ES6 CDN Bundle (gzipped + minified) 18.87 KB (+0.72% 🔺)
@sentry/browser - ES6 CDN Bundle (minified) 55.96 KB (+0.84% 🔺)
@sentry/browser - Webpack (gzipped + minified) 20.62 KB (+0.66% 🔺)
@sentry/browser - Webpack (minified) 67.4 KB (+0.68% 🔺)
@sentry/react - Webpack (gzipped + minified) 20.64 KB (+0.65% 🔺)
@sentry/nextjs Client - Webpack (gzipped + minified) 48.17 KB (+0.28% 🔺)
@sentry/browser + @sentry/tracing - ES5 CDN Bundle (gzipped + minified) 27.87 KB (+3.05% 🔺)
@sentry/browser + @sentry/tracing - ES6 CDN Bundle (gzipped + minified) 25.44 KB (+0.6% 🔺)
@sentry/replay ES6 CDN Bundle (gzipped + minified) 42.9 KB (+0.28% 🔺)
@sentry/replay - Webpack (gzipped + minified) 36.97 KB (+0.41% 🔺)
@sentry/browser + @sentry/tracing + @sentry/replay - ES6 CDN Bundle (gzipped + minified) 60.6 KB (+0.43% 🔺)
@sentry/browser + @sentry/replay - ES6 CDN Bundle (gzipped + minified) 54.14 KB (+0.44% 🔺)

Lms24
Lms24 approved these changes Feb 28, 2023
View reviewed changes
Copy link
Member

@Lms24 Lms24 left a comment

Choose a reason for hiding this comment

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

As long as we strictly keep this opt-in, this LGTM.

@mattjohnsonpint mattjohnsonpint mentioned this pull request Mar 7, 2023
Open
8 tasks
@bruno-garcia bruno-garcia mentioned this pull request Mar 8, 2023
Closed
@AbhiPrasad AbhiPrasad mentioned this pull request Mar 9, 2023
Open
@bruno-garcia
Copy link
Member

Are we closing this one?

@mydea
Copy link
Member Author

mydea commented Mar 10, 2023

I will, working on a replacement for the sizes only at the moment, will close this once this is up (hopefully today)!

@mydea
Copy link
Member Author

mydea commented Mar 23, 2023

Closed in favor of #7589

@mydea mydea closed this Mar 23, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@billyvg billyvg billyvg left review comments

@Lms24 Lms24 Lms24 approved these changes

@lforst lforst Awaiting requested review from lforst

Assignees

@mydea mydea

Labels
Package: browser
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Replay: Optionally attach HTTP request/response payloads
4 participants
@mydea @bruno-garcia @billyvg @Lms24