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
    \$\begingroup\$ Questions containing hand-wavy code and asking us to speculate or extrapolate do tend to get downvoted. Rev 8 does look better. \$\endgroup\$ – 200_success Nov 9 '16 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 Aug 8 '17 at 17:17
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\$
  • \$\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 Nov 10 '16 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 May 10 '17 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 Dec 6 '17 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\$ – Anand Vaidya Dec 8 '17 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, privacy policy and cookie policy

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