-
Notifications
You must be signed in to change notification settings - Fork 125
Enable auditing #12
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
Enable auditing #12
Changes from all commits
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 |
---|---|---|
|
@@ -94,27 +94,97 @@ public static LoggerConfiguration File( | |
long? fileSizeLimitBytes = DefaultFileSizeLimitBytes, | ||
LoggingLevelSwitch levelSwitch = null, | ||
bool buffered = false) | ||
{ | ||
return ConfigureFile(sinkConfiguration.Sink, formatter, path, restrictedToMinimumLevel, fileSizeLimitBytes, levelSwitch, buffered); | ||
} | ||
|
||
/// <summary> | ||
/// Write log events to the specified file. | ||
/// </summary> | ||
/// <param name="sinkConfiguration">Logger sink configuration.</param> | ||
/// <param name="path">Path to the file.</param> | ||
/// <param name="restrictedToMinimumLevel">The minimum level for | ||
/// events passed through the sink. Ignored when <paramref name="levelSwitch"/> is specified.</param> | ||
/// <param name="levelSwitch">A switch allowing the pass-through minimum level | ||
/// to be changed at runtime.</param> | ||
/// <param name="formatProvider">Supplies culture-specific formatting information, or null.</param> | ||
/// <param name="outputTemplate">A message template describing the format used to write to the sink. | ||
/// the default is "{Timestamp} [{Level}] {Message}{NewLine}{Exception}".</param> | ||
/// <returns>Configuration object allowing method chaining.</returns> | ||
/// <remarks>The file will be written using the UTF-8 character set.</remarks> | ||
public static LoggerConfiguration File( | ||
this LoggerAuditSinkConfiguration sinkConfiguration, | ||
string path, | ||
LogEventLevel restrictedToMinimumLevel = LevelAlias.Minimum, | ||
string outputTemplate = DefaultOutputTemplate, | ||
IFormatProvider formatProvider = null, | ||
LoggingLevelSwitch levelSwitch = null) | ||
{ | ||
if (sinkConfiguration == null) throw new ArgumentNullException(nameof(sinkConfiguration)); | ||
if (path == null) throw new ArgumentNullException(nameof(path)); | ||
if (outputTemplate == null) throw new ArgumentNullException(nameof(outputTemplate)); | ||
|
||
var formatter = new MessageTemplateTextFormatter(outputTemplate, formatProvider); | ||
return File(sinkConfiguration, formatter, path, restrictedToMinimumLevel, levelSwitch); | ||
} | ||
|
||
/// <summary> | ||
/// Write log events to the specified file. | ||
/// </summary> | ||
/// <param name="sinkConfiguration">Logger sink configuration.</param> | ||
/// <param name="formatter">A formatter, such as <see cref="JsonFormatter"/>, to convert the log events into | ||
/// text for the file. If control of regular text formatting is required, use the other | ||
/// overload of <see cref="File(LoggerSinkConfiguration, string, LogEventLevel, string, IFormatProvider, long?, LoggingLevelSwitch, bool)"/> | ||
/// and specify the outputTemplate parameter instead. | ||
/// </param> | ||
/// <param name="path">Path to the file.</param> | ||
/// <param name="restrictedToMinimumLevel">The minimum level for | ||
/// events passed through the sink. Ignored when <paramref name="levelSwitch"/> is specified.</param> | ||
/// <param name="levelSwitch">A switch allowing the pass-through minimum level | ||
/// to be changed at runtime.</param> | ||
/// <returns>Configuration object allowing method chaining.</returns> | ||
/// <remarks>The file will be written using the UTF-8 character set.</remarks> | ||
public static LoggerConfiguration File( | ||
this LoggerAuditSinkConfiguration sinkConfiguration, | ||
ITextFormatter formatter, | ||
string path, | ||
LogEventLevel restrictedToMinimumLevel = LevelAlias.Minimum, | ||
LoggingLevelSwitch levelSwitch = null) | ||
{ | ||
return ConfigureFile(sinkConfiguration.Sink, formatter, path, restrictedToMinimumLevel, null, levelSwitch, false, true); | ||
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. Note here that auditing 1) does not allow buffered writes, and 2) doesn't support file size limiting. Seems like the safest starting point. We could probably enable file size limits and just throw when they're hit, but I'm okay with leaving this alone for the first cut. |
||
} | ||
|
||
static LoggerConfiguration ConfigureFile( | ||
this Func<ILogEventSink, LogEventLevel, LoggingLevelSwitch, LoggerConfiguration> addSink, | ||
ITextFormatter formatter, | ||
string path, | ||
LogEventLevel restrictedToMinimumLevel = LevelAlias.Minimum, | ||
long? fileSizeLimitBytes = DefaultFileSizeLimitBytes, | ||
LoggingLevelSwitch levelSwitch = null, | ||
bool buffered = false, | ||
bool propagateExceptions = false) | ||
{ | ||
if (addSink == null) throw new ArgumentNullException(nameof(addSink)); | ||
if (formatter == null) throw new ArgumentNullException(nameof(formatter)); | ||
if (path == null) throw new ArgumentNullException(nameof(path)); | ||
if (fileSizeLimitBytes.HasValue && fileSizeLimitBytes < 0) throw new ArgumentException("Negative value provided; file size limit must be non-negative"); | ||
|
||
FileSink sink; | ||
try | ||
{ | ||
sink = new FileSink(path, formatter, fileSizeLimitBytes, buffered: buffered); | ||
} | ||
catch (ArgumentException) | ||
{ | ||
throw; | ||
} | ||
catch (Exception ex) | ||
{ | ||
SelfLog.WriteLine("Unable to open file sink for {0}: {1}", path, ex); | ||
return sinkConfiguration.Sink(new NullSink()); | ||
|
||
if (propagateExceptions) | ||
throw; | ||
|
||
return addSink(new NullSink(), LevelAlias.Maximum, null); | ||
} | ||
|
||
return sinkConfiguration.Sink(sink, restrictedToMinimumLevel, levelSwitch); | ||
return addSink(sink, restrictedToMinimumLevel, levelSwitch); | ||
} | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -16,7 +16,6 @@ | |
using System.IO; | ||
using System.Text; | ||
using Serilog.Core; | ||
using Serilog.Debugging; | ||
using Serilog.Events; | ||
using Serilog.Formatting; | ||
|
||
|
@@ -53,7 +52,11 @@ public FileSink(string path, ITextFormatter textFormatter, long? fileSizeLimitBy | |
_textFormatter = textFormatter; | ||
_buffered = buffered; | ||
|
||
TryCreateDirectory(path); | ||
var directory = Path.GetDirectoryName(path); | ||
if (!string.IsNullOrWhiteSpace(directory) && !Directory.Exists(directory)) | ||
{ | ||
Directory.CreateDirectory(directory); | ||
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. These errors were previously handled locally; now because there is more suppression in the caller we don't need the additional EH here. Note that this change doesn't modify the overall behaviour of this constructor, because previously an initial exception, though handled and logged, would always be immediately followed with another exception from |
||
} | ||
|
||
var file = System.IO.File.Open(path, FileMode.Append, FileAccess.Write, FileShare.Read); | ||
var outputWriter = new StreamWriter(file, encoding ?? new UTF8Encoding(encoderShouldEmitUTF8Identifier: false)); | ||
|
@@ -69,22 +72,6 @@ public FileSink(string path, ITextFormatter textFormatter, long? fileSizeLimitBy | |
} | ||
} | ||
|
||
static void TryCreateDirectory(string path) | ||
{ | ||
try | ||
{ | ||
var directory = Path.GetDirectoryName(path); | ||
if (!string.IsNullOrWhiteSpace(directory) && !Directory.Exists(directory)) | ||
{ | ||
Directory.CreateDirectory(directory); | ||
} | ||
} | ||
catch (Exception ex) | ||
{ | ||
SelfLog.WriteLine("Failed to create directory {0}: {1}", path, ex); | ||
} | ||
} | ||
|
||
/// <summary> | ||
/// Emit the provided log event to the sink. | ||
/// </summary> | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,55 @@ | ||
using System; | ||
using Serilog; | ||
using Serilog.Sinks.File.Tests.Support; | ||
using Serilog.Tests.Support; | ||
using Xunit; | ||
|
||
namespace Serilog.Tests | ||
{ | ||
public class FileLoggerConfigurationExtensionsTests | ||
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. File IO failures are hard to emulate in tests - invalid paths and broken formatting should be close enough to IO errors to give a reasonable measure of confidence. |
||
{ | ||
const string InvalidPath = "/\\"; | ||
|
||
[Fact] | ||
public void WhenWritingCreationExceptionsAreSuppressed() | ||
{ | ||
new LoggerConfiguration() | ||
.WriteTo.File(InvalidPath) | ||
.CreateLogger(); | ||
} | ||
|
||
[Fact] | ||
public void WhenAuditingCreationExceptionsPropagate() | ||
{ | ||
Assert.Throws<ArgumentException>(() => | ||
new LoggerConfiguration() | ||
.AuditTo.File(InvalidPath) | ||
.CreateLogger()); | ||
} | ||
|
||
[Fact] | ||
public void WhenWritingLoggingExceptionsAreSuppressed() | ||
{ | ||
using (var tmp = TempFolder.ForCaller()) | ||
using (var log = new LoggerConfiguration() | ||
.WriteTo.File(new ThrowingLogEventFormatter(), tmp.AllocateFilename()) | ||
.CreateLogger()) | ||
{ | ||
log.Information("Hello"); | ||
} | ||
} | ||
|
||
[Fact] | ||
public void WhenAuditingLoggingExceptionsPropagate() | ||
{ | ||
using (var tmp = TempFolder.ForCaller()) | ||
using (var log = new LoggerConfiguration() | ||
.AuditTo.File(new ThrowingLogEventFormatter(), tmp.AllocateFilename()) | ||
.CreateLogger()) | ||
{ | ||
var ex = Assert.Throws<AggregateException>(() => log.Information("Hello")); | ||
Assert.IsType<NotImplementedException>(ex.GetBaseException()); | ||
} | ||
} | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,15 @@ | ||
using System; | ||
using System.IO; | ||
using Serilog.Events; | ||
using Serilog.Formatting; | ||
|
||
namespace Serilog.Tests.Support | ||
{ | ||
public class ThrowingLogEventFormatter : ITextFormatter | ||
{ | ||
public void Format(LogEvent logEvent, TextWriter output) | ||
{ | ||
throw new NotImplementedException(); | ||
} | ||
} | ||
} |
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.
The precondition checks have always thrown in
File()
; this PR changes what throws/what doesn't very slightly, in that invalid path errors are now always suppressed inWriteTo
scenarios and always propagate withAuditTo
.In general it's really hard to pin down which argument errors should be throwing ones. Serilog aims to always propagate API usage errors, and always (by default) suppress environmental/transient/runtime errors. In the case of the System.IO APIs and various configuration providers the line is fuzzy at times.