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

Improve observability by integrating sentry, datadog, and adding extended logging #15

Merged
merged 9 commits into from
Dec 31, 2024
Merged
Show file tree
Hide file tree
Changes from 5 commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -40,4 +40,6 @@ For advanced testing purposes.
| `S3_ACCESS_KEY` | A valid Amazon S3 access key ID. | ⚠ In staging/production configs | None |
| `S3_SECRET_KEY` | The secret key corresponding to the `S3_ACCESS_KEY`. | ⚠ In staging/production configs | None |
| `S3_BUCKET_NAME` | The name of the S3 bucket to use for beatmap storage. | ⚠ In staging/production configs | None |
| `SENTRY_DSN` | A valid Sentry DSN to use for logging application events. | ⚠ In staging/production configs | None |
| `DD_AGENT_HOST` | A hostname pointing to a Datadog agent instance to which metrics should be reported. | ⚠ In staging/production configs | None |

114 changes: 78 additions & 36 deletions osu.Server.BeatmapSubmission/BeatmapSubmissionController.cs

Large diffs are not rendered by default.

4 changes: 4 additions & 0 deletions osu.Server.BeatmapSubmission/Configuration/AppSettings.cs
Original file line number Diff line number Diff line change
Expand Up @@ -40,5 +40,9 @@ public static class AppSettings
Environment.GetEnvironmentVariable("S3_BUCKET_NAME")
?? throw new InvalidOperationException("S3_BUCKET_NAME environment variable not set. "
+ "Please set the value of this variable to the name of the bucket to be used for storing beatmaps on S3.");

public static string? SentryDsn => Environment.GetEnvironmentVariable("SENTRY_DSN");

public static string? DatadogAgentHost => Environment.GetEnvironmentVariable("DD_AGENT_HOST");
}
}
4 changes: 3 additions & 1 deletion osu.Server.BeatmapSubmission/DatabaseOperationExtensions.cs
Original file line number Diff line number Diff line change
Expand Up @@ -65,7 +65,7 @@ public static async Task<ulong> GetUserMonthlyPlaycountAsync(this MySqlConnectio
transaction) ?? 0;
}

public static async Task PurgeInactiveBeatmapSetsForUserAsync(this MySqlConnection db, uint userId, MySqlTransaction? transaction = null)
public static async Task<uint[]> PurgeInactiveBeatmapSetsForUserAsync(this MySqlConnection db, uint userId, MySqlTransaction? transaction = null)
{
uint[] beatmapSetIds = (await db.QueryAsync<uint>(@"SELECT `beatmapset_id` FROM `osu_beatmapsets` WHERE `user_id` = @user_id AND `active` = -1 AND `deleted_at` IS NULL",
new
Expand All @@ -86,6 +86,8 @@ await db.ExecuteAsync(@"DELETE FROM `osu_beatmapsets` WHERE `user_id` = @user_id
user_id = userId,
},
transaction);

return beatmapSetIds;
}

public static Task<(uint unranked, uint ranked)> GetUserBeatmapSetCountAsync(this MySqlConnection db, uint userId, MySqlTransaction? transaction = null)
Expand Down
10 changes: 10 additions & 0 deletions osu.Server.BeatmapSubmission/InvariantExceptionFilter.cs
Original file line number Diff line number Diff line change
Expand Up @@ -8,10 +8,20 @@ namespace osu.Server.BeatmapSubmission
{
public class InvariantExceptionFilter : IExceptionFilter
{
private readonly ILogger<InvariantExceptionFilter> logger;

public InvariantExceptionFilter(ILogger<InvariantExceptionFilter> logger)
{
this.logger = logger;
}

public void OnException(ExceptionContext context)
{
if (context.Exception is InvariantException invariantException)
{
context.Result = invariantException.ToResponseObject().ToActionResult();
logger.Log(invariantException.Severity, context.Exception, "Request rejected due to violated invariant");
}
}
}
}
32 changes: 32 additions & 0 deletions osu.Server.BeatmapSubmission/Logging/UserFactory.cs
Original file line number Diff line number Diff line change
@@ -0,0 +1,32 @@
// 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 System.Globalization;
using System.Security.Claims;
using osu.Server.BeatmapSubmission.Authentication;

namespace osu.Server.BeatmapSubmission.Logging
{
public class UserFactory : ISentryUserFactory
{
private readonly IHttpContextAccessor httpContextAccessor;

public UserFactory(IHttpContextAccessor httpContextAccessor)
{
this.httpContextAccessor = httpContextAccessor;
}

public SentryUser? Create()
{
var user = httpContextAccessor.HttpContext?.User;

if (user == null || !user.HasClaim(claim => claim.Type == ClaimTypes.NameIdentifier))
return null;

return new SentryUser
{
Id = user.GetUserId().ToString(CultureInfo.InvariantCulture),
};
}
}
}
5 changes: 4 additions & 1 deletion osu.Server.BeatmapSubmission/Models/InvariantException.cs
Original file line number Diff line number Diff line change
Expand Up @@ -11,9 +11,12 @@ namespace osu.Server.BeatmapSubmission.Models
/// </summary>
public class InvariantException : Exception
{
public InvariantException(string message)
public LogLevel Severity { get; }

public InvariantException(string message, LogLevel severity = LogLevel.Information)
: base(message)
{
Severity = severity;
}

public ErrorResponse ToResponseObject() => new ErrorResponse(Message);
Expand Down
45 changes: 44 additions & 1 deletion osu.Server.BeatmapSubmission/Program.cs
Original file line number Diff line number Diff line change
@@ -1,9 +1,13 @@
using System.Net;
using System.Reflection;
using JetBrains.Annotations;
using Microsoft.AspNetCore.Authentication.JwtBearer;
using Microsoft.Extensions.Options;
using osu.Server.BeatmapSubmission.Authentication;
using osu.Server.BeatmapSubmission.Configuration;
using osu.Server.BeatmapSubmission.Logging;
using osu.Server.BeatmapSubmission.Services;
using StatsdClient;

namespace osu.Server.BeatmapSubmission
{
Expand All @@ -24,7 +28,8 @@ public static void Main(string[] args)
{
logging.ClearProviders();
logging.AddConsole();
// TODO: sentry
if (AppSettings.SentryDsn != null)
logging.AddSentry();
});

builder.Services.AddSingleton<IConfigureOptions<JwtBearerOptions>, OsuWebSharedJwtBearerOptions>();
Expand Down Expand Up @@ -59,10 +64,48 @@ public static void Main(string[] args)
builder.Services.AddHttpClient();
builder.Services.AddTransient<ILegacyIO, LegacyIO>();
builder.Services.AddTransient<IMirrorService, MirrorService>();

if (AppSettings.SentryDsn == null)
{
throw new InvalidOperationException("SENTRY_DSN environment variable not set. "
+ "Please set the value of this variable to a valid Sentry DSN to use for logging events.");
}

if (AppSettings.DatadogAgentHost == null)
{
throw new InvalidOperationException("DD_AGENT_HOST environment variable not set. "
+ "Please set the value of this variable to a valid hostname of a Datadog agent.");
}

break;
}
}

if (AppSettings.SentryDsn != null)
{
builder.Services.AddSingleton<ISentryUserFactory, UserFactory>();
builder.WebHost.UseSentry(options =>
{
options.Environment = builder.Environment.EnvironmentName;
options.SendDefaultPii = true;
options.Dsn = AppSettings.SentryDsn;
});
}

if (AppSettings.DatadogAgentHost != null)
{
DogStatsd.Configure(new StatsdConfig
{
StatsdServerName = AppSettings.DatadogAgentHost,
Prefix = "osu.server.beatmap-submission",
ConstantTags = new[]
{
$@"hostname:{Dns.GetHostName()}",
$@"startup:{DateTimeOffset.UtcNow.ToUnixTimeSeconds()}"
}
});
}

var app = builder.Build();

if (app.Environment.IsDevelopment())
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -83,7 +83,7 @@ public async Task<MemoryStream> PatchBeatmapAsync(
await beatmapStorage.ExtractBeatmapSetAsync(beatmapSetId, tempDirectory.FullName);

if (beatmap.filename == null)
throw new InvalidOperationException("Could not find the old .osu file for the beatmap being modified.");
throw new InvariantException("Could not find the old .osu file for the beatmap being modified.", LogLevel.Warning);

File.Delete(Path.Combine(tempDirectory.FullName, beatmap.filename));

Expand Down
11 changes: 8 additions & 3 deletions osu.Server.BeatmapSubmission/Services/LegacyIO.cs
Original file line number Diff line number Diff line change
Expand Up @@ -12,10 +12,12 @@
public class LegacyIO : ILegacyIO
{
private readonly HttpClient httpClient;
private readonly ILogger<LegacyIO> logger;

public LegacyIO(HttpClient httpClient)
public LegacyIO(HttpClient httpClient, ILogger<LegacyIO> logger)
{
this.httpClient = httpClient;
this.logger = logger;
}

private async Task runLegacyIO(HttpMethod method, string command, dynamic? postObject = null)
Expand All @@ -27,6 +29,9 @@
long time = DateTimeOffset.UtcNow.ToUnixTimeSeconds();
string url = $"{AppSettings.LegacyIODomain}/_lio/{command}{(command.Contains('?') ? "&" : "?")}timestamp={time}";

string? serialisedPostObject = postObject == null ? null : JsonSerializer.Serialize(postObject);
logger.LogDebug("Performing LIO request to {method} {url} (params: {params})", method, url, serialisedPostObject);

try
{
string signature = hmacEncode(url, Encoding.UTF8.GetBytes(AppSettings.SharedInteropSecret));
Expand All @@ -44,7 +49,7 @@

if (postObject != null)
{
httpRequestMessage.Content = new ByteArrayContent(Encoding.UTF8.GetBytes(JsonSerializer.Serialize(postObject)));
httpRequestMessage.Content = new ByteArrayContent(Encoding.UTF8.GetBytes(serialisedPostObject));

Check warning on line 52 in osu.Server.BeatmapSubmission/Services/LegacyIO.cs

View workflow job for this annotation

GitHub Actions / Test

Possible null reference argument for parameter 's' in 'byte[] Encoding.GetBytes(string s)'.
Fixed Show fixed Hide fixed
httpRequestMessage.Content.Headers.ContentType = MediaTypeHeaderValue.Parse("application/json");
}

Expand All @@ -60,7 +65,7 @@
{
if (retryCount-- > 0)
{
Console.WriteLine($"Legacy IO request to {url} failed with {e}, retrying..");
logger.LogError(e, "Legacy IO request to {url} failed, retrying ({retries} remaining)", url, retryCount);
Thread.Sleep(1000);
goto retry;
}
Expand Down
10 changes: 7 additions & 3 deletions osu.Server.BeatmapSubmission/Services/MirrorService.cs
Original file line number Diff line number Diff line change
Expand Up @@ -10,10 +10,12 @@ namespace osu.Server.BeatmapSubmission.Services
public class MirrorService : IMirrorService
{
private readonly HttpClient client;
private readonly ILogger<MirrorService> logger;

public MirrorService(HttpClient client)
public MirrorService(HttpClient client, ILogger<MirrorService> logger)
{
this.client = client;
this.logger = logger;
}

public async Task PurgeBeatmapSetAsync(MySqlConnection db, uint beatmapSetId)
Expand All @@ -29,6 +31,8 @@ public async Task PurgeBeatmapSetAsync(MySqlConnection db, uint beatmapSetId)

private async Task<string?> performMirrorAction(osu_mirror mirror, string action, Dictionary<string, string> data)
{
logger.LogInformation("Performing action {action} (data: {data}) on mirror {mirror}", action, data, mirror.base_url);

data["ts"] = DateTimeOffset.Now.ToUnixTimeSeconds().ToString();
data["action"] = action;
data["cs"] = ($"{data.GetValueOrDefault("s")}{data.GetValueOrDefault("fd")}{data.GetValueOrDefault("fs")}{data.GetValueOrDefault("ts")}"
Expand All @@ -42,9 +46,9 @@ public async Task PurgeBeatmapSetAsync(MySqlConnection db, uint beatmapSetId)
var response = await client.SendAsync(request);
return await response.Content.ReadAsStringAsync();
}
catch (Exception)
catch (Exception e)
{
// TODO: log error
logger.LogError(e, "Attempting to perform action {action} on mirror {mirror} failed", action, mirror.base_url);
return null;
}
}
Expand Down
23 changes: 18 additions & 5 deletions osu.Server.BeatmapSubmission/Services/S3BeatmapStorage.cs
Original file line number Diff line number Diff line change
Expand Up @@ -21,10 +21,12 @@ public class S3BeatmapStorage : IBeatmapStorage
private const string osu_directory = "beatmaps";
private const string versioned_file_directory = "beatmap_files";

private readonly ILogger<S3BeatmapStorage> logger;
private readonly AmazonS3Client client;

public S3BeatmapStorage()
public S3BeatmapStorage(ILogger<S3BeatmapStorage> logger)
{
this.logger = logger;
client = new AmazonS3Client(
new BasicAWSCredentials(AppSettings.S3AccessKey, AppSettings.S3SecretKey),
new AmazonS3Config
Expand Down Expand Up @@ -61,13 +63,15 @@ public async Task StoreBeatmapSetAsync(uint beatmapSetId, byte[] beatmapPackage,

await Task.WhenAll([
uploadBeatmapPackage(beatmapSetId, beatmapPackage, stream),
..uploadAllVersionedFiles(allFiles),
..uploadAllBeatmapFiles(beatmapFiles)
..uploadAllVersionedFiles(beatmapSetId, allFiles),
Copy link
Member

Choose a reason for hiding this comment

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

I know I already let this syntax in (by oversight) but I am not sure I can get used to this. It's quite non-obvious what this is doing.

diff --git a/osu.Server.BeatmapSubmission/Services/S3BeatmapStorage.cs b/osu.Server.BeatmapSubmission/Services/S3BeatmapStorage.cs
index f78441b..cbce08e 100644
--- a/osu.Server.BeatmapSubmission/Services/S3BeatmapStorage.cs
+++ b/osu.Server.BeatmapSubmission/Services/S3BeatmapStorage.cs
@@ -60,11 +60,10 @@ namespace osu.Server.BeatmapSubmission.Services
                     beatmapFiles.Add((beatmapId, contents));
             }
 
-            await Task.WhenAll([
-                uploadBeatmapPackage(beatmapSetId, beatmapPackage, stream),
-                ..uploadAllVersionedFiles(beatmapSetId, allFiles),
-                ..uploadAllBeatmapFiles(beatmapSetId, beatmapFiles)
-            ]);
+            await uploadBeatmapPackage(beatmapSetId, beatmapPackage, stream);
+            await Task.WhenAll(uploadAllVersionedFiles(beatmapSetId, allFiles));
+            await Task.WhenAll(uploadAllBeatmapFiles(beatmapSetId, beatmapFiles));
+
             logger.LogInformation("All file uploads for beatmapset {beatmapSetId} concluded successfully.", beatmapSetId);
         }
 
@@ -165,7 +164,8 @@ namespace osu.Server.BeatmapSubmission.Services
             {
                 foreach (var file in files)
                 {
-                    using var response = await client.GetObjectAsync(AppSettings.S3CentralBucketName, getPathToVersionedFile(BitConverter.ToString(file.File.sha2_hash).Replace("-", string.Empty).ToLowerInvariant()));
+                    using var response = await client.GetObjectAsync(AppSettings.S3CentralBucketName,
+                        getPathToVersionedFile(BitConverter.ToString(file.File.sha2_hash).Replace("-", string.Empty).ToLowerInvariant()));
                     zipWriter.Write(file.VersionFile.filename, response.ResponseStream);
                 }
             }

is probably what i'd stick with.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Replaced with slightly different functional equivalent in 684247d

Copy link
Member

Choose a reason for hiding this comment

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

I'd still just use multiple awaits for better readability, but I'm not going to get hung up on it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I sort of explicitly didn't want to do multiple awaits, as that reduces concurrency 🤷

Copy link
Member

Choose a reason for hiding this comment

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

Fair, I wasn't considering that part.

..uploadAllBeatmapFiles(beatmapSetId, beatmapFiles)
]);
logger.LogInformation("All file uploads for beatmapset {beatmapSetId} concluded successfully.", beatmapSetId);
}

private Task<PutObjectResponse> uploadBeatmapPackage(uint beatmapSetId, byte[] beatmapPackage, MemoryStream stream)
{
logger.LogInformation("Beginning upload of package for beatmapset {beatmapSetId}...", beatmapSetId);
return client.PutObjectAsync(new PutObjectRequest
{
BucketName = AppSettings.S3BucketName,
Expand All @@ -82,8 +86,9 @@ private Task<PutObjectResponse> uploadBeatmapPackage(uint beatmapSetId, byte[] b
});
}

private IEnumerable<Task> uploadAllVersionedFiles(List<byte[]> files)
private IEnumerable<Task> uploadAllVersionedFiles(uint beatmapSetId, List<byte[]> files)
{
logger.LogInformation("Beginning upload of all versioned files for beatmapset {beatmapSetId}...", beatmapSetId);
return files.Select(file => Task.Run(async () =>
{
var fileStream = new MemoryStream(file);
Expand All @@ -104,8 +109,9 @@ await client.PutObjectAsync(new PutObjectRequest
}));
}

private IEnumerable<Task> uploadAllBeatmapFiles(List<(int beatmapId, byte[] contents)> beatmapFiles)
private IEnumerable<Task> uploadAllBeatmapFiles(uint beatmapSetId, List<(int beatmapId, byte[] contents)> beatmapFiles)
{
logger.LogInformation("Beginning upload of all .osu beatmap files for beatmapset {beatmapSetId}...", beatmapSetId);
return beatmapFiles.Select(file => Task.Run(async () =>
{
var fileStream = new MemoryStream(file.contents);
Expand All @@ -128,7 +134,10 @@ await client.PutObjectAsync(new PutObjectRequest

public async Task ExtractBeatmapSetAsync(uint beatmapSetId, string targetDirectory)
{
logger.LogInformation("Retrieving package for beatmap set {beatmapSetId}", beatmapSetId);
using var response = await client.GetObjectAsync(AppSettings.S3BucketName, getPathToPackage(beatmapSetId));
logger.LogInformation("Package for beatmap set {beatmapSetId} retrieved successfully.", beatmapSetId);

// S3-provided `HashStream` does not support seeking which `ZipArchiveReader` does not like.
var memoryStream = new MemoryStream(await response.ResponseStream.ReadAllRemainingBytesToArrayAsync());

Expand All @@ -152,6 +161,8 @@ public async Task<Stream> PackageBeatmapSetFilesAsync(IEnumerable<PackageFile> f
{
var memoryStream = new MemoryStream();

logger.LogInformation("Retrieving requested files...");

using (var zipWriter = new ZipWriter(memoryStream, BeatmapPackagePatcher.DEFAULT_ZIP_WRITER_OPTIONS))
{
foreach (var file in files)
Expand All @@ -161,6 +172,8 @@ public async Task<Stream> PackageBeatmapSetFilesAsync(IEnumerable<PackageFile> f
}
}

logger.LogInformation("Package files retrieved successfully.");

memoryStream.Seek(0, SeekOrigin.Begin);
return memoryStream;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@
<PackageReference Include="BouncyCastle.Cryptography" Version="2.5.0" />
<PackageReference Include="Dapper" Version="2.1.35" />
<PackageReference Include="Dapper.Contrib" Version="2.0.78" />
<PackageReference Include="DogStatsD-CSharp-Client" Version="8.0.0" />
<PackageReference Include="Microsoft.AspNetCore.Authentication.JwtBearer" Version="8.0.10" />
<PackageReference Include="Microsoft.AspNetCore.OpenApi" Version="8.0.10"/>
<PackageReference Include="ppy.osu.Game" Version="2024.1130.0" />
Expand All @@ -24,6 +25,7 @@
<PackageReference Include="ppy.osu.Game.Rulesets.Osu" Version="2024.1130.0" />
<PackageReference Include="ppy.osu.Game.Rulesets.Taiko" Version="2024.1130.0" />
<PackageReference Include="ppy.osu.Server.OsuQueueProcessor" Version="2024.1111.0" />
<PackageReference Include="Sentry.AspNetCore" Version="5.0.0" />
<PackageReference Include="Swashbuckle.AspNetCore" Version="7.1.0" />
<PackageReference Include="Swashbuckle.AspNetCore.ReDoc" Version="7.1.0" />
</ItemGroup>
Expand Down
Loading