Add option to retain files based on age

[Wedvich]

The current implementation of the rolling file sink supports a file count retention policy. This works well for single-process environments, but in a multi-process environment where the log files are exclusively locked, the number of files quickly ramps up. Without a very generous file count limit (or no limit at all), log files quickly disappear, which makes it very hard to trace back a couple of days (or even hours) when troubleshooting reported issues.

This change offers an alternative retention policy by also setting an age limit for files. Intuitively enough, any log files older than the maximum age will be removed. Being able to control file retention based on their age and not just the number of files seems like a common enough use case that it should be part of the core rolling file sink.

Right now I've implemented this by moving the retention policy into an IRetentionPolicy interface, and adding two implementations for retainedFileCountLimit and retainedFileAgeLimit, respectively. These implementations are then created internally based on the RollingFileSink constructor parameters. I haven't exposed this interface publicly, but it could be used to add support for custom retention policies.

[Wedvich]

Moving the .NET Core App framework to the top was needed to get the tests to run, as the test discovery failed with the following error:

Unable to start C:\Program Files\dotnet\dotnet.exe
dotnet-test Error: 0 : [ReportingChannel]: Waiting for message failed System.IO.IOException: Unable to read data from the transport connection: An established connection was aborted by the software in your host machine. ---> System.Net.Sockets.SocketException: An established connection was aborted by the software in your host machine
at System.Net.Sockets.Socket.Receive(Byte[] buffer, Int32 offset, Int32 size, SocketFlags socketFlags)
at System.Net.Sockets.NetworkStream.Read(Byte[] buffer, Int32 offset, Int32 size)
--- End of inner exception stack trace ---
at System.Net.Sockets.NetworkStream.Read(Byte[] buffer, Int32 offset, Int32 size)
at System.IO.Stream.ReadByte()
at System.IO.BinaryReader.ReadByte()
at System.IO.BinaryReader.Read7BitEncodedInt()
at System.IO.BinaryReader.ReadString()
at Microsoft.DotNet.Tools.Test.ReportingChannel.ReadMessages()

This looks related to xunit/xunit#900, which is more than likely a bug with the .NET Core tooling somewhere and not with xUnit.net itself. For now, this (silly) workaround seems to do the trick.

[nblumhardt]

Thanks for the PR!

#16 (just merged a second ago :-)) implements multi-process file sharing, so the root cause here should no longer be (as much of) an issue, if the shared: true option is passed.

Providing an age-based, rather than count-based, retention policy sounds good, but I think we should get shared file sets out there first and reassess based on our experiences with that. Are you able to give 3.0.0-dev-00733 a try?

[Wedvich]

Oh, awesome! Currently testing out the shared option in a pretty busy system, and so far it seems like everything is able to be logged in a single file. It's a much better fix than just keeping hundreds (or thousands) of files around, so I hope it makes its way into the stable branch soon!

I'd still like to see the option of an age-based retention policy as well as size-based rollover, like mentioned in #14. I understand wanting to keep RollingFile simple, but I think these features are within the basic requirements one would expect for a rolling logger, and that they should make their way into the base RollingFile sink. Personally I feel that creating a more advanced FlexibleRollingFile would be more confusing than going the other way and deriving a SimpleRollingFile sink with just the basic rollover and file count retention, and expanding the RollingFile feature set. Going from 2.x to 3.x , there is the option of breaking changes, so now would be a good opportunity to consider that.

Is it mainly a bandwidth problem when it comes to maintaining the file-based sinks? Either way I see a lot of file-based logging on my current and past projects, so I'm willing to help if wanted.

[nblumhardt]

Thanks for the feedback, great to hear shared is doing well. I think we'll try to get it (3.x) onto master fairly soon, but there's always a 4.0 around the corner and it's sounding like additional rolling options should be the theme of that one.

Bandwidth is the top consideration, but I've also been hesitant to wrap a lot of speculative options up in one large policy-driven sink, rather than encourage multiple sinks with clearer/more specific codebases. There are a lot of possible configuration options out there, and things could become spaghetti pretty quickly without some restraint :-)

Size-based rolling has become a fairly regular request however, so I think we've got the data that we need to support adding it here. I would still like to hold off doing date-based retention for now, but perhaps we can open a new issue to discuss the design of size-based rolling and try to move that one forward?

[Wedvich]

Yeah, that makes sense, I agree it's better to put effort into something that's more commonly requested. I'll experiment a bit more with the date-based retention in my fork in case it comes back on the table again, but closing this PR for now. Thanks for the consideration! :)

[nblumhardt]

Thanks, appreciate all the effort on this one nonetheless!