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

Start logging logins to osu_logins table #253

Merged
merged 4 commits into from
Jan 8, 2025

Conversation

peppy
Copy link
Member

@peppy peppy commented Jan 6, 2025

This table is mostly used for bookkeeping, but it's worth populating it since there's a few systems which expect the data to be present.

As it stands, users which only play on lazer look as if they never login.

  • Test against full stack (in progress)

@peppy
Copy link
Member Author

peppy commented Jan 6, 2025

Looks to work well:

JetBrains.Rider.2025-01-06.at.06.20.40.mp4

peppy added a commit to peppy/osu-web that referenced this pull request Jan 6, 2025
peppy added 2 commits January 6, 2025 15:36
This table is mostly used for bookkeeping, but it's worth populating it
since there's a few systems which expect the data to be present.

As it stands, users which only play on lazer look as if they never
login.
@peppy peppy force-pushed the log-logins-to-table branch from 479872d to 5b9e1b8 Compare January 6, 2025 06:36
@bdach bdach self-requested a review January 6, 2025 09:08
Comment on lines 187 to 188
// although arguably we should at least be logging failures somewhere because this kind of thing could stop working
// without anyone noticing. same goes for other try-catch methods in this class..
Copy link
Collaborator

Choose a reason for hiding this comment

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

Could log this stuff to sentry but not at error level maybe? Something like (beware, very untested):

diff --git a/osu.Server.Spectator/Database/DatabaseAccess.cs b/osu.Server.Spectator/Database/DatabaseAccess.cs
index 634a1fe..f3fd266 100644
--- a/osu.Server.Spectator/Database/DatabaseAccess.cs
+++ b/osu.Server.Spectator/Database/DatabaseAccess.cs
@@ -7,6 +7,7 @@ using System.Diagnostics;
 using System.Linq;
 using System.Threading.Tasks;
 using Dapper;
+using Microsoft.Extensions.Logging;
 using Microsoft.IdentityModel.JsonWebTokens;
 using MySqlConnector;
 using osu.Game.Online.Metadata;
@@ -19,6 +20,12 @@ namespace osu.Server.Spectator.Database
     public class DatabaseAccess : IDatabaseAccess
     {
         private MySqlConnection? openConnection;
+        private readonly ILogger<DatabaseAccess> logger;
+
+        public DatabaseAccess(ILoggerFactory loggerFactory)
+        {
+            logger = loggerFactory.CreateLogger<DatabaseAccess>();
+        }
 
         public async Task<int?> GetUserIdFromTokenAsync(JsonWebToken jwtToken)
         {
@@ -180,12 +187,9 @@ namespace osu.Server.Spectator.Database
                     IP = userIp
                 });
             }
-            catch (MySqlException)
+            catch (MySqlException ex)
             {
-                // we really don't care about failures in this as it's throwaway logging.
-                //
-                // although arguably we should at least be logging failures somewhere because this kind of thing could stop working
-                // without anyone noticing. same goes for other try-catch methods in this class..
+                logger.LogWarning(ex, "Could not log login for user {UserId}", userId);
             }
         }
 
diff --git a/osu.Server.Spectator/Database/DatabaseFactory.cs b/osu.Server.Spectator/Database/DatabaseFactory.cs
index 282b0fd..37577c0 100644
--- a/osu.Server.Spectator/Database/DatabaseFactory.cs
+++ b/osu.Server.Spectator/Database/DatabaseFactory.cs
@@ -1,10 +1,19 @@
 // Copyright (c) ppy Pty Ltd <[email protected]>. Licensed under the MIT Licence.
 // See the LICENCE file in the repository root for full licence text.
 
+using Microsoft.Extensions.Logging;
+
 namespace osu.Server.Spectator.Database
 {
     public class DatabaseFactory : IDatabaseFactory
     {
-        public IDatabaseAccess GetInstance() => new DatabaseAccess();
+        private readonly ILoggerFactory loggerFactory;
+
+        public DatabaseFactory(ILoggerFactory loggerFactory)
+        {
+            this.loggerFactory = loggerFactory;
+        }
+
+        public IDatabaseAccess GetInstance() => new DatabaseAccess(loggerFactory);
     }
 }
diff --git a/osu.Server.Spectator/Startup.cs b/osu.Server.Spectator/Startup.cs
index f9e5463..081c2ed 100644
--- a/osu.Server.Spectator/Startup.cs
+++ b/osu.Server.Spectator/Startup.cs
@@ -69,7 +69,7 @@ namespace osu.Server.Spectator
                 logging.ClearProviders();
                 logging.AddConsole();
 #if !DEBUG
-                logging.AddSentry();
+                logging.AddSentry(options => options.MinimumEventLevel = LogLevel.Warning);
 #endif
 
                 // IdentityModelEventSource.ShowPII = true;

Not sure how much we care. Maybe best done separately as a bulk change to all of these places.

Copy link
Member Author

Choose a reason for hiding this comment

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

Let's give it a try, what could possibly go wrong.

Copy link
Collaborator

@bdach bdach Jan 8, 2025

Choose a reason for hiding this comment

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

For what it's worth I tested the patch and it does appear to work on my local dev setup (sentry interaction included) so... hopefully works on production too 😅

@bdach bdach enabled auto-merge January 8, 2025 08:31
@bdach bdach merged commit 2d57213 into ppy:master Jan 8, 2025
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants