Update serilog to 4 and move batching to Serilog core #545
Conversation
Update serilog v3.1.1 to 4.0.0 Update Microsoft.Data.SqlClient to 5.2.1 PerioadingBatching moved to batching in serilog core.
| @@ -281,6 +283,8 @@ public static class LoggerConfigurationMSSqlServerExtensions | |||
|
|
|||
| var auditSink = auditSinkFactory.Create(connectionString, sinkOptions, formatProvider, columnOptions, logEventFormatter); | |||
|
|
|||
| if (auditSink == null) return null; | |||
There was a problem hiding this comment.
Serilog 4 throws exception if sink is null.
.../Serilog.Sinks.MSSqlServer.Tests/Configuration/Factories/PeriodicBatchingSinkFactoryTests.cs
Show resolved
Hide resolved
|
Hi @cancakar35! Thank you for your PR! Serilog 4 update was already on our list. We really appreciate your contribution! :) |
|
There is still the open question regarding the returned null on sink creation. Can you please look into this again? |
Additional info for that lines: In serilog 4 this line added https://github.com/serilog/serilog/blob/8c82a50711fb20c6e31ffd60b585359aeb9336ed/src/Serilog/Configuration/LoggerSinkConfiguration.cs#L69 So, I thought it made sense to return null as before, instead of let it throw exception. I didn't see any problems with this approach in tests. |
|
What's blocking this PR currently? |
|
This is my last concern with this PR #545 (comment) We should not handle an exception and then return null on sink creation. Instead the exception should be handled by the caller. I wanted to play with it to better understand why @cancakar35 implemented it like this but I did not yet get around to do this. Sorry for that. @cremor if you could investigate this, I would highly appreciate this. Ideally we could just remove the exception handler but if not we should find a better solution. |
Microsoft.Data.SqlClient change hasn't been reverted yet. |
|
Maybe there is some mock missing. I'll look into this as soon as I can. |
|
I have fixed the unit tests and created a new PR #557. But there are further open questions which I will handle in the new PR. |
Related issiue: #543 (comment)
Update Serilog v3.1.1 to 4.0.0 https://github.com/serilog/serilog/releases/tag/v4.0.0
Update Microsoft.Data.SqlClient to 5.2.1
Remove dependency Serilog.Sinks.PeriodicBatching
PerioadingBatching moved to BatchingSink in serilog core. serilog/serilog#2055