2
\$\begingroup\$

We have a few servlets like the one below, taking JSON request and produce JSON response. And the requirement is to log all API call traffic.

import org.slf4j.*;
import javax.servlet.http.HttpServletRequest;
import javax.servlet.http.HttpServletResponse;
import java.io.IOException;
public class CommentServlet {
  private static final Logger LOG = LoggerFactory.getLogger(CommentServlet.class);
  public void postComment(HttpServletRequest request, HttpServletResponse response) throws IOException {
    String commentRequested = request.getParameter("comment");
    LOG.info(commentRequested);
    Object postResultModel = processComment(commentRequested);
    JsonUtil.writeResponse(response, postResultModel);
  }
  /**
  * doesn't reallty matter what processComment does
  */
  public Object processComment(String comment) {
    return new Object();
  }
}
import org.slf4j.*;
import javax.servlet.http.HttpServletRequest;
import javax.servlet.http.HttpServletResponse;
import com.fasterxml.jackson.databind.ObjectMapper;
import java.io.IOException;
public class JsonUtil {
    private static final Logger LOG = LoggerFactory.getLogger(JsonUtil.class);
    private static final ObjectMapper OBJECT_MAPPER = new ObjectMapper();
    public static void writeResponse(HttpServletResponse response, Object modelObject) throws IOException {
        String modelJson = OBJECT_MAPPER.writeValueAsString(modelObject);
        LOG.info(modelJson);
        response.setContentType("application/json");
        response.getWriter().write(modelJson);
    }
}

if you can mainly review how the logging is done, I'd like feedback on the general approach as much as whether there's any way to improve it.

\$\endgroup\$
2
  • 2
    \$\begingroup\$ Questions containing hand-wavy code and asking us to speculate or extrapolate do tend to get downvoted. Rev 8 does look better. \$\endgroup\$ Commented Nov 9, 2016 at 21:52
  • 1
    \$\begingroup\$ You should add servlet filters in your application to log all api calls.You can create servlet filter by implementing javax.servlet.Filter and add it to web.xml. Read this article to get more information journaldev.com/1933/java-servlet-filter-example-tutorial \$\endgroup\$
    – akm.rocks
    Commented Aug 8, 2017 at 17:17

2 Answers 2

1
\$\begingroup\$
private static final Logger LOG = LoggerFactory.getLogger(CommentServlet.class);

This is considered as bad practice ... of course there is an argument from the other side as well.

The right thing to use should be

private final Logger LOG = LoggerFactory.getLogger(getClass()); 

This basically helps you leverage the capabilities of polymorphism and use the same logger in inherited classes with its real name.

\$\endgroup\$
4
  • \$\begingroup\$ Welcome to Code Review, your first answer looks good, but perhaps you could clarify the downside of the proposed fix, if there's one as mentioned in the first sentence. Perhaps also a link if there's already some more documentation on this. Enjoy your stay! \$\endgroup\$
    – ferada
    Commented Nov 10, 2016 at 13:23
  • \$\begingroup\$ No, not really bad practice. The Log4jFactory has a synchronized(this) block to look up existing/creating/adding loggers to its internal cache. Also, the non-static variant will be serialized, so it should actually be declared transient. More info: wiki.apache.org/commons/Logging/StaticLog. The main issue for the static variant is when it's running in an application server, which provides its own logging libraries. \$\endgroup\$
    – slowy
    Commented May 10, 2017 at 10:08
  • \$\begingroup\$ It's bad practice because the logger gets instantiated on class load, whether you need it or not, and can never be garbage collected. Nothing to do with synchronization. As the OP says it depends on your use model, but generally folks don't encourage static loggers. transient is a good point if the class is serializable, which the OP isn't. \$\endgroup\$
    – markspace
    Commented Dec 6, 2017 at 19:31
  • \$\begingroup\$ Now a days Project Lombok has a @Logger annotation that can be very well used to do all these things. \$\endgroup\$ Commented Dec 8, 2017 at 6:16
0
\$\begingroup\$

IMHO, the problem is that all the responses are using the same JsonUtil logger, and each request is using the logger of each Servlet. This is making it difficult to configure meaningful appender for logs. (imagine if you want to configure "comment" feature as mentioned above to have certain log level and write to a comment.log file).

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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