Fulfills #20 - add option to block when the queue is full, instead of dropping events #21
Conversation
Also, use `GetConsumingEnumerable` to enumerate the queue, instead of a `while` loop
cocowalla
changed the title
| } | ||
|
|
||
| [Fact] | ||
| public async Task WhenEmitSingle_ThenRelaysToInnerSink() | ||
| { | ||
| var logEvent = CreateEvent(); | ||
| _sink.Emit(logEvent); | ||
| _sink.Dispose(); |
There was a problem hiding this comment.
I think the goal of this test is to check the normal operation (not dispose/flush) case; should still pass without Dispose() here.
| readonly MemorySink _innerSink; | ||
| readonly BackgroundWorkerSink _sink; | ||
| BackgroundWorkerSink _sink; |
There was a problem hiding this comment.
Since some tests now use _sink as initialized, and others replace it, perhaps it would be clearer to get rid of this field altogether and use something like:
var sink = CreateWithDefaultOptions();in each test that needs it, where the method would just be a shortcut to new BackgroundWorkerSink(_logger, 10000, false)?
There was a problem hiding this comment.
Actually, BackgroundWorkerSinkSpec implements IDisposable so it has a Dispose() method which dispoes of the existing _sink field. If we still want to properly dispose of the sink that would mean wrapping creation in a using block in every test, e.g.:
using (var sink = CreateWithDefaultOptions())
{
// Test here
}
This adds quite a bit of monkey code to each test - do you still want to do it that way, or stick with the way it is?
There was a problem hiding this comment.
Good point. I think I'd still probably prefer the using block to the mutated field - easier to refactor, and less opportunity for odd behavior (e.g. in the current version of the PR, we don't dispose _sink before replacing it).
|
|
||
| // If we *weren't* dropped events, the delay in the inner sink would mean the 5 events would take | ||
| // at least 15 seconds to process | ||
| await Task.Delay(TimeSpan.FromSeconds(18)); |
There was a problem hiding this comment.
Perhaps this could be scaled down into 200 ms steps, rather than 2 seconds, so that it doesn't take so long to run the tests? 18 seconds here plus 12 below adds quite a bit to testing turnaround.
There was a problem hiding this comment.
Agreed. The existing code was using a 3s delay somewhere, so I just did the same without thinking about it.
|
|
||
| public void Emit(LogEvent logEvent) | ||
| { | ||
| if (DelayEmit) | ||
| Task.Delay(TimeSpan.FromSeconds(3)).Wait(); |
There was a problem hiding this comment.
Could bool DelayEmit become TimeSpan? EmitDelay?
| if (_disposed) | ||
| return; | ||
|
|
||
| if (!this._blockWhenFull) |
There was a problem hiding this comment.
Inverting the condition here to if (_blockWhenFull) would make the code read a little more smoothly.
Nit: don't need the explicit this.
There was a problem hiding this comment.
Sorry about the this- I generally use this in my own code with no underscore prefix for fields. Was trying to follow Serilog style, but I guess Resharper slipped one through for me :)
| } | ||
|
|
||
| public void Dispose() | ||
| { | ||
| _disposed = true; | ||
| _cancel.Cancel(); | ||
| _queue.CompleteAdding(); |
There was a problem hiding this comment.
Not sure why this wasn't used originally - looks like a good change to make. Can we grab some benchmark output from dev vs the PR branch, just to make sure perf wasn't an issue with this?
There was a problem hiding this comment.
Sure, it's late now so will add something tomorrow
There was a problem hiding this comment.
The issue with this one is that TryAdd can potentially throw with InvalidOperationException on some race condition
There was a problem hiding this comment.
@skomis-mm yes, you're right - it's probably best to just do away with the disposed check in Emit and wrap it in a try...catch for InvalidOperationException
| /// <returns>A <see cref="LoggerConfiguration"/> allowing configuration to continue.</returns> | ||
| public static LoggerConfiguration Async( | ||
| this LoggerSinkConfiguration loggerSinkConfiguration, | ||
| Action<LoggerSinkConfiguration> configure, | ||
| int bufferSize = 10000) | ||
| int bufferSize = 10000, | ||
| bool blockWhenFull = false) |
There was a problem hiding this comment.
Since this is used by quite a lot of downstream packages, we'll need to follow the old pattern of adding an overload with the old parameter set, marked [EditorBrowsable(EditorBrowsableState.Never)] with no default values applied.
There was a problem hiding this comment.
Sorry, I figured using a default would preserve current behaviour for existing users. Wasn't aware of that attribute
Response to review of #21
|
Before and after benchmarks |
|
Added some thoughts on the test factoring; I still need a quiet moment to sit and think through the implementation thoroughly, but all seems good as far as I can see right now 👍 |
Instantiate sink in in test instead. Response to feedback in #21
|
Awesome 👍 - will be published as 1.2.0-dev-nnnnn shortly. |
|
See also PR for monitoring:- #29 - I added an example to the README for this, so it would be good to get suggestions for improvement. |
Also, use
GetConsumingEnumerableto enumerate the queue, instead of awhileloop