-
Notifications
You must be signed in to change notification settings - Fork 759
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
Allow UseLogging even when there's no ILoggerFactory #5687
Conversation
1b1ff49
to
b0a53ac
Compare
Can you specify what scenario this is for? Why would someone need to no-op instead of fixing it (by adding logging services or removing To me it seems strange to no-op when the application has explicitly tried to enable logging. In general I'd expect a "required service missing" exception in these cases otherwise the application developer will struggle to understand why I'm guessing some scenario prompted you to implement this so maybe there is a need for it, but it would be good to clarify the reasoning. My best guess is it's for cases when |
Any kind of component/library that's instantiating a client. In general any such component should be enabling logging if a logger factory is in DI, which means they're all effectively going to need to do the same thing: call AddLogging but only if DI contains a logger factory. So why force them to all do: ChatClientBuilder builder = innerClient
.WhateverA()
.WhateverB();
if (services.GetService<ILoggerFactory>() is {} loggerFactory)
{
builder.AddLogging(loggerFactory);
}
IChatClient client = builder.Build(services); rather than: IChatClient client = innerClient
.WhateverA()
.WhateverB()
.AddLogging()
.Build(services); On top of that, the logging infrastructure makes the notion of a nop-logger first class, with NullLoggerFactory.Instance being in the abstractions lib, so it's not clear to me why GetService returning null vs a null logger should result in AddLogging throwing an exception vs nop'ing. Many uses across the ecosystem end up doing the equivalent of We also have AddOpenTelemetry and AddFunctionInvocation, both of which enable logging of their pieces optionally based on whether a logger factory is available; they don't throw an exception if a logger isn't there, they just don't enable that functionality. Effectively, AddLogging feels like it should really be AddLoggingIfEnabled, and we've just chosen a shorter name. That's my rationale for why to do it. The counter-argument would be "someone is asking for logging and there's no logging support available, so it's better to throw to tell them they're holding it wrong". It really comes down to what semantics we want here. |
That's true. We have a lot of prior art of framework pieces (in ASP.NET Core and Blazor at least) doing logging or secondary tasks conditionally based on what DI services are registered. However I'm not sure there's prior art when it's the primary or only task, as if is here. Logging is an interesting case because unlike most other middleware, it has no behavioral impact so it would actually be safe to silently ignore So altogether I'm coming round towards being persuaded that falling back on |
Ok, thanks. I'll go ahead with this then. |
🎉 Good job! The coverage increased 🎉
Full code coverage report: https://dev.azure.com/dnceng-public/public/_build/results?buildId=880030&view=codecoverage-tab |
I tend to agree with @SteveSandersonMS's initial comment. While I get that it unblocks certain usage patterns in intermediate libraries, falling back to the null logger in the context of an application's composition root is almost certainly an indication of misconfiguration being silently ignored. Would exposing an additional |
I thought of that, or of renaming the method AddLoggingIfAvailable, or of having two differently named methods, but I keep coming back to the idea that: If we really don't like the idea of AddLogging behaving as if it were AddLoggingIfAvailable, then I'd still want to add the optimization to AddLogging that checks whether the provided logger factory is NullLoggerFactory.Instance, and then a consumer that wants that optional behavior can do That's my thinking at least. |
I'm not too sure about that. The presence of logs in an application isn't something you can easily test for in an automated fashion, so this makes it easier for logs in prod to be lost. I don't object to this behaviour being made available, but I just think it should either use an explicit name or have an opt in flag. |
I don't think logging associated with this subsystem should implicitly be the canary for the whole application. I think this method really is AddLoggingIfAvailable just with a simpler name, just like AddOpenTelemetry is really AddLoggingIfAvailableAndCreateActivityAndMetricsIfEnabled. If someone wants to validate that there is a logger and pass that in, it's just passing GetRequiredService to the factory parameter. |
@SteveSandersonMS, opinions on this? I'm torn.
Microsoft Reviewers: Open in CodeFlow