Refactor to support use as a wrapper sink; add sink options

[nblumhardt]

This sorts out some of the constructor proliferation discussed in #30 and implements the option suggested in #38.

All of the existing behavior is maintained, but consumers are encouraged to switch away from subclassing towards composition. The implementation achieves this currently by "composing" with itself to support the earlier subclassing usage.

Changes are shown in the updated README, copied below.

---

Getting started

Sinks that, for performance reasons, need to emit events in batches, can be implemented using PeriodicBatchingSink
from this package.

First, install the package into your Sink project:

dotnet add package Serilog.Sinks.PeriodicBatching

Then, instead of implementing Serilog's ILogEventSink, implement IBatchedLogEventSink in your sink class:

class ExampleBatchedSink : IBatchedLogEventSink
{
    public async Task EmitBatchAsync(IEnumerable<LogEvent> batch)
    {
        foreach (var logEvent in batch)
            Console.WriteLine(logEvent);
    }
    
    public Task OnEmptyBatchAsync() { }
}

Finally, in your sink's configuration method, construct a PeriodicBatchingSink that wraps your batched sink:

public static class LoggerSinkExampleConfiguration
{
    public static LoggerConfiguration Example(this LoggerSinkConfiguration loggerSinkConfiguration)
    {
        var exampleSink = new ExampleBatchedSink();
        
        var batchingOptions = new PeriodicBatchingSinkOptions
        {
            BatchSize = 100,
            Period = TimeSpan.FromSeconds(2),
            EagerlyEmitFirstEvent = true,
            QueueSizeLimit = 10000
        };
        
        var batchingSink = new PeriodicBatchingSink(exampleSink, batchingOptions);
        
        return loggerSinkConfiguration.Sink(batchingSink);
    }
}

[skomis-mm]

Like the new design that compatible with existing sinks. Looks great 👍

[julealgon]

Very interesting approach with the decorator + adapter implementation at the same time to retain compatibility with the old version.

While still not that big of a fan of introducing a second interface, this approach is significantly better than the previous version and I understand the challenges of minimizing breaking changes: my CompositeLogEvent idea would be a lot more drastic than this.

Couple comments:

1. The settings class is created with invalid values out of the gate: if you pass a fresh new instance to the sink, multiple properties will cause argument validation exceptions to be thrown due to their default values being invalid. I think this is bad design and would suggest reconsidering the strategy. A much more robust approach would be to move the validations to the options class constructor and making it immutable for example. This would also simplify the sink implementation considerably and improve on Law of Demeter and SRP fronts.

2. You ended up replacing one of the old constructors (the overload with the nullable int) with this change. I'm wondering if you considered bumping the major version instead of the minor version, as that's a potential breaking change for consumers that depended on it. In any case, I'd recommend a note about migration from old to new if you have release notes somewhere. Alternatively, you could add back the old constructor overload for compatibility reasons and keep the minor bump.

[skomis-mm]

Hi, @julealgon . As for the second, the constructor with int? isn't in stable version of the package, so it is acceptable to not bump the major version because of it.

[julealgon]

the constructor with int? isn't in stable version of the package

Ahhh I see. That makes sense, thanks for clarifying @skomis-mm !

[nblumhardt]

RE your first point @julealgon - I left this as a bit of a "todo" while thinking about whether default values or a constructor on PeriodicBatchingSinkOptions would be preferable. Pros and cons to each - given the goal of avoiding constructor proliferation long-term I think I'm happy with default values, even if they're completely arbitrary. PR incoming now :-)