search

serilog-mssql / serilog-sinks-mssqlserver

A Serilog sink that writes events to Microsoft SQL Server and Azure SQL
Apache License 2.0
283 stars 148 forks source link

Update serilog to 4 and move batching to Serilog core #545

Closed cancakar35 closed 2 months ago

cancakar35 commented 4 months ago

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

ckadluba commented 4 months ago

Hi @cancakar35!

Thank you for your PR! Serilog 4 update was already on our list. We really appreciate your contribution! :)

ckadluba commented 4 months ago

There is still the open question regarding the returned null on sink creation. Can you please look into this again?

cancakar35 commented 3 months ago

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.

cremor commented 2 months ago

What's blocking this PR currently?

ckadluba commented 2 months ago

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.

ad-eg-dk commented 2 months ago

What's blocking this PR currently?

Microsoft.Data.SqlClient change hasn't been reverted yet.

cancakar35 commented 2 months ago

@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.

ckadluba commented 2 months ago

Maybe there is some mock missing. I'll look into this as soon as I can.

ckadluba commented 2 months ago

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.