Closed nblumhardt closed 4 years ago
Very interesting approach with the decorator + adapter implementation at the same time to retain compatibility with the old version.
While still not that big of a fan of introducing a second interface, this approach is significantly better than the previous version and I understand the challenges of minimizing breaking changes: my CompositeLogEvent
idea would be a lot more drastic than this.
Couple comments:
The settings class is created with invalid values out of the gate: if you pass a fresh new
instance to the sink, multiple properties will cause argument validation exceptions to be thrown due to their default values being invalid. I think this is bad design and would suggest reconsidering the strategy. A much more robust approach would be to move the validations to the options class constructor and making it immutable for example. This would also simplify the sink implementation considerably and improve on Law of Demeter and SRP fronts.
You ended up replacing one of the old constructors (the overload with the nullable int) with this change. I'm wondering if you considered bumping the major version instead of the minor version, as that's a potential breaking change for consumers that depended on it. In any case, I'd recommend a note about migration from old to new if you have release notes somewhere. Alternatively, you could add back the old constructor overload for compatibility reasons and keep the minor bump.
Hi, @julealgon . As for the second, the constructor with int?
isn't in stable version of the package, so it is acceptable to not bump the major version because of it.
the constructor with int? isn't in stable version of the package
Ahhh I see. That makes sense, thanks for clarifying @skomis-mm !
RE your first point @julealgon - I left this as a bit of a "todo" while thinking about whether default values or a constructor on PeriodicBatchingSinkOptions
would be preferable. Pros and cons to each - given the goal of avoiding constructor proliferation long-term I think I'm happy with default values, even if they're completely arbitrary. PR incoming now :-)
This sorts out some of the constructor proliferation discussed in #30 and implements the option suggested in #38.
All of the existing behavior is maintained, but consumers are encouraged to switch away from subclassing towards composition. The implementation achieves this currently by "composing" with itself to support the earlier subclassing usage.
Changes are shown in the updated README, copied below.
Getting started
Sinks that, for performance reasons, need to emit events in batches, can be implemented using
PeriodicBatchingSink
from this package.First, install the package into your Sink project:
Then, instead of implementing Serilog's
ILogEventSink
, implementIBatchedLogEventSink
in your sink class:Finally, in your sink's configuration method, construct a
PeriodicBatchingSink
that wraps your batched sink: