Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

TLoggerProFileAppender File Rotation options #92

Open
fastbike opened this issue Jul 30, 2024 · 8 comments
Open

TLoggerProFileAppender File Rotation options #92

fastbike opened this issue Jul 30, 2024 · 8 comments
Labels
accepted Issue has been accepted and inserted in a future milestone

Comments

@fastbike
Copy link
Contributor

fastbike commented Jul 30, 2024

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
Copy link
Contributor

luebbe commented Jul 31, 2024

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

@danieleteti
Copy link
Owner

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

@danieleteti danieleteti added the accepted Issue has been accepted and inserted in a future milestone label Jul 31, 2024
@fastbike
Copy link
Contributor Author

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
Copy link
Contributor Author

fastbike commented Aug 5, 2024

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
Copy link
Contributor Author

fastbike commented Aug 5, 2024

@danieleteti what is the preference for JSON handling ?

  • system.json, or
  • JsonDataObjects

@danieleteti
Copy link
Owner

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

uses
  System.IOUtils,
{$IF Defined(USE_JDO)}
  JsonDataObjects
{$ELSE}
  System.JSON
{$ENDIF}
;

@fastbike
Copy link
Contributor Author

fastbike commented Aug 5, 2024

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 fastbike changed the title TLoggerProFileAppenderBase hides required methods TLoggerProFileAppender File Rotation options Aug 5, 2024
@fastbike
Copy link
Contributor Author

fastbike commented Aug 5, 2024

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

fastbike added a commit to fastbike/loggerpro that referenced this issue Aug 6, 2024
Re factored file rotation:
- moved functionality into own helper object with base interface to define behaviour
- provide two concrete implementations:
  - Rotate by file count
  - Rotate by file date
- Tidied up the LogFileFormat checks to use an enumeration
- Moved file maintenance methods (Delete / Rename) to the  helper objects
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
accepted Issue has been accepted and inserted in a future milestone
Projects
None yet
Development

No branches or pull requests

3 participants