-
-
Notifications
You must be signed in to change notification settings - Fork 372
ref: Use Swift integrations #6862
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
Conversation
ddc979e to
979af12
Compare
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #6862 +/- ##
=============================================
+ Coverage 84.914% 85.053% +0.138%
=============================================
Files 453 448 -5
Lines 27589 27524 -65
Branches 12113 12052 -61
=============================================
- Hits 23427 23410 -17
+ Misses 4114 4070 -44
+ Partials 48 44 -4
... and 22 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
|
b0d6dfe to
4a01fe6
Compare
Performance metrics 🚀
|
| Revision | Plain | With Sentry | Diff |
|---|---|---|---|
| effeafa | 1225.88 ms | 1249.78 ms | 23.90 ms |
| 483cc24 | 1218.00 ms | 1242.59 ms | 24.59 ms |
| c5d3752 | 1221.51 ms | 1245.63 ms | 24.11 ms |
| 27e7514 | 1229.47 ms | 1245.60 ms | 16.13 ms |
| afaa522 | 1234.71 ms | 1256.19 ms | 21.48 ms |
| 7bd90de | 1233.48 ms | 1249.47 ms | 15.99 ms |
| e5773c1 | 1235.10 ms | 1264.15 ms | 29.04 ms |
| 42a95d5 | 1206.00 ms | 1224.26 ms | 18.26 ms |
| 2c889f6 | 1206.35 ms | 1233.29 ms | 26.94 ms |
| f9270f9 | 1227.90 ms | 1253.24 ms | 25.34 ms |
App size
| Revision | Plain | With Sentry | Diff |
|---|---|---|---|
| effeafa | 23.74 KiB | 926.64 KiB | 902.89 KiB |
| 483cc24 | 24.14 KiB | 1.01 MiB | 1010.98 KiB |
| c5d3752 | 24.14 KiB | 1.01 MiB | 1013.49 KiB |
| 27e7514 | 23.75 KiB | 919.69 KiB | 895.94 KiB |
| afaa522 | 23.74 KiB | 996.91 KiB | 973.17 KiB |
| 7bd90de | 23.75 KiB | 933.33 KiB | 909.58 KiB |
| e5773c1 | 23.75 KiB | 1.00 MiB | 1005.08 KiB |
| 42a95d5 | 23.75 KiB | 906.08 KiB | 882.33 KiB |
| 2c889f6 | 23.75 KiB | 1010.42 KiB | 986.67 KiB |
| f9270f9 | 24.14 KiB | 1.01 MiB | 1012.18 KiB |
Previous results on branch: swiftIntegrations
Startup times
| Revision | Plain | With Sentry | Diff |
|---|---|---|---|
| 8f73e7e | 1210.94 ms | 1248.61 ms | 37.68 ms |
| 149ab6c | 1228.02 ms | 1264.00 ms | 35.98 ms |
| c1ceeee | 1218.08 ms | 1245.14 ms | 27.06 ms |
| 06f5f40 | 1222.22 ms | 1252.16 ms | 29.94 ms |
| 83b15ea | 1194.19 ms | 1220.71 ms | 26.52 ms |
| f2dd1d1 | 1209.92 ms | 1249.12 ms | 39.20 ms |
| 9ef3a04 | 1217.35 ms | 1248.14 ms | 30.79 ms |
| 8ed4b29 | 1192.37 ms | 1216.15 ms | 23.78 ms |
| f71365a | 1222.02 ms | 1248.70 ms | 26.68 ms |
| 255abfa | 1190.61 ms | 1219.54 ms | 28.93 ms |
App size
| Revision | Plain | With Sentry | Diff |
|---|---|---|---|
| 8f73e7e | 24.14 KiB | 1.01 MiB | 1013.45 KiB |
| 149ab6c | 24.14 KiB | 1.01 MiB | 1013.45 KiB |
| c1ceeee | 24.14 KiB | 1.02 MiB | 1016.82 KiB |
| 06f5f40 | 24.14 KiB | 1.02 MiB | 1016.00 KiB |
| 83b15ea | 24.14 KiB | 1.01 MiB | 1013.50 KiB |
| f2dd1d1 | 24.14 KiB | 1.01 MiB | 1012.98 KiB |
| 9ef3a04 | 24.14 KiB | 1.01 MiB | 1015.52 KiB |
| 8ed4b29 | 24.14 KiB | 1.01 MiB | 1013.50 KiB |
| f71365a | 24.14 KiB | 1.01 MiB | 1015.45 KiB |
| 255abfa | 24.14 KiB | 1.01 MiB | 1012.98 KiB |
a35d333 to
10b2476
Compare
10b2476 to
4a2d481
Compare
4a2d481 to
d0cf7ab
Compare
d0cf7ab to
91e9878
Compare
philipphofmann
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Honestly, I'm unsure about this approach. I will have another look tomorrow, but here's what I already found.
add9db2 to
3a17a75
Compare
3a17a75 to
2c7422a
Compare
philipphofmann
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry that it took so long. I had to give this some thought.
Sources/Sentry/include/HybridPublic/SentryIntegrationProtocol.h
Outdated
Show resolved
Hide resolved
ecb08a9 to
acc1dbe
Compare
Sources/Swift/Integrations/UserFeedback/SentryFeedbackAPI.swift
Outdated
Show resolved
Hide resolved
35ce160 to
2ce5d0a
Compare
philipphofmann
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, I still found a view things, but we're getting close to a LGTM
Sources/Swift/Integrations/UserFeedback/SentryFeedbackAPI.swift
Outdated
Show resolved
Hide resolved
Sources/Swift/Integrations/UserFeedback/UserFeedbackIntegration.swift
Outdated
Show resolved
Hide resolved
2ce5d0a to
da8ae24
Compare
0b74c44 to
853000a
Compare
philipphofmann
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, LGTM
Adds a new protocol for integrations that takes dependencies as an input. For the easiest migration conformers of the protocol can satisfy the
Dependenciesassociated type with justtypealias Dependencies = SentryDependencyContainer. This PR shows how it can be used to make the dependencies just define the minimum necessary dependencies so it can be easily mocked in tests. There are a few benefits to this over the current objc protocol:letconstants rather than vars with default nil value. Making these immutable with help with a lot of swift properties like thread safety, and remove the need from handling null cases that should be unreachable.#skip-changelog
Closes #6863