search

cocowalla / serilog-sinks-file-archive

Plugin for the Serilog File sink that works with rolling log files, archiving completed log files before they are deleted by Serilog's retention mechanism
Apache License 2.0
31 stars 8 forks source link

Using CompressionLevel.NoCompression #16

Closed schaapkabap closed 9 months ago

schaapkabap commented 1 year ago

If I'm using CompressionLevel.NoCompression and retainedFileCountLimit its not possible to execute that action. Meanwhile if I remove the following code it is possible. What's the reason to decline that combination and is possible to remove that check?

            if (compressionLevel == CompressionLevel.NoCompression)
                throw new ArgumentException($"{nameof(compressionLevel)} must not be 'NoCompression' when using {nameof(retainedFileCountLimit)}", nameof(compressionLevel));
cocowalla commented 1 year ago

Hi, sorry it's taken me a while to get to this 😅

I've taken a look at the code in question, and I think it's perhaps a typo; I think it should really be making the same check that the other ctor does: compressionLevel == CompressionLevel.NoCompression && targetDirectory == null (the purpose of which is explained here, but when I fix this I'll add to the code comments!).

@EamonHetherton as the original author, do you concur?

schaapkabap commented 1 year ago

Cool, that makes more sense. I updated already the #17 for it :)

EamonHetherton commented 1 year ago

I can't recall specifically, and I don't use it without compression. Having a quick look at the code, it might have been an overly defensive move due to the LogFileComparer being very basic and only ordering by name. Depending on your file name format, without compression it could possibly remove the current file instead of only archive files? When compression is used it filters to *.gz files first so while it could still end up doing something unexpected, it would only affect the archived files not the current uncompressed log file. The current implementation meets all my needs so I haven't bothered to improve it but I'm happy if this check is removed; it won't affect my use.

cocowalla commented 9 months ago

Fixed by #17. v1.0.5 now released to NuGet: https://www.nuget.org/packages/Serilog.Sinks.File.Archive/1.0.5