Add support for provider registration in logging #20215
Conversation
| @@ -0,0 +1,59 @@ | |||
| // Copyright (c) .NET Foundation. All rights reserved. | |||
There was a problem hiding this comment.
I'll be working on adding more tests here but this is ready for review in the meanwhile.
src/Components/WebAssembly/WebAssembly/src/Services/Logging/WebAssemblyLoggerFactory.cs
Show resolved
Hide resolved
|
Looks like a great starting point! It would be great if you change the standalone app to use the same pattern ASP.NET Core apps use to configure logging through config. (If it's picked automatically). The other thing I would point out, is that the way of configuring logging seems a bit different than regular ASP.NET Core apps. Should we make the pattern similar? |
|
@captainsafia Is it OK if I review this tomorrow? (Currently I'm really focused on getting some things ready for the ASP.NET community standup later today) |
I was planning on doing the configuration bits in a separate PR, but I'll tack it all into this one since it's the last week for preview4 work.
Sure. I will reping team once this is ready for review with the configuration changes. |
src/Components/WebAssembly/WebAssembly/src/Services/Logging/WebAssemblyConsoleLoggerProvider.cs
Show resolved
Hide resolved
| /// <inheritdoc /> | ||
| public ILogger CreateLogger(string name) | ||
| { | ||
| return _loggers.GetOrAdd(name, loggerName => new WebAssemblyConsoleLogger<object>(name, _jsRuntime)); |
There was a problem hiding this comment.
Maybe this implementation comes from somewhere else, but I'm surprised it's not an error to call this with a name that's already registered, since the underlying dictionary can't store more than one with the same name.
There was a problem hiding this comment.
This is based on the implementation of the ConsoleLoggerProvider in Microsoft.Extensions.Logging.
since the underlying dictionary can't store more than one with the same name.
Hm. GetOrAdd is used here so that constraint should be satisfied even if the user calls CreateLogger("someName") twice.
| { | ||
| if (!_disposed) | ||
| { | ||
| _loggers = null; |
There was a problem hiding this comment.
Is there any chance we need to Dispose the ILogger instances before nulling this out?
| exceptions.Add(ex); | ||
| } | ||
|
|
||
| return false; |
There was a problem hiding this comment.
This logic to aggregate exceptions looks super robust, though TBH I'm not certain what the use case for it is. If we didn't catch any of this, and just let the first exception be unhandled (since it's never a legit scenario to have loggers throw), would that be sufficient? Are there cases where it's necessary to collect more information than that?
There was a problem hiding this comment.
This is heavily inspired by the Logger implementation in Microsoft.Extensions.Logging.
The intent behind this logic was to collect all the exceptions that arise from each logger that is registered and throw an aggregate exception for them. It is possible to bail and throw at the first exception thrown from a logger but I figured I would keep this logic because:
- It's more helpful to the developer to get all the exceptions from buggy loggers instead of getting them one-by-one.
- Wanted to follow existing experiences that devs would be used to. This didn't seem, outrageous enough to nuke.
src/Components/WebAssembly/WebAssembly/src/Services/Logging/WebAssemblyLoggerFactory.cs
Show resolved
Hide resolved
src/Components/WebAssembly/WebAssembly/src/Hosting/WebAssemblyHostBuilder.cs
Show resolved
Hide resolved
pranavkm
requested review from
333fred and
BrennanConroy
and removed request for
333fred
|
Closing this PR in favor of an approach that relies on the Logger factory within Microsoft.Extensions.Logging. The WebAssemblyLoggerFactory was originally included in the code base because there was a concern that bringing in Microsoft.Extensions.Logging as a dependency would increase the total size of DLLs that would have to be pulled down. I ran some quick validations on the BasicTestApp to see the size difference between the compressed DLLs that including the reference would bring. It's negligible enough that we can proceed with an approach that doesn't use a custom logger factory.
|
Changes in this PR
Most of the logic in
WebAssemblyLogger,WebAssemblyLoggerFactory, andWebAssemblyLoggerInformationis lifted from the implementations inMicrosoft.Extensions.Logging.Addresses #19737