-
-
Notifications
You must be signed in to change notification settings - Fork 318
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
fix: reimplement crash logs written to file on disk #4061
Conversation
… SentryCrashLOGGER_CBufferSize -> SentryCrashLogger_CBufferSize
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #4061 +/- ##
=============================================
+ Coverage 91.120% 91.156% +0.036%
=============================================
Files 607 610 +3
Lines 47715 48001 +286
Branches 17135 17318 +183
=============================================
+ Hits 43478 43756 +278
- Misses 4144 4152 +8
Partials 93 93
... and 30 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
|
Performance metrics 🚀
|
Revision | Plain | With Sentry | Diff |
---|---|---|---|
efb0147 | 1242.00 ms | 1257.31 ms | 15.31 ms |
ecd9ecd | 1191.76 ms | 1216.92 ms | 25.16 ms |
4d68229 | 1233.50 ms | 1262.92 ms | 29.42 ms |
608755c | 1220.23 ms | 1239.76 ms | 19.52 ms |
2719ce6 | 1211.75 ms | 1237.16 ms | 25.41 ms |
24c8a0f | 1224.65 ms | 1232.69 ms | 8.03 ms |
e998fd0 | 1241.49 ms | 1262.63 ms | 21.14 ms |
230ba67 | 1230.18 ms | 1252.32 ms | 22.13 ms |
881a955 | 1254.14 ms | 1268.43 ms | 14.29 ms |
d914696 | 1225.76 ms | 1238.83 ms | 13.08 ms |
App size
Revision | Plain | With Sentry | Diff |
---|---|---|---|
efb0147 | 22.84 KiB | 403.52 KiB | 380.67 KiB |
ecd9ecd | 20.76 KiB | 420.23 KiB | 399.47 KiB |
4d68229 | 20.76 KiB | 432.34 KiB | 411.58 KiB |
608755c | 20.76 KiB | 436.33 KiB | 415.57 KiB |
2719ce6 | 20.76 KiB | 435.13 KiB | 414.37 KiB |
24c8a0f | 21.58 KiB | 418.69 KiB | 397.11 KiB |
e998fd0 | 21.58 KiB | 414.59 KiB | 393.01 KiB |
230ba67 | 20.76 KiB | 427.35 KiB | 406.59 KiB |
881a955 | 22.85 KiB | 407.63 KiB | 384.79 KiB |
d914696 | 21.58 KiB | 629.83 KiB | 608.25 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.
Looking good so far.
I'm not sure why I opened this as a draft, it should be ready for review. I'm not sure any of the test failures are related. |
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 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.
Again, LGTM.
This is helpful to debug crash handling, which can't be done with the debugger attached. Reading log files is easier than getting the same info out of the system console.
I removed the file-based local level definitions, so that it's completely centralized.
Also moved the macro def that effectively switches between console and file logging to the header so that it can be used to conditionally compile the other parts that set up file-based logging. Those pieces of code have been brought back after being removed in #2440
#skip-changelog