-
-
Notifications
You must be signed in to change notification settings - Fork 372
fix: Remove property that had no effect #6646
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
b0e6375 to
d86974c
Compare
❌ 1 Tests Failed:
View the top 1 failed test(s) by shortest run time
To view more test analytics, go to the Test Analytics Dashboard |
d86974c to
2f0e6ee
Compare
Performance metrics 🚀
|
| Revision | Plain | With Sentry | Diff |
|---|---|---|---|
| 4267263 | 1233.13 ms | 1266.13 ms | 33.00 ms |
| 1f48bb7 | 1223.83 ms | 1241.85 ms | 18.02 ms |
| f2bfecd | 1234.92 ms | 1250.34 ms | 15.42 ms |
| cb07a0e | 1217.47 ms | 1251.94 ms | 34.47 ms |
| 3ffd0e5 | 1228.04 ms | 1253.04 ms | 25.00 ms |
| f37e3fe | 1230.24 ms | 1254.51 ms | 24.27 ms |
| fccc4e5 | 1237.80 ms | 1264.16 ms | 26.37 ms |
| a6f5396 | 1211.81 ms | 1245.63 ms | 33.82 ms |
| 04ff3ec | 1220.71 ms | 1253.86 ms | 33.15 ms |
| e64d3d4 | 1241.90 ms | 1260.10 ms | 18.20 ms |
App size
| Revision | Plain | With Sentry | Diff |
|---|---|---|---|
| 4267263 | 23.75 KiB | 988.03 KiB | 964.28 KiB |
| 1f48bb7 | 23.75 KiB | 971.98 KiB | 948.24 KiB |
| f2bfecd | 23.75 KiB | 919.68 KiB | 895.93 KiB |
| cb07a0e | 23.75 KiB | 1.01 MiB | 1016.13 KiB |
| 3ffd0e5 | 23.75 KiB | 947.55 KiB | 923.80 KiB |
| f37e3fe | 23.75 KiB | 919.70 KiB | 895.95 KiB |
| fccc4e5 | 23.75 KiB | 974.94 KiB | 951.19 KiB |
| a6f5396 | 23.75 KiB | 989.12 KiB | 965.38 KiB |
| 04ff3ec | 23.75 KiB | 880.26 KiB | 856.52 KiB |
| e64d3d4 | 23.75 KiB | 855.37 KiB | 831.62 KiB |
Previous results on branch: removeUselessInAppExcludes
Startup times
| Revision | Plain | With Sentry | Diff |
|---|---|---|---|
| acffe26 | 1234.77 ms | 1264.96 ms | 30.18 ms |
| 3394e99 | 1221.17 ms | 1242.04 ms | 20.88 ms |
App size
| Revision | Plain | With Sentry | Diff |
|---|---|---|---|
| acffe26 | 23.75 KiB | 1022.92 KiB | 999.18 KiB |
| 3394e99 | 23.75 KiB | 1022.79 KiB | 999.05 KiB |
|
Instead of removing the field we should fix the usage of it internally. This field also exists for Sentry Java: https://docs.sentry.io/platforms/java/configuration/options/#inAppExcludes |
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.
See feedback above
|
@philprime how would you fix it exactly? The default is for a frame that doesn't appear in inAppInclude or inAppExclude to be considered excluded, so since inAppInclude is checked first, inAppExclude does nothing |
|
Yes, currently sentry-cocoa/Sources/Swift/Core/Helper/SentryInAppLogic.swift Lines 75 to 81 in 2b0eac0
This is just extra logic and confuses users. I'm OK with removing it, but we must also update the docs here https://docs.sentry.io/platforms/apple/guides/ios/configuration/options/#inAppExclude We should also udpate the code comment here sentry-cocoa/Sources/Swift/Core/Helper/SentryInAppLogic.swift Lines 51 to 61 in 2b0eac0
And also this branch still has a couple of occurrences of
|
|
@philipphofmann I just made a PR for the docs: getsentry/sentry-docs#15399 |
2f0e6ee to
c771423
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, @noahsmartin
|
I also created an issue #6677 and added it to the docs PR v9 milestone in Linear https://linear.app/getsentry/project/cocoa-sdk-900-19a03b661aba/overview#milestone-1389bf73-963e-490a-ac6e-3de6df5515fb, so we don't forget to merge it once V9 is out. |
c771423 to
476c923
Compare
476c923 to
7a6765c
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 param did nothing because the default is to consider a library excluded. Since it does nothing, we can remove the property
#skip-changelog
Closes #6647