Introduce a new IBatchTextFormatter for handling batches of log events

0xced commented 3 years ago

This is useful if you want to format log events inside a table of an html email for example.

This new interface allows you to write a header, e.g. <html><body><table> before writing a batch of log events and a footer, e.g. </table></body></html>after the batch of log events is written.

0xced commented 2 years ago

I would love to see this merged because my current workaround is terrible and somewhat broken. Any chance you to have a look at that pull request, @nblumhardt?

nblumhardt commented 2 years ago

Howdy! Thanks for the PR :pray:

Maintenance time on this repo is unfortunately in very short supply, and we have a bit of a backlog of issues to wade through, making it difficult to accept new features while we're not on top of the features we already have :-)

One thing that would make this much easier to merge is if the tests were able to run in CI, without a mail server (some internally-visible interface/types may be needed); is there any chance you could give this a try, and drop out the Skip from the added test (and possibly the others above?).

0xced commented 2 years ago

if the tests were able to run in CI, without a mail server (some internally-visible interface/types may be needed)

Actually, @mgrosperrin already did all the work required by introducing an IEmailTransport interface in #55.

Writing tests with an intercepting implementation of IEmailTransport would be trivial with #55 merged. 😉

nblumhardt commented 2 years ago

@0xced thanks - I pinged the older PR thread, will see if we can get that one through somehow! 👍

0xced commented 2 years ago

With #98 being merged, I was able to write a test that doesn't require a SMTP server. 🥳

nblumhardt commented 2 years ago

🎉 awesome :-)

Just had a thought - perhaps we'd leave a little more room for flexibility if we renamed IBatchTextFormatter into IWriteHeaderAndFooter (or a name with similar connotations) and make it not derive from ITextFormatter?

The main benefit would be that if we want to add more capabilities in the future, we can add similar interfaces and not be forced into the dilemma of whether they should inherit ITextFormatter or IBatchTextFormatter.

Since the code only touches WriteHeader and WriteFooter on this interface, this would seem to be in sync with what the implementation needs from the interface. What do you think?

0xced commented 2 years ago

The problem is the signature of the Email logger sink configuration extension method:

public static LoggerConfiguration Email( this LoggerSinkConfiguration loggerConfiguration, EmailConnectionInfo connectionInfo, ITextFormatter textFormatter, LogEventLevel restrictedToMinimumLevel = LevelAlias.Minimum, int batchPostingLimit = DefaultBatchPostingLimit, TimeSpan? period = null, string mailSubject = EmailConnectionInfo.DefaultSubject)

How should we pass the formatter if we don't inherit from ITextFormatter? Did you had something in mind?

nblumhardt commented 2 years ago

IWriteHeaderAndFooter would be treated as an optional interface; it'd only have those two members on it (WriteHeader and WriteFooter).

This would mean that custom batched footers would implement both ITextFormatter (required) and IWriteHeaderAndFooter (optional), so they would always be able to be passed directly through the ITextFormatter formatter argument.

0xced commented 2 years ago

Thanks, I see now.

I totally understand your concern from a purely technical point of view. But from a business point of view, can you think of what additional capabilities would make sense?

Having IBatchTextFormatter inheriting ITextFormatter looks simpler to me (also from a documentation point of view) but I'm happy adapting my pull request to have separate interfaces if you think it's a better way to go.

nblumhardt commented 2 years ago

Sorry about the slow reply. Here's the line of thinking that I was on:

Imagine we decide to switch the body content type based on whether it should be plain text, or HTML, so that HTML's special characters < etc. are always handled properly by the receiving mail client. In that case we might add an optional IHtmlSafeFormatter marker interface for formatters that guarantee valid HTML output, and otherwise, default to sending plain text email bodies.

If we continued down the inheritance path at that point, we'd have to decide whether IHtmlSafeFormatter should derive from ITextFormatter or IBatchTextFormatter. If we take the interfaces-as-decorators path, things would stay consistent - formatters could choose to implement any combination of ITextFormatter, IWriteHeaderAndFooter, and IHtmlSafeFormatter.

Just picked this scenario off the top of my head; but you can imagine that given enough time, we'll end up supporting more capabilities once we start :-)

0xced commented 2 years ago

I have addressed this concern in b8bea892d3ffae3daca6ceba4ef1c2f4ae95febc and adapted the documentation accordingly.

I have not renamed IBatchTextFormatter into IWriteHeaderAndFooter because I'm not 100% sold on the proposed name but I'm open to change it (to anything else) if you think IBatchTextFormatter is not good enough.

nblumhardt commented 2 years ago

Awesome, thanks for the updates, @0xced 👍

Agreed that IWriteHeaderAndFooter is a bit awful :-) .. The thing that makes me question IBatchTextFormatter is that it doesn't format batches anymore, esp. since it doesn't inherit the ITextFormatter methods.

I'd expect a "batch text formatter" to have something like FormatBatch(batch) exposed - and thinking about it some more, that could end up being a more useful interface (imagine tables with striped rows, or special styling when the batch contains only a single event).

I know we've circled around this for a while - but what about if we landed at something like:

public interface IBatchTextFormatter : ITextFormatter
{
    void FormatBatch(IEnumerable<LogEvent> logEvents, TextWriter output);

    void Format(LogEvent logEvent, TextWriter output) =>
        FormatBatch(new[] { logEvent}, output);
}

The dispatching logic would then be:

            if (_textFormatter is IBatchTextFormatter batchTextFormatter)
            {
                batchTextFormatter.FormatBatch(events, payload);
            }
            else
            {
                foreach (var logEvent in events)
                {
                    _textFormatter.Format(logEvent, payload);
                }
            }

Still not exactly what either of us would probably design, given a completely clean slate, but seems to me like it might get closer to justifying IBatchTextFormatter "is a" ITextFormatter?

0xced commented 2 years ago

While I appreciate when my pull requests are quickly merged, I also enjoy a thorough discussion with many iterations to reach a better solution. 😀

And I really like this new proposal! It gives greater flexibility and opens new possibilities such as grouping events by log level or category for example.

I have implemented this solution and updated the documentation in 26b8d70fea1a5c5606fbb2f58cdb337f3338a644.

Note that I could not use default interface methods as you suggested because Serilog.Sinks.Email targets .NET Framework/.NET Standard and default interface implementation requires new features in the .NET Core 3.0 CLR.

nblumhardt commented 2 years ago

Awesome, let's roll with that 👍

0xced commented 2 years ago

Awesome, thanks for merging! 🥳

Do you plan to release a new version of the NuGet package anytime soon?

0xced commented 2 years ago

Oh I see version 2.4.1-dev-00147 was already released on nuget.org. I'll test it soon and report here.

serilog / serilog-sinks-email

Introduce a new IBatchTextFormatter for handling batches of log events #97