8000 Log exception set in IDiagnosticContext by angularsen · Pull Request #271 · serilog/serilog-aspnetcore · GitHub
[go: up one dir, main page]
Skip to content

Log exception set in IDiagnosticContext #271

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 2 commits into from
Mar 4, 2022
Merged

Log exception set in IDiagnosticContext #271

merged 2 commits into from
Mar 4, 2022

Conversation

angularsen
Copy link
Contributor
@angularsen angularsen commented Nov 4, 2021 •
edited
Loading

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

This was referenced Nov 4, 2021
Merged
Closed
@angularsen
Copy link
Contributor Author
angularsen commented Nov 4, 2021 •
edited
Loading

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 🚀

@angularsen
Log exception set in IDiagnosticContext
33abd28 
If there is no unhandled exception, then log the exception set in IDiagnosticContext.
@angularsen
Upgrade to Serilog.Extensions.Hosting 4.2.1-dev-00092
b8081d8
@angularsen angularsen force-pushed the agl/diagnostic-exception branch from 1fdff60 to b8081d8 Compare March 3, 2022 12:16
@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
@nblumhardt
Bump major version; breaking changes in #271
532 8000 fd1b
@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
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Provide exception in IDiagnosticContext
2 participants
@angularsen @nblumhardt
0