-
-
Notifications
You must be signed in to change notification settings - Fork 375
ref: Convert SentryDsn to swift #6942
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 #6942 +/- ##
=============================================
+ Coverage 85.041% 85.042% +0.001%
=============================================
Files 454 454
Lines 27703 27739 +36
Branches 12150 12161 +11
=============================================
+ Hits 23559 23590 +31
- Misses 4100 4106 +6
+ Partials 44 43 -1
... and 3 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
|
|
@sentry review |
Performance metrics 🚀
|
| Revision | Plain | With Sentry | Diff |
|---|---|---|---|
| 7908e84 | 1224.33 ms | 1246.39 ms | 22.06 ms |
| 9c75c11 | 1228.42 ms | 1262.81 ms | 34.39 ms |
| 795dd39 | 1216.88 ms | 1245.47 ms | 28.59 ms |
| ea12acf | 1217.67 ms | 1252.69 ms | 35.03 ms |
| fae97e5 | 1229.20 ms | 1256.27 ms | 27.06 ms |
| 1bf44b4 | 1238.14 ms | 1275.45 ms | 37.31 ms |
| 9389467 | 1218.62 ms | 1244.86 ms | 26.24 ms |
| 781f560 | 1232.83 ms | 1263.56 ms | 30.73 ms |
| ea3f7b6 | 1228.14 ms | 1258.71 ms | 30.57 ms |
| 102cf89 | 1218.31 ms | 1239.78 ms | 21.47 ms |
App size
| Revision | Plain | With Sentry | Diff |
|---|---|---|---|
| 7908e84 | 23.74 KiB | 872.75 KiB | 849.00 KiB |
| 9c75c11 | 23.74 KiB | 1022.95 KiB | 999.20 KiB |
| 795dd39 | 23.75 KiB | 908.16 KiB | 884.41 KiB |
| ea12acf | 23.75 KiB | 974.89 KiB | 951.14 KiB |
| fae97e5 | 23.75 KiB | 912.37 KiB | 888.62 KiB |
| 1bf44b4 | 23.75 KiB | 1021.20 KiB | 997.45 KiB |
| 9389467 | 23.75 KiB | 866.51 KiB | 842.76 KiB |
| 781f560 | 23.75 KiB | 1.02 MiB | 1016.46 KiB |
| ea3f7b6 | 23.75 KiB | 1.01 MiB | 1008.77 KiB |
| 102cf89 | 23.74 KiB | 891.02 KiB | 867.27 KiB |
Previous results on branch: antonis/ref-sentrydsn-swift
Startup times
| Revision | Plain | With Sentry | Diff |
|---|---|---|---|
| 013d4db | 1201.14 ms | 1226.33 ms | 25.19 ms |
| 0db71b7 | 1225.65 ms | 1258.75 ms | 33.10 ms |
| 6d23dcf | 1212.77 ms | 1248.32 ms | 35.55 ms |
| 84f383d | 1216.06 ms | 1259.74 ms | 43.68 ms |
| 7f44bd8 | 1210.07 ms | 1240.93 ms | 30.87 ms |
| 9343623 | 1219.81 ms | 1251.72 ms | 31.91 ms |
| 913d81e | 1229.75 ms | 1259.63 ms | 29.88 ms |
| 7b9a358 | 1215.63 ms | 1247.16 ms | 31.53 ms |
App size
| Revision | Plain | With Sentry | Diff |
|---|---|---|---|
| 013d4db | 24.14 KiB | 1.02 MiB | 1017.01 KiB |
| 0db71b7 | 24.14 KiB | 1.02 MiB | 1020.13 KiB |
| 6d23dcf | 24.14 KiB | 1.02 MiB | 1020.13 KiB |
| 84f383d | 24.14 KiB | 1.02 MiB | 1020.14 KiB |
| 7f44bd8 | 24.14 KiB | 1.02 MiB | 1022.49 KiB |
| 9343623 | 24.14 KiB | 1.02 MiB | 1023.31 KiB |
| 913d81e | 24.14 KiB | 1.02 MiB | 1022.49 KiB |
| 7b9a358 | 24.14 KiB | 1.02 MiB | 1022.49 KiB |
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.
Almost LGTM
|
@philipphofmann @itaybre I had a quick discussion on this with @antonis and @noahsmartin in person and we believe this is a breaking change because everyone importing Do you agree with this approach? |
|
It was decided today in the Cocoa Sync with @itaybre @philipphofmann and myself that we will release this breaking change in a minor release, as the impact is only going to Objective-C users having to remove the |
|
Actually, @antonis can you please add a note to the CHANGELOG about this change with a warning callout at the top of the unreleased section so users adopting the next version are aware we had to do a breaking change? |
Good idea @philprime! I've added a warning with 1764341 |
|
Thank you @antonis for the update. I'll merge this now so we can proceed with a release. |
🚨 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:
|
itaybre
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
📜 Description
Converts SentryDsn to swift
💡 Motivation and Context
See getsentry/sentry-react-native#5273 (comment)
💚 How did you test it?
Ci checks, Sample apps
📝 Checklist
You have to check all boxes before merging:
sendDefaultPIIis enabled.