Make logger.error(ex);, logger.info(ex);, etc. print stack trace by default

[Niko-O]

It is too easy to write code like this:

try
{
    doThing();
}
catch (ThingFailedException ex)
{
    logger.error("Doing this thing failed.");
    logger.error("Context ID: " + context.id);
    logger.error("SomeSettings: " + someSetting);
    logger.error(ex);
    throw new WhateverException(ex);
}

Especially if you come back to a project after months and you don't remember anything about the log4j API specifics.
You think you you log everything you need, but you don't get the exception's stack trace here. You'd have to write logger.error("", ex); here, which is extremely unintuitive! Or logger.catching(ex), but that's another special thing I'd need to memorize.

I propose to change the default behavior such that the stack trace is printed in this case. I don't care how it's achieved, e.g. by adding a void error(Throwable) overload, or adding a check in the existing void error(Object) overload. The point is that it should be the default behavior because that's what's expected.

[vy]

@rgoers @ppkarwasz WDYT about this proposal? Instead of adding one more method to the Logger, can't we achieve this by branching on message instanceof Throwable in MessageFactory#newMessage(Object) implementations?

[rgoers]

First, I have to ask some obvious questions.

1. Why isn't your first thought to recommend changing this to

logger.error("Doing this thing failed. Context Id: {}, SomeSettings: {}", context.id, someSetting, ex);

2. Assuming for some reason they didn't want the first 3 log lines what would you tell someone who just did

logger.error(ex);

If I saw that I would tell them to add some descriptive text to describe what was being attempted. A stack trace with no other info is a software engineers nightmare.

to be honest, I would very close to doing:

if (obj instanceof Throwable) {
     LOGGER.warn("You need new software engineers");
}

and then ignoring the event.

[ppkarwasz]

Some thoughts:

• This is a recurring question. I can't find the reference now, but multiple times users asked for log(Object) to treat exceptions in a different way.
• There is a Logger.catching method that does exactly what @Niko-O asks.

Given Ralph's remarks, we should probably have an Error Prone rule that issues a warning if a Throwable is passed as a logging message and suggests using catching instead.

[Niko-O]

1. Why isn't your first thought to recommend changing this to

Short answer: Because it's not the same thing.
Longer answer: I have a few situations where I need to log quite a lot of stuff. Reading through all of that in a single line is painful. It's much easier to read if it's split into multiple lines. "This thing failed. Log this data. Log that data. Log the exception. Continue." is so much less mental burden than "This thing failed. Log this data. Log that data. Log the exception, but oh, wait, I need to add a pointless empty string as an argument here or the stack trace won't actually show up. Continue."

2. Assuming for some reason they didn't want the first 3 log lines what would you tell someone who just did
   logger.error(ex);

Assuming someone only wrote this:

try
{
    doThing();
}
catch (ThingFailedException ex)
{
    logger.error(ex);
    throw new WhateverException(ex); // Maybe also this
}

Yes, it's kidna weird that they don't add any additional context, but I guess it works for them ¯\(ツ)/¯. Maybe this is the only place that this specific exception is thrown/caught anyways. Maybe the exception message already contains all the additional context they need. I don't want to dictate how other people debug their own code, I primarily care about my own debugging experience.

LOGGER.warn("You need new software engineers");

That's unreasonable.

[ppkarwasz]

try
{
    doThing();
}
catch (ThingFailedException ex)
{
    logger.error(ex);
    throw new WhateverException(ex); // Maybe also this
}

Try:

try {
    doThing();
} catch (ThingFailedException ex) {
    throw logger.throwing(new WhateverException(ex));
}

Note: this is an anti-pattern (cc/@rgoers), because you don't trust the caller to properly handle the exception, so you log it for auditing purposes. If the caller handles the exception properly, it will appear twice in the logs, but if he ignores the exception or only shows it to the user (that later denies it) you can easily prove you didn't hide the error.

I understand the intention, I see it often and sometimes I use it myself (should I trust the frontend dev ❓ ), but it is not a best practice.