Add LoggerFactory.Create method by pakrym · Pull Request #640 · dotnet/extensions

pakrym · 2018-12-13T03:57:25Z

Adds the LoggerFactory.Create that simplifies LoggerFactory creation in cases where dependency injection is not used in the application.

So the pattern becomes:

var loggerFactory = LoggerFactory.Create(builder => 
    builder.AddEventSourceLogger()
           .AddConsole()
           .AddFilter(...));

Fixes: #615

src/Logging/Logging/src/LoggerFactory.cs

davidfowl · 2018-12-13T12:05:54Z

src/Logging/Logging/src/Microsoft.Extensions.Logging.csproj


    <Reference Include="Microsoft.Extensions.Configuration.Binder" />
-    <Reference Include="Microsoft.Extensions.DependencyInjection.Abstractions" />
+    <Reference Include="Microsoft.Extensions.DependencyInjection" />


This is slightly unfortunate. But not the end of the world I guess.

src/Logging/Logging/src/LoggerFactory.cs

davidfowl · 2018-12-13T12:09:10Z

src/Logging/Logging/src/LoggerFactory.cs

            public bool ShouldDispose;
        }
+
+        private class DisposingLoggerFactory: ILoggerFactory


The fact that we need to hold onto the container means all of the sample code is sorta busted

Busted how?

Maybe you’re saying we don’t need to dispose the service provider?

Why does this wrapper exist to begin with?

To dispose ILoggerProviders created by DI

Doesn’t disposing the factory do that?

Ah, I see it wouldn’t. That makes the current usage without this API leaky.

If you don't dispose the service provider - yes.

I’m taking about examples that you would use today (even in our samples)

Pushed a sample fix.

ryanbrandenburg · 2018-12-13T20:51:53Z

src/Logging/Logging/src/Microsoft.Extensions.Logging.csproj


    <Reference Include="Microsoft.Extensions.Configuration.Binder" />
-    <Reference Include="Microsoft.Extensions.DependencyInjection.Abstractions" />
+    <Reference Include="Microsoft.Extensions.DependencyInjection" />


It seems that this change caused these failures in my PR build. It actually showed up in your PR check too, but as a warning instead of an error.

@natemcmaster 1. Is the proper solution here (assuming it is an approved breaking change) to edit this line? 2. Should this kind of change be a warning or an error?

@ryanbrandenburg does your PR invoke build with --warnaserror?

Yes, that's what Arcade defaults to. We can certainly override that anywhere (or everywhere), but I thought I'd ask since this seems like the kind of thing we might want to fail a build (even if it's just so we can document that this was an intended break).

Commit migrated from dotnet/extensions@1414cac

Add LoggerFactory.Create method

17ac9ac

pakrym requested review from ajcvickers and davidfowl December 13, 2018 03:57

davidfowl reviewed Dec 13, 2018

View reviewed changes

src/Logging/Logging/src/LoggerFactory.cs Outdated Show resolved Hide resolved

davidfowl reviewed Dec 13, 2018

View reviewed changes

src/Logging/Logging/src/LoggerFactory.cs Outdated Show resolved Hide resolved

davidfowl reviewed Dec 13, 2018

View reviewed changes

src/Logging/Logging/src/LoggerFactory.cs Outdated Show resolved Hide resolved

davidfowl reviewed Dec 13, 2018

View reviewed changes

pakrym added 2 commits December 13, 2018 08:16

Feedback

011ce52

Fixed sample

4f20c2d

davidfowl approved these changes Dec 13, 2018

View reviewed changes

pakrym merged commit 1414cac into master Dec 13, 2018

ryanbrandenburg reviewed Dec 13, 2018

View reviewed changes

natemcmaster deleted the pakrym/LF-Create branch December 13, 2018 22:09

pakrym mentioned this pull request Dec 17, 2018

ILogger implementation for ApplicationInsights microsoft/ApplicationInsights-dotnet-logging#239

Merged

maryamariyan pushed a commit to maryamariyan/runtime that referenced this pull request Feb 28, 2020

Add LoggerFactory.Create method (dotnet/extensions#640)

fa75231

Commit migrated from dotnet/extensions@1414cac

maryamariyan pushed a commit to maryamariyan/runtime that referenced this pull request Mar 2, 2020

Add LoggerFactory.Create method (dotnet/extensions#640)

0d94e73

Commit migrated from dotnet/extensions@1414cac

maryamariyan pushed a commit to maryamariyan/runtime that referenced this pull request Mar 11, 2020

Add LoggerFactory.Create method (dotnet/extensions#640)

893b9d2

Commit migrated from dotnet/extensions@1414cac

maryamariyan pushed a commit to maryamariyan/runtime that referenced this pull request Mar 27, 2020

Add LoggerFactory.Create method (dotnet/extensions#640)

5cacb02

Commit migrated from dotnet/extensions@1414cac

antoinebj mentioned this pull request Nov 6, 2020

Microsoft.Extensions.Http depends on concrete logging and dependency injection packages dotnet/runtime#44349

Closed

ghost locked as resolved and limited conversation to collaborators May 29, 2023

Conversation

pakrym commented Dec 13, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

davidfowl Dec 13, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pakrym Dec 13, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

pakrym commented Dec 13, 2018 •

edited

Loading

davidfowl Dec 13, 2018 •

edited

Loading

pakrym Dec 13, 2018 •

edited

Loading