-
-
Notifications
You must be signed in to change notification settings - Fork 341
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
chore: Duplicate ANRTracker classes #4262
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #4262 +/- ##
=============================================
+ Coverage 91.542% 91.568% +0.026%
=============================================
Files 612 616 +4
Lines 49470 49886 +416
Branches 17916 18044 +128
=============================================
+ Hits 45286 45680 +394
- Misses 4091 4113 +22
Partials 93 93
... and 4 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
|
Performance metrics 🚀
|
Revision | Plain | With Sentry | Diff |
---|---|---|---|
e998fd0 | 1241.49 ms | 1262.63 ms | 21.14 ms |
b15521e | 1221.69 ms | 1237.51 ms | 15.82 ms |
ef5821b | 1253.18 ms | 1265.46 ms | 12.28 ms |
533c68f | 1236.54 ms | 1256.68 ms | 20.14 ms |
4ae9b7a | 1223.94 ms | 1243.90 ms | 19.96 ms |
5230990 | 1222.86 ms | 1243.59 ms | 20.73 ms |
bd2afa6 | 1241.37 ms | 1246.20 ms | 4.83 ms |
189b629 | 1211.16 ms | 1224.30 ms | 13.14 ms |
3f1be0f | 1208.12 ms | 1225.72 ms | 17.60 ms |
3de9971 | 1224.38 ms | 1245.33 ms | 20.95 ms |
App size
Revision | Plain | With Sentry | Diff |
---|---|---|---|
e998fd0 | 21.58 KiB | 414.59 KiB | 393.01 KiB |
b15521e | 21.58 KiB | 573.17 KiB | 551.59 KiB |
ef5821b | 21.58 KiB | 414.96 KiB | 393.37 KiB |
533c68f | 21.58 KiB | 630.28 KiB | 608.70 KiB |
4ae9b7a | 21.58 KiB | 632.04 KiB | 610.46 KiB |
5230990 | 21.58 KiB | 682.39 KiB | 660.81 KiB |
bd2afa6 | 20.76 KiB | 420.55 KiB | 399.79 KiB |
189b629 | 20.76 KiB | 399.69 KiB | 378.93 KiB |
3f1be0f | 20.76 KiB | 414.44 KiB | 393.69 KiB |
3de9971 | 21.58 KiB | 574.17 KiB | 552.59 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.
Looks good. There's not much to review other than the duplication. However, I would keep this PR open and merge the other changes related to ANR into this one, so we don't make a release with this duplication.
Something that I believe fits this PR is a protection to avoid enabling both integrations at the same time. Users should either use V1 or V2
The downside with that approach is that we could run into possible merge conflicts, and I need to merge main multiple times into this branch. I wouldn't worry too much about the duplication. It's around 500 lines of code.
The option V2 is currently in SentryOptions+Private.h, so nobody can enable it. I will ensure that this can't happen in an upcoming PR. Then it's also easier to review. Furthermore, we might just swap V1 with V2 and not offer the possibility of having both. |
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.
I would prefer that some changes be made to the classes already. I will leave this to your discretion.
This is the first step for #3492, which is part of the EPIC AppHang improvements #4261.
The changes for adding the option to ignore non-fully blocking app hangs are bloating up, and the PR would be huge. Instead, we first duplicate the AppHangTracker classes, and then we make smaller digestible PRs. Once we're happy with the AppHang changes, we delete the old V1 version and replace it with the new one. We could also run a beta with the V2 first before changing the algorithm for everyone.
#skip-changelog