Make exception logging in `RequestLoggingMiddleware` optional

[cremor]

Is your feature request related to a problem? Please describe. If my application uses UseSerilogRequestLogging all unhandled exceptions are logged twice. This causes logs to be more long/complicated than they need to be.

Describe the solution you'd like RequestLoggingOptions should have an option to not log the exception.

Describe alternatives you've considered Maybe exception logging should even be disabled by default since I couldn't find any case where the exception isn't logged by something else.

Additional context There are multiple other classes which already log the exception, depending on your middleware configuration:

• If you use UseDeveloperExceptionPage (or are running in Development hosting environment which enables this by default) then the exception is logged by Microsoft.AspNetCore.Diagnostics.DeveloperExceptionPageMiddleware.
• If you use UseExceptionHandler then the exception is logged by Microsoft.AspNetCore.Diagnostics.ExceptionHandlerMiddleware.
• If you use neither then the exception is logged by Microsoft.AspNetCore.Server.Kestrel.

[sungam3r]

It looks like you can disable particular log sources by Serilog's level overrides.

[cremor]

I don't want to disable the log. I still want to see the log line HTTP GET /api/TodoItems responded 500 in 8.9821 ms. I just don't want to see the exception details with that log line.

[nblumhardt]

I think this is doable, but we'd have to be careful about how we handle any exceptions captured by IDiagnosticContext (collector, in the middleware code), since those won't propagate and would still need to be logged by the middleware regardless of the optional "include exceptions" setting.

This pushes the setting name/semantics towards IncludeEscapingExceptions or something of that kind - meaning, turning the option off would not necessarily suppress all exception information in the completion events.

🤔

[cremor]

Where/how is this IDiagnosticContext used? Or rather, who calls its SetException method?

[pcmcoomans]

Have there any updates regarding this issue?

[nblumhardt]

@cremor users can call IDiagnosticContext.SetException() from anywhere in an executing web request to set the Exception included in the final log event. I think we'd still need to record these, because they'll never reach any other exception handler.

Given the possible additional complications around logging cancellation exceptions, how about an enum for the option, which gives us some flexibility for future extensions?

The final values for the IncludeEscapingExceptions setting then might be along the lines of:

• None - don't log any exceptions ever
• ExplicitOnly - only log exceptions received from IDiagnosticScope
• IgnoreCanceled - log everything except cancellation exceptions (Task and Operation?)
• All - the default, current behavior

Initially we could consider implementing only the first and last, or some selection of those.

[cremor]

I think this is a good idea. Some details:

• About None and ExplicitOnly: That would still log the line "HTTP GET /api/test responded 500 in 55.9105 ms", right? So only the exception part is suppressed?
• About "Task and Operation?": TaskCanceledException is derived from OperationCanceledException.
• About IgnoreCanceled: DeveloperExceptionPageMiddlewareImpl and ExceptionHandlerMiddlewareImpl implement a bit more logic than just checking the exception type to detect cancelled requests. Maybe RequestLoggingMiddleware should do the same.
• I'd set ExplicitOnly as the new default. If you don't want that, maybe at least set IgnoreCanceled? See #372 for my reasons.

[nblumhardt]

Thanks for the feedback, sorry about the long delay, time has been a bit short :)

About None and ExplicitOnly: That would still log the line "HTTP GET /api/test responded 500 in 55.9105 ms", right? So only the exception part is suppressed?

Yes, that's correct

About "Task and Operation?": TaskCanceledException is derived from OperationCanceledException.

Ah great, no need to worry about the task case explicitly, then.

About IgnoreCanceled: DeveloperExceptionPageMiddlewareImpl and ExceptionHandlerMiddlewareImpl implement a bit more logic than just checking the exception type to detect cancelled requests. Maybe RequestLoggingMiddleware should do the same.

Following the lead of those components sounds good.

I'd set ExplicitOnly as the new default. If you don't want that, maybe at least set IgnoreCanceled? See https://github.com/serilog/serilog-aspnetcore/issues/372 for my reasons.

Because the middleware has a different default today, and is very widely used, I don't think we'd default to ExplicitOnly, but IgnoreCanceled sounds like a useful improvement on today's behavior and should probably be the new default 👍

[silkfire]

Hope this can be implemented soon, would be very useful.