Roll on year/month/day/hour/minute, on file size, or on a combination of both #35

nblumhardt · 2017-10-13T05:43:33Z

Fixes serilog/serilog-sinks-rollingfile#33, Fixes serilog/serilog-sinks-rollingfile#52.

This PR introduces some additional parameters to WriteTo.File().

First, rollOnFileSizeLimit, causes additional log files to be created when the file size limit is reached. E.g.:

    .WriteTo.File("log.txt", rollOnFileSizeLimit: true)

will roll the file from log.txt to log_001.txt, log_002.txt and so-on when fileSizeLimitBytes is reached (the default is 1 GB). Old files will be cleaned up as per retainedFileCountLimit - the default is 31.

A second parameter, rollingInterval, rolls the file by RollingInterval.Year, Month, Day, Hour or Minute into log20180101.txt, log20180102.txt and so on.

    .WriteTo.File("log.txt", rollingInterval: RollingInterval.Hour)

If neither parameter is supplied, the behavior is exactly as before - the file name is static and no additional files will ever be created.

If both parameters are specified, the first file for a rolling interval will be named like log20180101.txt, rolling into log20180101_001.txt, log20180101.txt_002.txt and so on, until the next interval when the counter resets.

If either parameter is supplied, and the log file can't be opened, the suffix _NNN will be appended to find a filename that is available.

Goals

Address the need for roll-on-size
Bring the code for RollingFileSink into the same package as FileSink so that they can be extended and refactored in future (e.g. for many of the other long-standing items in https://github.com/serilog/serilog-sinks-rollingfile/issues)
Simplify the user experience (most users find File first, but really want RollingFile - very few apps use File
Move away from -{Date} path templates - they're misleading, as formatting information is not supported, and the placeholder can only appear in a very limited portion of the path
Re-use the rolling file sink code to get things unblocked quickly; the port to the new rolling scheme was very direct - where given the choice, I made the existing patterns work instead of introducing new code
Update to VS2017 tooling

Non-goals

Introduce much more flexibility - the combined codebases need some attention before adding code
Replace Serilog.Sinks.RollingFile - new apps should move to File, but there's no reason existing apps should move if the functionality over there is satisfactory

Breaking changes/evolution

The *Sink classes added in this PR are internal; the existing ones like FileSink are now marked [Obsolete] so that they too can become internal in a far-future release, to support some refactoring and improvement of the architecture
The default output template has been modernized with :u3 short level names and :lj message formatting (this is the reason for the major version bump)

I'm sure I'll think of more :-)

nblumhardt · 2017-10-13T05:53:45Z

(Could use more test coverage; the tests are mostly for the scenarios that overlap with Serilog.Sinks.RollingFile - will loop back and try to add some more when possible.)

optical · 2017-10-13T07:31:05Z

Awesome work @nblumhardt! This will let me retire my forked sink with a messy implementation of these features.

ritasker · 2017-10-13T11:21:21Z

Looks great thanks. 👍

…ig from serilog/serilog

nblumhardt · 2017-10-13T21:25:41Z

Great, thanks for taking a look!

Any thoughts on naming, anything else? I guess the surface area is pretty low. My current thinking is that I'll try adding a few more tests, then merge this through to open up to wider feedback.

nblumhardt · 2017-10-13T21:26:54Z

Add: fix whatever is breaking the Travis build to that list :-)

tsimbalar · 2017-10-14T19:04:02Z

src/Serilog.Sinks.File/Sinks/File/IOErrors.cs

@@ -0,0 +1,31 @@
+// Copyright 2013-2016 Serilog Contributors


nitpicking : Year is now 2017 :) (also seen in several other files)

tsimbalar · 2017-10-14T19:10:51Z

src/Serilog.Sinks.File/FileLoggerConfigurationExtensions.cs

        /// <param name="fileSizeLimitBytes">The approximate maximum size, in bytes, to which a log file will be allowed to grow.
        /// For unrestricted growth, pass null. The default is 1 GB. To avoid writing partial events, the last event within the limit
        /// will be written in full even if it exceeds the limit.</param>
        /// <param name="buffered">Indicates if flushing to the output file can be buffered or not. The default
        /// is false.</param>
        /// <param name="shared">Allow the log file to be shared by multiple processes. The default is false.</param>
        /// <param name="flushToDiskInterval">If provided, a full disk flush will be performed periodically at the specified interval.</param>
+        /// <param name="rollingInterval">The interval at which logging will roll over to a new file.</param>
+        /// <param name="rollOnFileSizeLimit">If <code>true</code>, a new file will be created when the file size limit is reached. Filenames 
+        /// will have a number appended in the format <code>_NNNNN</code>, with the first filename given no number.</param>


If the file is formatted with int.ToString("000"), I guess it should say "_NNN" here

tsimbalar · 2017-10-14T19:11:19Z

src/Serilog.Sinks.File/FileLoggerConfigurationExtensions.cs

@@ -92,6 +189,12 @@ public static class FileLoggerConfigurationExtensions
        /// is false.</param>
        /// <param name="shared">Allow the log file to be shared by multiple processes. The default is false.</param>
        /// <param name="flushToDiskInterval">If provided, a full disk flush will be performed periodically at the specified interval.</param>
+        /// <param name="rollingInterval">The interval at which logging will roll over to a new file.</param>
+        /// <param name="rollOnFileSizeLimit">If <code>true</code>, a new file will be created when the file size limit is reached. Filenames 
+        /// will have a number appended in the format <code>_NNNNN</code>, with the first filename given no number.</param>


same here, shouldn't it be _NNN ?

tsimbalar · 2017-10-14T19:16:39Z

test/Serilog.Sinks.File.Tests/RollingFileSinkTests.cs

+        }
+
+        [Fact]
+        public void AssemblyVersionIsFixedAt200()


Does that test pass, considering current File Sink (nuget package) version is at least v3.2.0+ ?

Thanks for all the feedback!

This one does pass - both sinks have had a frozen assembly version since 2.0.0, as a mitigation against the endless stream of binding redirect issues people hit (and report) otherwise :-)

…version required to build

tsimbalar · 2017-10-16T04:29:28Z

test/Serilog.Sinks.File.Tests/TemplatedPathRollerTests.cs

+        [Fact]
+        public void TheLogFileIncludesDateToken()
+        {
+            var roller = new PathRoller("Logs\\log-.txt", RollingInterval.Day);


Aren't backslashes problematic on Linux ? You should probably use Path.Combine , or maybe Path.DirectorySeparatorChar for the tests to work cross-platform

Also maybe Path.GetInvalidFileNameChars can be useful to avoid generating invalid filenames no matter the platform

oh well, I see it's covered :) 👍

Thanks for the check; yes, just hacking away at this between other jobs today, missed your comment but have it covered now.

skomis-mm · 2017-10-16T06:17:38Z

src/Serilog.Sinks.File/Sinks/File/RollingIntervalExtensions.cs

+                    return new DateTime(instant.Year, 0, 0, 0, 0, 0, instant.Kind);
+                case RollingInterval.Month:
+                    return new DateTime(instant.Year, instant.Month, 0, 0, 0, 0, instant.Kind);
+                case RollingInterval.Day:


I see a lot of ArgumentOutOfRangeException coming with the sample project, probably
new DateTime(instant.Year, 1, 1, 0, 0, 0, instant.Kind);
new DateTime(instant.Year, instant.Month, 1, 0, 0, 0, instant.Kind);
are needed

Thanks, fixing now 👍

(Adding some tests....)

nblumhardt · 2017-10-16T06:41:49Z

Finally made myself get WSL back up and running, instead of working in the dark, think everything should be running smoothly now.

…kely this will be found in the short term

nblumhardt · 2017-10-16T07:22:49Z

README updated: https://github.com/nblumhardt/serilog-sinks-file/blob/af88bceba9ef183e0e9520606541066c2fdf6d89/README.md

Time to push this through to dev? What do you think, folks? :-)

skomis-mm · 2017-10-16T08:31:12Z

Didn't spot any issues more, looks great 👍

nblumhardt · 2017-10-16T22:48:24Z

Thanks @skomis-mm

nblumhardt added 4 commits October 11, 2017 15:27

Rolling WIP

b8986cb

WIP

7614553

Tests pass

f04c568

Updated build scripts

937cdfe

This was referenced Oct 13, 2017

One log file per month serilog/serilog-sinks-rollingfile#52

Closed

Feature to roll on file size and date serilog/serilog-sinks-rollingfile#33

Closed

Update File Retention Policy BedeGaming/sinks-rollingfile#43

Open

Attempt to fix Travis build by blindly importing the scripts and conf…

b2f2c26

…ig from serilog/serilog

tsimbalar reviewed Oct 14, 2017

View reviewed changes

nblumhardt added 5 commits October 16, 2017 11:27

Review feedback

bf2060d

Travis .NET tooling version

2c16619

Run .NET Core 2.0 as test target on Travis, since that's the tooling …

ce534dc

…version required to build

Temp folder location on Linux

c36a0ea

Use test paths that are happy on Linux

6988cb4

tsimbalar reviewed Oct 16, 2017

View reviewed changes

nblumhardt added 5 commits October 16, 2017 14:35

Proper path concatenation in tests

0b514f2

Directory listing ordering

e17e37e

Return some more useful info on test failure

638a932

Might as well be more exact on this assertion

6f20742

More test twiddling

4605f0c

skomis-mm reviewed Oct 16, 2017

View reviewed changes

Surprisingly, _ sorts before . by default on Ubuntu 16.04

20423c3

nblumhardt added 2 commits October 16, 2017 16:23

Use VB.NET indexing for months and days ;-)

b183ea4

Tests for interval/checkpoint calculation

483e4e8

nblumhardt added 2 commits October 16, 2017 17:15

Bring the README quality into line with the rolling file sink's

6ad69c1

Might as well not specify -Pre in the installation instructions, unli…

af88bce

…kely this will be found in the short term

nblumhardt merged commit d636b08 into serilog:dev Oct 16, 2017

nblumhardt mentioned this pull request Oct 18, 2017

Preparations for https://github.com/serilog/serilog-sinks-rollingfile… #26

Closed

tsimbalar mentioned this pull request Oct 19, 2017

Add some guidance about Serilog.Sinks.File vs Serilog.Sinks.RollingFile serilog/serilog-sinks-rollingfile#62

Closed

ddprince17 mentioned this pull request Oct 19, 2017

On app startup - Doesn't log at all when the existing file size limit has already been reached serilog/serilog-sinks-rollingfile#61

Closed

nblumhardt mentioned this pull request Oct 27, 2017

4.0.0 Release #37

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Roll on year/month/day/hour/minute, on file size, or on a combination of both #35

Roll on year/month/day/hour/minute, on file size, or on a combination of both #35

nblumhardt commented Oct 13, 2017 •

edited

Loading

nblumhardt commented Oct 13, 2017

optical commented Oct 13, 2017

ritasker commented Oct 13, 2017

nblumhardt commented Oct 13, 2017

nblumhardt commented Oct 13, 2017

tsimbalar Oct 14, 2017 •

edited

Loading

tsimbalar Oct 14, 2017

tsimbalar Oct 14, 2017

tsimbalar Oct 14, 2017

nblumhardt Oct 16, 2017

tsimbalar Oct 16, 2017

tsimbalar Oct 16, 2017 •

edited

Loading

tsimbalar Oct 16, 2017

nblumhardt Oct 16, 2017

skomis-mm Oct 16, 2017

nblumhardt Oct 16, 2017

nblumhardt Oct 16, 2017

nblumhardt commented Oct 16, 2017

nblumhardt commented Oct 16, 2017

skomis-mm commented Oct 16, 2017

nblumhardt commented Oct 16, 2017

Roll on year/month/day/hour/minute, on file size, or on a combination of both #35

Roll on year/month/day/hour/minute, on file size, or on a combination of both #35

Conversation

nblumhardt commented Oct 13, 2017 • edited Loading

Goals

Non-goals

Breaking changes/evolution

nblumhardt commented Oct 13, 2017

optical commented Oct 13, 2017

ritasker commented Oct 13, 2017

nblumhardt commented Oct 13, 2017

nblumhardt commented Oct 13, 2017

tsimbalar Oct 14, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tsimbalar Oct 16, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nblumhardt commented Oct 16, 2017

nblumhardt commented Oct 16, 2017

skomis-mm commented Oct 16, 2017

nblumhardt commented Oct 16, 2017

nblumhardt commented Oct 13, 2017 •

edited

Loading

tsimbalar Oct 14, 2017 •

edited

Loading

tsimbalar Oct 16, 2017 •

edited

Loading