-
Notifications
You must be signed in to change notification settings - Fork 4.9k
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
Microsoft.Extensions.Logging.Console: should log Exception.Data #35922
Comments
something like https://gist.github.com/nblumhardt/dddaa2139bbf4b561fa7 |
This seems reasonable. Care to submit a PR. |
/cc @maryamariyan |
Hi, current implementation is:
but, a few aspects must be considered exception.Tostring() output has multiple "sections" separated by newlines:
our trace can only be appended to the result of exception.ToString() basically:
"best" solution: but touching System.Exception is a different story ;-) My current playground |
@WernerMairl thank you for the prototype. A couple notes
One question that arrises here is that, do we want to have an option for the log formatter that toggles between logging Exception.Data or do we expect that to always be showing up for exceptions containing this information. If the answer is we do not need the toggling behaviour then there is no need for you to make the API addition. @WernerMairl, while your at it you could also perhaps make use of the gist in this comment to play around with custom log formatting, not only the built-in ones if you like. |
@maryamariyan a few ideas and answers:
Why modifying exception.cs ?
I need some feedback about going forward this way - specially adopting System.Exception in this or another way.... |
How is JsonConsoleFormatter the only one that is ignoring InnerException? |
SystemDConsoleFormatter and SimpleConsoleFormatter are calling exception.ToString() and the "ToString()" default implementation is calling innerException.ToString(). The JSonConsoleFormatter (in my understanding) is NOT calling exception.ToString() but it prints out all the exception properties one by one IGNORING innerexception..... runtime/src/libraries/Microsoft.Extensions.Logging.Console/src/JsonConsoleFormatter.cs Line 55 in 51ad8e4
This is the result from a quick investigation on the JSonConsoleFormatter. I can write a test about this "innerException" thing to provide evidence if necessary.... |
The documentation of Exception.Data says that each key is "typically an identifying string". If a key is not a string, I think key.ToString() will be good enough for logging. From https://source.dot.net/ and https://referencesource.microsoft.com/, it looks like keys are almost always constant strings, except:
Exception.InnerException in JsonConsoleFormatter is #40507. (It might also be useful to translate properties like SqlException.Number to JSON properties, but doing that in JsonConsoleFormatter would require API changes to let the application control which properties of each exception type are interesting and how their values should be formatted. For instance, ReflectionTypeLoadException.Types is public but not interesting.) |
Good point. Seems like a proper thing to do is for JsonConsoleFormatter to also call exception.ToString() as also described in this comment: #40507 (comment) |
It is possible for |
Maybe we could create a new properties on the Exception making it explicit that it's will be logged, so with this suggestion no breaking change is inserted for previous code that used the data field and doesn't expect it to be logged. |
Is your feature request related to a problem? Please describe.
I add some data to my exception and i didn't see them in the Console.
but when i go into the console, the output doesn't display the data.
i was expecting that the console log provider would display someting like
Describe the solution you'd like
Add the Data field in the log.
The text was updated successfully, but these errors were encountered: