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.

Sign up for GitHub

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

add rollOnFileSizeLimit #60

Open
wants to merge 2 commits into
base: dev
Choose a base branch
from
Open

add rollOnFileSizeLimit #60

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
a067745
add rollOnFileSizeLimit
kgrodimov Jul 6, 2023
cca0c2d
fix review points
kgrodimov Jul 13, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 3 additions & 1 deletion README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -136,6 +136,7 @@ The `AddFile()` method exposes some basic options for controlling the connection
| `levelOverrides` | A dictionary mapping logger name prefixes to minimum logging levels. | |
| `isJson` | If true, the log file will be written in JSON format. | `true` |
| `fileSizeLimitBytes` | The maximum size, in bytes, to which any single log file will be allowed to grow. For unrestricted growth, pass`null`. The default is 1 GiB. | `1024 * 1024 * 1024` |
| `rollOnFileSizeLimit` | If true, a new file will be created when the file size limit is reached. The default is `false`. | `true` |
| `retainedFileCountLimit` | The maximum number of log files that will be retained, including the current log file. For unlimited retention, pass `null`. The default is `31`. | `31` |
| `outputTemplate` | The template used for formatting plain text log output. The default is `{Timestamp:o} {RequestId,13} [{Level:u3}] {Message} ({EventId:x8}){NewLine}{Exception}` | `{Timestamp:o} {RequestId,13} [{Level:u3}] {Message} {Properties:j} ({EventId:x8}){NewLine}{Exception}` |

Expand Down Expand Up @@ -169,6 +170,7 @@ In addition to the properties shown above, the `"Logging"` configuration support
| -------- | ----------- | ------- |
| `Json` | If `true`, the log file will be written in JSON format. | `true` |
| `FileSizeLimitBytes` | The maximum size, in bytes, to which any single log file will be allowed to grow. For unrestricted growth, pass`null`. The default is 1 GiB. | `1024 * 1024 * 1024` |
| `RollOnFileSizeLimit` | If true, a new file will be created when the file size limit is reached. The default is `false`. | `true` |
| `RetainedFileCountLimit` | The maximum number of log files that will be retained, including the current log file. For unlimited retention, pass `null`. The default is `31`. | `31` |
| `OutputTemplate` | The template used for formatting plain text log output. The default is `{Timestamp:o} {RequestId,13} [{Level:u3}] {Message} ({EventId:x8}){NewLine}{Exception}` | `{Timestamp:o} {RequestId,13} [{Level:u3}] {Message} {Properties:j} ({EventId:x8}){NewLine}{Exception}` |

Expand All @@ -182,6 +184,6 @@ The following packages are used to provide `loggingBuilder.AddFile()`:
* [Serilog.Formatting.Compact](https://github.com/serilog/serilog-formatting-compact) - JSON event formatting
* [Serilog.Extensions.Logging](https://github.com/serilog/serilog-extensions-logging) - ASP.NET Core integration
* [Serilog.Sinks.Async](https://github.com/serilog/serilog-sinks-async) - wrapper to perform log writes asynchronously
* [Serilog.Sinks.RollingFile](https://github.com/serilog/serilog-sinks-rollingfile) - rolling file output
* [Serilog.Sinks.File](https://github.com/serilog/serilog-sinks-file) - rolling file output

If you decide to switch to the full Serilog API and need help, please drop into the [Gitter channel](https://gitter.im/serilog/serilog) or post your question on [Stack Overflow](http://stackoverflow.com/questions/tagged/serilog).
27 changes: 18 additions & 9 deletions src/Serilog.Extensions.Logging.File/Microsoft/Extensions/Logging/FileLoggerExtensions.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,7 @@ public static ILoggerFactory AddFile(this ILoggerFactory loggerFactory, IConfigu
var minimumLevel = GetMinimumLogLevel(configuration);
var levelOverrides = GetLevelOverrides(configuration);

return loggerFactory.AddFile(config.PathFormat, minimumLevel, levelOverrides, config.Json, config.FileSizeLimitBytes, config.RetainedFileCountLimit, config.OutputTemplate);
return loggerFactory.AddFile(config.PathFormat, minimumLevel, levelOverrides, config.Json, config.FileSizeLimitBytes, config.RetainedFileCountLimit, config.OutputTemplate, config.RollOnFileSizeLimit);
}

/// <summary>
Expand All @@ -55,6 +55,8 @@ public static ILoggerFactory AddFile(this ILoggerFactory loggerFactory, IConfigu
/// log file. For unlimited retention, pass null. The default is 31.</param>
/// <param name="outputTemplate">The template used for formatting plain text log output. The default is
/// "{Timestamp:o} {RequestId,13} [{Level:u3}] {Message} ({EventId:x8}){NewLine}{Exception}"</param>
/// <param name="rollOnFileSizeLimit">If true, a new file will be created when the file size limit is reached. Filenames
/// will have a number appended in the format _NNN, with the first filename given no number. The default is false</param>
/// <returns>A logger factory to allow further configuration.</returns>
public static ILoggerFactory AddFile(
this ILoggerFactory loggerFactory,
Expand All @@ -64,9 +66,10 @@ public static ILoggerFactory AddFile(
bool isJson = false,
long? fileSizeLimitBytes = FileLoggingConfiguration.DefaultFileSizeLimitBytes,
int? retainedFileCountLimit = FileLoggingConfiguration.DefaultRetainedFileCountLimit,
string outputTemplate = FileLoggingConfiguration.DefaultOutputTemplate)
string outputTemplate = FileLoggingConfiguration.DefaultOutputTemplate,
bool rollOnFileSizeLimit = false)
{
var logger = CreateLogger(pathFormat, minimumLevel, levelOverrides, isJson, fileSizeLimitBytes, retainedFileCountLimit, outputTemplate);
var logger = CreateLogger(pathFormat, minimumLevel, levelOverrides, isJson, fileSizeLimitBytes, retainedFileCountLimit, outputTemplate, rollOnFileSizeLimit);
return loggerFactory.AddSerilog(logger, dispose: true);
}

Expand All @@ -91,7 +94,7 @@ public static ILoggingBuilder AddFile(this ILoggingBuilder loggingBuilder, IConf
var minimumLevel = GetMinimumLogLevel(configuration);
var levelOverrides = GetLevelOverrides(configuration);

return loggingBuilder.AddFile(config.PathFormat, minimumLevel, levelOverrides, config.Json, config.FileSizeLimitBytes, config.RetainedFileCountLimit, config.OutputTemplate);
return loggingBuilder.AddFile(config.PathFormat, minimumLevel, levelOverrides, config.Json, config.FileSizeLimitBytes, config.RetainedFileCountLimit, config.OutputTemplate, config.RollOnFileSizeLimit);
}

/// <summary>
Expand All @@ -109,6 +112,8 @@ public static ILoggingBuilder AddFile(this ILoggingBuilder loggingBuilder, IConf
/// log file. For unlimited retention, pass null. The default is 31.</param>
/// <param name="outputTemplate">The template used for formatting plain text log output. The default is
/// "{Timestamp:o} {RequestId,13} [{Level:u3}] {Message} ({EventId:x8}){NewLine}{Exception}"</param>
/// <param name="rollOnFileSizeLimit">If true, a new file will be created when the file size limit is reached. Filenames
/// will have a number appended in the format _NNN, with the first filename given no number. The default is false</param>
/// <returns>The logging builder to allow further configuration.</returns>
public static ILoggingBuilder AddFile(this ILoggingBuilder loggingBuilder,
string pathFormat,
Expand All @@ -117,9 +122,10 @@ public static ILoggingBuilder AddFile(this ILoggingBuilder loggingBuilder,
bool isJson = false,
long? fileSizeLimitBytes = FileLoggingConfiguration.DefaultFileSizeLimitBytes,
int? retainedFileCountLimit = FileLoggingConfiguration.DefaultRetainedFileCountLimit,
string outputTemplate = FileLoggingConfiguration.DefaultOutputTemplate)
string outputTemplate = FileLoggingConfiguration.DefaultOutputTemplate,
bool rollOnFileSizeLimit = false)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since this is a binary breaking change, we should bump the version number to 4.0.0.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

changed number

{
var logger = CreateLogger(pathFormat, minimumLevel, levelOverrides, isJson, fileSizeLimitBytes, retainedFileCountLimit, outputTemplate);
var logger = CreateLogger(pathFormat, minimumLevel, levelOverrides, isJson, fileSizeLimitBytes, retainedFileCountLimit, outputTemplate, rollOnFileSizeLimit);

return loggingBuilder.AddSerilog(logger, dispose: true);
}
Expand All @@ -130,7 +136,8 @@ private static Serilog.Core.Logger CreateLogger(string pathFormat,
bool isJson,
long? fileSizeLimitBytes,
int? retainedFileCountLimit,
string outputTemplate)
string outputTemplate,
bool rollOnFileSizeLimit)
{
if (pathFormat == null) throw new ArgumentNullException(nameof(pathFormat));

Expand All @@ -143,13 +150,15 @@ private static Serilog.Core.Logger CreateLogger(string pathFormat,
var configuration = new LoggerConfiguration()
.MinimumLevel.Is(Conversions.MicrosoftToSerilogLevel(minimumLevel))
.Enrich.FromLogContext()
.WriteTo.Async(w => w.RollingFile(
.WriteTo.Async(w => w.File(
formatter,
Environment.ExpandEnvironmentVariables(pathFormat),
fileSizeLimitBytes: fileSizeLimitBytes,
retainedFileCountLimit: retainedFileCountLimit,
shared: true,
flushToDiskInterval: TimeSpan.FromSeconds(2)));
flushToDiskInterval: TimeSpan.FromSeconds(2),
rollingInterval: RollingInterval.Day,
rollOnFileSizeLimit: rollOnFileSizeLimit));

if (!isJson)
{
Expand Down
4 changes: 2 additions & 2 deletions src/Serilog.Extensions.Logging.File/Serilog.Extensions.Logging.File.csproj
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@

<PropertyGroup>
<Description>Add file logging to ASP.NET Core apps with one line of code.</Description>
<VersionPrefix>3.0.1</VersionPrefix>
<VersionPrefix>4.0.1</VersionPrefix>
<Authors>Serilog Contributors</Authors>
<TargetFrameworks>netstandard2.0;netcoreapp3.1;net5.0;net6.0</TargetFrameworks>
<TreatWarningsAsErrors>true</TreatWarningsAsErrors>
Expand Down Expand Up @@ -32,9 +32,9 @@
<ItemGroup>
<PackageReference Include="Serilog" Version="2.10.0" />
<PackageReference Include="Serilog.Sinks.Async" Version="1.5.0" />
<PackageReference Include="Serilog.Sinks.RollingFile" Version="3.3.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" />
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@nblumhardt Hello, can you help here?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

so how can I add roll by size if old library does not support this parameter

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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!

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@nblumhardt

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry about the slow reply; yes, I think throwing an exception in that case would make sense 👍

</ItemGroup>

<ItemGroup Condition="'$(TargetFramework)' == 'netstandard2.0'">
Expand Down
8 changes: 8 additions & 0 deletions ...rilog.Extensions.Logging.File/Serilog/Extensions/Logging/File/FileLoggingConfiguration.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,5 +46,13 @@ public int? RetainedFileCountLimit
/// The default is "{Timestamp:o} {RequestId,13} [{Level:u3}] {Message} ({EventId:x8}){NewLine}{Exception}"
/// </summary>
public string OutputTemplate { get; set; } = DefaultOutputTemplate;

/// <summary>
/// If true, a new file will be created when the file size limit is reached. Filenames
/// will have a number appended in the format _NNN, with the first filename given no number.
/// The default is false
/// </summary>
public bool RollOnFileSizeLimit
{ get; set; } = false;
}
}