Skip to content

Commit

Permalink
Log score uploader errors to sentry
Browse files Browse the repository at this point in the history
Before you open the diff, a few paragraphs of explanation.

For ASP.NET Core apps, there appear to be three disparate pathways to
getting events onto sentry.

- The first, and most direct one, is directly calling
  `SentrySdk.CaptureException()`. Generally does what you'd expect it
  to.

- The second is via `IWebHostBuilder.UseSentry()`. This is an
  ASP.NET-ism that works by injecting a sentry-provided middleware into
  the request handling stack. Thus, all requests that fail due to a
  thrown exception will be reported to sentry, as the middleware will
  catch the error, log it, and throw it back up to the rest of the
  ASP.NET stack to handle.

  However, note that *this does not apply to background workers*, as
  they are generally outside of this entire flow. Even if we weren't
  hand-rolling them via singletons instantiated in `Startup`,
  and instead using `IHostedService` / `BackgroundService`
  which is the most correct ASP.NET-ism for that, for a proper
  persistent background service *you can't throw exceptions because
  you'd kill both the background service, and the entire server
  with it*.

- Therefore, the third method, and the one officially recommended by the
  sentry team for background worker use cases
  (getsentry/sentry-dotnet#190 (comment))
  is to use sentry's extension of `Sentry.Extensions.Logging`. Doing
  this will middleman all log calls to `Microsoft.Extensions.Logging`
  and push all errors (and warnings too, I believe) to sentry.

In the context of all of the above,
#215 becomes doubly
relevant; however, because that change requires more infra preparations
and we probably want sentry logging on the replay upload process *now*,
this is the minimal required change to make that happen.

A side effect of this change is that the replay upload errors - which
would be printed to stdout via `Console.WriteLine()` - will still be
printed there, but using ASP.NET's default logging format, which is a
little more... talkative, as the example below shows:

	fail: ScoreUploader[0]
	      Error during score upload
	      System.InvalidOperationException: nuh uh
		 at osu.Server.Spectator.Storage.ThrowingScoreStorage.WriteAsync(Score score) in /home/dachb/Documents/opensource/osu-server-spectator/osu.Server.Spectator/Storage/ThrowingScoreStorage.cs:line 12
		 at osu.Server.Spectator.Hubs.ScoreUploader.Flush() in /home/dachb/Documents/opensource/osu-server-spectator/osu.Server.Spectator/Hubs/ScoreUploader.cs:line 117

Additionally, note that we have two *other* background workers like
`ScoreUploader`, and they are: `MetadataBroadcaster` and
`BuildUserCountUpdater`. I don't consider them anywhere as key as replay
upload, therefore they are left as they are until we can get the full
logging unification changes in.
  • Loading branch information
bdach committed Feb 1, 2024
1 parent 54f667d commit ac95336
Show file tree
Hide file tree
Showing 4 changed files with 22 additions and 5 deletions.
7 changes: 6 additions & 1 deletion osu.Server.Spectator.Tests/ScoreUploaderTests.cs
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
using System;
using System.Threading;
using System.Threading.Tasks;
using Microsoft.Extensions.Logging;
using Moq;
using osu.Game.Online.API.Requests.Responses;
using osu.Game.Scoring;
Expand Down Expand Up @@ -33,8 +34,12 @@ public ScoreUploaderTests()
var databaseFactory = new Mock<IDatabaseFactory>();
databaseFactory.Setup(factory => factory.GetInstance()).Returns(mockDatabase.Object);

var loggerFactory = new Mock<ILoggerFactory>();
loggerFactory.Setup(factory => factory.CreateLogger(It.IsAny<string>()))
.Returns(new Mock<ILogger>().Object);

mockStorage = new Mock<IScoreStorage>();
uploader = new ScoreUploader(databaseFactory.Object, mockStorage.Object);
uploader = new ScoreUploader(loggerFactory.Object, databaseFactory.Object, mockStorage.Object);
uploader.UploadInterval = 1000; // Set a high timer interval for testing purposes.
}

Expand Down
7 changes: 6 additions & 1 deletion osu.Server.Spectator.Tests/SpectatorHubTest.cs
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@
using Microsoft.AspNetCore.SignalR;
using Microsoft.Extensions.Caching.Distributed;
using Microsoft.Extensions.Caching.Memory;
using Microsoft.Extensions.Logging;
using Microsoft.Extensions.Options;
using Moq;
using osu.Game.Beatmaps;
Expand Down Expand Up @@ -61,8 +62,12 @@ public SpectatorHubTest()
var databaseFactory = new Mock<IDatabaseFactory>();
databaseFactory.Setup(factory => factory.GetInstance()).Returns(mockDatabase.Object);

var loggerFactory = new Mock<ILoggerFactory>();
loggerFactory.Setup(factory => factory.CreateLogger(It.IsAny<string>()))
.Returns(new Mock<ILogger>().Object);

mockScoreStorage = new Mock<IScoreStorage>();
scoreUploader = new ScoreUploader(databaseFactory.Object, mockScoreStorage.Object);
scoreUploader = new ScoreUploader(loggerFactory.Object, databaseFactory.Object, mockScoreStorage.Object);

var mockScoreProcessedSubscriber = new Mock<IScoreProcessedSubscriber>();

Expand Down
12 changes: 9 additions & 3 deletions osu.Server.Spectator/Hubs/ScoreUploader.cs
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
using System.Collections.Concurrent;
using System.Threading;
using System.Threading.Tasks;
using Microsoft.Extensions.Logging;
using osu.Game.Scoring;
using osu.Server.Spectator.Database;
using osu.Server.Spectator.Database.Models;
Expand All @@ -31,11 +32,16 @@ public class ScoreUploader : IEntityStore, IDisposable
private readonly IScoreStorage scoreStorage;
private readonly CancellationTokenSource cancellationSource;
private readonly CancellationToken cancellationToken;
private readonly ILogger logger;

public ScoreUploader(IDatabaseFactory databaseFactory, IScoreStorage scoreStorage)
public ScoreUploader(
ILoggerFactory loggerFactory,
IDatabaseFactory databaseFactory,
IScoreStorage scoreStorage)
{
this.databaseFactory = databaseFactory;
this.scoreStorage = scoreStorage;
logger = loggerFactory.CreateLogger(nameof(ScoreUploader));

cancellationSource = new CancellationTokenSource();
cancellationToken = cancellationSource.Token;
Expand Down Expand Up @@ -104,7 +110,7 @@ public async Task Flush()
{
if (dbScore == null)
{
Console.WriteLine($"Score upload timed out for token: {item.Token}");
logger.LogError("Score upload timed out for token: {tokenId}", item.Token);
return;
}

Expand All @@ -127,7 +133,7 @@ public async Task Flush()
}
catch (Exception e)
{
Console.WriteLine($"Error during score upload: {e}");
logger.LogError(e, "Error during score upload");
}
}

Expand Down
1 change: 1 addition & 0 deletions osu.Server.Spectator/Startup.cs
Original file line number Diff line number Diff line change
Expand Up @@ -67,6 +67,7 @@ public void ConfigureServices(IServiceCollection services)

logging.ClearProviders();
logging.AddConsole();
logging.AddSentry();

// IdentityModelEventSource.ShowPII = true;
});
Expand Down

0 comments on commit ac95336

Please sign in to comment.