Option to disable flushing the first message

[LordMike]

"Proving we work, yay!" is not really a good way to operate in production. In our setup, we're running a client on tens of thousands of machines, and are using the peridiodic batching base to flush our logs from the clients, to our central server. Each batch involves some overhead, which is of course something, but most of all - we just don't want to expend time and resources on handling yet another file (yet another network request) to handle one log entry.

Following the last comment (from 2016) here #7, can we please have an option to disable batching the first message? It can be enabled by default, but then we can at least disable it when we "know it works, yay!". :)

[nblumhardt]

Thanks for the PR, Mike.

Just to make sure I understand you correctly - are you running a lot of short-lived processes? Sending one single-event batch doesn't seem like it should get onto the performance radar.

The disincentive for making this change is that it'd need to be exposed on each individual sink that inherits from PeriodicBatchingSink, which is a lot of churn and noise for something that won't benefit most users.

(There's also always the goal to keep API surface under control, so that we don't accrue large numbers of parameters.)

Cheers! Nick

[LordMike]

It is. We expect to receive a few hundred log messages per application, which directly translates to one batch - however, with this behavior, we receive double that.

So, changing the api there is maybe overreaching a bit.

How about a public property, that can be set by end users?

Mike.

[nblumhardt]

Thanks for the follow-up. Unfortunately the public property route isn't an option, since the sink classes are hidden behind their configuration methods.

What kind of sink are you sending the logs to?

[LordMike]

In our case, we're batching up logs and writing them to files through a custom transport to S3. We apply compression and more, and eventually we'll also create ephemeral encryption keys.

But most of all - we really dislike having a 1 KB log file next to a 2 MB compressed logfile.

Adding as a property wouldn't be doable?

If I f.ex. add in Sinks in this manner:

I would be able to interact with the ElasticsearchSink, which then inherits from the Periodic thingy.

It's true that I wouldn't be able to configure the ES sink with this option using the Serilog configuration system, since it locates extension methods on LoggerSinkConfiguration - right?

[nblumhardt]

The ES sink is a bit of an outlier here, in that it's exposed as a sink-derived class as well as through the configuration method. Not the norm, but definitely would be a reasonable way to approach it 👍

(I'd be hesitant to have the design suggest that sinks should be exposed as public classes, which we've otherwise avoided - some configuration methods work as abstract factories, where the one logical "sink" is implemented as several different concrete types behind the scenes. Using a protected property would resolve this, making the option available to sink subclasses only.)

May sound a little crazy, but, since your sink is a custom one, would embedding the four files from https://github.com/serilog/serilog-sinks-periodicbatching/tree/dev/src/Serilog.Sinks.PeriodicBatching/Sinks/PeriodicBatching be an acceptable option? (As you can tell, we're really very keen to avoid having a large number of options to maintain - over time, without this approach, modifying a really widely-deployed codebase like this one gets harder and harder :-))

@serilog/reviewers-core any thoughts on this?

[LordMike]

Our fallback is definitely to include parts of periodic batching sink in our project, and modifying as necessary - but we're on the other hand reluctant to include code and thereby not receive updates. :P

The Emit method is not virtual, so we won't have success with subclassing PeriodicBatchingSink and just copying in that part .. Oh well.

But I get your reservations. I still find the eager-send-first-message-to-prove-we-work odd.. :P

Closing this issue.

[nblumhardt]

Thanks, Mike 👍 Good luck with it, and get in touch if you hit anything in there that you need a hand with.

[meinsiedler]

Hi!

We are having the same issue as @LordMike - we would like to override the default behavior to emit the first log immediately.

Our use case is a custom email-sending sink. Currently we receive a separate email for the first log and another one for all other logs within the batch.

Any chance that we get an option to override the default behavior without needing to copy the PeriodicBatchingSink code as suggested?

[nblumhardt]

Hi @meinsiedler, thanks for the feedback. Disabling the behavior for a custom sink does seem easier to achieve non-disruptively, but then again, would a quicker workaround just be to construct and emit a dummy "first" event in the constructor of your sink? E.g.:

readonly LogEvent _first = new LogEvent(...);

public MySink()
{
    // .. construct, then
    Emit(_first);
}

Your batching logic can then just do a reference equality comparison with _first and ignore that event.

[meinsiedler]

Thank you @nblumhardt. We'll implement the suggested workaround. With that, this issue is resolved for us.