capture response body for failed requests and if tracing is enabled in SentryHttpClient

[martinhaintz]

trafficstars

:scroll: Description

If sendDefaultPii is enabled and maxResponseBodySize is bigger than the responseBodySize, the body is logged in sentry.

:bulb: Motivation and Context

close #2220

:green_heart: How did you test it?

:pencil: Checklist

• [ ] I reviewed submitted code
• [x] I added tests to verify changes
• [ ] No new PII added or SDK only sends newly added PII if sendDefaultPii is enabled
• [ ] I updated the docs if needed
• [x] All tests passing
• [x] No breaking changes

:crystal_ball: Next steps

[github-actions[bot]]

	Messages
:book:	Do not forget to update Sentry-docs with your feature once the pull request gets approved.

Generated by :no_entry_sign: dangerJS against a5c5b8ea3972d0dd0e75df290b7cd9ee5bde390d

[codecov[bot]]

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 86.97%. Comparing base (9e7630d) to head (a5c5b8e).

Additional details and impacted files

@@            Coverage Diff             @@
##               v9    #2293      +/-   ##
==========================================
+ Coverage   86.92%   86.97%   +0.05%     
==========================================
  Files         259      260       +1     
  Lines        9245     9285      +40     
==========================================
+ Hits         8036     8076      +40     
  Misses       1209     1209              

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

[github-actions[bot]]

iOS Performance metrics :rocket:

	Plain	With Sentry	Diff
Startup time	1254.80 ms	1273.88 ms	19.08 ms
Size	8.38 MiB	9.78 MiB	1.40 MiB

Baseline results on branch: main

Startup times

Revision	Plain	With Sentry	Diff
4b29d6e00433309052c4edd19c448fb33dd371f5	1208.18 ms	1230.92 ms	22.73 ms
299a3f6457ab47c8b36b22b9749ed343d55c9693	1231.02 ms	1249.00 ms	17.98 ms
21845e2c4fd3604443616251b5d44240ea390663	1279.37 ms	1298.81 ms	19.45 ms
b98109e83d99d504cde5fbc4211536366741297c	1254.19 ms	1279.90 ms	25.71 ms
7faee576f69687eaf846228638db70b8d2b006a5	1232.65 ms	1246.10 ms	13.45 ms
d4120ac3aa1b3297e224a93b2f9d790adb0e683b	1260.61 ms	1274.09 ms	13.47 ms
3ad66e4789a5212fa687083e41baaaecb74bac9c	1230.44 ms	1245.08 ms	14.65 ms
8e133ad2c8f1058e17c287ff9685dcca278c7d07	1268.19 ms	1277.37 ms	9.18 ms
89ea268a7f626fd1b23c6ffa55ec68da030ea450	1252.33 ms	1253.58 ms	1.26 ms
49a149b6624c052f31433241245b0c8b7a2194e0	1296.47 ms	1320.20 ms	23.73 ms

App size

Revision	Plain	With Sentry	Diff
4b29d6e00433309052c4edd19c448fb33dd371f5	8.32 MiB	9.38 MiB	1.05 MiB
299a3f6457ab47c8b36b22b9749ed343d55c9693	8.38 MiB	9.74 MiB	1.36 MiB
21845e2c4fd3604443616251b5d44240ea390663	8.15 MiB	9.12 MiB	991.34 KiB
b98109e83d99d504cde5fbc4211536366741297c	8.10 MiB	9.17 MiB	1.08 MiB
7faee576f69687eaf846228638db70b8d2b006a5	8.33 MiB	9.64 MiB	1.31 MiB
d4120ac3aa1b3297e224a93b2f9d790adb0e683b	8.28 MiB	9.34 MiB	1.06 MiB
3ad66e4789a5212fa687083e41baaaecb74bac9c	8.32 MiB	9.38 MiB	1.05 MiB
8e133ad2c8f1058e17c287ff9685dcca278c7d07	8.10 MiB	9.16 MiB	1.07 MiB
89ea268a7f626fd1b23c6ffa55ec68da030ea450	8.09 MiB	9.16 MiB	1.06 MiB
49a149b6624c052f31433241245b0c8b7a2194e0	8.15 MiB	9.12 MiB	986.26 KiB

Previous results on branch: feat/capture-http-response-body-for-sentry-http-client

Startup times

Revision	Plain	With Sentry	Diff
9e2ff1264458bacff57ccc1ac5a8db4a65e9e4fe	1251.00 ms	1274.70 ms	23.70 ms
0282579d25f8185c33b9dfb3479101cd5b83436f	1255.33 ms	1276.70 ms	21.36 ms
bc3d263874f099dae408154b49ce6041b9b41212	1240.98 ms	1259.69 ms	18.71 ms
b0bca7d7e96304e16985e7907cf91ca71883df70	1251.67 ms	1272.80 ms	21.13 ms
ac2c668d8ac2370aac18340f6130387f37b8f2d1	1243.00 ms	1263.92 ms	20.92 ms
b38f597510a5d7e57affec88638fb6910ffd17b2	1245.52 ms	1266.73 ms	21.21 ms
f9abe1663282f462134ad27da2449f9fdbb788de	1245.40 ms	1274.94 ms	29.54 ms
23081222cfe90d03a2ba29d16006d3dc03fa566a	1231.69 ms	1256.85 ms	25.16 ms
a188061b860173c4ab74c06859cdecf96a2e9f0f	1242.08 ms	1264.28 ms	22.20 ms
8896a1e39234a1711361330d4912df527647899a	1246.29 ms	1269.15 ms	22.85 ms

App size

Revision	Plain	With Sentry	Diff
9e2ff1264458bacff57ccc1ac5a8db4a65e9e4fe	8.38 MiB	9.77 MiB	1.39 MiB
0282579d25f8185c33b9dfb3479101cd5b83436f	8.38 MiB	9.76 MiB	1.39 MiB
bc3d263874f099dae408154b49ce6041b9b41212	8.38 MiB	9.75 MiB	1.37 MiB
b0bca7d7e96304e16985e7907cf91ca71883df70	8.38 MiB	9.77 MiB	1.39 MiB
ac2c668d8ac2370aac18340f6130387f37b8f2d1	8.38 MiB	9.77 MiB	1.39 MiB
b38f597510a5d7e57affec88638fb6910ffd17b2	8.38 MiB	9.77 MiB	1.39 MiB
f9abe1663282f462134ad27da2449f9fdbb788de	8.38 MiB	9.75 MiB	1.37 MiB
23081222cfe90d03a2ba29d16006d3dc03fa566a	8.38 MiB	9.73 MiB	1.36 MiB
a188061b860173c4ab74c06859cdecf96a2e9f0f	8.38 MiB	9.77 MiB	1.39 MiB
8896a1e39234a1711361330d4912df527647899a	8.38 MiB	9.77 MiB	1.39 MiB

[buenaflor]

@kahest JS Replay uses captureNetworkBodies for both request and response

https://github.com/getsentry/sentry-javascript/pull/7589

wdyt if we also adopt this flag?

[kahest]

@kahest JS Replay uses captureNetworkBodies for both request and response

getsentry/sentry-javascript#7589

wdyt if we also adopt this flag?

I think this has since been dropped in favor of networkDetailAllowUrls - see docs. I also don't see references to the option in code anymore.

I like the new option - it makes it more granular and explicit, though of course it's more work. It's still only available for JS SR I think, so not widely adopted. If there's no competing option for other HTTP integrations on other SDKs, I'm fine with adopting this

[buenaflor]

Ah I missed that, thx. I'll have a look at it

[buenaflor]

@kahest I haven't found anything specific in other SDKs that allow sending http response bodies other than replay.

However it's possible a user can do this:

beforeSend: (event, hint) async {
    final response = hint.get(TypeCheckHint.httpResponse);
    if (response is StreamedResponse) {
        final body = getResponseBody(response)
        // user can now use it
    }
}

this also aligns with what the js http integration would like to add: https://github.com/getsentry/sentry-javascript/issues/12544

[kahest]

@buenaflor IIUC this would work for http+dio? I like the idea, it's very flexible and allows users to decide based on endpoint etc. if they want to add, but of course it's more complex to use than a switch

[buenaflor]

this would work for http+dio

yes but only errors so this won't work for transactions (like the user in the referenced issue wants) because we don't have a system in place to pair hints with transactions

I'll take a look for alternatives

[github-actions[bot]]

Android Performance metrics :rocket:

	Plain	With Sentry	Diff
Startup time	440.60 ms	504.91 ms	64.31 ms
Size	6.49 MiB	7.56 MiB	1.07 MiB

Baseline results on branch: main

Startup times

Revision	Plain	With Sentry	Diff
ed2ae087f58aef43c92ff2e06cdcd762fd45338b	360.06 ms	440.92 ms	80.86 ms
bffc2c57d27178214d7b567c9abbf911a4f61801	348.00 ms	399.89 ms	51.89 ms
ecb4003e3aa60b9295e74bd20dc987dafe567263	332.58 ms	385.15 ms	52.57 ms
b66cc27043174c97585950af77a8328f86f342f5	493.11 ms	554.66 ms	61.55 ms
0bed04dc3a892af53ccd5ce7840bf134c3fcbc24	382.15 ms	458.33 ms	76.18 ms
09eab47b4dc3260f0d675166619e5b2ef1e7c0a3	455.30 ms	478.46 ms	23.16 ms
f922f8fd57e0574f28866f4b860f5d674bdb15e2	332.31 ms	374.67 ms	42.37 ms
6e9c5a2e82cbb6e1fa94f35e64670297a0e148c8	392.32 ms	498.51 ms	106.19 ms
31b2afb442b730ab5e1c190ef0de71fbcee6bb38	397.04 ms	475.09 ms	78.04 ms
7ec9238637dbe38b6170da172bc0650eaa157885	414.02 ms	513.94 ms	99.91 ms

App size

Revision	Plain	With Sentry	Diff
ed2ae087f58aef43c92ff2e06cdcd762fd45338b	6.27 MiB	7.20 MiB	958.73 KiB
bffc2c57d27178214d7b567c9abbf911a4f61801	6.34 MiB	7.28 MiB	966.31 KiB
ecb4003e3aa60b9295e74bd20dc987dafe567263	6.06 MiB	7.09 MiB	1.03 MiB
b66cc27043174c97585950af77a8328f86f342f5	6.49 MiB	7.56 MiB	1.07 MiB
0bed04dc3a892af53ccd5ce7840bf134c3fcbc24	6.33 MiB	7.30 MiB	987.71 KiB
09eab47b4dc3260f0d675166619e5b2ef1e7c0a3	6.49 MiB	7.56 MiB	1.07 MiB
f922f8fd57e0574f28866f4b860f5d674bdb15e2	5.94 MiB	6.95 MiB	1.01 MiB
6e9c5a2e82cbb6e1fa94f35e64670297a0e148c8	6.35 MiB	7.42 MiB	1.07 MiB
31b2afb442b730ab5e1c190ef0de71fbcee6bb38	6.34 MiB	7.28 MiB	966.36 KiB
7ec9238637dbe38b6170da172bc0650eaa157885	6.35 MiB	7.42 MiB	1.06 MiB

Previous results on branch: feat/capture-http-response-body-for-sentry-http-client

Startup times

Revision	Plain	With Sentry	Diff
a188061b860173c4ab74c06859cdecf96a2e9f0f	714.20 ms	766.96 ms	52.76 ms
b38f597510a5d7e57affec88638fb6910ffd17b2	488.83 ms	566.78 ms	77.95 ms
41904673f5a703f34bbc6a4d44489e27dfc2f42b	458.57 ms	480.06 ms	21.49 ms
f9abe1663282f462134ad27da2449f9fdbb788de	456.26 ms	480.49 ms	24.23 ms
9e2ff1264458bacff57ccc1ac5a8db4a65e9e4fe	514.25 ms	517.24 ms	2.99 ms
b0bca7d7e96304e16985e7907cf91ca71883df70	461.92 ms	523.16 ms	61.24 ms
23081222cfe90d03a2ba29d16006d3dc03fa566a	422.54 ms	491.65 ms	69.11 ms
ac2c668d8ac2370aac18340f6130387f37b8f2d1	469.81 ms	490.12 ms	20.31 ms
bc3d263874f099dae408154b49ce6041b9b41212	454.46 ms	498.00 ms	43.54 ms
0282579d25f8185c33b9dfb3479101cd5b83436f	503.49 ms	528.57 ms	25.08 ms

App size

Revision	Plain	With Sentry	Diff
a188061b860173c4ab74c06859cdecf96a2e9f0f	6.49 MiB	7.56 MiB	1.07 MiB
b38f597510a5d7e57affec88638fb6910ffd17b2	6.49 MiB	7.56 MiB	1.07 MiB
41904673f5a703f34bbc6a4d44489e27dfc2f42b	6.49 MiB	7.56 MiB	1.07 MiB
f9abe1663282f462134ad27da2449f9fdbb788de	6.49 MiB	7.57 MiB	1.08 MiB
9e2ff1264458bacff57ccc1ac5a8db4a65e9e4fe	6.49 MiB	7.56 MiB	1.07 MiB
b0bca7d7e96304e16985e7907cf91ca71883df70	6.49 MiB	7.56 MiB	1.07 MiB
23081222cfe90d03a2ba29d16006d3dc03fa566a	6.49 MiB	7.55 MiB	1.07 MiB
ac2c668d8ac2370aac18340f6130387f37b8f2d1	6.49 MiB	7.56 MiB	1.07 MiB
bc3d263874f099dae408154b49ce6041b9b41212	6.49 MiB	7.57 MiB	1.08 MiB
0282579d25f8185c33b9dfb3479101cd5b83436f	6.49 MiB	7.56 MiB	1.07 MiB

[buenaflor]

before I look at the PR, does this also work for transactions? because afaik beforeSend is not triggered for transactions @martinhaintz

[buenaflor]

In order to make this fully compatible with tracing we should add the hint to the beforeSendTransaction API

It is available in our sdks so imo it's safe to implement it:

• .net
• javascript

[martinhaintz]

before I look at the PR, does this also work for transactions? because afaik beforeSend is not triggered for transactions @martinhaintz

it wasn't done previously, but I implemented it.

[martinhaintz]

In order to make this fully compatible with tracing we should add the hint to the beforeSendTransaction API

It is available in our sdks so imo it's safe to implement it:

• .net
• javascript

this is by far, the better solution. I will change it tomorrow.

[martinhaintz]

This can break existing applications and cause OOM errors in some edge cases. Are there other implementation options that wouldn't have this issue?

For example, only do the capture of the response body with limits on response size (read the response header first)

@vaind You are right. Thanks for the hint. I will take a different approach.

[buenaflor]

This can break existing applications

true, but I'm not sure if adding a hint to beforeSendTransaction would warrant a new major, although I'm not opposed to marking this as a v9 feature

only do the capture of the response body with limits on response size

we generally don't want to support this with response bodies (as far as I can tell after some searching other sdks don't allow users to capture response bodies automatically with a max size flag), so we'd like to make this dependent on the user as much as possible

imo I'd prefer using hints for this so the user can decide themselves whether to add it to their events

[martinhaintz]

@vaind I implemented a cache for the streamedResponse see streamed_response_copier.dart

~@buenaflor @vaind A copied stream is now available via the hint with TypeCheckHint.httpResponse. I am not sure if it is the best way to hold the hint in the SentryTracer, but as the response is from/inside a children this was the only way I could return it as a hint. Let me know about your feedback.~

UPDATE: This is not a good idea. For every transaction we have only one hint element which is a key/value pair with TypeCheckHint.httpResponse for the response. However, a transaction can contain multiple spans, with every span containing their own response element. Therefore I think the better solution would be to access the hint via transaction.spans[0].hint in the beforeSendTransaction.

I also updated the analyzer in some plugins to ignore the mocks.mocks.dart files, because it threw some warnings after rebuilding.

[martinhaintz]

This is the current approach for accessing the hints for two spans inside a transaction:

options.beforeSendTransaction = (transaction, hint) {
        final firstHint = transaction!.spans[0].hint
        final secondHint = transaction!.spans[1].hint
        return transaction;
      };

@vaind @buenaflor should I revert the changes regarding the parameter options.beforeSendTransaction = (transaction, hint) {} and remove the hint, or leave it there to have the hint already there for the future?

[buenaflor]

We can add the hint for beforeSendTransaction there but probably will mark this as a v9 feature unless we think this is okay to do in v8

[martinhaintz]

@vaind In your previous review you requested some changes, which I (hopefully) addressed. Can you please review again? Thanks!

[buenaflor]

Once we are done here, I'll change the base branch to v9

[martinhaintz]

2. I don't see how you've addressed my remark about OOM on large responses. Could you elaborate, please?

As the responseBody is only accessible once, we must somehow cache it to make sure, that the return value of the send method contains the responseBody and also make the responseBody available via the hint in the beforeSendTransaction. So I don't know how we can address the OOM on large responses. If you have something in mind, feel free to share.

[vaind]

2. I don't see how you've addressed my remark about OOM on large responses. Could you elaborate, please?

As the responseBody is only accessible once, we must somehow cache it to make sure, that the return value of the send method contains the responseBody and also make the responseBody available via the hint in the beforeSendTransaction. So I don't know how we can address the OOM on large responses. If you have something in mind, feel free to share.

1. If the Content-Length header is present, we can read that first. This way we can check and compare the value to MaxResponseBodySize to decide whether we're even going to try and read the body.
2. If the body is reasonable small (some fixed amount, TBD, maybe 1MiB, we can just read it as you did)
3. if it's larger or we don't know (because the header is not present), we can create a pass-through response stream. This way when the user actually accesses the body, we'll collect it as well. Once it exceeds MaxResponseBodySize, we'll drop whatever we have already collected to clear the memory and just stream transparently to the user. Once the body is read, we can update the context. We should be able to achieve this with a Completer.

The request body is now also available via the hint in beforeSendTransaction so the user can do further steps.

Where does this decision come from? The linked RFC describes putting this information into the response context

• https://github.com/getsentry/rfcs/blob/main/text/0022-response-context.md#proposal-namely-option-2
• https://develop.sentry.dev/sdk/data-model/event-payloads/contexts/#response-context (docs don't seem to be updated yet)

[buenaflor]

Where does this decision come from? The linked RFC describes putting this information into the response context

the use case is to read the response payloads which is as far as I can see not part of the RFC, only the body size. and since it's highly pii sensitive imo it's more preferrable to let users have the decision of what to do with the payload, see javascript or .net

[martinhaintz]

1. If the Content-Length header is present, we can read that first. This way we can check and compare the value to MaxResponseBodySize to decide whether we're even going to try and read the body.

As I remember correctly, we decided to ignore the "MaxResponseBodySize" for this feature.

if it's larger or we don't know (because the header is not present), we can create a pass-through response stream. This way when the user actually accesses the body, we'll collect it as well. Once it exceeds MaxResponseBodySize, we'll drop whatever we have already collected to clear the memory and just stream transparently to the user. Once the body is read, we can update the context. We should be able to achieve this with a Completer.

I don't think we should capture the response body based on if the response body from the the original request is accessed. At least, this is not what was discussed in the issue. It also would complicate the behavior for the user who looks through sentry and wonders, why some events have response bodies and some not.

@buenaflor what do you think?

[buenaflor]

imo I'd just make it simpler:

• no headers present, we don't read the body because we don't know the size of it
• otherwise if a header exists, we introduce a hard limit, maybe 0.15mb or higher, this is what javascript does for replay and read the body if it is under the threshold
• if it exceeds the hard limit we don't read the body

wdyt?

[vaind]

imo I'd just make it simpler:

• no headers present, we don't read the body because we don't know the size of it
• otherwise if a header exists, we introduce a hard limit, maybe 0.15mb or higher, this is what javascript does for replay and read the body if it is under the threshold
• if it exceeds the hard limit we don't read the body

wdyt?

sounds good, although why not use the existing option (MaxResponseBodySize) for the limit?

[buenaflor]

sounds good, although why not use the existing option (MaxResponseBodySize) for the limit?

we could do that, we'd have to adjust the enum value MaxResponseBodySize.always or rename it to keep in mind the hard limit

we were considering removing it since no other SDK supports this option

so ultimately this would be the change

• either remove or possibly rename maxResponseBodySize.always since we can't always add it due to size limits
• if possible we read the body and make them available in hints, similar to .net and javascript
• automatically capturing the body data will be removed from our dio integration

or instead of not reading the body we just truncate it up to the limit

[vaind]

we were considering removing it since no other SDK supports this option

That makes sense, no reason to use it than.

[buenaflor]

I'll close this PR for now, we will continue this for v9