search

serilog-mssql / serilog-sinks-mssqlserver

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

Retain and Cleaning Policy #376

Closed MehrdadDw closed 2 years ago

MehrdadDw commented 2 years ago

A mechanism to delete old logs and clean up the logs table.

ckadluba commented 2 years ago

Hi @Dowlatabadi!

Thank you for your contribution. :) It looks quite good but I made a review and had some comments. Please add the necessary changes.

Do you have some performance data for the change? Approximately how much longer does a batch with cleaning take compared to one without (in percent)?

Thank you very much and best regards! Christian

MehrdadDw commented 2 years ago

@ckadluba Thanks for the comments and tips, I will apply changes and will do some performance tests.

@jonorossi Thanks for the comments. Honestly, we had this need and I thought it would be easier to let the sink does that for us, and IMO as @ckadluba mentioned choosing an appropriate interval would prevent mentioned timeout problem. Although, in the case of using multiple logger applications with different retention configs, it still might be prone to performance issues or some sort of inconsistency of retention policies.

MehrdadDw commented 2 years ago

Do you have some performance data for the change? Approximately how much longer does a batch with cleaning take compared to one without (in percent)?

The performance tests I did perform showed .001 percent slowness caused by every pruning event. This average has been calculated during filling up to 5 million log records. The retention period was large enough to make sure that the deletion process actually would not delete any records. If in another configuration every deletion query causes deletion of many records, actually the logging speed increases, which makes speed up rather than slowness. Results can be found here

jonorossi commented 2 years ago

@ckadluba this pull request is stale and still needs quite a lot of work to finish. As much as I don't see value in the feature, I want to see it maintainable and not affect the normal code path. Please re-request my review if you are considering merging it.

ckadluba commented 2 years ago

@Dowlatabadi I really thank you for your the work and effort you put into this but I think we won't be able to merge this anytime soon. For me there are still open questions and uncertainties about how this could impact the behavior and stability of the sink in a production scenario especially when it comes to edge cases or misconfigurations.

I see the good core idea and the implementation you provided us with. Still I would recommend to do log table retention and cleaning on the database level.

I hope you are not too disappointed if I will close the PR without merging it.