Skip to content
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

Log exception set in IDiagnosticContext #271

Merged
merged 2 commits into from Mar 4, 2022

Conversation

angularsen
Copy link
Contributor

@angularsen angularsen commented Nov 4, 2021

Fixes: #270
Depends on: serilog/serilog-extensions-hosting#56

Changes

  • RequestLoggingMiddleware.LogCompletion enriches with the exception set in IDiagnosticContext, unless an unhandled exception takes precedence.

Testing

  • Add tests on enriching with collected exception and favoring unhandled exception
  • Functionally tested and Serilog request log event is correctly enriched with exception set in IDiagnosticContext

chrome_7VOs0WYLjb_masked

@angularsen
Copy link
Contributor Author

angularsen commented Nov 4, 2021

Until serilog/serilog-extensions-hosting#56 is merged and the dependency version of Serilog.Extensions.Hosting is bumped, these tests will fail due to missing overload DiagnosticContextCollector.TryComplete(IEnumerable<LogEventProperty>, Exception).

@angularsen
Copy link
Contributor Author

@nblumhardt Continuing from serilog/serilog-extensions-hosting#56 (comment):

Since the stored exception is last in wins, should any exception caught in the request handling middleware take precedence over the exception stored in the context?

In our scenario, the response pipeline looks like this:

  • MVC Action throws exception A
  • Hellang middleware swallows it, calls IDiagnosticContext.SetException(), sets a problem details response
  • Some other middleware throws exception B
  • Serilog middleware goes wut do? ¯\(ツ)

Going on my gut feeling, I would expect exception B to be logged since it was thrown last in the response pipeline. Also, I believe this requires no additional change to this PR.

What do you think?

I assume a new serilog-extensions-hosting nuget must be released before this PR can pass the tests on the new SetExceptions() method.

@nblumhardt
Copy link
Member

Sounds reasonable to me @angularsen 👍

Serilog.Extensions.Hosting 4.2.1-dev-00092 is now on NuGet and can be referenced from this repo to get the PR building 🚀

If there is no unhandled exception, then log the exception set in IDiagnosticContext.
@angularsen
Copy link
Contributor Author

@nblumhardt Rebased and upgraded to prerelease nuget, tests are now green.

@nblumhardt
Copy link
Member

Looks good! Will be great to try this out in the wild :-)

@nblumhardt nblumhardt merged commit c15ab6d into serilog:dev Mar 4, 2022
nblumhardt added a commit that referenced this pull request Mar 4, 2022
@angularsen angularsen deleted the agl/diagnostic-exception branch March 4, 2022 21:42
@angularsen
Copy link
Contributor Author

Awesome! 🎉 I will give it a spin the coming week and report back.

@angularsen
Copy link
Contributor Author

Works like a charm!
Exception details enriched as normal when logging exceptions, plus the stacktrace at the bottom.

image
image

@nblumhardt nblumhardt mentioned this pull request Mar 21, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Provide exception in IDiagnosticContext
2 participants