Skip to content
This repository has been archived by the owner on Oct 2, 2024. It is now read-only.

Ensure exception gets logged when CustomErrors mode is On #13

Merged
merged 2 commits into from
Dec 7, 2019
Merged

Ensure exception gets logged when CustomErrors mode is On #13

merged 2 commits into from
Dec 7, 2019

Conversation

AroglDarthu
Copy link

Related to serilog-web/classic#48

500 errors get logged nicely, but the exception is missing when CustomErrors mode is On.
By using a HandleErrorAttribute the exception can be preserved with a simple call to AddSerilogWebError(exception).

@AroglDarthu
Copy link
Author

@tsimbalar I also bumped the versions of the referenced Microsoft and Serilog packages

Copy link
Member

@tsimbalar tsimbalar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks a lot for the PR !

Overall, I think the idea is good, but in order to have the minimum impact on consuming apps, I would :

  • use IExceptionFilter instead of HandleErrorAttribute (unless I'm missing something obvious)
  • only bump the version of SerilogWeb.Classic to v5.0.52, but leave all other dependencies unchanged.

What do you think ?

@AroglDarthu
Copy link
Author

Now using IExceptionFilter and reverted all NuGet package updates except SerilogWeb.Classic.

@tsimbalar
Copy link
Member

Great, thanks !

This will probably need a bit of testing just to be sure we don't screw it up for some users, as there are many combinations of configuration, with Custom Errors enabled / disabled , as listed in serilog-web/classic#29 ... but I don't think we would break any app with those changes, so LGTM 🚀 !!

Thanks !

@tsimbalar tsimbalar merged commit a848369 into serilog-web:master Dec 7, 2019
@tsimbalar
Copy link
Member

This should be available soon :)

https://www.nuget.org/packages/SerilogWeb.Classic.Mvc/2.1.25

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants