-
-
Notifications
You must be signed in to change notification settings - Fork 372
ref: SentryOptions in Swift #6628
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
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #6628 +/- ##
=============================================
- Coverage 85.145% 85.118% -0.027%
=============================================
Files 453 453
Lines 27689 27706 +17
Branches 12111 12143 +32
=============================================
+ Hits 23576 23583 +7
- Misses 4069 4078 +9
- Partials 44 45 +1
... and 4 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
|
5169bbd to
1b852ec
Compare
031e40c to
3581fc0
Compare
Performance metrics 🚀
|
| Revision | Plain | With Sentry | Diff |
|---|---|---|---|
| 707c222 | 1223.06 ms | 1260.67 ms | 37.61 ms |
| 5200f5b | 1231.73 ms | 1254.35 ms | 22.62 ms |
| ea12acf | 1217.67 ms | 1252.69 ms | 35.03 ms |
| 005f255 | 1218.42 ms | 1247.09 ms | 28.67 ms |
| 2137530 | 1223.39 ms | 1253.25 ms | 29.86 ms |
| ad964ca | 1234.73 ms | 1254.88 ms | 20.15 ms |
| 3cdbc22 | 1231.63 ms | 1251.06 ms | 19.43 ms |
| 11853e6 | 1220.37 ms | 1252.83 ms | 32.46 ms |
| d72784d | 1214.31 ms | 1241.35 ms | 27.04 ms |
| 324c109 | 1228.35 ms | 1252.47 ms | 24.12 ms |
App size
| Revision | Plain | With Sentry | Diff |
|---|---|---|---|
| 707c222 | 23.75 KiB | 1.00 MiB | 1005.07 KiB |
| 5200f5b | 23.75 KiB | 933.33 KiB | 909.58 KiB |
| ea12acf | 23.75 KiB | 974.89 KiB | 951.14 KiB |
| 005f255 | 23.75 KiB | 1.01 MiB | 1016.13 KiB |
| 2137530 | 23.75 KiB | 933.33 KiB | 909.58 KiB |
| ad964ca | 23.75 KiB | 913.17 KiB | 889.42 KiB |
| 3cdbc22 | 23.75 KiB | 928.14 KiB | 904.40 KiB |
| 11853e6 | 23.75 KiB | 1.01 MiB | 1016.04 KiB |
| d72784d | 23.75 KiB | 988.01 KiB | 964.27 KiB |
| 324c109 | 23.75 KiB | 919.91 KiB | 896.16 KiB |
Previous results on branch: optionsInSwift
Startup times
| Revision | Plain | With Sentry | Diff |
|---|---|---|---|
| 607275f | 1199.10 ms | 1225.73 ms | 26.63 ms |
| 4d700d3 | 1226.50 ms | 1255.66 ms | 29.16 ms |
| e2114d5 | 1216.94 ms | 1237.02 ms | 20.08 ms |
| b65d755 | 1234.36 ms | 1258.38 ms | 24.02 ms |
App size
| Revision | Plain | With Sentry | Diff |
|---|---|---|---|
| 607275f | 24.15 KiB | 1.01 MiB | 1015.07 KiB |
| 4d700d3 | 24.15 KiB | 1.01 MiB | 1014.65 KiB |
| e2114d5 | 23.75 KiB | 1.03 MiB | 1.01 MiB |
| b65d755 | 23.75 KiB | 1.03 MiB | 1.01 MiB |
796b619 to
29baa9f
Compare
f2f0e74 to
d0ea6bc
Compare
e29a276 to
4238cf0
Compare
4238cf0 to
4567e9b
Compare
4567e9b to
cfed0e0
Compare
cfed0e0 to
2a4de7b
Compare
philprime
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.
LGTM, left minor comments and some of the tests are not compiling because of the refactoring. I believe it's good to go after fixing them
a3442e1 to
2ae0f7f
Compare
2ae0f7f to
e932b9c
Compare
e932b9c to
04729f2
Compare
143a269 to
f563548
Compare
f563548 to
2d1b0eb
Compare
🚨 Detected changes in high risk code 🚨High-risk code can easily blow up and is hard to test. We had severe bugs in the past. Be extra careful when changing these files, and have an extra careful look at these:
|
This converts SentryOptions.h/.m to SentryOptions.swift and in doing so removes the last blocker for using SPM. It also cleans up a lot of gross bridging we had to do, like with SentryExperimentalOptions and the beforeSendLog callback. This is now replaced with one piece of cross bridging, which I'll get to later...
Almost all the relevant implementation here is in
Options.swift. This matches the old ObjC API almost exactly, except that the class is nowfinal. The API json file changed a lot structurally just because it represents Swift and ObjC differently, not because the actual API changed.The tricky part of this is that we can't call an objc header that uses a swift type from Swift code. (This works in xcodebuild but not cocoapods/SPM) To work around this the value that gets passed to
SentrySDKInternalneeds to be type-erased. This is the same as what we had to do with types defined in Swift on SentryOptions.h prior to this PR. To make it easy to follow this change I've kept the Swift type almost everywhere, but in a few header files I had to useSentryOptionsObjcwhich is just an alias forNSObject. There are extensions at the end ofOptions.swiftthat explains what is done in more detail.#skip-changelog
Closes #6690