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

Add Overloads That Support LogMessageGenerator with Exception #2014

Closed
TheXenocide opened this issue Mar 15, 2017 · 8 comments
Closed

Add Overloads That Support LogMessageGenerator with Exception #2014

TheXenocide opened this issue Mar 15, 2017 · 8 comments

Comments

@TheXenocide
Copy link

Type: Feature request

NLog version: 4.4.2

Platform: .Net 4.5

While working on our internal logging facade I noticed NLog doesn't support deferred message generation on method overloads that also support Exceptions. In many common scenarios I can see how Exceptions would be less likely to be filtered, but given support for logging exceptions as Verbose or Information it seems like it would still be relevant to allow message generation to happen after filtering rules are applied the same as any other message.

@snakefoot
Copy link
Contributor

snakefoot commented Mar 15, 2017

One reason could be that logging exceptions is a rare thing, and when they are logged then it is expected to that they reach a log-target. So it doesn't make much sense to have extra logic for deferred rendering, since it is expected to always be executed.

But the change is just a Pull-Request (PR) away.

@TheXenocide
Copy link
Author

I figured that would be the rationale, but there are known exceptions that can be handled but logged for diagnostic purposes as verbose/debug/etc. which are much less likely to be automatically redirected to the Event Log than warning/error/fatal. Our project is on a very tight timeline at the moment, so for now we're just invoking our delegate and passing the result along, but it would be nice in the long run so maybe I'll see if I'm able to come back to it later. Still, thought I'd bring it up while passing through. Thanks for everything you guys do :)

@304NotModified
Copy link
Member

304NotModified commented Mar 15, 2017

Well forgot it, and I think it should be there for consistency :)

Thanks for reporting! PR would be nice ;)

@c0shea
Copy link
Contributor

c0shea commented Apr 4, 2017

Submitted PR #2051 for this feature

@304NotModified 304NotModified added this to the 4.4.6 milestone Apr 9, 2017
@TheXenocide
Copy link
Author

Awesome, thanks guys! Out of curiosity, any idea when 4.4.6 will be hitting NuGet?

@304NotModified
Copy link
Member

304NotModified commented Apr 10, 2017

fixed by #2051. Thanks @c0shea !

@304NotModified
Copy link
Member

304NotModified commented Apr 10, 2017

This week :)

If you need it earlier, I can push a beta

update: 4.4.6-beta3 will be there soon. (it's now in the AppVeyor queue)

@TheXenocide
Copy link
Author

We're really close to the end of our project so I think we'll wait for the stable build just to reduce likelihood of issues, but that's great, thanks a bunch!

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

No branches or pull requests

4 participants