[Yanel-dev] exceptions, logging and related best practices (was: Re: [Yanel-commits] rev 48212 - public/yanel/trunk/src/contributions/resources/policymanager/src/java/org/wyona/yanel/impl/resources/policymanager)

[Guillaume Déflache]

Michael Wechner schrieb:
> Dear Guillaume
> 
> guillaume at wyona.com wrote:
>>              return sb.toString();
>>          } catch(Exception e) {
>> -            log.error(e, e);
>> +            log.error(e.getMessage(), e);
>>   
> 
> Why are you changing these?

Because for some reason I believed that toString() called 
getLocalizedMessage() by default (that would have produced French 
messages if your JVM locale was set to French, which believe me, are 
almost uncomprehensible even to French people compared to the English 
ones!).

But after rereading the Javadoc for Throwable it seems I was completely 
mistaken, so I won't do that anymore from now on!

Thanks for noticing it!


> I am afraid this will lead to a commit war, because I will change it 
> back shortly ;-)

Indeed toString() seems to provide additional details and to call 
getMessage() by default: 
http://java.sun.com/javase/6/docs/api/java/lang/Throwable.html#toString%28%29
And it's also nicely shorter and more readable if a little odd-looking 
at 1st glance.


> So, let's discuss it first and write down some rules how we want to 
> handle it.

I'll start then (notes: T extends Throwable, what is applicable to 
org.apache.log4j.Logger#error(Object, Throwable) calls is also 
applicable to other logging methods with similar signatures):


0) vital and always applicable: do not lose stacktraces

0.0) for ignorable exceptions never do:
} catch (Exception e) {
     // empty catch block
}
...but instead do at least the following:
} catch (Exception e) {
     if (logger.isDebugEnabled()) logger.debug("swallowed exception "+e, e);
}
...because it won't hurt performance much, whereas throwing a lot of 
exceptions and not knowing about it will!

0.1) never do logger.error(throwable) but do logger.error(throwable, 
throwable) because else the original stacktrace gets lost ...but not 
logger.error(throwable.getMessage(), throwable) as explained above


1) very important and almost always applicable: do not "over-log" exceptions

1.0) do not log _and_ throw the same exception: do not do:
} catch (Exception e) {
     log.error(e, e);
     throw new RuntimeException(e, e);
}
...but do:
} catch (Exception e) {
     throw new RuntimeException(e, e);
}
...or maybe:
} catch (Exception e) {
     log.error(e, e); // for the administrator and programmer
 
eventuallyUserFacingErrorReportMechanism.addError(friendlyErrorMessage/*, 
e*/); // for the user (and optionally priviledged users may be allowed 
to see the exception and related stacktrace)
}
...except at the highest level when you are sure no one will log the 
exception further up, e.g.:
- in the 'main' method
- in a servlet in the highest-level protected methods inherited from 
HttpServlet, e.g. 'service', 'destroy', etc.
- in the 'run' method of a Thread
...else your stacktraces will get so big they will get truncated!

1.1) avoid rewrapping exceptions that do not need to be rewrapped 
because they are already of the needed type: e.g. do not do:
} catch (Exception e) {
     throw new RuntimeException(e);
}
...but rather do:
} catch (RuntimeException e) {
    throw e;
} catch (Exception e) {
     throw new RuntimeException(e);
}
... else your stacktraces will get bigger (see above)


That's all I have in mind for now.
HTH,
    Guillaume