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

Make the return value of LoggerConfiguration.CreateLogger() concrete #719

Merged
merged 2 commits into from Apr 20, 2016
Merged

Make the return value of LoggerConfiguration.CreateLogger() concrete #719

merged 2 commits into from Apr 20, 2016

Conversation

nblumhardt
Copy link
Member

This PR communicates the dispose requirement of the logging pipeline by returning a concrete disposable type, Logger, from CreateLogger().

One consequence is that using syntax can be applied:

using (var log = new LoggerConfiguration()
    .WriteTo.Console()
    .CreateLogger())
{
    // <app runs here>
}

It's not anticipated that users will generally wrap logger creation in a using block however; rather in 2.0 the CloseAndFlush() method will cover apps that base logging off of the static Log class.

Log.Logger = new LoggerConfiguration()
    .WriteTo.Console()
    .CreateLogger();

// <app runs here>

Log.CloseAndFlush();

Doing this however clarifies at the API level that the logger needs to be flushed in some way. This is important as .NET Core doesn't expose any AppDomain unload-time hooks to trigger flushing, which historically we've relied on.

See also #718, where working around this design issue has lead us astray.

Caveats

This regresses one minor point - a logger with no sinks configured is no longer switched for a SilentLogger behind the scenes; users relying on this behaviour instead need to avoid configuring the logger at all. The alternative - returning something that could be shared in common with SilentLogger, would require one of the less-appealing options discussed in Alternatives below.

Another caveat is that in order to prevent a public-surface-area explosion, the constructors of this type are internal. This should not be a significant issue as ILogger should be the interface used in application code.

Alternatives

Instead of exposing the concrete Logger class, the following options were also available:

  • Return a disposable interface - ILoggingPipeline or similar: disposable interfaces are a semantic wart since only implementations determine the need for disposal
  • Return a new abstract base class - this adds the need for virtual dispatch in more places, for example level checking, which adds up if not kept under control

…e so that disposal requirement is communicated
@merbla
Copy link
Contributor

merbla commented Apr 20, 2016

👍 I like this. It is clear where the responsibility lies. I have been caught out not closing and flushing loggers. This clears up this problem.

:shipit:

@nblumhardt nblumhardt merged commit 3212714 into serilog:dev Apr 20, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants