-
Notifications
You must be signed in to change notification settings - Fork 4.7k
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
[Feature Request] Allow libraries that don't use DI to make use of ILogger #50777
Comments
Tagging subscribers to this area: @maryamariyan Issue DetailsMy biggest current challenge recommending library authors to use ILogger in preference to EventSource is that their library may not use dependency injection. I mean this in the general sense that their library may not have a constructor parameter or a property where an ILogger object could be passed to them and they either can't or don't want to modify their library to add those parameters. Currently EventSource satisfies that use-case well because they can be new'ed up anywhere and automatically register globally, eliminating the need to pass them around. If we want ILogger to support a similar range of usage then we need to support some globally scoped ILoggers. To be upfront - I've never had a specific library author push back because this issue affected them. However on the other hand I avoid making blanket statements that library authors should use ILogger, nor do I write that guidance in our docs, primarily because of this concern. Its possible if I started pushing there would be no issue at all, or alternatively the complaints might start rolling in that we didn't think this through before we started encouraging more people to use it. My best guess is that people would quietly make workarounds by adding mutable static fields to their libraries and adding APIs like Library.InitLogger(ILogger) and writing to it with MyLibrary.s_logger?.Log(...). It works for simple cases but it isn't a great place to be:
I think this entails two steps:
At the moment the 2nd option sounds better to me, but there isn't broad agreement on it. The scenario I am imagining is something like you've got multiple test cases, each of which have their own DI container, their own LoggerFactory, and their own logging output. However all the test cases interact with a shared library and it would improve diagnosability if the log output from that shared library could be added to the test logging as well. The 2nd option appears to allow that easily whereas with the 1st option I didn't see a straightforward approach.
|
I have been thinking in detail about whether or not we could come up with a set of best practices (in case it doesn't exist yet) when dealing with libraries such as Logging that have been designed with DI in mind as their object creation pipeline. I have gathered a set of bulletpoints (general ideas) below that I wanted to share here as it seems to be very relevant to the discussion here. These points are not specific to logging, but in general might make sense to always keep in mind when we want to design libraries that need to work well in combination with DI.
I am in favor of making sure our Logging libraries are robust enough such the developers/library authors using it can still take advantage of it whether they are
From the point of view of the Developer using our library,
From the point of view of the logging library itself:
My idea is that, it is good practice if library authors always consider having only one object creation pipeline in their applications. I want to learn about when it is actually appropriate for an application to have multiple DI containers available. If the above bullet points are somewhat agreed upon, then we could hopefully use them as a basis to understand what we are missing for libraries authors/developers that do not use DI to make use of ILogger. I came across this blog post as well (a bit old but still relevant) which helps back the bulletpoints I added above: https://docs.microsoft.com/en-us/archive/msdn-magazine/2009/november/dependency-injection-in-libraries#howd-we-do |
Poking around a little on github it shows ~8000 results for C# code creating new LoggerFactory instances and 50,000 results for C# code creating new ServiceCollection instances. I didn't dive very deep, but a lot of the usage appears to be test cases. Looking at the ASP.NET team's tests we see lots of xunit test cases that derive from LoggedTest and the Initialize() method creates a unique ServiceCollection and LoggerFactory object for each one. Each test's LoggerFactory is hooked up to a unique SerilogLoggerProvider that outputs to a unique named file. If all the tests were attempting to share a single global LoggerFactory (or a single DI container with a single factory inside it) then they would be unable to have those unique configurations.
I think there are two different Developer points of view, not sure which you were refering to?
The scenario I had in mind is:
Trivial example, library code: class Foo
{
void DoSomething()
{
// As the library author I want to log this error. I am willing to change the implementation but
// I do not want to change the public API (aka I am unwilling/unable to use a DI pattern).
// This means there is no API that lets my caller pass me an ILogger instance.
string errorString = "Oops a bad thing happened";
}
} Trivial example, app code: LoggerFactory f = ConfigureFactory();
Foo f = new Foo();
// As the app author I want to see the error in this code get logged. I am flexible about what
// code goes inside ConfigureFactory() to make that happen.
f.DoSomething(); |
Thanks for the explanation. Based on the issue description, it seems like from a library author's standpoint, a sample problem at hand is regarding how they could potentially get a hold of a global ILogger in their codebase in a case like below:
I think this makes it even more pressing that there needs to be general best practices here for library designs, not only for our logging library itself but also for any 3rd party library author dealing with ILogger in this case:
And the I am guessing the scope of this issue is to find out any sort of API or functionality that is missing in our logging library such that getting from the above example only requires the library authors to refactor their code using (a) internal APIs or (b) extension methods to make accessing of arbitrary ILoggers If that is the ask here, I would argue that the main purpose of trying to do DI constructor injection is to clearly state dependencies upfront. The alternative would be to hide this dependency through some sort of mechanism, abstraction, or factories, etc. I assume that it should be fine for a library author to not necessarily rely on DI ctor injection to declare its own inner dependencies, but they should do that in a format that does not block their own users from wanting to use DI containers, if they wanted, to get access to ILogger for example. this blog post is very relevant to this issue https://docs.microsoft.com/en-us/archive/msdn-magazine/2009/november/dependency-injection-in-libraries, and it presents a very relevant logging case study as well. |
I've been looking into this more lately and pondering if we had cheaper options that might involve guidance/samples rather than new public APIs. Above I was suggesting that library developers wouldn't want to modify their public API at all but that is probably too strong a requirement. What seems more likely is that they don't want to make sweeping changes or modify their existing types, but they would be willing to add a small amount of new surface area that allows better interaction with ILogger. In fact in the initial description I proposed they would write some Instead of that App codeThe app author gets to pick one of these: App option 1 - No-host app:static void Main(string[] args)
{
ConfigureFactory();
Foo f = new Foo(); // this type is provided by library author and it has logging inside it
f.DoSomething();
}
static void ConfigureFactory()
{
ILoggerFactory factory = LoggerFactory.Create(builder =>
{
// whatever arbitrary config the app dev wants here...
builder.AddConsole();
});
factory.AddFooLibLogging(); // this method was provided by the library author
} App Option 2 - hosted appstatic void Main(string[] args)
{
IHost host = Host.CreateDefaultBuilder(args)
.ConfigureLogging(logging =>
{
// whatever arbitrary config the app dev wants here...
logging.AddConsole();
})
.ConfigureServices(services =>
{
services.AddFooLib(); // this method was provided by the library author
})
.Build();
// one of many ways the app developer can run some code with their host...
IHostApplicationLifetime lifetime = host.Services.GetService<IHostApplicationLifetime>();
lifetime.ApplicationStarted.Register(() =>
{
Foo f = new Foo(); // this type is provided by library author and it has logging inside it
f.DoSomething();
});
host.Run();
} Library CodeThe library dev gets to pick one of these depending on whether they want to reuse existing EventSources or they are instrumenting with ILogger directly: Library Option 1 - Using ILoggerLibrary implements their Foo type with a static ILogger instance: class Foo
{
internal static ILogger<Foo> Log { get; set; } = new NullLogger<Foo>();
public void DoSomething()
{
Log.LogInformation("Something happened");
}
} Library implements the extension methods to let app authors enable the logging: public static class ExtensionFunctions
{
public static ILoggerFactory AddFooLibLogging(this ILoggerFactory factory)
{
// this impl doesn't support multiplexing to multiple LoggerFactories but
// but a more complex one could
Foo.Log = factory.CreateLogger<Foo>();
return factory;
}
internal static void RemoveFooLibLogging(this ILoggerFactory factory)
{
Foo.Log = new NullLogger<Foo>();
}
public static IServiceCollection AddFooLib(this IServiceCollection services)
{
services.AddHostedService<LoggerFactoryForwarder>();
return services;
}
}
class LoggerFactoryForwarder : IHostedService
{
ILoggerFactory _loggerFactory;
public LoggerFactoryForwarder(ILoggerFactory factory)
{
_loggerFactory = factory;
}
public Task StartAsync(CancellationToken cancellationToken)
{
_loggerFactory.AddFooLibLogging();
return Task.CompletedTask;
}
public Task StopAsync(CancellationToken cancellationToken)
{
_loggerFactory.RemoveFooLibLogging();
return Task.CompletedTask;
}
} Library Option 2 - Adapt pre-existing EventSource instrumentationLibrary author has their existing implementation: [EventSource(Name = "FooCompany-Foo")]
class FooEventSource : EventSource
{
[Event(1, Message = "Something happened")]
public void SomethingHappened()
{
WriteEvent(1);
}
}
class Foo
{
internal static FooEventSource Log { get; private set; } = new FooEventSource();
public void DoSomething()
{
Log.SomethingHappened();
}
} Library dev creates this (sadly non-trivial) adapter. This is roughly cribbed from what Azure SDK is doing. public static class ExtensionFunctions
{
static LoggerFactoryForwarder s_forwarder;
public static ILoggerFactory AddFooLibLogging(this ILoggerFactory factory)
{
s_forwarder = new LoggerFactoryForwarder(factory);
return factory;
}
public static IServiceCollection AddFooLib(this IServiceCollection services)
{
services.AddHostedService<LoggerFactoryForwarder>();
return services;
}
}
class LoggerFactoryForwarder : EventListener, IHostedService
{
ILoggerFactory _loggerFactory;
ILogger<Foo> _logger;
public LoggerFactoryForwarder(ILoggerFactory factory)
{
_loggerFactory = factory;
}
public Task StartAsync(CancellationToken cancellationToken)
{
Start();
return Task.CompletedTask;
}
public void Start()
{
_logger = _loggerFactory.CreateLogger<Foo>();
EnableEvents(Foo.Log, EventLevel.Informational);
}
public Task StopAsync(CancellationToken cancellationToken)
{
Stop();
return Task.CompletedTask;
}
public void Stop()
{
DisableEvents(Foo.Log);
}
protected override void OnEventWritten(EventWrittenEventArgs eventData)
{
// AzureSDK had some custom formatting logic here which makes the adapter yet larger
// I omitted it for brevity
_logger.Log(ConvertLogLevel(eventData.Level), eventData.Message, new EventSourceEvent(eventData));
}
LogLevel ConvertLogLevel(EventLevel level)
{
return level switch
{
EventLevel.LogAlways => LogLevel.Critical,
EventLevel.Critical => LogLevel.Critical,
EventLevel.Error => LogLevel.Error,
EventLevel.Warning => LogLevel.Warning,
EventLevel.Informational => LogLevel.Information,
_ => LogLevel.Trace
};
}
private readonly struct EventSourceEvent : IReadOnlyList<KeyValuePair<string, object>>
{
public EventWrittenEventArgs EventData { get; }
public EventSourceEvent(EventWrittenEventArgs eventData)
{
EventData = eventData;
}
public IEnumerator<KeyValuePair<string, object>> GetEnumerator()
{
for (int i = 0; i < Count; i++)
{
yield return new KeyValuePair<string, object>(EventData.PayloadNames[i], EventData.Payload[i]);
}
}
IEnumerator IEnumerable.GetEnumerator()
{
return GetEnumerator();
}
public int Count => EventData.PayloadNames.Count;
public KeyValuePair<string, object> this[int index] => new KeyValuePair<string, object>(EventData.PayloadNames[index], EventData.Payload[index]);
}
} I think seeing these working gives me a little more confidence giving blanket recommendations for ILogger, even though I feel a little gross asking a library dev to write that EventSource -> ILogger adapter if they find themselves in option 2. Maybe there aren't many devs that are in that situation, but if we get feedback that are a lot of people doing this we should try to make some shared community or runtime implementation of the forwarder. Thoughts? |
👍 on this, ran into this problem when adopting Microsoft.Extensions.Logging for the Npgsql library, where ADO.NET is not DI-compatible. Some best practices and documentation would be helpful. |
@roji thanks for feedback! Were you primarily suggesting that the info above was good but needed to be more easily discoverable, or that more info is needed, or both?
I'll admit I have seen very few real world examples so far. I think and hope the above suggestions will be effective, but I wouldn't want to claim this is a well proven technique if Azure SDK is my only data point : ) |
@noahfalk I think the info above is good - it's mostly that it needs a home under the logging documentation section (i.e. a sub-page for implementing logging for libraries?). One thing about "Library Option 1": there's a suggested AddFooLib which accepts an IServiceCollection, for making it easier to use in DI scenarios. First of all, I wouldn't necessarily recommend that applications take a dependency on Microsoft.Extensions.DependencyInjection.Abstractions just in order to provide such an adapter; if the library really isn't DI-compatible (after all, it accepts its ILoggerFactory statically) then it probably makes sense for users to just invoke the static configuration method directly. If it is possible for the library to just get the ILoggerFactory from DI, like a proper DI-aware library (similar to ASP.NET, EF Core), then obviously static configuration of ILoggerFactory is out of place in the first place. So when explaining this to library maintainers, I'd maybe split this into "DI-aware libraries" and "non-DI-aware libraries", where only the latter accepts ILoggerFactory manually. Apart from that, I haven't seen many libraries which have a pre-existing EventSource which they'd like to adapt to logging - but maybe that's useful. |
The tradeoff appears to be the library can either take the dependency on M.E.DI.Abstractions (43KB binary) + write some boiler plate code to get a more idiomatic looking configuration or it can avoid the reference and have a less idiomatic initialization. If the library author does do the extra work I think you get an app developer experience like this: var builder = WebApplication.CreateBuilder(args);
// Add services to the container.
builder.Services.AddRazorPages();
builder.Services.AddFooLib();
var app = builder.Build();
... and if you don't do the extra work you get an app developer experience like this: var builder = WebApplication.CreateBuilder(args);
// Add services to the container.
builder.Services.AddRazorPages();
var app = builder.Build();
app.Services.GetService<ILoggerFactory>().AddFooLibLogging();
... Admitedly as a guy who does most of my coding in not so pretty code inside the coreclr.dll this feels like a pretty minor distinction so I'm inclined to agree with you - it doesn't seem worth the extra effort. However I don't know if folks like @davidfowl have stronger opinions about the style/idioms he is trying to encourage. If I don't hear anything further I'm going to go with your suggestion that the non-DI-aware libraries make an 'AddFooLibLogging()' method available only. |
Yeah, I do get the argument about API niceness. Another argument here is that builder.Services.AddFooLib() gives the impression of a proper DI-aware library, where the ILoggerFactory gets managed within the service provider. This isn't the case - under the hood it's actually managed statically, which e.g. means that you can't have two service providers with different logging configurations in the same application. I'd say it's better not to hide that fact, and make the static aspect very explicit; in fact, rather than an AddFooLibLogging extension method, I'd simply tell users to call an explicitly static method to initialize logging, e.g. FooLoggingConfiguration.Initialize(ILoggerFactory loggerFactory). |
Not necessarily a (public) library author myself so take my opinion with a grain of salt. However:
I don't see how this matters to be honest. One doesn't need to use "dependency injection containers" to use proper inversion of control best practices. Some people even go with "poor-man's DI" on big projects and have success with that. A library could still take an
Well if they don't want to do anything on their side, I don't think the framework should either. Adding static versions of ILogger and things like that would massively degrade the quality of these APIs and might push people away from proper inversion of control on new applications. I'd be 100% opposed of providing any static abstractions in
Supporting this specific use case directly is a mistake IMHO. There is even an One such case is the .NET OpenTelemetry integration, which currently doesn't support logging through |
Adding some color to it from Azure SDK / System.ClientModel perspective. Let's assume we take Let's assume there is non-negligeable number of users that don't use DI or use our clients unconventionally - e.g. don't register them in the DI and don't pass Now, imagine such user app has a problem and now they can't enable logs without code change and full redeployment. This is the critical part of the problem for us. We don't necessarily need a perfect way to configure logger factory as a static singleton, we need the operational fallback. As a solution we could do some form of Option 2 from the issue description:
All of these options are possible, but have problems. It seems to be a generic problem though and it'd be great if |
Can you provide an example of how that would look like for a hypothetical library class? I just don't see why the library should make these types of decisions for the consumer. If they are using your class directly, and purposefully not passing an That has nothing to do with DI containers. The only difference is that, to more simply support DI registrations, your library would provide a If the user opts to not use DI, then it's up to them to provide something the library can use. If they decide they don't need any logging and pass in a
What would these fallbacks look like exactly though? |
It can look like this:
The fallback could be the following:
Then users who configured logging will have it, those who don't can start listening to the corresponding event source via PerfView, dotnet-monitor or any other similar consumer without the need to change their code and redeploy the app. |
It wasn't clear what the request would be for M.E.L.Abstractions? It looked like you were suggesting the library would create its own LoggerFactory via LoggerFactory.Create() which is already a public API and something you could do without any runtime changes. |
@noahfalk Given that it's a generic problem for any library that can be used without DI, the ask is to have something operational by default out-of-the-box. The alternative could be auto-discovery based on installed libraries - It's a common logging practice to enable logging by detecting an implementation package and/or configuring env vars. But with M.E.Logging we ask users to install things AND also pass |
I'd like to make sure we don't make it easy for MEL to be used in that way. The testing situation with event source is not great and shared mutable state is a downfall of testability 😄. Let's not introduce this problem here. |
The trade-off is default user experience vs testability. For tracing and metrics we chose user experience and for logging we keep choosing testability. Why? [Update] I'd be happy with any solution that reliably provides some default experience (not requiring users to pass |
I think the default for all of these APIs should be testability, but that's not the primary purpose. It's the side effects of what building a testable (and sometimes mockable) API enables. The ability to isolate calls to a specific logger factory instance (and same with meters and traces) is extremely useful when trying to build complex software. Serilog has an interesting design here where there's a static Logger.Log can be assigned an ILogger instance: using Serilog;
Log.Logger = new LoggerConfiguration()
.WriteTo.Console()
.CreateLogger();
try
{
// Your program here...
const string name = "Serilog";
Log.Information("Hello, {Name}!", name);
throw new InvalidOperationException("Oops...");
}
catch (Exception ex)
{
Log.Error(ex, "Unhandled exception");
}
finally
{
await Log.CloseAndFlushAsync(); // ensure all logs written before app exits
} The reason this is "less bad" is because application code is supposed to do this, not library code. Library code should not be assuming shared static configuration (when it can be avoided). |
I agree with all you're saying, but it does not solve the problem for libraries. If BCL does not provide a fallback, we'd need to do it ourselves. Some of libs might decide that lack of default experience is a blocker and will not do logging at all (or keep using EventSource). All libs that will implement a fallback will do it differently making users life hard. |
The only blocker libraries should have is the dependency on ILoggerFactory. The only reason to log using event source is if you're unwilling to take that dependency or you are below the dependency graph of the ILoggerFactory (like you are implementing one and need to log errors from the logger implementation). The library just has to worry about providing a way to set the ILoggerFactory so that the application can decide which configuration the library should use. PS: This is what the Azure SDK should do instead of using the EventSource. It would make it much easier to use and test without the event source -> Ilogger adapter. |
I feel we're talking about different things. Let's forget about what Azure SDKs currently do and let's imagine a perfect future. What we'd like to have:
What I'd like to have to solve the p2.2
|
What sorts of apps are your primary concern? Practically, it seems like what is being proposed here is to provide an API that creates a logger factory with an event source logger provider. What do you tell the customer to do see the logs? Use dotnet trace or perfview? Maybe the console logger is a more pragmatic solution… |
nothing in particular. Any non-ASP.NET Core apps where DI is not a common practice.
👍
Any of those (+ dotnet-monitor), it's ok that it's inconvenient - it's the last resort to get some information out. We want people to configure logging properly. Console logger might be an option too. Maybe the fact that it writes something (warn+) to console by default would make it even more obvious that logging is not configured properly. |
Why would DI not be a common practice? If it isn't, then you should push for it.
Make it mandatory and provide a If you want to fully decouple your library from logging concerns (and dependencies), create a separate package that people can opt into in "raw" scenarios. Example:
Then, on I still don't understand why you are trying to optimize for a bad default of having no DI though. Just ask people to use generic host which is what they should be doing anyways (IMHO).
If logging is not configured and you want to make that obvious... make it mandatory, throwing if |
I'm comparing these two scenarios in my head when the library user doesn't provide ILogger (whether by choice, by accident, or because the library's author didn't provide an API to allow it) 1. Library creates and uses its own default LoggerFactory (lib authors could do this today)static class MyLibrary
{
internal static LoggerFactory DefaultLoggerFactory = LoggerFactory.Create(builder =>
{
builder.AddEventSourceLogger();
});
}
public class SomeLibraryType
{
ILogger _logger;
public SomeLibraryType()
{
_logger = MyLibrary.DefaultLoggerFactory.CreateLogger<SomeLibraryType>();
}
} 2. M.E.L.A provides a static factory (in some future version of .NET)M.E.L.A: static class Logging
{
public static ILoggerFactory DefaultLoggerFactory = LoggerFactory.Create(builder =>
{
builder.AddEventSourceLogger(); // this could be configured other ways too, ignoring that
// aspect for now
});
} Library code: public class SomeLibraryType
{
ILogger _logger;
public SomeLibraryType()
{
_logger = Logging.DefaultLoggerFactory .CreateLogger<SomeLibraryType>();
}
} I'm curious to hear what you think of the pros/cons? This is what I thought about:
|
@noahfalk what I'd like to have class MyClient
{
private readonly ILogger<MyClient> _logger;
public MyClient(MyOptions options) {
_logger = options.LoggerFactory.CreateLogger<MyClient>()
}
public DoSomething() {
...
_logger.LogInfo("Did something!")
}
}
class MyOptions {
public ILoggerFactory LoggerFactory { get; set; } = CreateDefault();
private static ILoggerFactory CreateDefault() {
// Option 1 - you decide
return LoggerFactory.CreateDefault();
// Option 2 - we decide
return LoggerFactory.Create(b => b.AddEventSourceLogger());
}
} We want users to pass configured The sole purpose of this is to have some way of getting logs out (without code change) if users forgot to configure logging/did it incorrectly/don't use DI/don't know how to use DI, etc. That's the fallback - not the way to configure things. [Update] You do Option 1 now and provide |
I don't have a strong opinion whether something should be added or not. If something gets added, I think it would make sense for it to be a Something like: static class FallbackLogger
{
static void SetInstance(ILoggerFactory); // Could throw InvalidOperationException if already set.
static ILoggerFactory Instance { get; } // Returns NullLogger.Instance when not set.
} A library developer could then: public MyClient(ILoggerFactory? factory = null) => _factory = factory ?? FallbackLogger.Instance; An application developer can initialize the fall back logger in their using ILoggerFactory loggerFactory = LoggerFactory.Create(builder => builder.AddConsole());
FallbackLogger.SetInstance(loggerFactory);
... I think the Though this doesn't initialize a default |
@lmolkova passing service dependencies through an Once again, to me, you are just making things more complicated than they need to be by doing things like this. |
private static ILoggerFactory CreateDefault() {
// Option 1 - you decide
return LoggerFactory.CreateDefault();
// Option 2 - we decide
return LoggerFactory.Create(b => b.AddEventSourceLogger());
} Cool that refines the 'what' part a bit. Where I am still struggling is the 'why?' The pros/cons for this seem close to the example I gave above and so far I don't see a compelling reason to build option (1) as a new thing when option (2) already exists. You mentioned standardization and that this would apply to many libraries, but given the (lack of) feedback I've seen from other library authors I don't have an expectation many library authors would create these fallback paths even if the API existed. I'd also guess that the library authors interested in that API would be content to write option (2) instead if we recommended it and they knew it would have the same outcome. |
@noahfalk that's fair. E.g. in Java you configure logging by providing logging implementation and configuring it with env vars/files. You can provide logging implementation without changing the code (by adding In .NET we can get this amazing experience for tracing and metrics, but not for logs. For logs we can get nothing at all if users didn't do the DI stuff in a certain way. So this is why the logging feels incomplete and not on par with the experience our users can get in other languages. The fallback could address some of it in the most basic way. I take the point that Azure SDKs probably care the most about it (and the only ones that would even consider |
Thanks @lmolkova!
I'll gladly admit the lack of symmetry between the logging API and the other telemetry signal APIs bugs the part of my brain that likes everything orderly. If I wanted to appease that part of my brain I'd probably have an API like this: ILogger logger = loggerFactory.Create<Foo>(...);
ILogger logger = new Logger<Foo>(); // this one is missing
Meter m = meterFactory.Create(...);
Meter m = new Meter(...);
ActivitySource a = activitySourceFactory.Create(...); // this one is missing
ActivitySource a = new ActivitySource(...); But a few things hold me back from acting on that impulse for logger:
I think of this as two separate pitfalls that lead to a similar outcome:
For (2) I land back where I was above... I don't think where we are is ideal but there are decent workarounds + the lack of feedback saying this is a significant problem discourages me from investing in changes. If that changed, either broader feedback or finding the existing workarounds were more problematic, then I'd have more motivation to explore changes. |
I'm against a shared mutable static global configuration API for ILogger (I don't like the ones we have for meter and activities either). I'm less concerned about a default logger factory implementation that has the event source logger. We can just recommend that library authors provide a fallback that uses the event source logger (I understand the event source is shared mutable global state). |
My biggest current challenge recommending library authors to use ILogger in preference to EventSource is that their library may not use dependency injection. I mean this in the general sense that their library may not have a constructor parameter or a property where an ILogger object could be passed to them and they either can't or don't want to modify their library to add those parameters. Currently EventSource satisfies that use-case well because they can be new'ed up anywhere and automatically register globally, eliminating the need to pass them around. If we want ILogger to support a similar range of usage then we need to support some globally scoped ILoggers.
To be upfront - I've never had a specific library author push back because this issue affected them. However on the other hand I avoid making blanket statements that library authors should use ILogger, nor do I write that guidance in our docs, primarily because of this concern. Its possible if I started pushing there would be no issue at all, or alternatively the complaints might start rolling in that we didn't think this through before we started encouraging more people to use it. My best guess is that people would quietly make workarounds by adding mutable static fields to their libraries and adding APIs like Library.InitLogger(ILogger) and writing to it with MyLibrary.s_logger?.Log(...). It works for simple cases but it isn't a great place to be:
I think this entails two steps:
At the moment the 2nd option sounds better to me, but there isn't broad agreement on it. The scenario I am imagining is something like you've got multiple test cases, each of which have their own DI container, their own LoggerFactory, and their own logging output. However all the test cases interact with a shared library and it would improve diagnosability if the log output from that shared library could be added to the test logging as well. The 2nd option appears to allow that easily whereas with the 1st option I didn't see a straightforward approach.
The text was updated successfully, but these errors were encountered: