Make response body accessible via `hint` in `beforeSendTransaction` callback for SentryHttpClient #2293

martinhaintz · 2024-09-16T15:10:16Z

📜 Description

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

options.beforeSendTransaction = (transaction, hint) {
  final firstHint = transaction.spans[0].hint;
  final firstResponseBody = firstHint?.get(TypeCheckHint.httpResponse);
  final secondHint = transaction.spans[1].hint;
  final secondResponseBody = secondHint?.get(TypeCheckHint.httpResponse);
  // user can now use it
  return transaction;
};

💡 Motivation and Context

close #2220

💚 How did you test it?

📝 Checklist

I reviewed submitted code
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
All tests passing
No breaking changes

🔮 Next steps

github-actions · 2024-09-16T15:11:00Z

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

Generated by 🚫 dangerJS against a5c5b8e

codecov · 2024-09-16T15:11:42Z

Codecov Report

All modified and coverable lines are covered by tests ✅

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

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

github-actions · 2024-09-16T15:19:50Z

iOS Performance metrics 🚀

	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
`4b29d6e`	1208.18 ms	1230.92 ms	22.73 ms
`299a3f6`	1231.02 ms	1249.00 ms	17.98 ms
`21845e2`	1279.37 ms	1298.81 ms	19.45 ms
`b98109e`	1254.19 ms	1279.90 ms	25.71 ms
`7faee57`	1232.65 ms	1246.10 ms	13.45 ms
`d4120ac`	1260.61 ms	1274.09 ms	13.47 ms
`3ad66e4`	1230.44 ms	1245.08 ms	14.65 ms
`8e133ad`	1268.19 ms	1277.37 ms	9.18 ms
`89ea268`	1252.33 ms	1253.58 ms	1.26 ms
`49a149b`	1296.47 ms	1320.20 ms	23.73 ms

App size

Revision	Plain	With Sentry	Diff
`4b29d6e`	8.32 MiB	9.38 MiB	1.05 MiB
`299a3f6`	8.38 MiB	9.74 MiB	1.36 MiB
`21845e2`	8.15 MiB	9.12 MiB	991.34 KiB
`b98109e`	8.10 MiB	9.17 MiB	1.08 MiB
`7faee57`	8.33 MiB	9.64 MiB	1.31 MiB
`d4120ac`	8.28 MiB	9.34 MiB	1.06 MiB
`3ad66e4`	8.32 MiB	9.38 MiB	1.05 MiB
`8e133ad`	8.10 MiB	9.16 MiB	1.07 MiB
`89ea268`	8.09 MiB	9.16 MiB	1.06 MiB
`49a149b`	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
`9e2ff12`	1251.00 ms	1274.70 ms	23.70 ms
`0282579`	1255.33 ms	1276.70 ms	21.36 ms
`bc3d263`	1240.98 ms	1259.69 ms	18.71 ms
`b0bca7d`	1251.67 ms	1272.80 ms	21.13 ms
`ac2c668`	1243.00 ms	1263.92 ms	20.92 ms
`b38f597`	1245.52 ms	1266.73 ms	21.21 ms
`f9abe16`	1245.40 ms	1274.94 ms	29.54 ms
`2308122`	1231.69 ms	1256.85 ms	25.16 ms
`a188061`	1242.08 ms	1264.28 ms	22.20 ms
`8896a1e`	1246.29 ms	1269.15 ms	22.85 ms

App size

Revision	Plain	With Sentry	Diff
`9e2ff12`	8.38 MiB	9.77 MiB	1.39 MiB
`0282579`	8.38 MiB	9.76 MiB	1.39 MiB
`bc3d263`	8.38 MiB	9.75 MiB	1.37 MiB
`b0bca7d`	8.38 MiB	9.77 MiB	1.39 MiB
`ac2c668`	8.38 MiB	9.77 MiB	1.39 MiB
`b38f597`	8.38 MiB	9.77 MiB	1.39 MiB
`f9abe16`	8.38 MiB	9.75 MiB	1.37 MiB
`2308122`	8.38 MiB	9.73 MiB	1.36 MiB
`a188061`	8.38 MiB	9.77 MiB	1.39 MiB
`8896a1e`	8.38 MiB	9.77 MiB	1.39 MiB

dart/lib/src/sentry_options.dart

buenaflor · 2024-10-15T11:25:37Z

@kahest JS Replay uses captureNetworkBodies for both request and response

getsentry/sentry-javascript#7589

wdyt if we also adopt this flag?

kahest · 2024-10-15T17:54:09Z

@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 · 2024-10-15T18:15:37Z

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

buenaflor · 2024-10-16T11:08:15Z

@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: getsentry/sentry-javascript#12544

kahest · 2024-10-16T17:00:58Z

@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 · 2024-10-17T10:54:43Z

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

…ttp-client

github-actions · 2024-11-04T09:36:00Z

Android Performance metrics 🚀

	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
`ed2ae08`	360.06 ms	440.92 ms	80.86 ms
`bffc2c5`	348.00 ms	399.89 ms	51.89 ms
`ecb4003`	332.58 ms	385.15 ms	52.57 ms
`b66cc27`	493.11 ms	554.66 ms	61.55 ms
`0bed04d`	382.15 ms	458.33 ms	76.18 ms
`09eab47`	455.30 ms	478.46 ms	23.16 ms
`f922f8f`	332.31 ms	374.67 ms	42.37 ms
`6e9c5a2`	392.32 ms	498.51 ms	106.19 ms
`31b2afb`	397.04 ms	475.09 ms	78.04 ms
`7ec9238`	414.02 ms	513.94 ms	99.91 ms

App size

Revision	Plain	With Sentry	Diff
`ed2ae08`	6.27 MiB	7.20 MiB	958.73 KiB
`bffc2c5`	6.34 MiB	7.28 MiB	966.31 KiB
`ecb4003`	6.06 MiB	7.09 MiB	1.03 MiB
`b66cc27`	6.49 MiB	7.56 MiB	1.07 MiB
`0bed04d`	6.33 MiB	7.30 MiB	987.71 KiB
`09eab47`	6.49 MiB	7.56 MiB	1.07 MiB
`f922f8f`	5.94 MiB	6.95 MiB	1.01 MiB
`6e9c5a2`	6.35 MiB	7.42 MiB	1.07 MiB
`31b2afb`	6.34 MiB	7.28 MiB	966.36 KiB
`7ec9238`	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
`a188061`	714.20 ms	766.96 ms	52.76 ms
`b38f597`	488.83 ms	566.78 ms	77.95 ms
`4190467`	458.57 ms	480.06 ms	21.49 ms
`f9abe16`	456.26 ms	480.49 ms	24.23 ms
`9e2ff12`	514.25 ms	517.24 ms	2.99 ms
`b0bca7d`	461.92 ms	523.16 ms	61.24 ms
`2308122`	422.54 ms	491.65 ms	69.11 ms
`ac2c668`	469.81 ms	490.12 ms	20.31 ms
`bc3d263`	454.46 ms	498.00 ms	43.54 ms
`0282579`	503.49 ms	528.57 ms	25.08 ms

App size

Revision	Plain	With Sentry	Diff
`a188061`	6.49 MiB	7.56 MiB	1.07 MiB
`b38f597`	6.49 MiB	7.56 MiB	1.07 MiB
`4190467`	6.49 MiB	7.56 MiB	1.07 MiB
`f9abe16`	6.49 MiB	7.57 MiB	1.08 MiB
`9e2ff12`	6.49 MiB	7.56 MiB	1.07 MiB
`b0bca7d`	6.49 MiB	7.56 MiB	1.07 MiB
`2308122`	6.49 MiB	7.55 MiB	1.07 MiB
`ac2c668`	6.49 MiB	7.56 MiB	1.07 MiB
`bc3d263`	6.49 MiB	7.57 MiB	1.08 MiB
`0282579`	6.49 MiB	7.56 MiB	1.07 MiB

…ttp-client

buenaflor · 2024-11-11T12:16:45Z

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

buenaflor · 2024-11-12T12:14:02Z

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:

martinhaintz · 2024-11-13T16:19:58Z

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 · 2024-11-13T16:20:33Z

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.

…ttp-client

vaind

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)

dart/lib/src/utils/http_deep_copy_streamed_response.dart

…ttp-client

martinhaintz · 2024-11-19T16:21:40Z

@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.

…ttp-client

martinhaintz · 2024-11-20T10:17:29Z

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 · 2024-11-20T10:47:34Z

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

…ttp-client

martinhaintz · 2024-11-25T12:36:53Z

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

buenaflor · 2024-11-25T13:52:38Z

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

vaind

This is pretty large so I won't have the time to properly review this any sooner than Wednesday.

As for what I've noticed from a quick look:

it's a breaking change so should be merged to v9 right?
I don't see how you've addressed my remark about OOM on large responses. Could you elaborate please?

CHANGELOG.md

martinhaintz · 2024-12-03T16:48:28Z

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.

…p-client

vaind · 2024-12-04T14:07:26Z

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.

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.
If the body is reasonable small (some fixed amount, TBD, maybe 1MiB, we can just read it as you did)
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 · 2024-12-04T14:42:08Z

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 · 2024-12-09T09:09:23Z

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 · 2024-12-10T16:13:00Z

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 · 2024-12-10T16:55:35Z

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 · 2024-12-10T17:09:03Z

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 · 2024-12-10T18:37:38Z

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

That makes sense, no reason to use it than.

capture response body for failed requests and if tracing is enabled.

2308122

unified event capturing for request and response

da865b2

buenaflor reviewed Sep 18, 2024

View reviewed changes

dart/lib/src/sentry_options.dart Outdated Show resolved Hide resolved

Merge branch 'main' into feat/capture-http-response-body-for-sentry-h…

bc3d263

…ttp-client

martinhaintz added 3 commits November 5, 2024 16:40

make response body accessible via hint

710ab1a

fix warning

f9abe16

add changelog entry

a309f16

martinhaintz marked this pull request as ready for review November 5, 2024 16:57

martinhaintz requested review from krystofwoldrich and stefanosiano as code owners November 5, 2024 16:57

martinhaintz requested a review from buenaflor November 5, 2024 16:58

martinhaintz changed the title ~~capture response body for failed requests and if tracing is enabled in SentryHttpClient~~ Make response body accessible via hint in beforSend callback for SentryHttpClient Nov 5, 2024

Merge branch 'main' into feat/capture-http-response-body-for-sentry-h…

715ab7d

…ttp-client

Merge branch 'main' into feat/capture-http-response-body-for-sentry-h…

52ecb5e

…ttp-client

vaind requested changes Nov 14, 2024

View reviewed changes

dart/lib/src/utils/http_deep_copy_streamed_response.dart Outdated Show resolved Hide resolved

Merge branch 'main' into feat/capture-http-response-body-for-sentry-h…

0282579

…ttp-client

martinhaintz added 4 commits November 19, 2024 15:32

fix analyze options

9b4000b

cleanup and code reformating

86ffb80

revert changes

b325b09

remove unused import

b38f597

martinhaintz requested a review from vaind November 19, 2024 16:13

martinhaintz added 2 commits November 20, 2024 11:03

removed hint from tracer

47f50aa

Merge branch 'main' into feat/capture-http-response-body-for-sentry-h…

a188061

…ttp-client

martinhaintz added 2 commits November 25, 2024 09:17

Merge branch 'main' into feat/capture-http-response-body-for-sentry-h…

4d43edd

…ttp-client

fix changelog example

537230b

martinhaintz changed the title ~~Make response body accessible via hint in beforSend callback for SentryHttpClient~~ Make response body accessible via hint in beforeSendTransaction callback for SentryHttpClient Nov 25, 2024

martinhaintz changed the title ~~Make response body accessible via hint in beforeSendTransaction callback for SentryHttpClient~~ Make response body accessible via hint in beforeSendTransaction callback for SentryHttpClient Nov 25, 2024

martinhaintz marked this pull request as ready for review November 25, 2024 08:26

buenaflor added the 9.0.0 label Nov 25, 2024

vaind reviewed Nov 25, 2024

View reviewed changes

CHANGELOG.md Outdated Show resolved Hide resolved

fix changelog example

d9c2674

buenaflor changed the base branch from main to v9 December 3, 2024 21:31

Merge branch 'v9' into feat/capture-http-response-body-for-sentry-htt…

a5c5b8e

…p-client

Make response body accessible via hint in beforeSendTransaction callback for SentryHttpClient #2293

Are you sure you want to change the base?

Make response body accessible via hint in beforeSendTransaction callback for SentryHttpClient #2293

Conversation

martinhaintz commented Sep 16, 2024 • edited Loading

📜 Description

💡 Motivation and Context

💚 How did you test it?

📝 Checklist

🔮 Next steps

github-actions bot commented Sep 16, 2024 • edited Loading

codecov bot commented Sep 16, 2024 • edited Loading

Codecov Report

github-actions bot commented Sep 16, 2024 • edited Loading

iOS Performance metrics 🚀

Baseline results on branch: main

Startup times

App size

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

Startup times

App size

buenaflor commented Oct 15, 2024

kahest commented Oct 15, 2024 • edited Loading

buenaflor commented Oct 15, 2024

buenaflor commented Oct 16, 2024 • edited Loading

kahest commented Oct 16, 2024

buenaflor commented Oct 17, 2024

github-actions bot commented Nov 4, 2024 • edited Loading

Android Performance metrics 🚀

Baseline results on branch: main

Startup times

App size

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

Startup times

App size

buenaflor commented Nov 11, 2024 • edited Loading

buenaflor commented Nov 12, 2024

martinhaintz commented Nov 13, 2024

martinhaintz commented Nov 13, 2024

vaind left a comment • edited Loading

Choose a reason for hiding this comment

martinhaintz commented Nov 19, 2024 • edited Loading

martinhaintz commented Nov 20, 2024

buenaflor commented Nov 20, 2024 • edited Loading

martinhaintz commented Nov 25, 2024

buenaflor commented Nov 25, 2024

vaind left a comment • edited Loading

Choose a reason for hiding this comment

martinhaintz commented Dec 3, 2024

vaind commented Dec 4, 2024

buenaflor commented Dec 4, 2024 • edited Loading

martinhaintz commented Dec 9, 2024

buenaflor commented Dec 10, 2024 • edited Loading

vaind commented Dec 10, 2024

buenaflor commented Dec 10, 2024 • edited Loading

vaind commented Dec 10, 2024 • edited Loading

Make response body accessible via `hint` in `beforeSendTransaction` callback for SentryHttpClient #2293

Make response body accessible via `hint` in `beforeSendTransaction` callback for SentryHttpClient #2293

martinhaintz commented Sep 16, 2024 •

edited

Loading

github-actions bot commented Sep 16, 2024 •

edited

Loading

codecov bot commented Sep 16, 2024 •

edited

Loading

github-actions bot commented Sep 16, 2024 •

edited

Loading

kahest commented Oct 15, 2024 •

edited

Loading

buenaflor commented Oct 16, 2024 •

edited

Loading

github-actions bot commented Nov 4, 2024 •

edited

Loading

buenaflor commented Nov 11, 2024 •

edited

Loading

vaind left a comment •

edited

Loading

martinhaintz commented Nov 19, 2024 •

edited

Loading

buenaflor commented Nov 20, 2024 •

edited

Loading

vaind left a comment •

edited

Loading

buenaflor commented Dec 4, 2024 •

edited

Loading

buenaflor commented Dec 10, 2024 •

edited

Loading

buenaflor commented Dec 10, 2024 •

edited

Loading

vaind commented Dec 10, 2024 •

edited

Loading