Re-implement sink using IBatchedLogEventSink

[bcallaghan-et]

Solves #77. This is a semver-major breaking change that allows the email sink to be used with alternative batching algorithms, rather than just the current PeriodicBatchingSink.

[nblumhardt]

Thanks for your PR, @bcallaghan-et ; I think this has the effect of removing batching from the sink by default, which might not be intended? There's another example of how IBatchedLogEventSink is used, here: https://github.com/serilog/serilog-sinks-seq/blob/dev/src/Serilog.Sinks.Seq/SeqLoggerConfigurationExtensions.cs#L109

In particular, the Sink() call needs to be passed a PeriodicBatchingSink instance that wraps the target IBatchedLogEventSink, and the batched sink should no longer need to implement ILogEventSink.

Hope this helps! Nick

[bcallaghan-et]

My intention was to make the user explicitly specify how batching should occur. The use of ILogEventSink was to make it compatible with wrapper sinks that use LoggerSinkConfiguration.Wrap as used here. This also has a side-effect that emails can be generated for each and every log event. I don't find that particularly useful, but somebody else might.

@nblumhardt Are you recommending that I remove ILogEventSink so that it can only be used with batching sinks?

[nblumhardt]

Hi Brian, thanks for the follow-up.

The design is interesting :+1: The sink is already used very widely, so the impact of changes on existing users needs consideration, though. Keeping backwards compatibility is generally the default for this project, unless there's a big win to be made.

Wrap() would be a nice way to achieve flexible batching, and I think the whole approach deserves more investigation, but it's not usable from configuration (JSON or <appSettings>), for example.

[bcallaghan-et]

@nblumhardt Are you saying that my approach is not usable from configuration or that Wrap is not usable from configuration? The standard Async sink uses Wrap, and it works with JSON configuration. I've also written my own (closed-source) batching sink that uses Wrap to send batches to the email sink I propose here, again with JSON configuration.

Admittedly, I made these changes in order to address a specific use case within my company, and the changes have been quite successful there. If this requires changes to be useful for a broader audience, I will help where I can.

[nblumhardt]

Thanks for the reply. Neither the approach nor direct Wrap() can currently be used from configuration. The existing email sink supports batching directly, so when users upgrade to the new version, it'd be normal to expect that their existing configurations will continue to work in a batched way. Hope I'm not missing anything - if I've overlooked anything please let me know :-)

[bcallaghan-et]

@nblumhardt You are correct that my approach does not support batching directly. I favor the approach that consumers specify how batching occurs. This is a breaking change, as stated at the top of the PR. How do you propose that we let consumers specify their own batching without breaking other, existing consumers who are fine with the current behavior?

[ericvb]

@bcallaghan-et If I read and understand your code proposal correctly, the user must create his own inherited class where he must implement the desired batching algorithm? Imagine that your PR is implemented and a user installs the nuget package and configures it by configuration, he will get a mail per logging event by default without his own implementation, correct?

If we go this way, I propose to use an virtual method with the PeriodicBatchingSink implemented, called by the LoggerConfigurationEmailExtension. By this way, the users will have the PeriodicBatchingSink behaviour like before. At the same time, I should also implement the new configuration option EagerlyEmitFirstEvent from the PeriodicBatchingSink! This makes the email sink much more useable. If the user wants a different BatchingSink, he must just override the method with his own implementation in his new derived emailsink class.

@nblumhardt and @bcallaghan-et What do you think?

[bcallaghan-et]

@ericvb My proposal would require users to supply their own batching algorithm. This could be something they write on their own, or they could use one from another NuGet package. For example, a user could reconstruct the existing behavior by supplying a PeriodicBatchingSink that is then passed the EmailSink as the inner sink. Another example (which I use in many internal projects) is a batching algorithm that maintains a buffer of past messages, and emits all messages simultaneously when an Error-level event occurs, similar to the SmtpAppender of log4net. The batching algorithm must implement ILogEventSink and must wrap an inner sink that implements IBatchedLogEventSink. If no batching algorithm is supplied, then yes, an email would be generated for each and every log event.

IMO, we should not have a default batching algorithm on the EmailSink. The new interface IBatchedLogEventSink is meant to replace the existing pattern of inheriting from PeriodicBatchingSink. Notice that the documentation for PeriodicBatchingSink no longer mentions inheritance. The protected constructors also include the phrase "New code should avoid subclassing". Yes, this is a breaking change, and yes, users that want to keep the current functionality will have to modify their configuration. However, this gives users more control over their logging, and it allows batching and emailing to evolve separately. With explicit control, users could make use of the option EagerlyEmitFirstEvent or any other options that may be added to batching without modifying the EmailSink.

[nblumhardt]

Hi @bcallaghan-et - thanks for the follow up. If we were starting from scratch I'd be on board with batching being an external concern, but, the support load it would generate rules this out right now. Making the default work as before (sans eager first event) seems like the right way forward.

From that point on, though, extending Serilog.Sinks.PeriodicBatching with a WriteTo.PeriodicBatching(wt => wt.Email()), utilizing Wrap(), would be a great improvement in the overall composability of sinks, and help us along towards making batching a properly-orthogonal, first-class concept.

Appreciate all the thought going into this, thanks for the help :+1:

[ericvb]

@nblumhardt I'm not sure what's next now?

If we were starting from scratch I'd be on board with batching being an external concern, but, the support load it would generate rules this out right now.

It maybe me, but I don't understand the sentence :-( I think you mean to convert the current email implementation anyway to use the interface IBatchedLogEventSink and IDisposable and to implement by default the PeriodicBatching like this?

        public static LoggerConfiguration Email(
            this LoggerSinkConfiguration loggerConfiguration,
            EmailConnectionInfo connectionInfo,
            ITextFormatter textFormatter,
            LogEventLevel restrictedToMinimumLevel = LevelAlias.Minimum,
            int batchPostingLimit = DefaultBatchPostingLimit,
            bool eagerlyEmitFirstEvent = false,
            TimeSpan? period = null,
            string mailSubject = EmailConnectionInfo.DefaultSubject)
        {
            if (connectionInfo == null) throw new ArgumentNullException("connectionInfo");
            if (textFormatter == null) throw new ArgumentNullException("textFormatter");

            ITextFormatter mailSubjectFormatter = new MessageTemplateTextFormatter(mailSubject, null);

            var defaultedPeriod = period ?? DefaultPeriod;

            var batchingOptions = new PeriodicBatchingSinkOptions
            {
                BatchSizeLimit = batchPostingLimit,
                Period = defaultedPeriod,
                EagerlyEmitFirstEvent = eagerlyEmitFirstEvent,
                QueueLimit = 10000
            };
            var batchingSink = new PeriodicBatchingSink(new EmailSink(connectionInfo, textFormatter, mailSubjectFormatter), batchingOptions);

            return loggerConfiguration.Sink(batchingSink, restrictedToMinimumLevel);
        }

I did that locally here to test and it is working well without breaking the current behaviour of the email sink. And with the EagerlyEmitFirstEvent only one mail! I can do a pull request if you want?

I don't have enough knowledge about the internal serilog implementation, I'm just scratching the surface :-) So, I'm a little bit lost at extending Serilog.Sinks.PeriodicBatching with a WriteTo.PeriodicBatching(wt => wt.Email()), utilizing Wrap().

[nblumhardt]

Hi @ericvb!

the support load it would generate rules this out right now.

I mean, if we shipped the PR as is, and effectively turned off batching for everyone who currently uses this sink, I'd spend hours/days/weeks dealing with requests for help from people whose email sink has broken 😄

The challenge with popular packages like this one is that while all is quiet when the going is good, breakages = major headaches once they impact thousands of users.

I think the code you've posted looks good; I don't think we need to allow configuration of the eagerlyEmitFirstEvent option, and should probably keep the signature exactly the same as the current one, but default EagerlyEmitFirstEvent to false.

I linked another similar change in https://github.com/serilog/serilog-sinks-email/pull/78#issuecomment-622190944

RE:

From that point on, though, extending Serilog.Sinks.PeriodicBatching with a WriteTo.PeriodicBatching(wt => wt.Email()), utilizing Wrap(), would be a great improvement in the overall composability of sinks, and help us along towards making batching a properly-orthogonal, first-class concept.

Probably safe to ignore these musings for now :-)

Non-breaking PR that converted to IBatchedLogEventSink would be most welcome 👍

@bcallaghan-et I think we should close this PR for now; sorry we ended up heading in a different direction, but thanks again!