add rollOnFileSizeLimit #60
Conversation
|
some tests might be hlpful |
|
write unit tests or test locally and attach screenshot that code working? |
|
for now, project do not have any tests) that's why I am asking) https://github.com/serilog/serilog-extensions-logging-file/tree/dev/test/Serilog.Extensions.Logging.File.Tests |
README.md
Outdated
| @@ -109,7 +109,13 @@ log-20160701.txt | |||
| log-20160702.txt | |||
| ``` | |||
|
|
|||
| To prevent outages due to disk space exhaustion, each file is capped to 1 GB in size. If the file size is exceeded, events will be dropped until the next roll point. | |||
| To prevent outages due to disk space exhaustion, each file is capped to 1 GB in size. If the file size is exceeded, events will be dropped until the next roll point or new file with number appended in the format <code>_NNN</code>, with the first filename given no number, if rollOnFileSizeLimit is true. | |||
There was a problem hiding this comment.
Since it's a non-default option, I think we can keep this simple and add the new information under the documentation for the rollOnFileSizeLimit parameter only.
| @@ -3,6 +3,8 @@ | |||
| "PathFormat": "Logs/log-{Date}.txt", | |||
| "OutputTemplate": "{Timestamp:o} {RequestId,13} [{Level:u3}] {Message} ({EventId:x8}) {Properties:j}{NewLine}{Exception}", | |||
| "IncludeScopes": true, | |||
| "RollOnFileSizeLimit": true, | |||
| "FileSizeLimitBytes": 10485760, | |||
There was a problem hiding this comment.
No need to include these here; while they're useful, only a handful of users will want to configure them.
There was a problem hiding this comment.
yeah just tested locally and left for tests
| @@ -117,9 +122,10 @@ public static ILoggingBuilder AddFile(this ILoggingBuilder loggingBuilder, IConf | |||
| bool isJson = false, | |||
| long? fileSizeLimitBytes = FileLoggingConfiguration.DefaultFileSizeLimitBytes, | |||
| int? retainedFileCountLimit = FileLoggingConfiguration.DefaultRetainedFileCountLimit, | |||
| string outputTemplate = FileLoggingConfiguration.DefaultOutputTemplate) | |||
| string outputTemplate = FileLoggingConfiguration.DefaultOutputTemplate, | |||
| bool rollOnFileSizeLimit = false) | |||
There was a problem hiding this comment.
Since this is a binary breaking change, we should bump the version number to 4.0.0.
| <PackageReference Include="Serilog.Extensions.Logging" Version="3.1.0" /> | ||
| <PackageReference Include="Serilog.Formatting.Compact" Version="1.1.0" /> | ||
| <PackageReference Include="Serilog.Sinks.File" Version="5.0.0" /> |
There was a problem hiding this comment.
It's not possible to migrate to Serilog.Sinks.File without a substantial breaking change because of the differences in path formatting and rolling options; I think it's time we migrated, but there's more to discuss around it, so it'd be best discussed in a separate PR.
There was a problem hiding this comment.
so I do not need to migrate in this pr? I do not see any option in old library about it, https://github.com/serilog/serilog-sinks-rollingfile/blob/dev/src/Serilog.Sinks.RollingFile/RollingFileLoggerConfigurationExtensions.cs and without option rotation will not work
There was a problem hiding this comment.
@nblumhardt Hello, can you help here?
There was a problem hiding this comment.
Hi @kgrodimov - no need to migrate in this PR. The old library determines the rolling interval from the output path format string - if it includes {Date} then it rolls by day, etc.
There was a problem hiding this comment.
so how can I add roll by size if old library does not support this parameter
There was a problem hiding this comment.
Sorry, for not answering. I think throw exception is a better way, as user can expect to roll by HalfHour, for example.
So I need to check, if path contains:
{Date}, {Hour},{HalfHour} - params from there, https://github.com/serilog/serilog-sinks-rollingfile#filename-format-specifiers and throw exception something like this: $"File name format {pathString} is not supported, use 'rollingInterval' parameter for rolling by time". And also add required rollingInterval param passing and add this param to docs.
Something else?
@nblumhardt
There was a problem hiding this comment.
I think the log-{Date}.txt will cover the overwhelming majority of cases our there, so it'd be nice to keep accepting that and just trim out the {Date} to make the file paths compatible with the new implementation. We might end up with a flood of breakage/support otherwise.
This only works when {Date} immediately precedes the extension. If any other specifiers are there, or {Date} appears elsewhere in the path, we'd want to throw an exception.
If you're keen to explore it, we could also consider adding a RollingInterval argument and a shared flag, which would then cover 99% of the use cases for File.
HTH!
There was a problem hiding this comment.
So just trim out rollingInterval only if it immediately precedes the extension and pass correct interval with RollingInterval to Serilog.Sinks.File according to parsed value. So not only {Date}, but {Hour}, {HalfHour} also, or just {Day} as most popular.
New library does not support {HalfHour} btw, https://github.com/serilog/serilog-sinks-file/blob/2d2e00b31c4c1ed36dbbca8f4a43389856964fae/src/Serilog.Sinks.File/Sinks/File/RollingIntervalExtensions.cs#L23. So for {HalfHour} can I throw exception?
There was a problem hiding this comment.
Sorry about the slow reply; yes, I think throwing an exception in that case would make sense 👍
#24 (comment)