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.

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

Don't inherit parent Minimum Level for inline sublogger configuration #1472

Merged
merged 3 commits into from
Aug 2, 2020

Conversation

skomis-mm
Copy link
Contributor

@skomis-mm skomis-mm commented Jul 31, 2020

What issue does this PR address?
Fixes #1346 , #1453

Does this PR introduce a breaking change?
Behavior change for subloggers configured inline when no explicit default level specified for them.

Please check if the PR fulfills these requirements

  • The commit follows our guidelines
  • Unit Tests for the changes have been added (for bug fixes / features)

Other information
Inheriting minimum level was done before overrides where introduced, so it doesn't take them into account now for subloggers which results in counterintuitive behavior.

_applyInheritedConfiguration(lc);
lc.MinimumLevel.Is(LevelAlias.Minimum);
Copy link
Member

Choose a reason for hiding this comment

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

Since _applyInheritedConfiguration ends up calling:

        void ApplyInheritedConfiguration(LoggerConfiguration child)
        {
            if (_levelSwitch != null)
                child.MinimumLevel.ControlledBy(_levelSwitch);
            else
                child.MinimumLevel.Is(_minimumLevel);
        }

it seems like removing that call here might be a cleaner fix?

I had a quick attempt at figuring out the original purpose of _applyInheritedConfiguration and can't track it down; I suspect it may have been part of an earlier version that took advantage of Logger directly implementing ILogEventSink.Emit() (without any level check), but can't immediately tell from the source.

Removing it entirely seems not to break any tests. Am I missing anything?

Copy link
Contributor Author

@skomis-mm skomis-mm Aug 1, 2020

Choose a reason for hiding this comment

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

I've tracked it down to this pr. Part of it was the fix for 804 issue.

The issue was about case with default minimum level of the root less than Information which results in explicit setting minimum for subloggers by users.

The open question - why it was choosen the same as for the parent instead of Verbose..

Hint about Logger as ILogEventSink led me to examples:

Behavior before this pr:

new LoggerConfiguration()
    .MinimumLevel.Verbose()
    .WriteTo.Sink(
        new LoggerConfiguration()
            .MinimumLevel.Fatal()
            .WriteTo.Console()
            .CreateLogger())
    .CreateLogger()
    .Verbose("oops"); // surpasse the Fatal filter

but

new LoggerConfiguration()
    .MinimumLevel.Verbose()
    .WriteTo.Sink(
        new LoggerConfiguration()
            .MinimumLevel.Fatal()
            .WriteTo.Logger(lc => lc.WriteTo.Console())
            .CreateLogger())
    .CreateLogger()
    .Verbose("oops"); // suppressed by the sublogger's Fatal filter

After this pr both examples will result in surpassing the Fatal filter. And makes the behavior consistent :)

So it makes sense to remove these lines as you pointed.

@nblumhardt
Copy link
Member

Sorry I missed your mention/comments on 1453 - I've kept meaning to loop back around to this 😓

Making some time to catch up, now.

@skomis-mm
Copy link
Contributor Author

Removed ApplyInheritedConfiguration since it was used just in one place

@nblumhardt nblumhardt merged commit 308208e into serilog:dev Aug 2, 2020
@nblumhardt
Copy link
Member

LGTM 👍

@skomis-mm skomis-mm deleted the subMinLvl branch August 2, 2020 12:23
@nblumhardt nblumhardt mentioned this pull request Sep 10, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Sublogger Ignoring Events from Parent Logger
2 participants