perform most of top level request related recordings in StatisticsFilter

Author: Vladimir Kotal

opengrok-web/src/main/java/org/opengrok/web/AuthorizationFilter.java

@@ -24,8 +24,6 @@
 package org.opengrok.web;
 
 import java.io.IOException;
-import java.time.Duration;
-import java.time.Instant;
 import java.util.logging.Level;
 import java.util.logging.Logger;
 import javax.servlet.Filter;
@@ -37,9 +35,6 @@
 import javax.servlet.http.HttpServletRequest;
 import javax.servlet.http.HttpServletResponse;
 
-import io.micrometer.core.instrument.DistributionSummary;
-import io.micrometer.core.instrument.Timer;
-import org.opengrok.indexer.Metrics;
 import org.opengrok.indexer.configuration.Project;
 import org.opengrok.indexer.logger.LoggerFactory;
 import org.opengrok.indexer.web.PageConfig;
@@ -50,9 +45,6 @@ public class AuthorizationFilter implements Filter {
 
     private static final Logger LOGGER = LoggerFactory.getLogger(AuthorizationFilter.class);
 
-    private final DistributionSummary requests = Metrics.getRegistry().summary(StatisticsFilter.REQUESTS_METRIC);
-    private final Timer requestsForbidden = Metrics.getRegistry().timer("requests.forbidden");
-
     @Override
     public void init(FilterConfig fc) {
     }
@@ -62,7 +54,7 @@ public void doFilter(ServletRequest sr, ServletResponse sr1, FilterChain fc) thr
         HttpServletRequest httpReq = (HttpServletRequest) sr;
         HttpServletResponse httpRes = (HttpServletResponse) sr1;
 
-        // All RESTful API requests are allowed for now (also see LocalhostFilter).
+        // All RESTful API requests are allowed here for now because they go through LocalhostFilter.
         // The /search endpoint will go through authorization via SearchEngine.search()
         // so does not have to be exempted here.
         if (httpReq.getServletPath().startsWith(RestApp.API_PATH)) {
@@ -79,39 +71,24 @@ public void doFilter(ServletRequest sr, ServletResponse sr1, FilterChain fc) thr
 
         Project p = config.getProject();
         if (p != null && !config.isAllowed(p)) {
-            Instant start = Instant.now();
-            try {
-                if (LOGGER.isLoggable(Level.INFO)) {
-                    if (httpReq.getRemoteUser() != null) {
-                        LOGGER.log(Level.INFO, "Access denied for user ''{0}'' for URI: {1}",
-                                new Object[] {Laundromat.launderLog(httpReq.getRemoteUser()),
-                                        Laundromat.launderLog(httpReq.getRequestURI())});
-                    } else {
-                        LOGGER.log(Level.INFO, "Access denied for URI: {0}",
-                                Laundromat.launderLog(httpReq.getRequestURI()));
-                    }
-                }
-
-                /*
-                 * Add the request to the statistics. This is called just once for a
-                 * single request otherwise the next filter will count the same
-                 * request twice ({@link StatisticsFilter#collectStats}).
-                 *
-                 * In this branch of the if statement the filter processing stopped
-                 * and does not follow to the StatisticsFilter.
-                 */
-                requests.record(1);
-
-                if (!config.getEnv().getIncludeFiles().getForbiddenIncludeFileContent(false).isEmpty()) {
-                    sr.getRequestDispatcher("/eforbidden").forward(sr, sr1);
-                    return;
+            if (LOGGER.isLoggable(Level.INFO)) {
+                if (httpReq.getRemoteUser() != null) {
+                    LOGGER.log(Level.INFO, "Access denied for user ''{0}'' for URI: {1}",
+                            new Object[] {Laundromat.launderLog(httpReq.getRemoteUser()),
+                                    Laundromat.launderLog(httpReq.getRequestURI())});
+                } else {
+                    LOGGER.log(Level.INFO, "Access denied for URI: {0}",
+                            Laundromat.launderLog(httpReq.getRequestURI()));
                 }
+            }
 
-                httpRes.sendError(HttpServletResponse.SC_FORBIDDEN, "Access forbidden");
+            if (!config.getEnv().getIncludeFiles().getForbiddenIncludeFileContent(false).isEmpty()) {
+                sr.getRequestDispatcher("/eforbidden").forward(sr, sr1);
                 return;
-            } finally {
-                requestsForbidden.record(Duration.between(start, Instant.now()));
             }
+
+            httpRes.sendError(HttpServletResponse.SC_FORBIDDEN, "Access forbidden");
+            return;
         }
         fc.doFilter(sr, sr1);
     }

opengrok-web/src/main/java/org/opengrok/web/StatisticsFilter.java

@@ -32,12 +32,12 @@
 import javax.servlet.ServletRequest;
 import javax.servlet.ServletResponse;
 import javax.servlet.http.HttpServletRequest;
+import javax.servlet.http.HttpServletResponse;
 
 import io.micrometer.core.instrument.DistributionSummary;
 import io.micrometer.core.instrument.Timer;
 import org.opengrok.indexer.Metrics;
 import org.opengrok.indexer.web.PageConfig;
-import org.opengrok.indexer.web.Prefix;
 import org.opengrok.indexer.web.SearchHelper;
 
 public class StatisticsFilter implements Filter {
@@ -58,24 +58,24 @@ public void init(FilterConfig fc) throws ServletException {
     }
 
     @Override
-    public void doFilter(ServletRequest sr, ServletResponse sr1, FilterChain fc)
+    public void doFilter(ServletRequest servletRequest, ServletResponse servletResponse, FilterChain fc)
             throws IOException, ServletException {
-        /*
-         * Add the request to the statistics. Be aware of the colliding call in
-         * {@code AuthorizationFilter#doFilter}.
-         */
+
         requests.record(1);
 
-        HttpServletRequest httpReq = (HttpServletRequest) sr;
+        HttpServletRequest httpReq = (HttpServletRequest) servletRequest;
 
         Instant start = Instant.now();
 
         PageConfig config = PageConfig.get(httpReq);
 
-        fc.doFilter(sr, sr1);
+        fc.doFilter(servletRequest, servletResponse);
 
-        Duration duration = Duration.between(start, Instant.now());
+        measure((HttpServletResponse) servletResponse, httpReq, Duration.between(start, Instant.now()), config);
+    }
 
+    private void measure(HttpServletResponse httpResponse, HttpServletRequest httpReq,
+                         Duration duration, PageConfig config) {
         String category;
         if (isRoot(httpReq)) {
             category = "root";
@@ -84,17 +84,15 @@ public void doFilter(ServletRequest sr, ServletResponse sr1, FilterChain fc)
         }
 
         Timer categoryTimer = Timer.builder("requests.latency").
-                tags("category", category).
+                tags("category", category, "code", String.valueOf(httpResponse.getStatus())).
                 register(Metrics.getRegistry());
         categoryTimer.record(duration);
 
         SearchHelper helper = (SearchHelper) config.getRequestAttribute(SearchHelper.REQUEST_ATTR);
         if (helper != null) {
             if (helper.hits == null || helper.hits.length == 0) {
-                // empty search
                 emptySearch.record(duration);
             } else {
-                // successful search
                 successfulSearch.record(duration);
             }
         }

opengrok-web/src/main/webapp/WEB-INF/web.xml

@@ -16,19 +16,19 @@
         <listener-class>org.opengrok.web.WebappListener</listener-class>
     </listener>
     <filter>
-        <filter-name>AuthorizationFilter</filter-name>
-        <filter-class>org.opengrok.web.AuthorizationFilter</filter-class>
+        <filter-name>StatisticsFilter</filter-name>
+        <filter-class>org.opengrok.web.StatisticsFilter</filter-class>
     </filter>
     <filter-mapping>
-        <filter-name>AuthorizationFilter</filter-name>
+        <filter-name>StatisticsFilter</filter-name>
         <url-pattern>/*</url-pattern>
     </filter-mapping>
     <filter>
-        <filter-name>StatisticsFilter</filter-name>
-        <filter-class>org.opengrok.web.StatisticsFilter</filter-class>
+        <filter-name>AuthorizationFilter</filter-name>
+        <filter-class>org.opengrok.web.AuthorizationFilter</filter-class>
     </filter>
     <filter-mapping>
-        <filter-name>StatisticsFilter</filter-name>
+        <filter-name>AuthorizationFilter</filter-name>
         <url-pattern>/*</url-pattern>
     </filter-mapping>
     <filter>