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

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

Open
wants to merge 6 commits into
base: main
Choose a base branch
from
Open

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

wants to merge 6 commits into from

Conversation

martinhaintz
Copy link
Collaborator

@martinhaintz martinhaintz commented Sep 16, 2024 •
edited
Loading

📜 Description

The request body is now also available via the hint in beforeSend so the user can do further steps.
This is available for failed requests and if tracing is enabled.

options.beforeSend = (event, hint) async {
  final response = hint.get(TypeCheckHint.httpResponse);
  if (response is StreamedResponse) {
    final body = await response.stream.bytesToString();
    // user can now use it
  }
  return event;
};

💡 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 GitHub Actions
Copy link
Contributor

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

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

Generated by 🚫 dangerJS against a309f16

@codecov Codecov
Copy link

codecov bot commented Sep 16, 2024 •
edited
Loading

Codecov Report

Attention: Patch coverage is 98.52941% with 1 line in your changes missing coverage. Please review.

Project coverage is 84.78%. Comparing base (7c7c64f) to head (a309f16).

Files with missing lines Patch % Lines
dart/lib/src/http_client/sentry_http_client.dart 96.42% 1 Missing ⚠️
Additional details and impacted files 
@@            Coverage Diff             @@
##             main    #2293      +/-   ##
==========================================
+ Coverage   84.72%   84.78%   +0.06%     
==========================================
  Files         253      254       +1     
  Lines        9097     9131      +34     
==========================================
+ Hits         7707     7742      +35     
+ Misses       1390     1389       -1

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

@github-actions GitHub Actions
Copy link
Contributor

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

iOS Performance metrics 🚀

  Plain With Sentry Diff
Startup time 1245.40 ms 1274.94 ms 29.54 ms
Size 8.38 MiB 9.75 MiB 1.37 MiB

Baseline results on branch: main

Startup times

Revision Plain With Sentry Diff
d4d0807 1246.94 ms 1260.69 ms 13.75 ms
f9d18f3 1240.20 ms 1242.78 ms 2.57 ms
04bd9e6 1230.78 ms 1250.71 ms 19.94 ms
051e97a 1245.94 ms 1249.51 ms 3.57 ms
b8562d0 1249.92 ms 1267.56 ms 17.64 ms
affcf07 1240.61 ms 1266.49 ms 25.88 ms
2e8b1e1 1247.45 ms 1263.67 ms 16.22 ms
0f067d3 1245.71 ms 1269.59 ms 23.88 ms
f1314d5 1246.46 ms 1270.92 ms 24.46 ms
e239c83 1248.40 ms 1269.28 ms 20.89 ms

App size

Revision Plain With Sentry Diff
d4d0807 8.33 MiB 9.64 MiB 1.31 MiB
f9d18f3 8.29 MiB 9.36 MiB 1.07 MiB
04bd9e6 8.33 MiB 9.61 MiB 1.27 MiB
051e97a 8.28 MiB 9.34 MiB 1.06 MiB
b8562d0 8.33 MiB 9.54 MiB 1.22 MiB
affcf07 8.38 MiB 9.70 MiB 1.33 MiB
2e8b1e1 8.33 MiB 9.64 MiB 1.31 MiB
0f067d3 8.32 MiB 9.52 MiB 1.20 MiB
f1314d5 8.10 MiB 9.08 MiB 1004.30 KiB
e239c83 8.38 MiB 9.74 MiB 1.36 MiB

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

Startup times

Revision Plain With Sentry Diff
2308122 1231.69 ms 1256.85 ms 25.16 ms
bc3d263 1240.98 ms 1259.69 ms 18.71 ms

App size

Revision Plain With Sentry Diff
2308122 8.38 MiB 9.73 MiB 1.36 MiB
bc3d263 8.38 MiB 9.75 MiB 1.37 MiB

buenaflor
buenaflor reviewed Sep 18, 2024
View reviewed changes
dart/lib/src/sentry_options.dart Outdated
Comment on lines 282 to 283
"All Request and Responses are now logged, if `sendDefaultPii` is `true` and `maxRequestBodySize` and `maxResponseBodySize` conditions are met.")
bool captureFailedRequests = true;
Copy link
Contributor

Choose a reason for hiding this comment

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

let's rethink this, I don't think deprecating this flag is the right way to go

Copy link
Member

Choose a reason for hiding this comment

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

we still want to provide this flag to disable capturing of failed network requests completely, so yes we shouldn't deprecate this

@buenaflor
Copy link
Contributor

@kahest JS Replay uses captureNetworkBodies for both request and response

getsentry/sentry-javascript#7589

wdyt if we also adopt this flag?

@kahest
Copy link
Member

kahest commented Oct 15, 2024 •
edited
Loading

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

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

@buenaflor
Copy link
Contributor

buenaflor commented Oct 16, 2024 •
edited
Loading

@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
Copy link
Member

kahest commented Oct 16, 2024

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

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

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

Android Performance metrics 🚀

  Plain With Sentry Diff
Startup time 456.26 ms 480.49 ms 24.23 ms
Size 6.49 MiB 7.57 MiB 1.08 MiB

Baseline results on branch: main

Startup times

Revision Plain With Sentry Diff
1a93825 347.31 ms 424.54 ms 77.23 ms
a510d1d 295.63 ms 344.65 ms 49.03 ms
a7acb24 301.00 ms 357.38 ms 56.38 ms
6034b0a 406.35 ms 475.93 ms 69.59 ms
2d74010 400.42 ms 466.50 ms 66.08 ms
4b5a4f6 394.19 ms 471.81 ms 77.62 ms
bf8d36c 505.00 ms 563.18 ms 58.18 ms
e82709a 361.18 ms 423.50 ms 62.32 ms
c732386 316.84 ms 390.62 ms 73.78 ms
24f71aa 358.49 ms 455.90 ms 97.41 ms

App size

Revision Plain With Sentry Diff
1a93825 6.27 MiB 7.20 MiB 956.36 KiB
a510d1d 5.94 MiB 6.96 MiB 1.02 MiB
a7acb24 5.94 MiB 6.95 MiB 1.01 MiB
6034b0a 6.34 MiB 7.29 MiB 970.36 KiB
2d74010 6.33 MiB 7.26 MiB 943.41 KiB
4b5a4f6 6.34 MiB 7.28 MiB 962.57 KiB
bf8d36c 6.49 MiB 7.56 MiB 1.07 MiB
e82709a 6.34 MiB 7.29 MiB 970.37 KiB
c732386 6.26 MiB 7.20 MiB 958.78 KiB
24f71aa 6.06 MiB 7.03 MiB 990.30 KiB

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

Startup times

Revision Plain With Sentry Diff
bc3d263 454.46 ms 498.00 ms 43.54 ms
2308122 422.54 ms 491.65 ms 69.11 ms

App size

Revision Plain With Sentry Diff
bc3d263 6.49 MiB 7.57 MiB 1.08 MiB
2308122 6.49 MiB 7.55 MiB 1.07 MiB

@martinhaintz martinhaintz marked this pull request as ready for review November 5, 2024 16:57
@martinhaintz 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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kahest kahest kahest left review comments

@krystofwoldrich krystofwoldrich Awaiting requested review from krystofwoldrich krystofwoldrich is a code owner

@stefanosiano stefanosiano Awaiting requested review from stefanosiano stefanosiano is a code owner

@buenaflor buenaflor Awaiting requested review from buenaflor buenaflor is a code owner

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Capture HTTP Response Body for SentryHttpClient
3 participants
@martinhaintz @buenaflor @kahest