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

WriteTo.Sink(IBatchedLogEventSink, BatchingOptions, ...) #2055

Merged
merged 3 commits into from
May 5, 2024

Conversation

nblumhardt
Copy link
Member

@nblumhardt nblumhardt commented May 2, 2024

This code is being moved from https://github.com/serilog/serilog-sinks-periodicbatching

Fixes #2053, allowing Serilog.Sinks.PeriodicBatching's API to be frozen in its backwards-compatible shape.

This PR makes writing asynchronous, batched sinks as straightforward as regular ILogEventSink. This is achieved with a new interface, IBatchedLogEventSink, which closely mirrors the one exposed by Serilog.Sinks.PeriodicBatching for the past few years:

public interface IBatchedLogEventSink
{
    Task EmitBatchAsync(IReadOnlyCollection<LogEvent> batch);
    Task OnEmptyBatchAsync() { return Task.CompletedTask; }
}

Implementing a batched sink is easy:

class MyBatchedSink: IBatchedLogEventSink
{
    public async Task EmitBatchAsync(IReadOnlyCollection<LogEvent> batch)
    {
        foreach (var logEvent in batch)
        {
            await Console.WriteLineAsync(logEvent.Level);
        }
    }
}

And wiring it in is familiar:

Log.Logger = new LoggerConfiguration()
    .WriteTo.Sink<MyBatchedSink>(new { BatchSizeLimit = 10 })
    .CreateLogger();

Sink packages that need batching behind-the scenes will wrap up the WriteTo.Sink() call in their own configuration methods.

Design notes

System.Threading.Channels dependency: this is needed on older platforms; modern .NET has this library in-the-box, leaving Serilog dependency-free on .NET 6, .NET 8, etc.

WriteTo.Sink() vs WriteTo.BatchedSink() etc: the choice here is part in preparation for a potential IAsyncLogEventSink, which would work in a similar manner, and provide an implementation for a WriteTo.Async() style of API.

BatchingOptions pass by reference rather than callback: while many .NET Core APIs use an options => options.X style of configuration API, Serilog currently does this only when the callback argument supports WriteTo.X() style syntax. Passing the BatchingOptions object directly should keep the usage of callback arguments in the Serilog API consistent.

Migrating from Serilog.Sinks.PeriodicBatching

  • IBatchedLogEventSink.EmitBatchAsync() accepts IReadOnlyCollection<LogEvent> instead of IEnumerable<LogEvent>: this makes it possible to access the batch size without enumeration (or needing less-widely-supported enumeration APIs)
  • IBachedLogEventSink.OnEmptyBatchAsync() has a default implementation on platforms that support it.
  • BatchingSink is non-public; WriteTo.Sink() is used instead of explicitly constructing a wrapper sink
  • BatchingOptions calls the Period property BufferingTimeLimit

Copy link
Member

@bartelink bartelink left a comment

Choose a reason for hiding this comment

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

You know you should separate the collection initializer changes etc in order to make it more legible for other reviewers!
This reviewer is very impressed (but not surprised given the effort that went into the impl already) with impl cleanliness, comments quality, and xmldoc quality though

@nblumhardt
Copy link
Member Author

Thanks for the review, @bartelink! 🙏

Yes, sorry about that hanger-on collection initializer commit 😅 - opened up the project and the squigglies immediately got on my nerves. Must... resist... !!!

I'm looking forward to seeing where this one takes us.

@@ -17,11 +17,15 @@ namespace Serilog.Core;
/// <summary>
/// A destination for log events.
/// </summary>
/// <seealso cref="ILogEventSink"/>
Copy link
Contributor

Choose a reason for hiding this comment

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

IBatchedLogEventSink here?

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.

WriteTo.Batched() and IBatchedLogEventSink
3 participants