Why does squid:S1166 not accept exception messages only when logging caught exceptions?

Question

Quote from the description of the rule (SonarQube 4.5.5):

// Noncompliant - exception is lost (only message is preserved)   
try { /* ... */ } 
catch (Exception e) { LOGGER.info(e.getMessage()); }

By providing the exception class to the logger a stack trace is written to the logs.

The problem in our code base is this: By following the Tell, don't ask principle, we use checked exceptions as part of the, what we consider, normal execution paths and we don't want them to result in unreasonably large log messages.

A few examples: Servers responding with error codes, database statement executions failing on optimistic locking (concurrent users)...

My suggestion: Split this case in two.

// Noncompliant - exception is lost (only message is preserved)
try { /* ... */ } 
catch (Exception e) { LOGGER.info(e.getMessage()); } 

and

// Compliant - exception is lost (only message is preserved) but there is business logic handling the situation      

try { 
/* ... */  
} catch (Exception e) {   
   LOGGER.info(e.getMessage());  
   */ exception handling  */  
}

The rule squid:S00108 (code blocks must not be empty) would not catch the problem since there is a logging statement.

Is this not reasonable? Have I missed something of importance?

Note: I've rewritten the question to clarify my use case

Batish

Answer 1

I understand the arguments for maintaining the stack trace and all that, but I think it's going to bloat your logs for a < ERROR level event. One solution is to log the message as a WARN and log the exception object as DEBUG or TRACE. That way a normal user log config would not be flooded with business as usual stack traces, but it would still be possible to get a stack trace if necessary.

Fortunia

Answer 2

If it's causing hundreds of what you consider to be FP's then you should think about turning the rule off, or excluding it from your project files.

But to answer your question:

The point of exception logging is to leave enough information for investigators to figure out the cause of a problem.

If your messages are detailed, e.g.

The x in the y method broke because the frabjous was not day enough

then perhaps they fulfill that purpose. But what about a message like

Something went wrong

?

Further, you know exactly what each exception message means, but someday you'll presumably move on to bigger and better things. Will the next guy who supports the system have the same depth of knowledge? He may be grateful for the stacktraces and line numbers that tell him where to start looking...

But finally, I have to ask: why are you getting and logging so many exceptions that you flood the logger?

Definitely

I find the rule valid (for most cases) and want to keep it, otherwise I would not bother writing a post about it. The application in question is fairly big (~ 200 000 lines of code) and logs a lot of useful information about user sessions but when a user does something that would cause a checked exception (that is handled gracefully by the application) the logger is unreasonably flooded. Sure, I could log everything at a lower log level to avoid the rule violation but that would force me into all these rewrites just for the sake of a rule that I don't agree with. – Batish

Are we talking about specific exception types that perhaps the rule ought to ignore, @Batish ? – Definitely

I don't think that defining such a list would help as custom exception classes are common and sometimes come from frameworks, thus not standard library. – Batish

Note: Hundreds is an exaggeration but fairly close :-) One example of exception would be concurrency exceptions – Batish

A comment from a collegue on this issue: "If you always have Exceptions as something that is truly exceptional and unexpected then I would agree to the rule but I guess that most applications also have Exceptions as an unusual flow of events, but not unexpected, that you catch and handle appropriately. To me a compliant solution should be to have a catch block with some logic how to resolve the situation and to log the exception message, not the complete exception with its stack trace. " – Batish

The rule already ignores Interrupted, NumberFormat, Parse and MalformedURL exceptions. I was asking if there were standard exception types that, like NumberFormat, should reasonably be ignored. – Definitely

@Ann exceptions that are unexpected (even if they are checked exceptions, such as UnsupportedCharsetException) should be logged with a stacktrace, I'm fine with that. But given the explanations above, do you understand why this is a problem for us? By following the "tell dont ask" principle we end up with expected checked exceptions that we don't want to give much attention to as we consider them part of the normal flows – Batish

My original question may have been confusing so I took the liberty of rewriting it. – Batish

Answer 3

(Adding another answer to address the question as rewritten:)

Why would you both handle the exception and log it? If it's handled, there's no reason to log.

Definitely

We don't only log problems but also user flows (using the INFO log level), to be able to see what happened before a problem occurred. In this case we would probably log whatever happened with the WARN log level to mark that there was a problem but handled by the application (ERROR level to indicate something unexpected that could not be handled) – Batish

Answer 4

try to pass whole object to method than just a e.getMessage()LOGGER.info("INFO "e.);

Nevernever