planetarium / libplanet

Blockchain in C#/.NET for on-chain, decentralized gaming
https://docs.libplanet.io/
GNU Lesser General Public License v2.1
510 stars 144 forks source link

Reduce log clutter by getting rid of duplicate `Exception.ToString()`s #1632

Open greymistcube opened 2 years ago

greymistcube commented 2 years ago

Default output format via Serilog is to have {exception} at the end of log messages. This is to automatically output a message from a thrown Exepction when using Error(). However, currently, many, if not most, of Serilog's Error() in the code base explicitly include Exception.Message in message parameter. The result is unnecessary cluttering of the log output with duplicated Exception.Message.

dahlia commented 2 years ago

Does Serilog's default behavior maintain stack traces as well? If so, we'd better not to pass exceptions by hand.

greymistcube commented 2 years ago

As far as I know, it does. In any case,

Log.Error(e, "Some exception happened");

Automatically outputs the exception via the default setting of outputTemplate in appsettings.json as seen in the format

[{Timestamp:HH:mm:ss} {Level:u3}] {Message:lj}{NewLine}{Exception}

where exception's e.Message is printed.

What

Log.Error(e, "Some exception happened {E}", e);

does is that not only does it duplicate the printing of e.Message for {E} and {Exception} on console, but also duplicates the property in json with E property and Exception property.

greymistcube commented 2 years ago

I haven't tested for unit tests since it might not be using the provided template, since appsettings.json is in headless, but in that case, in my opinion, the right way to resolve the issue is configure the logger separately for unit testing instead of abusing the library by using an unnecessary extra property.

greymistcube commented 2 years ago

Update/Errata: It's actually Exception.ToString() which contains the stack trace that gets printed not Exception.Message.

stale[bot] commented 2 years ago

This issue has been automatically marked as stale because it has not had recent activity. Thank you for your contributions.

CodingPythonMan commented 2 years ago

stale[bot] commented 1 year ago

This issue has been automatically marked as stale because it has not had recent activity. Thank you for your contributions.

stale[bot] commented 1 year ago

This issue has been automatically marked as stale because it has not had recent activity. Thank you for your contributions.