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.

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

feat: HttpClient response capture #1093

Closed
wants to merge 3 commits into from
Closed

Conversation

vaind
Copy link
Collaborator

@vaind vaind commented Oct 26, 2022

📜 Description

💡 Motivation and Context

closes #969

💚 How did you test it?

📝 Checklist

  • I reviewed submitted code
  • I added tests to verify changes
  • I updated the docs if needed
  • All tests passing
  • No breaking changes

🔮 Next steps

@github-actions
Copy link
Contributor

github-actions bot commented Oct 26, 2022

Fails
🚫 Please consider adding a changelog entry for the next release.
Messages
📖 Do not forget to update Sentry-docs with your feature once the pull request gets approved.

Instructions and example for changelog

Please add an entry to CHANGELOG.md to the "Unreleased" section. Make sure the entry includes this PR's number.

Example:

## Unreleased

- HttpClient response capture ([#1093](https://github.com/getsentry/sentry-dart/pull/1093))

If none of the above apply, you can opt out of this check by adding #skip-changelog to the PR description.

Generated by 🚫 dangerJS against 0c3eb43

@codecov-commenter
Copy link

codecov-commenter commented Oct 26, 2022

Codecov Report

Base: 89.98% // Head: 91.32% // Increases project coverage by +1.34% 🎉

Coverage data is based on head (0c3eb43) compared to base (04db237).
Patch coverage: 100.00% of modified lines in pull request are covered.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1093      +/-   ##
==========================================
+ Coverage   89.98%   91.32%   +1.34%     
==========================================
  Files         115        9     -106     
  Lines        3585      196    -3389     
==========================================
- Hits         3226      179    -3047     
+ Misses        359       17     -342     
Impacted Files Coverage Δ
dio/lib/src/dio_event_processor.dart 95.52% <100.00%> (-0.07%) ⬇️
...art/lib/src/http_client/failed_request_client.dart
dart/lib/src/http_client/sentry_http_client.dart
dart/lib/src/platform_checker.dart
dart/lib/src/protocol/max_body_size.dart
dart/lib/src/protocol/sentry_geo.dart
dart/lib/src/protocol/sentry_response.dart
dart/lib/src/protocol/debug_meta.dart
dart/lib/src/protocol/sentry_transaction_info.dart
dart/lib/src/protocol/sentry_baggage_header.dart
... and 97 more

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.

@github-actions
Copy link
Contributor

github-actions bot commented Oct 26, 2022

Android Performance metrics 🚀

  Plain With Sentry Diff
Startup time 331.37 ms 388.80 ms 57.42 ms
Size 5.94 MiB 6.95 MiB 1.01 MiB

Baseline results on branch: main

Startup times

Revision Plain With Sentry Diff
04db237 330.16 ms 428.38 ms 98.22 ms
870f5eb 329.45 ms 369.29 ms 39.84 ms
ef2f368 350.06 ms 429.44 ms 79.38 ms
72dfc83 298.62 ms 340.14 ms 41.52 ms
559d28f 302.35 ms 339.53 ms 37.18 ms
322aa66 284.98 ms 341.76 ms 56.78 ms
4efee31 308.92 ms 368.68 ms 59.76 ms
2f8f173 323.31 ms 373.29 ms 49.97 ms
eecbbca 324.37 ms 352.49 ms 28.12 ms
613760b 373.42 ms 399.33 ms 25.92 ms

App size

Revision Plain With Sentry Diff
04db237 5.94 MiB 6.95 MiB 1.01 MiB
870f5eb 5.94 MiB 6.92 MiB 1005.77 KiB
ef2f368 5.94 MiB 6.89 MiB 975.81 KiB
72dfc83 5.94 MiB 6.92 MiB 1001.71 KiB
559d28f 5.94 MiB 6.92 MiB 1001.70 KiB
322aa66 5.94 MiB 6.92 MiB 1005.75 KiB
4efee31 5.94 MiB 6.92 MiB 1003.76 KiB
2f8f173 5.94 MiB 6.95 MiB 1.01 MiB
eecbbca 5.94 MiB 6.89 MiB 975.78 KiB
613760b 5.94 MiB 6.92 MiB 1005.98 KiB

@github-actions
Copy link
Contributor

github-actions bot commented Oct 26, 2022

iOS Performance metrics 🚀

  Plain With Sentry Diff
Startup time 1292.85 ms 1301.62 ms 8.77 ms
Size 8.15 MiB 9.13 MiB 1003.19 KiB

Baseline results on branch: main

Startup times

Revision Plain With Sentry Diff
9c5aec6 1266.51 ms 1274.65 ms 8.14 ms
4efee31 1270.33 ms 1285.75 ms 15.42 ms
eecbbca 1264.90 ms 1286.33 ms 21.43 ms
56810ff 1267.59 ms 1293.48 ms 25.89 ms
ef2f368 1259.12 ms 1277.04 ms 17.92 ms
72dfc83 1262.50 ms 1289.75 ms 27.25 ms
1c6eb5b 1277.85 ms 1285.71 ms 7.86 ms
2f8f173 1280.61 ms 1292.20 ms 11.59 ms
559d28f 1265.04 ms 1288.96 ms 23.92 ms
6d317ea 1277.27 ms 1287.47 ms 10.20 ms

App size

Revision Plain With Sentry Diff
9c5aec6 8.15 MiB 9.12 MiB 986.23 KiB
4efee31 8.15 MiB 9.12 MiB 991.35 KiB
eecbbca 8.15 MiB 9.10 MiB 965.26 KiB
56810ff 8.15 MiB 9.12 MiB 987.35 KiB
ef2f368 8.15 MiB 9.10 MiB 965.24 KiB
72dfc83 8.15 MiB 9.12 MiB 987.30 KiB
1c6eb5b 8.15 MiB 9.12 MiB 986.27 KiB
2f8f173 8.15 MiB 9.13 MiB 1000.39 KiB
559d28f 8.15 MiB 9.12 MiB 987.32 KiB
6d317ea 8.15 MiB 9.12 MiB 986.26 KiB

Previous results on branch: feat/http-response-body

Startup times

Revision Plain With Sentry Diff
0c3eb43 1279.85 ms 1294.64 ms 14.79 ms

App size

Revision Plain With Sentry Diff
0c3eb43 8.15 MiB 9.13 MiB 1003.21 KiB

@marandaneto
Copy link
Contributor

@vaind we've decided that the body should not be part of the event because of event limits and PII.
ideally, the body is an attachment, but this still requires an RFC because of PII concerns, frontend changes, etc.
It'd be part of this feature.
I'd not work on this right now unless we want to start with the RFC now.

@vaind vaind changed the title refactor: max body size shouldAddBody() - use switch feat: HttpClient response capture Oct 27, 2022
@vaind
Copy link
Collaborator Author

vaind commented Oct 27, 2022

@vaind we've decided that the body should not be part of the event because of event limits and PII. ideally, the body is an attachment, but this still requires an RFC because of PII concerns, frontend changes, etc. It'd be part of this feature. I'd not work on this right now unless we want to start with the RFC now.

What about this RFC that (I'm pretty sure I've seen previously linked from the issue)? getsentry/rfcs#22

and that DIO integration already does this?

@marandaneto
Copy link
Contributor

marandaneto commented Oct 27, 2022

@vaind we've decided that the body should not be part of the event because of event limits and PII. ideally, the body is an attachment, but this still requires an RFC because of PII concerns, frontend changes, etc. It'd be part of this feature. I'd not work on this right now unless we want to start with the RFC now.

What about this RFC that (I'm pretty sure I've seen previously linked from the issue)? getsentry/rfcs#22

and that DIO integration already does this?

That RFC does not contain the body because of the given reasons, we'd need to start a new one and get input from different teams, Ingest, security, frontend, etc.
Pretty much proposing a new envelope item type, running data scrubbing, and suggesting frontend changes (how is this going to be displayed with different type of data?)., etc...

@ueman
Copy link
Collaborator

ueman commented Oct 27, 2022

and that DIO integration already does this?

The dio integration predates all of those RFCs, so the concerns in those RFCs aren't taken into account in that.

@vaind
Copy link
Collaborator Author

vaind commented Oct 27, 2022

That RFC does not contain the body because of the given reasons, we'd need to start a new one and get input from different teams, Ingest, security, frontend, etc. Pretty much proposing a new envelope item type, running data scrubbing, and suggesting frontend changes (how is this going to be displayed with different type of data?)., etc...

gotcha. Do you think it makes sense to finish this without the response body then? That's actually already done, I'd just remove the TODO & update test to check the other fields in the response.

@marandaneto
Copy link
Contributor

#624

Yes but this PR should close this Issue and not this Issue, the later is blocked since we should not offer an option to attach the response body yet.
I'd only set the fields from getsentry/rfcs#22 as well.
Ideally, let's also add @Deprecated to the other fields from Response as well, and later fix the new Response protocol for Dio as well, does that make sense?

@vaind
Copy link
Collaborator Author

vaind commented Oct 27, 2022

replaced by #1095

@vaind vaind closed this Oct 27, 2022
@vaind vaind deleted the feat/http-response-body branch October 27, 2022 17:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support response field for the HttpClient integration
4 participants