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

Change default layout to include exceptions #1292

Closed
sliekens opened this issue Mar 5, 2016 · 14 comments · Fixed by #3637
Closed

Change default layout to include exceptions #1292

sliekens opened this issue Mar 5, 2016 · 14 comments · Fixed by #3637
Labels
breaking change Breaking API change (different to semantic change) discussion
Milestone

Comments

@sliekens
Copy link

sliekens commented Mar 5, 2016

The default layout should include a renderer for exceptions.

${longdate}|${level:uppercase=true}|${logger}|${message}|${exception:format=ToString}

Relates to #1290 and #1291

@304NotModified
Copy link
Member

Thanks for reporting this!

I agree the writing the exception on default is a good suggestion. But I'm doubting between ${exception:format=ToString} and ${exception} - so writing the inner exception or not. The innerException can be pretty long.

@sliekens
Copy link
Author

sliekens commented Mar 5, 2016

I believe that ${exception} is the same as ${exception:format=message}. I don't think that's very useful. There should at least be a stack trace of the outermost exception + the type name of the exception.

@sliekens
Copy link
Author

sliekens commented Mar 5, 2016

After reading #1034, it seems that this is a better default

${longdate}|${level:uppercase=true}|${logger}|${message:withException=true}

@304NotModified 304NotModified added discussion breaking change Breaking API change (different to semantic change) labels Mar 5, 2016
@304NotModified 304NotModified added this to the 5.0 (new) milestone Jan 10, 2018
@304NotModified
Copy link
Member

note:

  • this is also in the xml docs (lot of places)
  • we need maybe a exception seperator, too avoid newlines

suggested: (by @snakefoot - see #2503 (comment))

 ${longdate}|${level:uppercase=true}|${logger}|${message:withException=true:exceptionSeparator=|}

@304NotModified
Copy link
Member

304NotModified commented May 3, 2019

@snakefoot I see it was confused in #2503 with ExceptionLayoutRenderer.Separator

I think #1292 (comment) is basically the same and more clear IMO

@snakefoot
Copy link
Contributor

I think #1292 (comment) is basically the same and more clear IMO

But it always inserts | after the ${message}, which is not the case with my suggestion

@304NotModified
Copy link
Member

Ow yes, you're right.

Maybe we could add another renderer so it's clearer to communicate? Or do you think #1292 (comment) is easy to understand? (i'm afraid we have to explain it every time)

@snakefoot
Copy link
Contributor

My personal opinion is that this is much easier to the eye:

${longdate}|${level:uppercase=true}|${logger}|${message}|${exception:format=ToString}

And is actually the Layout that I use normally use. But if NLog 5.0 is meant as minimal breaking changes, then it probably not that good.

@304NotModified
Copy link
Member

FYI We need to fix in NLog 5 that the exception isn't logged by default. That's an issue for ages. It's breaking and so this is the right moment

@304NotModified
Copy link
Member

304NotModified commented May 3, 2019

If there aren't exceptions, then your suggestion is more backwards compatible (from #1292 (comment))

That's a +1

@snakefoot
Copy link
Contributor

One could also consider changing exceptionSeparator to be | by default (So it matches the default-layout and it would be a very little breaking change as it will only have effect when withException=true). Then it will be like this:

${longdate}|${level:uppercase=true}|${logger}|${message:withException=true}

Other suggestion could be changing ${exception} to have format=ToString by default. Since most people expects ${exception} to include everything, and not just the exception-message. But it would be a bigger breaking change.

@304NotModified
Copy link
Member

One could also consider changing exceptionSeparator to be | by default

Sounds good to me!

Other suggestion could be changing ${exception} to have format=ToString by default

Also interesting. Imo it's indeed a better default. I think it's important to have good defaults.

@snakefoot
Copy link
Contributor

@304NotModified Has this been resolved with #3458 ? (So part of NLog 5.0)

@304NotModified
Copy link
Member

Not yet. This is about the default for Layout

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking change Breaking API change (different to semantic change) discussion
Projects
None yet
3 participants