18

Our webapplication is using an ExceptionMapper to map some exceptions to Response. We log the exception messages before throwing a new exception as follows:

catch (SomeException ex) {
  LOG.error(ex.getMessage());
  throw new MyException(ex.getMessage());
}

We are not re-throwing the same exception, so my question is if this would be considered a Log and Throw antipattern. And thus, would it be better to remove the logging in similar places and move them to the several ExceptionMapper classes as follows:

@Provider
public class MyExceptionMapper implements ExceptionMapper<MyException> {

  // bla bla 

  @Override
  public Response toResponse(final MyException ex) {
    LOG.error(ex.getMessage());
    return Response.status(400).entity("something").build();
  }
}

migrated from stackoverflow.com Sep 3 '15 at 18:06

This question came from our site for professional and enthusiast programmers.

  • 3
    I would stop you as soon as you logged ex.getMessage(), that's already wrong. – biziclop Sep 2 '15 at 14:35
  • 1
    possible duplicate of Should you report the message text of exceptions? – Raedwald Sep 2 '15 at 14:40
  • Arguable, reporting the message text of exceptions is always a bad idea. – Raedwald Sep 2 '15 at 14:40
  • IMO it is not really fair to not mention at all that this is a webservice. That really does change the game rules, because for example just using regular exception handling mechanics can be a security risk; you do not want any and all exception to just be sent back in a 500 error response, that needs to be checked and filtered. Aggressive on-the-spot logging is also far more common when dealing with systems that have external clients directly invoking it. Logging stacktraces in service calls that get invoked repeatedly can lead to unmanageable log file sizes and performance problems. – Gimby Sep 2 '15 at 15:58
  • @Gimby In this case OP doesn't send any error details in the response (probably a boilerplate "error occurred" content). Also, how do you diagnose a problem without a stacktrace? Rolling loggers routinely take care of storage size and log data is embarassingly compressible. – Marko Topolnik Sep 2 '15 at 17:25
34

Your code actually sports not one, but three antipatterns:

  1. log and rethrow;
  2. rethrow without wrapping the original cause;
  3. log only the message and not the stacktrace (this is the worst one).

If you followed the best practice to:

  1. not catch at all (let the exception propagate on its own);
  2. if forced to catch a checked exception, wrap into unchecked and rethrow;
  3. never log anything except at the top level;
  4. log the entire exception stacktrace with log.error("Error occurred", e);

then you would not face any dilemmas, including your current one, because the logged stacktrace would include all the wrapped exceptions as well.

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.