Skip to content

Conversation

nblumhardt
Copy link
Member

Serilog sinks push for factory methods for sink construction and composition (WriteTo.File()), rather than an "OO" style of public sink types and constructors (new FileSink(...)).

The existing LoggerSinkConfiguration.Wrap() method is a helper used by wrapping sink implementations, such as Serilog.Sinks.Async, and the core WriteTo.Conditional() method, to enclose one or more sinks in a "wrapper" that modifies their behavior in some way.

The existing Wrap() method has three roles: providing access to the enclosed sinks created via their factory methods, delegating IDisposable.Dispose() to the enclosed sinks so that wrappers don't need to implement it explicitly, and applying a LoggingLevelSwitch/minimum level to the completed result.

This PR pulls out the two main responsibilities, so that it's now possible to create an ILogEventSink without constructing the full logger pipeline (previously only possible using a hack based on the Wrap() call), and it's separately possible to wrap a given sink instance.

Creating sinks is performed using LoggerSinkConfiguration.CreateSink():

var sink = new LoggerSinkConfiguration.CreateSink(wt => wt.File(...));
// `sink` is now a `FileSink` or similar

Wrapping uses a new overload of LoggerSinkConfiguration.Wrap():

var wrapper = LoggerSinkConfiguration.Wrap(
    enclosed => new Wrapper(enclosed),
    wt => wt.File(...));

Although it would be possible to simplify this further into Wrap(sink, wrapper), i.e. fully-construct both sink and wrapper, and then just perform disposal delegation in Wrap():

  • the equal types of the two sink arguments would make usage error-prone,
  • almost all usage needs to apply a WriteTo callback anyway, and
  • it's possible to use pre-constructed sinks and wrappers with this API if necessary.

Finally, the LoggingLevelSwitch and minimum level responsibilities are now handed off to WriteTo.Sink(). Since CreateSink() and Wrap() just return ILogEventSink, there's no need for either of those to have any interaction with levelling.

Why make these changes now? With the introduction of WriteTo.Batched(), code that previously relied on constructing PeriodicBatchingSink will be ported over to it. Unlike the older API, no sink class is exposed - batching is only available through the factory method. This makes porting nontrivial usage of PBS harder. CreateSink() fixes this.

@nblumhardt nblumhardt added the v4 label May 10, 2024
s => new DelegatingSink(s.Emit),
w => w
.Sink(enclosed)
.Enrich.WithProperty(propertyName, 1));
Copy link
Member

Choose a reason for hiding this comment

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

Is there any way to set a tripwire for this? While it seems obvious, I would bet money I've spent time expecting this to work late some evening! (obv that would have downsides too)

Copy link
Member Author

Choose a reason for hiding this comment

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

I think it should be possible. After the creation/wrapping callback runs, some internal assertion could be implemented through the LoggerConfiguration-related types. May be quite a bit of code, perhaps out of scope for this PR, if consider this one to be a new face on existing functionality?

nblumhardt and others added 3 commits May 12, 2024 07:09
Co-authored-by: Ruben Bartelink <ruben@bartelink.com>
Co-authored-by: Ruben Bartelink <ruben@bartelink.com>
Co-authored-by: Ruben Bartelink <ruben@bartelink.com>
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.

2 participants