feat: capture Response context in HTTP Client #1095

vaind · 2022-10-27T17:28:19Z

📜 Description

💡 Motivation and Context

closes #969

💚 How did you test it?

📝 Checklist

🔮 Next steps

github-actions · 2022-10-27T17:29:29Z

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

Generated by 🚫 dangerJS against d0ac2c5

dart/lib/src/http_client/failed_request_client.dart

github-actions · 2022-10-27T17:39:30Z

Android Performance metrics 🚀

	Plain	With Sentry	Diff
Startup time	313.46 ms	401.38 ms	87.92 ms
Size	5.94 MiB	6.95 MiB	1.01 MiB

Baseline results on branch: main

Startup times

Revision	Plain	With Sentry	Diff
`559d28f`	302.35 ms	339.53 ms	37.18 ms
`56810ff`	309.72 ms	352.26 ms	42.54 ms
`3e5ee37`	317.56 ms	366.84 ms	49.28 ms
`1c6eb5b`	350.69 ms	393.86 ms	43.17 ms
`4efee31`	308.92 ms	368.68 ms	59.76 ms
`f922f8f`	332.31 ms	374.67 ms	42.37 ms
`613760b`	373.42 ms	399.33 ms	25.92 ms
`870f5eb`	329.45 ms	369.29 ms	39.84 ms
`d7758e8`	300.12 ms	349.88 ms	49.76 ms
`322aa66`	284.98 ms	341.76 ms	56.78 ms

App size

Revision	Plain	With Sentry	Diff
`559d28f`	5.94 MiB	6.92 MiB	1001.70 KiB
`56810ff`	5.94 MiB	6.92 MiB	1001.71 KiB
`3e5ee37`	5.94 MiB	6.92 MiB	1001.19 KiB
`1c6eb5b`	5.94 MiB	6.92 MiB	1001.53 KiB
`4efee31`	5.94 MiB	6.92 MiB	1003.76 KiB
`f922f8f`	5.94 MiB	6.95 MiB	1.01 MiB
`613760b`	5.94 MiB	6.92 MiB	1005.98 KiB
`870f5eb`	5.94 MiB	6.92 MiB	1005.77 KiB
`d7758e8`	5.94 MiB	6.95 MiB	1.01 MiB
`322aa66`	5.94 MiB	6.92 MiB	1005.75 KiB

Previous results on branch: feat/http-response

Startup times

Revision	Plain	With Sentry	Diff
`110b200`	318.88 ms	353.78 ms	34.90 ms
`8f551ef`	291.75 ms	328.16 ms	36.41 ms
`6cc09c5`	326.86 ms	379.84 ms	52.98 ms
`383665d`	320.82 ms	358.76 ms	37.94 ms
`3cf0e6f`	386.28 ms	465.84 ms	79.56 ms
`d433b49`	320.88 ms	366.70 ms	45.82 ms
`bf7a456`	328.06 ms	371.62 ms	43.56 ms
`9274023`	316.04 ms	364.59 ms	48.55 ms

App size

Revision	Plain	With Sentry	Diff
`110b200`	5.94 MiB	6.95 MiB	1.01 MiB
`8f551ef`	5.94 MiB	6.95 MiB	1.01 MiB
`6cc09c5`	5.94 MiB	6.95 MiB	1.01 MiB
`383665d`	5.94 MiB	6.95 MiB	1.01 MiB
`3cf0e6f`	5.94 MiB	6.95 MiB	1.01 MiB
`d433b49`	5.94 MiB	6.95 MiB	1.01 MiB
`bf7a456`	5.94 MiB	6.95 MiB	1.01 MiB
`9274023`	5.94 MiB	6.95 MiB	1.01 MiB

github-actions · 2022-10-27T17:46:51Z

iOS Performance metrics 🚀

	Plain	With Sentry	Diff
Startup time	1266.75 ms	1286.33 ms	19.58 ms
Size	8.15 MiB	9.12 MiB	990.56 KiB

Baseline results on branch: main

Startup times

Revision	Plain	With Sentry	Diff
`72dfc83`	1262.50 ms	1289.75 ms	27.25 ms
`9c5aec6`	1266.51 ms	1274.65 ms	8.14 ms
`eb1a7c1`	1281.25 ms	1295.40 ms	14.15 ms
`613760b`	1263.10 ms	1277.27 ms	14.16 ms
`559d28f`	1265.04 ms	1288.96 ms	23.92 ms
`6d317ea`	1277.27 ms	1287.47 ms	10.20 ms
`2f8f173`	1280.61 ms	1292.20 ms	11.59 ms
`d7758e8`	1271.69 ms	1288.08 ms	16.39 ms
`0ceb89c`	1252.02 ms	1271.78 ms	19.75 ms
`870f5eb`	1267.78 ms	1286.86 ms	19.08 ms

App size

Revision	Plain	With Sentry	Diff
`72dfc83`	8.15 MiB	9.12 MiB	987.30 KiB
`9c5aec6`	8.15 MiB	9.12 MiB	986.23 KiB
`eb1a7c1`	8.15 MiB	9.13 MiB	1000.10 KiB
`613760b`	8.15 MiB	9.13 MiB	1000.46 KiB
`559d28f`	8.15 MiB	9.12 MiB	987.32 KiB
`6d317ea`	8.15 MiB	9.12 MiB	986.26 KiB
`2f8f173`	8.15 MiB	9.13 MiB	1000.39 KiB
`d7758e8`	8.15 MiB	9.12 MiB	989.76 KiB
`0ceb89c`	8.15 MiB	9.12 MiB	989.78 KiB
`870f5eb`	8.15 MiB	9.13 MiB	1000.08 KiB

Previous results on branch: feat/http-response

Startup times

Revision	Plain	With Sentry	Diff
`8f551ef`	1277.45 ms	1294.31 ms	16.86 ms
`110b200`	1272.82 ms	1302.16 ms	29.35 ms
`6cc09c5`	1278.43 ms	1304.77 ms	26.34 ms
`fa59ae2`	1283.29 ms	1300.98 ms	17.69 ms
`3cf0e6f`	1264.98 ms	1297.61 ms	32.63 ms
`9274023`	1282.29 ms	1308.67 ms	26.39 ms
`d433b49`	1267.06 ms	1279.17 ms	12.11 ms
`383665d`	1285.77 ms	1299.59 ms	13.82 ms
`bf7a456`	1265.72 ms	1294.96 ms	29.23 ms

App size

Revision	Plain	With Sentry	Diff
`8f551ef`	8.15 MiB	9.12 MiB	990.59 KiB
`110b200`	8.15 MiB	9.12 MiB	990.57 KiB
`6cc09c5`	8.15 MiB	9.12 MiB	989.10 KiB
`fa59ae2`	8.15 MiB	9.12 MiB	990.59 KiB
`3cf0e6f`	8.15 MiB	9.12 MiB	990.61 KiB
`9274023`	8.15 MiB	9.12 MiB	990.04 KiB
`d433b49`	8.15 MiB	9.12 MiB	989.06 KiB
`383665d`	8.15 MiB	9.12 MiB	990.55 KiB
`bf7a456`	8.15 MiB	9.12 MiB	990.61 KiB

marandaneto · 2022-10-31T08:25:10Z

dart/lib/src/http_client/failed_request_client.dart

@@ -165,6 +167,7 @@ class FailedRequestClient extends BaseClient {
      url: urlWithoutQuery,
      queryString: query,
      cookies: sendDefaultPii ? request.headers['Cookie'] : null,
+      // TODO FIXME - this should check [sendDefaultPii], same as DIO integration.


No need, the default here is MaxRequestBodySize.never
So if the user has set something else, this is opt-in for whatever the data is, even if PII.

OK, according to that logic, the following should be removed too? https://github.com/getsentry/sentry-dart/blob/main/dio/lib/src/dio_event_processor.dart#L143

Mmm good catch, I've missed that, I guess its an oversight on the Dio integration them.
https://docs.sentry.io/platforms/php/configuration/options/#max-request-body-size
Does not mention thesendDefaultPii flag , let's keep this TODO here and I will double-check internally.

actually, with the following comment that I should remove body and replace it with bodySize, it's OK anyway, since body size itself is not PII.

Yep but body still lives in the Request, it's not only there in the Response.

Let's add the sendDefaultPii check here for the body

ok, added the check

dart/lib/src/http_client/failed_request_client.dart

dart/lib/src/protocol/sentry_response.dart

codecov-commenter · 2022-10-31T13:08:43Z

Codecov Report

Base: 89.98% // Head: 90.14% // Increases project coverage by +0.16% 🎉

Coverage data is based on head (d0ac2c5) compared to base (0ceb89c).
Patch coverage: 82.97% of modified lines in pull request are covered.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1095      +/-   ##
==========================================
+ Coverage   89.98%   90.14%   +0.16%     
==========================================
  Files         115      116       +1     
  Lines        3585     3573      -12     
==========================================
- Hits         3226     3221       -5     
+ Misses        359      352       -7

Impacted Files	Coverage Δ
dart/lib/src/platform_checker.dart	`100.00% <ø> (ø)`
dart/lib/src/protocol/max_body_size.dart	`50.00% <50.00%> (ø)`
dart/lib/src/protocol/sentry_response.dart	`82.75% <86.66%> (+2.75%)`	⬆️
...art/lib/src/http_client/failed_request_client.dart	`93.18% <100.00%> (+0.15%)`	⬆️
dart/lib/src/protocol/sentry_request.dart	`97.95% <100.00%> (+0.13%)`	⬆️
dart/lib/src/utils/iterable_extension.dart	`100.00% <100.00%> (ø)`
dio/lib/src/dio_event_processor.dart	`96.42% <100.00%> (+0.84%)`	⬆️
dart/lib/src/protocol/contexts.dart	`93.93% <0.00%> (+3.78%)`	⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

dart/lib/src/http_client/failed_request_client.dart

dio/lib/src/dio_event_processor.dart

…re setting, same as the DIO integration

dart/lib/src/protocol/sentry_response.dart

dart/test/mocks/mock_transport.dart

…e case insensitive

…quest

dart/test/mocks/mock_transport.dart

marandaneto · 2022-11-04T08:28:26Z

@vaind there are some linters:

error - test/mocks/mock_transport.dart:45:11 - Missing variable type for 'envelopeItemJson'. Try adding an explicit type, or remove implicit-dynamic from your analysis options file. - implicit_dynamic_variable
error - test/mocks/mock_transport.dart:46:33 - The argument type 'dynamic' can't be assigned to the parameter type 'Map<String, dynamic>'. - argument_type_not_assignable

vaind · 2022-11-04T08:45:51Z

@vaind there are some linters:

Fixed now. BTW any reason why do the various packages use different linter rules?

marandaneto · 2022-11-04T08:53:45Z

@vaind there are some linters:

Fixed now. BTW any reason why do the various packages use different linter rules?

nope, either an oversight or because it's a dep depending on other dep, and other packages, might differ a bit, but in general, they should be very similar if not the same.

marandaneto

Thanks @vaind

vaind mentioned this pull request Oct 27, 2022

feat: HttpClient response capture #1093

Closed

5 tasks

vaind commented Oct 27, 2022

View reviewed changes

dart/lib/src/http_client/failed_request_client.dart Outdated Show resolved Hide resolved

marandaneto reviewed Oct 31, 2022

View reviewed changes

dart/lib/src/http_client/failed_request_client.dart Outdated Show resolved Hide resolved

marandaneto reviewed Oct 31, 2022

View reviewed changes

dart/lib/src/protocol/sentry_response.dart Outdated Show resolved Hide resolved

vaind force-pushed the feat/http-response branch 2 times, most recently from d433b49 to 6cc09c5 Compare October 31, 2022 13:07

vaind marked this pull request as ready for review October 31, 2022 14:24

vaind requested review from brustolin and krystofwoldrich as code owners October 31, 2022 14:24

marandaneto reviewed Nov 2, 2022

View reviewed changes

dart/lib/src/http_client/failed_request_client.dart Show resolved Hide resolved

marandaneto reviewed Nov 2, 2022

View reviewed changes

dio/lib/src/dio_event_processor.dart Show resolved Hide resolved

vaind added 8 commits November 3, 2022 12:03

refactor: max body size shouldAddBody() - use switch

e4428b9

refactor: dio - call _responseFrom() when needed

47c2dea

feat: capture response in FailedRequestClient

f7b1d22

test: fix sentry_envelope_vm_test (windows, utc+1 TZ)

4e963fd

feat: update remove HTTP Response body from context

4811a1d

feat: remove SentryResponse fields: url, body, redirected, status

6cff316

chore: update changelog

31ee76e

feat: add cookies to SentryResponse

3cf0e6f

vaind force-pushed the feat/http-response branch from fa59ae2 to 3cf0e6f Compare November 3, 2022 11:03

vaind added 3 commits November 3, 2022 12:22

fix: SentryHttpClient request body capture checks default PII captu…

383665d

…re setting, same as the DIO integration

chore: formatting

f9929d5

chore: remove collection dependency

8f551ef

marandaneto reviewed Nov 3, 2022

View reviewed changes

dart/lib/src/protocol/sentry_response.dart Show resolved Hide resolved

marandaneto reviewed Nov 3, 2022

View reviewed changes

dart/lib/src/protocol/sentry_response.dart Show resolved Hide resolved

marandaneto reviewed Nov 3, 2022

View reviewed changes

dart/test/mocks/mock_transport.dart Outdated Show resolved Hide resolved

vaind added 2 commits November 3, 2022 19:26

refactor: move SentryRequest cookie discovery to a single place & mak…

4dc5a0e

…e case insensitive

chore: cleanup MockTransport, moving proper json handling to SentryRe…

110b200

…quest

marandaneto reviewed Nov 4, 2022

View reviewed changes

dart/test/mocks/mock_transport.dart Show resolved Hide resolved

chore: update mock_transport.dart copy in the DIO integration tests

d0ac2c5

vaind force-pushed the feat/http-response branch from bf7a456 to d0ac2c5 Compare November 4, 2022 08:45

marandaneto approved these changes Nov 4, 2022

View reviewed changes

marandaneto merged commit 5603ab2 into main Nov 4, 2022

marandaneto deleted the feat/http-response branch November 4, 2022 09:17

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: capture Response context in HTTP Client #1095

feat: capture Response context in HTTP Client #1095

vaind commented Oct 27, 2022 •

edited

Loading

github-actions bot commented Oct 27, 2022 •

edited

Loading

github-actions bot commented Oct 27, 2022 •

edited

Loading

Baseline results on branch: main

Startup times

App size

Previous results on branch: feat/http-response

Startup times

App size

github-actions bot commented Oct 27, 2022 •

edited

Loading

Baseline results on branch: main

Startup times

App size

Previous results on branch: feat/http-response

Startup times

App size

marandaneto Oct 31, 2022

vaind Oct 31, 2022

marandaneto Oct 31, 2022

vaind Oct 31, 2022

marandaneto Oct 31, 2022

marandaneto Nov 2, 2022

vaind Nov 3, 2022

codecov-commenter commented Oct 31, 2022 •

edited

Loading

marandaneto commented Nov 4, 2022

vaind commented Nov 4, 2022

marandaneto commented Nov 4, 2022

marandaneto left a comment

feat: capture Response context in HTTP Client #1095

feat: capture Response context in HTTP Client #1095

Conversation

vaind commented Oct 27, 2022 • edited Loading

📜 Description

💡 Motivation and Context

💚 How did you test it?

📝 Checklist

🔮 Next steps

github-actions bot commented Oct 27, 2022 • edited Loading

github-actions bot commented Oct 27, 2022 • edited Loading

Android Performance metrics 🚀

Baseline results on branch: main

Startup times

App size

Previous results on branch: feat/http-response

Startup times

App size

github-actions bot commented Oct 27, 2022 • edited Loading

iOS Performance metrics 🚀

Baseline results on branch: main

Startup times

App size

Previous results on branch: feat/http-response

Startup times

App size

marandaneto Oct 31, 2022

Choose a reason for hiding this comment

vaind Oct 31, 2022

Choose a reason for hiding this comment

marandaneto Oct 31, 2022

Choose a reason for hiding this comment

vaind Oct 31, 2022

Choose a reason for hiding this comment

marandaneto Oct 31, 2022

Choose a reason for hiding this comment

marandaneto Nov 2, 2022

Choose a reason for hiding this comment

vaind Nov 3, 2022

Choose a reason for hiding this comment

codecov-commenter commented Oct 31, 2022 • edited Loading

Codecov Report

marandaneto commented Nov 4, 2022

vaind commented Nov 4, 2022

marandaneto commented Nov 4, 2022

marandaneto left a comment

Choose a reason for hiding this comment

vaind commented Oct 27, 2022 •

edited

Loading

github-actions bot commented Oct 27, 2022 •

edited

Loading

github-actions bot commented Oct 27, 2022 •

edited

Loading

github-actions bot commented Oct 27, 2022 •

edited

Loading

codecov-commenter commented Oct 31, 2022 •

edited

Loading