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

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

Closed
julealgon opened this issue Feb 11, 2020 · 4 comments
Closed

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

julealgon opened this issue Feb 11, 2020 · 4 comments

Comments

@julealgon
Copy link

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
Copy link
Member

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. #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
Copy link
Author

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:

  • Standard, single event
  • Composite event

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
Copy link
Member

Thanks for the suggestions 👍 - 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
Copy link
Author

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants