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

Sink wrapping #955

Merged
merged 4 commits into from Apr 2, 2017

Conversation

Projects
None yet
3 participants
@johnduhart
Copy link
Contributor

johnduhart commented Mar 27, 2017

In our use of Serilog we'd like to be able to easily wrap existing sinks while also maintaining use of built-in extension methods. For example, we can create a decorator sink that adds the ability to disable sink output during runtime, and can be added to any existing sink.

This PR currently is a first pass on the API for this, so we can discuss the best approach for adding this in. The initial approach adds a new method to LoggerSinkConfiguration which accepts a callback to creating the wrapping Sink.

@nblumhardt

This comment has been minimized.

Copy link
Member

nblumhardt commented Mar 27, 2017

Hi John, thanks for the suggestion. Have you checked out https://github.com/serilog/serilog-sinks-async ? This is a wrapper along these lines.

I think the API there is okay (WriteTo.Async(w => ...) - it would be great to see whether we could improve on the way it's implemented: https://github.com/serilog/serilog-sinks-async/blob/dev/src/Serilog.Sinks.Async/LoggerConfigurationAsyncExtensions.cs - thoughts?

@johnduhart

This comment has been minimized.

Copy link
Contributor Author

johnduhart commented Mar 27, 2017

Hi Nick, wasn't aware of the async sink before, but I agree it's close to what I'm proposing here. My main concern with the implementation of LoggerConfigurationAsyncExtensions is that it requires a new logger pipeline to be instantiated. I'm not familiar with what performance impact that will have, but I would assume it's less expensive to decorate the sink itself.

With an API that makes defining wrapped sinks possible, the implementation of WriteTo.Async could be improved while retaining its existing API.

With the WrapSink API and a change to BackgroundWorkerSink to accept an ILogEventSink, the implementation becomes

        public static LoggerConfiguration Async(
            this LoggerSinkConfiguration loggerSinkConfiguration,
            Action<LoggerSinkConfiguration> configure,
            int bufferSize = 10000)
        {
            return loggerSinkConfiguration.WrapSink(s =>  new BackgroundWorkerSink(s, bufferSize), configure);
        }
@nblumhardt

This comment has been minimized.

Copy link
Member

nblumhardt commented Mar 28, 2017

Thanks for the follow-up. Yes, simplifying the implementation of WriteTo.Async() was what I had in mind 👍

WrapSink() is a helper method in this context; having it public on LoggerSinkConfiguration might add clutter that we can possibly avoid. I wonder if there's another way to expose this?

It's a little bit convoluted, but another option would be to make Wrap static:

return LoggerSinkConfiguration.Wrap(
    loggerSinkConfiguration,
    s => new BackgroundWorkerSink(s, bufferSize),
    configure);

I.e. by making it static and explicitly passing this, we'd avoid anything showing up after WriteTo.. Thoughts?

@johnduhart

This comment has been minimized.

Copy link
Contributor Author

johnduhart commented Mar 28, 2017

I completely agree, the extra method added to WriteTo. was one of the ugly parts of my original proposal. Making it a static method totally works around that though!

I'll update the PR with this approach.

@johnduhart johnduhart force-pushed the johnduhart:wrapped-sinks branch from 2c5e476 to 4982105 Mar 28, 2017

@softwaretecture

This comment has been minimized.

Copy link

softwaretecture commented Mar 28, 2017

To be honest, I'm slightly confused about a method named "Async". It sort of mixes the "async" concept with configuration of the logger. During configuration we don't really activate any I/O bound behaviors, so calling it Async seems very odd to me.

@nblumhardt

This comment has been minimized.

Copy link
Member

nblumhardt commented Mar 28, 2017

@johnduhart great, I'll take another look through ASAP 👍

@softwaretecture thanks for the feedback! I'd rather not go into too much depth on this one here, since we'll derail the thread. I think there might be some discussion in an old ticket on the https://github.com/serilog/serilog-sinks-async repository. Naming things is hard, of course :-) Cheers!

@johnduhart johnduhart changed the title [WIP] Sink wrapping Sink wrapping Mar 30, 2017

@johnduhart

This comment has been minimized.

Copy link
Contributor Author

johnduhart commented Mar 30, 2017

Taking the WIP tag off this, and added documentation to the method.

/// <summary>
/// Helper method for wrapping sinks.
/// </summary>
/// <param name="logggerSinkConfiguration">The parent sink configuration.</param>

This comment has been minimized.

@nblumhardt

nblumhardt Mar 30, 2017

Member

ggg

/// Helper method for wrapping sinks.
/// </summary>
/// <param name="logggerSinkConfiguration">The parent sink configuration.</param>
/// <param name="sinkDecorator">A function that allows for wrapping <see cref="ILogEventSink"/>s

This comment has been minimized.

@nblumhardt

nblumhardt Mar 30, 2017

Member

Since it's an action, and the API uses the term wrap rather than decorate, could we use wrapSink? Also missing a period/full-stop for this sentence.

public static LoggerConfiguration Wrap(
LoggerSinkConfiguration logggerSinkConfiguration,
Func<ILogEventSink, ILogEventSink> sinkDecorator,
Action<LoggerSinkConfiguration> sinkConfigurationAction)

This comment has been minimized.

@nblumhardt

nblumhardt Mar 30, 2017

Member

Ditto here, could be configureWrappedSink.

Func<ILogEventSink, ILogEventSink> sinkDecorator,
Action<LoggerSinkConfiguration> sinkConfigurationAction)
{
var capturingLoggerSinkConfiguration = new LoggerSinkConfiguration(

This comment has been minimized.

@nblumhardt

nblumhardt Mar 30, 2017

Member

Missing argument null checks.

@@ -577,5 +578,19 @@ public string Property
get { throw new Exception("Boom!"); }
}
}

[Fact]
public void WrappedSinks()

This comment has been minimized.

@nblumhardt

nblumhardt Mar 30, 2017

Member

WrappingDecoratesTheConfiguredSink()?

@@ -42,5 +41,15 @@ public static LoggerConfiguration WithDummyThreadId(this LoggerEnrichmentConfigu
{
return loggerSinkConfiguration.Sink(new DummyRollingFileAuditSink(), restrictedToMinimumLevel);
}

public static LoggerConfiguration DummyWrapping(

This comment has been minimized.

@nblumhardt

nblumhardt Mar 30, 2017

Member

WriteTo.Dummy()? :-)

@nblumhardt

This comment has been minimized.

Copy link
Member

nblumhardt commented Mar 30, 2017

Looks great - just a bunch of nitpicks from me.

There's one further twist we might handle, though I'm not sure if there's a clean way. It's a little to easy to accidentally create a wrapper that doesn't implement IDisposable (and forward the Dispose() call to the wrapped sink).

Could we check dynamically to enforce that the wrapped sink (returned from the decorator function) implements IDisposable and write a warning to SelfLog if it does not?

@johnduhart

This comment has been minimized.

Copy link
Contributor Author

johnduhart commented Mar 31, 2017

@nblumhardt Thanks, I'm terrible at writing method documentation.

That's an interesting point regarding IDisposable sinks. I can definitely add support to check for that.

johnduhart added some commits Mar 31, 2017

Wrapped Sinks feedback changes
* Arguments are now checked for null
* Method documentation is better
@nblumhardt

This comment has been minimized.

Copy link
Member

nblumhardt commented Apr 2, 2017

Love it 👍

@nblumhardt nblumhardt merged commit f566978 into serilog:dev Apr 2, 2017

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@nblumhardt nblumhardt referenced this pull request Jun 5, 2017

Merged

2.5.0 Release #980

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.