Skip null loggers when creating message loggers in Logger factory.

[petrroll]

No description provided.

[dotnet-policy-service]

Tagging subscribers to this area: @dotnet/area-extensions-logging
See info in area-owners.md if you want to be subscribed.

[petrroll]

I tried to think how to test this but other than adding InternalsVisibleTo (I assume no no) or reflection (eeh?) I don't see any way...

[KalleOlaviNiemitalo]

Could test the BeginScope optimization specifically.

1. Set up a DI container with two logger providers:
   • The first ILoggerProvider returns NullLogger.Instance only.
   • The second ILoggerProvider creates loggers in which ILogger.BeginScope returns an instance of a custom type.
2. Call ILoggerFactory.CreateLogger
3. Call ILogger.BeginScope.
4. Verify that the scope object is an instance of the custom type, i.e. it is not a wrapper created by Microsoft.Extensions.Logging.Logger.

[petrroll]

Clever, good idea, added :)

[tarekgh]

@petrroll please try to not force-push again, this reset the review, and it is hard to tell what has changed since last review. Thanks!

[KalleOlaviNiemitalo]

Are there benchmark numbers? For initialisation of logging, and for ILogger.Log in steady state after the ILogger has been created by LoggerFactory.

[petrroll]

@KalleOlaviNiemitalo do we need that? Creating logger will be marginally worse with especially worse allocations, logging will be faster - but depending on the Perf of any used logger might or might not be dwarfed.

It doesn't seem particularly useful. This is clean, relatively simple solution. What would we achieve with it?

[KalleOlaviNiemitalo]

I don't insist on benchmarking this, but if you have done the benchmark, then I am curious to see the numbers.

[petrroll]

I haven't so regrettably I can't share. It was motivated by being annoyed that one of may changes at work could to 20+ loggers that are reach only for one category and noticing other libraries doing similar things. Purely that :)

[tarekgh]

/ba-g there is one failure which is looks outage or infra unrelated issue.

[tarekgh]

Thanks @petrroll for providing the change.