1
\$\begingroup\$

I have written a filter that intercepts http calls and logs request parameters.

public void doFilter(ServletRequest request, ServletResponse response,
        FilterChain filterChain) throws IOException,
        ServletException
    {
        HttpServletRequest httpRequest = (HttpServletRequest) request;
        HttpServletResponse httpResponse = (HttpServletResponse) response;
        String uri = httpRequest.getRequestURI();
        String queryString = httpRequest.getQueryString();
        long startTime = System.currentTimeMillis();
        if (shouldBeRedirected(uri)) {
            String type = getType(uri);
            String redirectUrl = Util.getURLBasedUponStabilityLevel(Constants.STABILITY_LEVEL);
            ((HttpServletResponse) response).sendRedirect(redirectUrl + type);
            return;
        }
        filterChain.doFilter(httpRequest, httpResponse);
        long endTime = System.currentTimeMillis();
        Timestamp startTimestamp = Timestamp.getInstance(startTime);

        StringBuilder logString = new StringBuilder();
        logString.append("Request URL: ");
        logString.append(uri);
        logString.append(" Query String: ");
        logString.append(queryString);
        logString.append(" Session Id: ");
        logString.append(httpRequest.getRequestedSessionId());
        logString.append(" Took: ");
        logString.append(endTime - startTime);
        logString.append("milliseconds. ");
        logString.append(" StartTime: ");
        logString.append(startTimestamp);
        if(httpRequest.getMethod().equals("POST")){
            JSONObject postParameters = new JSONObject();
            Map parameterMap = httpRequest.getParameterMap();
            Iterator<Map.Entry<String, String[]>> it = parameterMap.entrySet().iterator();
            while (it.hasNext()) {
               Map.Entry<String, String[]> entry = it.next();
               if(entry.getValue().length == 1){
                   postParameters.put(entry.getKey(), entry.getValue()[0].toString());}
               else{
                  JSONArray valueList = new JSONArray();
                  for(String value : entry.getValue()){
                      valueList.add(value.toString());
                  }
                  postParameters.put(entry.getKey(), valueList);
               }
            }
            logString.append(" Post Parameters: ");
            logString.append(postParameters.toString());
        }
        LOG.info(logString.toString());
    }

Code looks a bit clumsy to me, any suggestions to improve this?

\$\endgroup\$
2
2
\$\begingroup\$

The redirect doesn't seem to be part of the logging. Maybe put it in a separate filter that runs before this one?

The logged string could be built using String.format() or MessageFormat which would be more readable. Most logging frameworks have such formatting built into their logging methods anyway.

Do you really need to convert the post parameters into JSON? Isn't the standard toString representation of the Map good enough for logging? Apart from that most JSON libraries have a built-in mechanism to convert a Map into a JSON object so you don't need to do it yourself.

\$\endgroup\$
2
\$\begingroup\$

Elaborating on RoToRa's comments:

  1. If you are going to build a string by concatenation, then using a string builder like that only improves performance if there is a loop. That doesn't apply here. So your code could be written as:

    String logString = "Request URL: ") + uri + " Query String: " + queryString
            + " Session Id: " + httpRequest.getRequestedSessionId() + " Took: "
            + (endTime - startTime) + " milliseconds. " + " StartTime: "
            + startTimestamp 
            + (httpRequest.getMethod().equals("POST") ? 
                  httpRequest.getParameterMap() : "");
    LOG.info(logString);
    
  2. You should be using a format String and parameters; e.g.

    LOG.info("Request URL: {} Query String: {} Session Id: {} Took: {} " +
             "milliseconds. StartTime: {} {}",
             uri, queryString, httpRequest.getRequestedSessionId(), 
             (endTime - startTime), startTimestamp,
             (httpRequest.getMethod().equals("POST") ?
                  httpRequest.getParameterMap() : ""));
    

    This is not just neater. It is also a lot more efficient, since the work of interpolating the parameters into the format will only happen if "info" level logging is enabled.

    (Just using String.format is not the solution ...)

\$\endgroup\$
4
  • \$\begingroup\$ As per my understanding "Request URL: ") + uri will create a string object say str1 then str1 + " Query String: " will create another string object ..... and hence we will end up creating lots of throwaway strings. Isn't this suboptimal? \$\endgroup\$ – Lokesh Agrawal Aug 8 '19 at 18:19
  • \$\begingroup\$ Also, I am confused after your answer (#1 and #2 above can't be done at the same time right?). If I am creating logString then why will I use format string? can you please provide the complete snippet with optimizations you feel are correct? \$\endgroup\$ – Lokesh Agrawal Aug 8 '19 at 18:22
  • \$\begingroup\$ Your understanding is incorrect. The Java compiler optimizes a simple sequence of concatenations to use a string builder. You don't need to do it yourself. You only need to resort to using StringBuilder explicitly in cases involving loops that the compiler doen't optimize. \$\endgroup\$ – Stephen C Aug 8 '19 at 23:24
  • \$\begingroup\$ You are correct that you do #1 or #2, not both. The code snippets in my answer are more or less complete replacements for your code. \$\endgroup\$ – Stephen C Aug 8 '19 at 23:26

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.