search

saleem-mirza / serilog-sinks-sqlite

A Serilog sink that writes to SQLite
Apache License 2.0
56 stars 44 forks source link

Log entry retention #3

Closed ronnieoverby closed 7 years ago

ronnieoverby commented 7 years ago

Allows the sink to be configured to delete older entries.

Some ideas I thought of but didn't implement were VACUUM every so often and putting an index on Timestamp.

ronnieoverby commented 7 years ago

Since adding this retention feature, I believe that continuing to allow the developer to choose whether the timestamp is stored as UTC or not is probably a mistake.

If a local time TimeStamp is stored, deletion of records around the time of DST changes could result in deleting more or less rows than intended.

Thoughts?

saleem-mirza commented 7 years ago

@ronnieoverby these tools are for developer and by developer. Usually there is team deliberation to establish protocol of usage so I don't think offering option to store date as UTC is mistake. There are use cases where datetime as UTC makes much more sense. e.g. distributed and disconnected application can store log in local store and then sync with remote server when connection is available e.g. Azure DocumentDB etc.

saleem-mirza commented 7 years ago

@ronnieoverby , great work 👍

ronnieoverby commented 7 years ago

Thanks!

About Timestamp storage, I was actually trying to say that storing UTC maybe ought to be mandatory because those string values will sort properly when it comes time to delete rows.

For local times produced from DateTimeOffset.Now the offset will jump either back or forward for some time zones at daylight savings time transitions and could cause critical log entries to be deleted at the wrong time.

I'm not suggesting that you should make a breaking change, but as for my apps, I will certainly store only UTC representations of those Timestamps, however unlikely it may be that I'll lose something important.

Another thought that crossed my mind was to change the DELETE query to apply a function to the Timestamp before comparison: https://www.sqlite.org/lang_datefunc.html

If that works, it would make this whole topic moot, at least until an index gets introduced on the Timestamp column which could affect sargability of the DELETE predicate.

It's just something to consider. By the way, thanks for taking time to build this library!

saleem-mirza commented 7 years ago

@ronnieoverby, you are most welcome. I highly appreciate community contribution so please feel free to introduce valuable features.