-
Notifications
You must be signed in to change notification settings - Fork 398
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
Add a session timestamp prefix to log files (and retain 7 days of logs) #6063
Merged
Merged
Changes from all commits
Commits
Show all changes
8 commits
Select commit
Hold shift + click to select a range
bac2d61
Add a session timestamp prefix to log files
peppy 1dd4f15
Remove unnecessary deleting of previous log file
peppy 270c7f4
Cycle logs at startup (keeping 7 days)
peppy ea53982
Remove static logger clearing
peppy cdcddad
Catch any and all errors taht could occur when cycling logs
peppy 03bbab7
Make `Logger.Storage` publicly accessible
peppy 3c917b6
Only delete `.log` files
peppy 97d9dad
Merge branch 'master' into logger-timestamp-prefix
smoogipoo File filter
Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
Just noting this: changing the logger storage when the game is running will no longer write the logger header to the new log files.
Changing the logger storage during runtime is dubious, and the old code is incorrect since it wasn't locking
flush_sync_lock
when accessing a thread-shared resource (static_loggers
).So I'm not against this change. Maybe change the setter to
internal
to reflect that we don't support changing the storage.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.
Unfortunately the setter is used osu! side:
I actually removed this because it was writing the headers more than once to the same file location. I guess I'll add it back and fix that part instead..
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 second thoughts, I guess this depends on what we consider the log header to be. Should it signal the start of a session and therefore not be present on a partial log? I could imagine this being one correct way of looking at it.
Here's a diff to make the log header output correctly after storage changes only, in case that is the preferred direction:
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 consider it as the start of a session, but I wouldn't mind it being output every time.
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 think I'd rather it didn't output every time so we know that some logs might be missing from the start.