-
Notifications
You must be signed in to change notification settings - Fork 68
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
Re-implement sink using IBatchedLogEventSink #78
Conversation
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 In particular, the Hope this helps! |
My intention was to make the user explicitly specify how batching should occur. The use of @nblumhardt Are you recommending that I remove |
Hi Brian, thanks for the follow-up. The design is interesting 👍 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.
|
@nblumhardt Are you saying that my approach is not usable from configuration or that 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. |
Thanks for the reply. Neither the approach nor direct |
@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? |
@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? If we go this way, I propose to use an virtual method with the @nblumhardt and @bcallaghan-et What do you think? |
@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 IMO, we should not have a default batching algorithm on the |
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 Appreciate all the thought going into this, thanks for the help 👍 |
@nblumhardt
It maybe me, but I don't understand the sentence :-(
I did that locally here to test and it is working well without breaking the current behaviour of the email sink. And with the I don't have enough knowledge about the internal serilog implementation, I'm just scratching the surface :-) |
Hi @ericvb!
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 I linked another similar change in #78 (comment) RE:
Probably safe to ignore these musings for now :-) Non-breaking PR that converted to @bcallaghan-et I think we should close this PR for now; sorry we ended up heading in a different direction, but thanks again! |
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.