Log unhandled exceptions to custom logger #19606

SteveSandersonMS · 2020-03-05T16:34:05Z

... and make WebAssemblyConsoleLogger use the browser's console.* APIs properly, e.g., so LogLevel.Error translates into a call to console.error (and so on, for other log levels).

I said this would be a 20 minute fix. Lols. It took half the day.

I still think this is low-risk enough to put into preview 2. There's not much reason why we'd benefit from keeping it back until preview 3.

The effect of sending unhandled exceptions to the registered ILoggerFactory is that developers can now use this as a centralised place for sending unhandled exception info to a backend collection service.

SteveSandersonMS · 2020-03-05T16:34:51Z

src/Components/Web.JS/src/Platform/Mono/MonoPlatform.ts


  module.printErr = line => {
-    console.error(`WASM: ${line}`);
+    console.error(line);


I removed the WASM: prefix because it serves no purpose. Developers who call Console.WriteLine should be able to choose exactly what string gets logged, without the framework enforcing an arbitrary prefix.

javiercn · 2020-03-05T16:57:18Z

src/Components/WebAssembly/WebAssembly/src/Services/WebAssemblyConsoleLogger.cs

+            if (!(exception is null))
+            {
+                formattedMessage += FormatException(exception);
+            }


It is a bit weird that the logger does this. Does the ConsoleLogger do the same thing?

I would look at what we do in ASP.NET Core and strive for consistency with what is there.

See https://github.com/dotnet/extensions/blob/master/src/Logging/Logging.Console/src/ConsoleLogger.cs#L48-L66

Good point. Changing as per #19606 (comment)

javiercn · 2020-03-05T17:01:59Z

src/Components/test/testassets/BasicTestApp/Program.cs

+            // Replace the default logger with a custom one that wraps it
+            var originalLoggerDescriptor = builder.Services.Single(d => d.ServiceType == typeof(ILoggerFactory));
+            builder.Services.AddSingleton<ILoggerFactory>(services =>
+            {
+                var originalLogger = (ILoggerFactory)Activator.CreateInstance(
+                    originalLoggerDescriptor.ImplementationType,
+                    new object[] { services });
+                return new PrependMessageLoggerFactory("Custom logger", originalLogger);
+            });
+


Do we need APIs to configure this in the WebAssemblyHostbuilder? It seems cumbersome to do at the moment and I can already think of people wanting to configure things like application insights and other telemetry tools here.

Discussed offline. Will file an issue to consider this as a future enhancement.

The right thing to do in the longer term will be to replicate all the logic from LoggerFactory and Logger in https://github.com/dotnet/extensions/blob/master/src. The reason we're not using that package's implementation directly is because of size concerns.

Filed #19737

javiercn · 2020-03-05T17:12:08Z

src/Components/WebAssembly/WebAssembly/src/Rendering/WebAssemblyRenderer.cs

+
+            // Since this is where generic unhandled errors get routed, we want the category
+            // name to be something generic, not "WebAssemblyRenderer".
+            _logger = loggerFactory.CreateLogger(".NET");


I feel that this is a bit too generic .NET. Maybe we can use something more specific, like Blazor.

Is the problem that we have multiple logging sources comming from different places? What do we do on server-side Blazor. Can we be consistent with that?

On Blazor Server, we do this:

aspnetcore/src/Components/Server/src/Circuits/CircuitFactory.cs

Line 67 in bc6bc2d

_loggerFactory.CreateLogger<RemoteRenderer>());

It makes a bit more sense for Blazor Server because you potentially have a lot of different things going on within your server-side ASP.NET Core process, so differentiating things from a circuit versus some other form of request handling is useful.

It doesn't make so much sense for Blazor WebAssembly because our programming model doesn't include other ways of running code.

However on balance I'm now siding with your consistency point, and will change this to loggerFactory.CreateLogger<WebAssemblyRenderer>(). It is a bit weird for that name to be what you have to use to filter these kinds of error messages, but theoretically you might run other code outside the Blazor rendering process, in which case it is a relevant differentiator.

javiercn · 2020-03-05T17:14:07Z

src/Components/WebAssembly/WebAssembly/src/Services/WebAssemblyLoggerFactory.cs

+        {
+            _jsRuntime = (IJSInProcessRuntime)services.GetRequiredService<IJSRuntime>();
+        }
+
        public void AddProvider(ILoggerProvider provider)
        {
            // No-op


I understand that this is not part of this PR, but shouldn't we implement this factory properly?

I believe the existing pattern has extension methods on the factory to add different providers. I'm fine if we file a follow-up issue for this, but I don't want it to get lost in the ether

This probably also needs to respect login levels comming from config (as asp.net core does)

Filed #19737

javiercn

I'm signing-off as the changes are an improvement and we don't have to wait for this to be in preview2.

I've left some comments regarding more deep questions. I would ask that you file a bug so that we can discuss them in more depth and address them as part of either preview3 or preview4 work.

pranavkm · 2020-03-05T18:23:31Z

src/Components/WebAssembly/WebAssembly/src/Rendering/WebAssemblyRenderer.cs

-            {
-                Console.Error.WriteLine(exception);
-            }
+            _logger.LogCritical(exception, "Unhandled exception");


Any reason this isn't the same pattern as the server: https://github.com/dotnet/aspnetcore/blob/master/src/Components/Server/src/Circuits/RemoteRenderer.cs#L388-L391?

Good point.

I'm changing WebAssemblyConsoleLogger and WebAssemblyLoggerFactory to be as similar to the server-side implementations as I think makes sense. There are still some deliberate differences:

In Blazor Server, the severity of an unhandled component rendering exception is "warning", because it's not fatal to the server (only to the circuit). In Blazor WebAssembly, the severity for this is "critical" because it marks the point where the whole environment enters an undefined state and we show the gold "error occurred" bar. It makes sense for these to be different because the logging is done from the point of view of a bigger environment on the server.

The exact details of message formatting are mostly the same, but not precisely. This is because the browser logger (1) doesn't want to see trailing newlines and (2) has its own built-in behavior around colorization and highlighting different severity levels.

The WebAssembly implementations still don't support configuring log levels, because (1) the configuration feature isn't available yet, and (2) it's out of scope for this PR, and tracked by Support more logger features #19737. So it's still hardcoded to "warning" level and above.

pranavkm · 2020-03-05T18:25:18Z

src/Components/WebAssembly/WebAssembly/src/Services/WebAssemblyConsoleLogger.cs

+            if (!(exception is null))
+            {
+                formattedMessage += FormatException(exception);
+            }


See https://github.com/dotnet/extensions/blob/master/src/Logging/Logging.Console/src/ConsoleLogger.cs#L48-L66

SteveSandersonMS · 2020-03-10T11:35:30Z

Although this was already approved, it's now been changed quite a lot since then so I want to give @javiercn and @pranavkm a chance to re-review before merging.

It's missed the boat for preview 2 so we should consider this a preview 3 thing now.

javiercn

Great addition!

…ssemblyConsoleLogger.

… as possible to server-side implementations

SteveSandersonMS added area-blazor Includes: Blazor, Razor Components feature-blazor-wasm This issue is related to and / or impacts Blazor WebAssembly labels Mar 5, 2020

SteveSandersonMS requested review from pranavkm and javiercn March 5, 2020 16:34

SteveSandersonMS commented Mar 5, 2020

View reviewed changes

javiercn reviewed Mar 5, 2020

View reviewed changes

javiercn approved these changes Mar 5, 2020

View reviewed changes

pranavkm reviewed Mar 5, 2020

View reviewed changes

SteveSandersonMS force-pushed the stevesa/logging-unhandled-exceptions branch from 43a6530 to 0ada0f5 Compare March 10, 2020 09:49

javiercn approved these changes Mar 12, 2020

View reviewed changes

SteveSandersonMS force-pushed the stevesa/logging-unhandled-exceptions branch from 2b869b3 to 2f86865 Compare March 12, 2020 18:11

SteveSandersonMS added 13 commits March 13, 2020 09:27

Prepare test cases

8b22eec

Implement actual support for different logging levels

6982f99

Remove pointless "WASM:" prefix on Console output

c3ed431

Smarter handling of "None" and "Critical" errors

48600a6

Handle formatting for arbitrary exceptions. Move this logic into WebA…

9dab4c5

…ssemblyConsoleLogger.

Add E2E tests

8f0c407

CR: Change logger category

3e9b351

Make AddProvider throw so we're safe to implement this in the future

4a066fa

Make WebAssemblyLoggerFactory and WebAssemblyConsoleLogger as similar…

58127bb

… as possible to server-side implementations

Update E2E tests

db52af8

Make log name optional

d0c40f4

Fix for DI

e868067

Update .js files

288783b

SteveSandersonMS force-pushed the stevesa/logging-unhandled-exceptions branch from 2f86865 to 288783b Compare March 13, 2020 09:31

SteveSandersonMS merged commit 9343d2b into blazor-wasm Mar 13, 2020

SteveSandersonMS deleted the stevesa/logging-unhandled-exceptions branch March 13, 2020 10:40

SteveSandersonMS mentioned this pull request Mar 13, 2020

In Blazor WebAssembly, use ILoggerFactory for unhandled exceptions #19533

Closed

captainsafia mentioned this pull request Apr 1, 2020

Add support for provider registration in logging #20215

Closed

Log unhandled exceptions to custom logger #19606

Log unhandled exceptions to custom logger #19606

Uh oh!

Conversation

SteveSandersonMS commented Mar 5, 2020

Uh oh!

SteveSandersonMS Mar 5, 2020 • 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!

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!

javiercn left a comment

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!

SteveSandersonMS commented Mar 10, 2020

Uh oh!

javiercn left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

SteveSandersonMS Mar 5, 2020 •

edited

Loading