-
Notifications
You must be signed in to change notification settings - Fork 42
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.
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
base: dev
Are you sure you want to change the base?
Changes from 1 commit
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. yeah just tested locally and left for tests There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. removed |
||
"LogLevel": { | ||
"Default": "Warning" | ||
} | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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> | ||
|
@@ -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, | ||
|
@@ -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); | ||
} | ||
|
||
|
@@ -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> | ||
|
@@ -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, | ||
|
@@ -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) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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); | ||
} | ||
|
@@ -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)); | ||
|
||
|
@@ -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) | ||
{ | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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>3.1.1</VersionPrefix> | ||
<Authors>Serilog Contributors</Authors> | ||
<TargetFrameworks>netstandard2.0;netcoreapp3.1;net5.0;net6.0</TargetFrameworks> | ||
<TreatWarningsAsErrors>true</TreatWarningsAsErrors> | ||
|
@@ -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" /> | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @nblumhardt Hello, can you help here? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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: Something else? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think the This only works when If you're keen to explore it, we could also consider adding a HTH! There was a problem hiding this comment. Choose a reason for hiding this commentThe 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? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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'"> | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
removed