search

serilog / serilog-sinks-periodicbatching

Infrastructure for Serilog sinks that process events in batches.
Apache License 2.0
73 stars 29 forks source link

Add option to not immediately emit the first log event #38

Closed julealgon closed 4 years ago

julealgon commented 4 years ago

I was surprised when I first used my custom sink and it was being hit the first time immediately, no matter the interval I configured in the constructor.

My scenario would benefit from only running the batch after the elapsed time, even in the first time.

This code block kills this possibility:

https://github.com/serilog/serilog-sinks-periodicbatching/blob/dev/src/Serilog.Sinks.PeriodicBatching/Sinks/PeriodicBatching/PeriodicBatchingSink.cs#L238-L246

Can we at least have an option to not immediately dispatch the first event?

This behavior is really weird to me and violates the principle of least astonishment.

nblumhardt commented 4 years ago

Hi! Thanks for the note.

Originally, we added this behavior to reduce the incidence of support cause by programs exiting without closing/flushing the logger - getting one event through generally proves that the sink is configured correctly and leads the user to look elsewhere

For subclassing some control has been desired for a while, but as this package is highly depended upon it's not easy to extend in a controlled manner. https://github.com/serilog/serilog-sinks-periodicbatching/pull/30 has been waiting for an additional option to appear so that we can disambiguate between some constructor overloads - I'll investigate merging that PR and adding a parameter to the new, longer, constructor argument list in that PR. Thanks!

julealgon commented 4 years ago

but as this package is highly depended upon it's not easy to extend in a controlled manner.

I think most of this stems from the fact that this is a base class instead of a decorator implementation. Have you considered creating a "CompositeLogEvent" instead of exposing a new IEnumerable<LogEvent> method? That would allow simpler composition of these behaviors by creating decorators instead of base classes (the interface would be unchanged).

Obviously, that would probably require extracting the LogEvent API into an interface and having 2 implementations of it:

Firing any methods on the composite instance would apply the operation to all events inside the composite (looking at the current interface, this appears quite doable).

This would allow us to implement a batching sink that:

  1. Stores all events until a condition is met
  2. Creates a single CompositeLogEvent with all the buffered items
  3. Passes the CompositeLogEvent into the decoratee, using the same interface

A decorator-based extension like that would be substantially more reusable and less coupled than a base class approach.

nblumhardt commented 4 years ago

Thanks for the suggestions :+1: - yes, this is a pretty old package, it's probably due for some updates along those lines. I'll take a proper look at this again :-)

julealgon commented 4 years ago

Very surprised to see this tackled so fast. Just in time for us to drop our hack to prevent the initial execution :)

I left a couple comments on the merged PR in case you are interested, but the current design should already work fine for our use case.

Thank you so much for the prompt resolution here.