Cartus
Update serilog to 4 and move batching to Serilog core
Related issiue: https://github.com/serilog-mssql/serilog-sinks-mssqlserver/issues/543#issue-2347936892
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. https://github.com/serilog/serilog/pull/2055
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?
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 Now its throw ArgumentNullException if the logEventSink parameter null Releated commit : https://github.com/serilog/serilog/pull/2055/commits/44c5e1563b4ee418b631cd65ab5d1ca08a9b8b14
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.
This is my last concern with this PR https://github.com/serilog-mssql/serilog-sinks-mssqlserver/pull/545#issuecomment-2249868768
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.
What's blocking this PR currently?
Microsoft.Data.SqlClient change hasn't been reverted yet.
@ckadluba I chose this approach because the unit tests were failing when we let it throw exception. If we can change the unit tests we can remove null returning.
@ad-eg-dk Please look into this pr: https://github.com/serilog-mssql/serilog-sinks-mssqlserver/pull/548 . If you have any problem , please open an issue.
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.