danieleteti / loggerpro

An modern and pluggable logging framework for Delphi
Apache License 2.0
357 stars 91 forks source link

TLoggerProFileAppender File Rotation options #92

Open fastbike opened 4 months ago

fastbike commented 4 months ago

I'm writing a File based LogAppender that writes all entries to a single file that is rotated by file size, and the number of files kept is limited by file age i.e. number of days.

I'm descending from TLoggerProFileAppenderBase as there is no point reinventing the functionality contained within. However there are too many private members which are required in the descendant (but are not visible in another unit). Ideally we'd refactor the LoggerPro.FileAppender.pas unit so it only contains the abstract base class but exposes the required methods as protected members. It also appears to have the file retention mechanism set to a certain number of files rather than a more generic "do we retain this file?" when log rotation occurs.

In particular CreateWriter and RetryMove needs to be available to descendants so that a completed file can be rotated and a fresh file created.

luebbe commented 4 months ago

This sounds like a very good idea to me. Do you want to have a go at it?

danieleteti commented 4 months ago

David, use the version deployed with dmvcframework in the repo (if you can). When dmvcframework 3.4.2-magnesium will be released, the loggerpro folder will become loggerpro version 2. You can make your changes directly there and then create a PR

fastbike commented 4 months ago

David, use the version deployed with dmvcframework in the repo (if you can). When dmvcframework 3.4.2-magnesium will be released, the loggerpro folder will become loggerpro version 2. You can make your changes directly there and then create a PR

OK, will take a look at this over the next week. The main design goal to abstract away the rollover and retention implementation. I.e.

  1. What to do when the file is full (currently increments and shuffles filenames, then creates a new file at the base)
  2. How to decide if it is time to prune the number of files (currently holds a fixed number of iterations)

NB: A stretch goal would be to allow for dynamic changing of the global LogLevel, although this sits outside of the FileAppenders. We run our apps to log Errors and Warnings, but when we get a production issue it would be good to switch to Debug for a short period to get additional data for analysis. I think we currently have to recycle the (IIS) app pool to make this happen. I will raise a separate ticket in DMVC for this change.

fastbike commented 3 months ago

I've been playing around with a couple of ideas. The one I like the best is to abstract out the file rotation into a separate object.

I've declared an interface to handle this and then use a dependency injection pattern on the LogFileAppender constructor so that the desired behaviour can be provided. I've created two concrete implementation classes: one that implements the existing "ByCount" file rotation, and a new "ByDate" that allows for a set number of days of log files to be kept. Both work with the existing LogFileAppenders (the one that create a separate file for each tag, and the one that puts all entries into one file). And have provided some sensible defaults to make it easy to get started.

I'm still running some development testing.

Which project do I create the PR in ? This one or the DMVC project ?

fastbike commented 3 months ago

@danieleteti what is the preference for JSON handling ?

danieleteti commented 3 months ago

There is an IFDEF to choose. Check LoggerPro.JSONLFileAppender.pas

uses
  System.IOUtils,
{$IF Defined(USE_JDO)}
  JsonDataObjects
{$ELSE}
  System.JSON
{$ENDIF}
;
fastbike commented 3 months ago

OK, I'm using JSON Parse and value reading functions so will need to do the IFDEF around those as well. Hopefully I'll get something posted today.

fastbike commented 3 months ago

I've renamed the ticket to better reflect the intended outcome of providing for configurable file rotation options