-
Notifications
You must be signed in to change notification settings - Fork 2
feat: save app and ldk logs to same file #713
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
base: master
Are you sure you want to change the base?
Conversation
ovitrif
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.
@claude attend review comments.
This comment has been minimized.
This comment has been minimized.
This comment was marked as resolved.
This comment was marked as resolved.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
ed51c18 to
bfa9a1f
Compare
- Both APP and LDK loggers now write to same file (bitkit_<timestamp>.log) - Logs are automatically in chronological order due to shared file - File writes serialized via single-threaded coroutine queue - APP/LDK tag prefixes distinguish log sources
Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
b4989ce to
778ca10
Compare
|
|
||
| val Logger = AppLogger() | ||
|
|
||
| class AppLogger(private val source: LogSource = LogSource.Bitkit) { |
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.
nit: this source parameter is not used anymore
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.
nit: could make LoggerImpl
|
@claude address review points of jvsena |
This comment has been minimized.
This comment has been minimized.
Co-authored-by: Ovi Trif <ovitrif@users.noreply.github.com>
| private const val TAG = "Logger" | ||
| private var saver: LogSaver? = null | ||
|
|
||
| fun getOrCreateSaver(): LogSaver { |
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 see this method is called by two classes on init(). could have a race condittion
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.
but as far as I can see, it would just replace the instance. WDYT?
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.
nit: Ldk LogSource is not used anymore, could be removed
| fun reset() { | ||
| warn("Wiping entire logs directory…", context = TAG) | ||
| runCatching { Env.logDir.deleteRecursively() } | ||
| saver = null | ||
| delegate = runCatching { createDelegate() }.getOrNull() | ||
| } |
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.
on app wipe, reset() could race with getOrCreateSaver()
jvsena42
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.
Worth take a look on the race condition comments
Closes #655
Summary
Both APP and LDK logs now write to the same file in chronological order.
Changes
bitkit_<timestamp>.logPreview
Testing
Or, as a dev just go to device explorer and the app's data folder and open that file in Android Studio to check.