fix: reduce log size #2680
fix: reduce log size #2680
Conversation
Codecov Report
@@ Coverage Diff @@
## main #2680 +/- ##
==========================================
- Coverage 79.76% 79.71% -0.05%
==========================================
Files 604 605 +1
Lines 21756 21787 +31
Branches 3947 3956 +9
==========================================
+ Hits 17354 17368 +14
- Misses 4350 4367 +17
Partials 52 52
Continue to review full report at Codecov.
|
src/utils/Logger.ts
Outdated
| // If the creation time of the file is more than 28 days ago, delete it. | ||
| RNFS.stat(logFilePath) | ||
| .then((stat) => { | ||
| if (+stat.ctime < +new Date() - 4 * 7 * 24 * 60 * 60 * 1000) { |
There was a problem hiding this comment.
Are we worried about losing valuable log content that could help support fix issues?
Another simple option that avoids this is to use a different log file for each day or week (and delete the old ones). We keep recent content, but can still clean up the old stuff that doesn't matter.
There was a problem hiding this comment.
If the user is sending a log to support in the worst case we would have from the application start to sending the contact support which would likely contain the issue; additionally, since all the transactions are on the chain support can and developers can also be triage via Celo Explorer.
We could create logs for each week or day and combine them when sending to support, we'd just need a bit more logic around combining logs alongside finding and deleting old logs. If this is preferred we can certainly do it either in a follow up PR or adjust this PR.
There was a problem hiding this comment.
It's always annoying to be missing important log content when debugging. Like you said @MuckT there's not so much chance the important info would be missing, but still it can happen.
I like the option of writing a different log files each week. And cleaning up old ones.
We don't necessarily need to combine them when sending to support. We can send the last X log files as individual attachments. But not sure it plays well with the current way we're sending logs via email.
There was a problem hiding this comment.
@jeanregisser I did end up creating log files for each day and combining them; however, given the performance hit of combining several files, can take upwards of 25 seconds, maybe this is a good path to go if we don't implement #2701.
src/utils/Logger.ts
Outdated
| // If the creation time of the file is more than 28 days ago, delete it. | ||
| RNFS.stat(logFilePath) | ||
| .then((stat) => { | ||
| if (+stat.ctime < +new Date() - 4 * 7 * 24 * 60 * 60 * 1000) { |
There was a problem hiding this comment.
It's always annoying to be missing important log content when debugging. Like you said @MuckT there's not so much chance the important info would be missing, but still it can happen.
I like the option of writing a different log files each week. And cleaning up old ones.
We don't necessarily need to combine them when sending to support. We can send the last X log files as individual attachments. But not sure it plays well with the current way we're sending logs via email.
📏 Size AnalysisTotal install size 68.1 MB | This change: ⬆️ +153.2 kB (+0.225%)🗂 See size breakdown
🔎 See the full size analysis (4e1055a) merging into main (b21bc3c) |
src/utils/Logger.ts
Outdated
| const writeLog = async (level: string, message: string) => { | ||
| // If log folder not present create it | ||
| if (!(await RNFS.exists(this.getReactNativeLogsDir()))) { | ||
| await RNFS.mkdir(this.getReactNativeLogsDir()) |
There was a problem hiding this comment.
this could throw, it's a toctou bug. you should handle the error, or have this code as part of log initialization (where's it's guaranteed to be serialized)
There was a problem hiding this comment.
The problem I am seeing testing the change with moving it to the log initialization, out of writeLog up into the overrideConsoleLogs call, is that we miss some of the early logs from the the app start.
There was a problem hiding this comment.
The first line in the daily logs should be Reactotron Configured from index.js if it's not that line we're missing logs from app start. This can be tested by creating a new app install locally and navigating to developer settings and selecting debug.
|
@jean ended up going with the option from #2680 (comment) as creating the combined logs was taking sometimes upwards of 25 seconds. The only thing left should be to cleanup the typings in src/account/SupportContact.tsx. |
|
Hey @MuckT, We have verified above issue on Latest Android Internal Release Build and iOS Test Flight Release build V1.37.0 & updated status in test rails. Observations:
Test rail link: Devices: Google Pixel 4a (12.0), iPhone 12(14.7.1), Google Pixel XL (10.0) Thanks..!! |
|
Hey @MuckT, We have verified above issue on Latest iOS Test Flight Release build V1.37.0 & updated status in test rails. Observations:
Test rail link: Device: iPhone 13 mini(15.1.1) Thanks..!! |
Description
Creates monthly logs files. If a log file is older than 60 days it's deleted on app start. When sending to support all log files are separately attached to the support email.
Other changes
Tested
Tested locally on iOS with a device to known reproduce the issue of failing to send logs to support.
How others should test
Perquisite: An email client present on test device.
Related issues
Backwards compatibility
Yes