-
-
Notifications
You must be signed in to change notification settings - Fork 352
Breaking: Remove stitchAsyncCode from SentryOption #2973
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
Performance metrics 🚀
|
Revision | Plain | With Sentry | Diff |
---|---|---|---|
ecd9ecd | 1204.67 ms | 1226.46 ms | 21.79 ms |
407ff99 | 1216.63 ms | 1235.50 ms | 18.87 ms |
8cb9355 | 1225.23 ms | 1231.22 ms | 5.99 ms |
102f2a6 | 1225.71 ms | 1244.28 ms | 18.57 ms |
369222e | 1232.14 ms | 1258.90 ms | 26.76 ms |
443723a | 1205.24 ms | 1220.52 ms | 15.28 ms |
5b6694b | 1221.71 ms | 1259.06 ms | 37.35 ms |
7fb7afb | 1235.00 ms | 1256.81 ms | 21.81 ms |
25737cb | 1235.02 ms | 1250.06 ms | 15.04 ms |
32e64d1 | 1249.42 ms | 1264.76 ms | 15.34 ms |
App size
Revision | Plain | With Sentry | Diff |
---|---|---|---|
ecd9ecd | 20.76 KiB | 420.23 KiB | 399.47 KiB |
407ff99 | 20.76 KiB | 427.87 KiB | 407.10 KiB |
8cb9355 | 20.76 KiB | 419.70 KiB | 398.95 KiB |
102f2a6 | 20.76 KiB | 433.18 KiB | 412.42 KiB |
369222e | 20.76 KiB | 419.67 KiB | 398.91 KiB |
443723a | 20.76 KiB | 414.44 KiB | 393.68 KiB |
5b6694b | 20.76 KiB | 426.11 KiB | 405.34 KiB |
7fb7afb | 20.76 KiB | 419.69 KiB | 398.94 KiB |
25737cb | 20.76 KiB | 436.29 KiB | 415.53 KiB |
32e64d1 | 20.76 KiB | 433.18 KiB | 412.42 KiB |
Previous results on branch: breaking/remove-stitch-async
Startup times
Revision | Plain | With Sentry | Diff |
---|---|---|---|
a244492 | 1217.64 ms | 1235.84 ms | 18.20 ms |
8e638fc | 1248.18 ms | 1264.13 ms | 15.95 ms |
7c2daef | 6266.53 ms | 6274.72 ms | 8.19 ms |
91a23c6 | 1261.22 ms | 1268.26 ms | 7.04 ms |
dd1b9cf | 1249.90 ms | 1265.82 ms | 15.92 ms |
App size
Revision | Plain | With Sentry | Diff |
---|---|---|---|
a244492 | 20.76 KiB | 436.33 KiB | 415.57 KiB |
8e638fc | 20.76 KiB | 435.38 KiB | 414.62 KiB |
7c2daef | 20.76 KiB | 436.33 KiB | 415.57 KiB |
91a23c6 | 20.76 KiB | 435.21 KiB | 414.45 KiB |
dd1b9cf | 20.76 KiB | 435.21 KiB | 414.45 KiB |
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.
@brustolin, I think it's okay not to remove all the code added with #998. Ideally, we would remove it, but as we have plans on fixing this feature this year, it's okay to keep it for now.
What about being a bit more strict, so we don't call SentryCrashWrapper.installAsyncHooks or uninstallAsyncHooks by accident during our tests. We could add an NSAssert or something to these methods with an appropriate message to endure nobody enables that feature during tests by accident.
Co-authored-by: Philipp Hofmann <philipp.hofmann@sentry.io>
Good Idea. |
…entry-cocoa into breaking/remove-stitch-async
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
Co-authored-by: Philipp Hofmann <philipp.hofmann@sentry.io>
📜 Description
Removed
stitchAsyncCode
from SentryOption because it is not working.💚 How did you test it?
📝 Checklist
You have to check all boxes before merging:
sendDefaultPII
is enabled.🔮 Next steps