Skip to content
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 8 commits into from Dec 5, 2023

Conversation

peppy
Copy link
Sponsor Member

@peppy peppy commented Nov 27, 2023

Will be useful for diagnosing user issues where they already restarted the game and as a consequence, nuked previous logs.

@@ -65,8 +73,7 @@ public static Storage Storage
{
storage = value ?? throw new ArgumentNullException(nameof(value));

// clear static loggers so they are correctly purged at the new storage location.
static_loggers.Clear();
Copy link
Member

@Susko3 Susko3 Nov 27, 2023

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.

Copy link
Sponsor Member Author

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:

CleanShot 2023-11-28 at 08 28 22

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..

Copy link
Sponsor Member Author

@peppy peppy Nov 28, 2023

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:

diff --git a/osu.Framework/Logging/Logger.cs b/osu.Framework/Logging/Logger.cs
index fb7301901..ea492be99 100644
--- a/osu.Framework/Logging/Logger.cs
+++ b/osu.Framework/Logging/Logger.cs
@@ -63,6 +63,8 @@ public class Logger
 
         private static Storage storage;
 
+        private static string storageFullPath = string.Empty;
+
         /// <summary>
         /// The storage to place logs inside.
         /// </summary>
@@ -71,8 +73,20 @@ public static Storage Storage
             private get => storage;
             set
             {
+                if (storage == value)
+                    return;
+
                 storage = value ?? throw new ArgumentNullException(nameof(value));
 
+                string newPath = storage.GetFullPath(string.Empty);
+
+                // If the full path has changed, recreate loggers to correctly purge and
+                // repopulate the log's header.
+                if (newPath == storageFullPath)
+                    static_loggers.Clear();
+
+                storageFullPath = newPath;
+
                 cycleLogs();
             }
         }

Copy link
Contributor

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.

Copy link
Sponsor Member Author

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.

{
scheduler.Add(() =>
DateTime logCycleCutoff = DateTime.UtcNow.AddDays(-7);
var logFiles = new DirectoryInfo(storage.GetFullPath(string.Empty)).GetFiles();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A bit worried about this, in-case storage is set incorrectly it could nuke unrelated files.

How about doing additional checks against the filename? Something like making sure it matches *.{Name}.log?

@@ -65,8 +73,7 @@ public static Storage Storage
{
storage = value ?? throw new ArgumentNullException(nameof(value));

// clear static loggers so they are correctly purged at the new storage location.
static_loggers.Clear();
Copy link
Contributor

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.

@bdach bdach requested a review from smoogipoo December 4, 2023 09:26
@smoogipoo smoogipoo merged commit 5252b00 into ppy:master Dec 5, 2023
20 of 21 checks passed
@peppy peppy deleted the logger-timestamp-prefix branch December 5, 2023 10:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants