Make it clear that Sub-logger MinimumLevel.Override is not supported #1033
Conversation
2db825e to
e2125d6
Compare
| "Overrides in a sub-logger are not supported and are therefore ignored. " + | ||
| "We recommend applying .MinimulLevel.Override on the root logger or " + | ||
| "using .Filter in your sub-loggers. " + | ||
| "Note that support for sub-loggers' minimum level overrides may be removed in the next major version."); |
There was a problem hiding this comment.
Reads very nicely 👍
We could probably cut it down a bit to just:
Minimum level overrides are not supported on sub-loggers and may be removed completely in a future version.
since it's googleable back to this issue :-) Happy to go with the longer version if you think it's preferable.
| "Note that support for sub-loggers' minimum level overrides may be removed in the next major version."); | ||
| } | ||
|
|
||
| ILogEventSink secondaryLoggerSink = new SecondaryLoggerSink(subLogger, attemptDispose: true); |
There was a problem hiding this comment.
Can use var. Variable with the same purpose is called secondarySink in the method below.
| return Sink(new SecondaryLoggerSink(logger, attemptDispose: false), restrictedToMinimumLevel); | ||
| ILogEventSink secondarySink = new SecondaryLoggerSink(logger, attemptDispose: false); | ||
|
|
||
| if (logger is Logger concreteLogger) |
There was a problem hiding this comment.
if (logger is Logger concreteLogger && concreteLogger.HasOverrideMap) ?
| var subLogger = lc.CreateLogger(); | ||
| if (subLogger.HasOverrideMap) | ||
| { | ||
| // Overrides in sub-loggers are not supported and will be ignored |
There was a problem hiding this comment.
The comment here is probably redundant, given the content of the message we are writing to SelfLog.
| "Overrides in a sub-logger are not supported and are therefore ignored. " + | ||
| "We recommend applying .MinimulLevel.Override on the root logger or " + | ||
| "using .Filter in your sub-loggers. " + | ||
| "Note that support for sub-loggers' minimum level overrides may be removed in the next major version."); |
There was a problem hiding this comment.
Dropping the comment and using the shorter message would reduce the sense of duplication here.
src/Serilog/LoggerConfiguration.cs
Outdated
| @@ -69,6 +69,11 @@ void ApplyInheritedConfiguration(LoggerConfiguration child) | |||
| /// </remarks> | |||
| public LoggerAuditSinkConfiguration AuditTo => new LoggerAuditSinkConfiguration(this, s => _auditSinks.Add(s), ApplyInheritedConfiguration); | |||
|
|
|||
| void AddOverride(string source, LoggingLevelSwitch levelSwitch) | |||
There was a problem hiding this comment.
I don't know that this method is as useful now given the change in direction; inlining the assignment in the next method can save us five lines.
There was a problem hiding this comment.
yup, I'll put that back to its original state
|
|
||
| namespace Serilog.Tests.Support | ||
| { | ||
| public class TemporarySelfLog : IDisposable |
There was a problem hiding this comment.
We've needed something like this for a while 👍
(A future version might modify this slightly to only siphon off messages for one logical call context/thread at a time, as this will have interleaving problems when we introduce more tests using it that might run in parallel.)
There was a problem hiding this comment.
Not quite sure what that would look like... ThreadStatic comes to mind, but not sure that would help in that case.
|
Thanks for the review ! |
Write a warning message to the SelfLog when using WriteTo.Logger() with a logger that has specified MinimumLevel overrides. Related to issue serilog#967 Related to discussion serilog#1032
e2125d6 to
48b4757
Compare
|
@nblumhardt not sure what is going wrong with the Travis CI build https://travis-ci.org/serilog/serilog/builds/285388771 |
|
Closed and re-opened to trigger a new build |
|
Looks great :-) |
What issue does this PR address?
Related to #967
This is a second PR after comments on #1027 and discussion in #1032
The idea is that we no longer try to support
MinimumLevel.Overridein sub-loggers, but write to theSelfLogthat it is not supported nor recommended, and using.Filteris better, or event better, the overrides of the root logger.Does this PR introduce a breaking change?
No, it actually just makes it explicit that
MinimumLevel.Overrideis not supported on sub-loggersPlease check if the PR fulfills these requirements
Other information:
I'm not super convinced about the message written to SelfLog :) Review from a native English speaker would be nice :)