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

Log score uploader errors to sentry #217

Merged
merged 1 commit into from
Feb 2, 2024

Conversation

bdach
Copy link
Collaborator

@bdach bdach commented Feb 1, 2024

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

Preview of how this looks from a private testing sentry instance I set up

Screenshot 2024-02-01 at 20-13-17 System InvalidOperationException nope — moonshot-incorporated — osu-spectator-server

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,
ppy#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.
@smoogipoo
Copy link
Contributor

Fyi I'm about ready to hit merge on #215, pending testing, because I don't see us moving back to K8s in the short-medium term.

@smoogipoo smoogipoo requested a review from peppy February 1, 2024 19:36
@smoogipoo smoogipoo merged commit ad93d3a into ppy:master Feb 2, 2024
2 checks passed
@bdach bdach deleted the score-upload-sentry-logging branch February 5, 2024 08:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Development

Successfully merging this pull request may close these issues.

2 participants