Add metrics for request filtering

vespa-engine · Nov 17, 2020 · 5e74541 · 5e74541
1 parent eeb4a60
commit 5e74541
Show file tree

Hide file tree

Showing 5 changed files with 75 additions and 26 deletions.
diff --git a/config-model/src/main/java/com/yahoo/vespa/model/admin/monitoring/VespaMetricSet.java b/config-model/src/main/java/com/yahoo/vespa/model/admin/monitoring/VespaMetricSet.java
@@ -215,6 +215,11 @@ private static Set<Metric> getContainerMetrics() {
         addMetric(metrics, "jdisc.http.jetty.threadpool.thread.total", List.of("sum", "count", "min", "max"));
         addMetric(metrics, "jdisc.http.jetty.threadpool.queue.size", List.of("sum", "count", "min", "max"));
 
+        addMetric(metrics, "jdisc.http.filtering.request.handled", List.of("rate"));
+        addMetric(metrics, "jdisc.http.filtering.request.unhandled", List.of("rate"));
+        addMetric(metrics, "jdisc.http.filtering.response.handled", List.of("rate"));
+        addMetric(metrics, "jdisc.http.filtering.response.unhandled", List.of("rate"));
+
         return metrics;
     }
 

diff --git a/jdisc_http_service/src/main/java/com/yahoo/jdisc/http/server/jetty/FilterResolver.java b/jdisc_http_service/src/main/java/com/yahoo/jdisc/http/server/jetty/FilterResolver.java
@@ -1,11 +1,13 @@
 // Copyright Verizon Media. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root.
 package com.yahoo.jdisc.http.server.jetty;
 
+import com.yahoo.jdisc.Metric;
 import com.yahoo.jdisc.http.filter.RequestFilter;
 import com.yahoo.jdisc.http.filter.ResponseFilter;
 
 import javax.servlet.http.HttpServletRequest;
 import java.net.URI;
+import java.util.Map;
 import java.util.Optional;
 
 import static com.yahoo.jdisc.http.server.jetty.JDiscHttpServlet.getConnector;
@@ -18,18 +20,37 @@
 class FilterResolver {
 
     private final FilterBindings bindings;
+    private final Metric metric;
 
-    FilterResolver(FilterBindings bindings) {
+    FilterResolver(FilterBindings bindings, Metric metric) {
         this.bindings = bindings;
+        this.metric = metric;
     }
 
     Optional<RequestFilter> resolveRequestFilter(HttpServletRequest servletRequest, URI jdiscUri) {
-        return bindings.resolveRequestFilter(jdiscUri, getConnector(servletRequest).listenPort())
-                .map(bindings::getRequestFilter);
+        Optional<String> maybeFilterId = bindings.resolveRequestFilter(jdiscUri, getConnector(servletRequest).listenPort());
+        if (maybeFilterId.isPresent()) {
+            metric.add(MetricDefinitions.FILTERING_REQUEST_HANDLED, 1L, createMetricContext(servletRequest, maybeFilterId.get()));
+        } else {
+            metric.add(MetricDefinitions.FILTERING_REQUEST_UNHANDLED, 1L, createMetricContext(servletRequest, null));
+        }
+        return maybeFilterId.map(bindings::getRequestFilter);
     }
 
     Optional<ResponseFilter> resolveResponseFilter(HttpServletRequest servletRequest, URI jdiscUri) {
-        return bindings.resolveResponseFilter(jdiscUri, getConnector(servletRequest).listenPort())
-                .map(bindings::getResponseFilter);
+        Optional<String> maybeFilterId = bindings.resolveResponseFilter(jdiscUri, getConnector(servletRequest).listenPort());
+        if (maybeFilterId.isPresent()) {
+            metric.add(MetricDefinitions.FILTERING_RESPONSE_HANDLED, 1L, createMetricContext(servletRequest, maybeFilterId.get()));
+        } else {
+            metric.add(MetricDefinitions.FILTERING_RESPONSE_UNHANDLED, 1L, createMetricContext(servletRequest, null));
+        }
+        return maybeFilterId.map(bindings::getResponseFilter);
+    }
+
+    private Metric.Context createMetricContext(HttpServletRequest request, String filterId) {
+        Map<String, String> extraDimensions = filterId != null
+                ? Map.of(MetricDefinitions.FILTER_CHAIN_ID_DIMENSION, filterId)
+                : Map.of();
+        return JDiscHttpServlet.getConnector(request).createRequestMetricContext(request, extraDimensions);
     }
 }
diff --git a/jdisc_http_service/src/main/java/com/yahoo/jdisc/http/server/jetty/JDiscContext.java b/jdisc_http_service/src/main/java/com/yahoo/jdisc/http/server/jetty/JDiscContext.java
@@ -20,7 +20,7 @@ public JDiscContext(FilterBindings filterBindings,
                         Metric metric,
                         ServerConfig serverConfig) {
 
-        this.filterResolver = new FilterResolver(filterBindings);
+        this.filterResolver = new FilterResolver(filterBindings, metric);
         this.container = container;
         this.janitor = janitor;
         this.metric = metric;

diff --git a/jdisc_http_service/src/main/java/com/yahoo/jdisc/http/server/jetty/MetricDefinitions.java b/jdisc_http_service/src/main/java/com/yahoo/jdisc/http/server/jetty/MetricDefinitions.java
@@ -15,6 +15,7 @@ class MetricDefinitions {
     static final String CLIENT_IP_DIMENSION = "clientIp";
     static final String CLIENT_AUTHENTICATED_DIMENSION = "clientAuthenticated";
     static final String REQUEST_SERVER_NAME_DIMENSION = "requestServerName";
+    static final String FILTER_CHAIN_ID_DIMENSION = "chainId";
 
     static final String NUM_OPEN_CONNECTIONS = "serverNumOpenConnections";
     static final String NUM_CONNECTIONS_OPEN_MAX = "serverConnectionsOpenMax";
@@ -69,5 +70,10 @@ class MetricDefinitions {
     static final String JETTY_THREADPOOL_TOTAL_THREADS = "jdisc.http.jetty.threadpool.thread.total";
     static final String JETTY_THREADPOOL_QUEUE_SIZE = "jdisc.http.jetty.threadpool.queue.size";
 
+    static final String FILTERING_REQUEST_HANDLED = "jdisc.http.filtering.request.handled";
+    static final String FILTERING_REQUEST_UNHANDLED = "jdisc.http.filtering.request.unhandled";
+    static final String FILTERING_RESPONSE_HANDLED = "jdisc.http.filtering.response.handled";
+    static final String FILTERING_RESPONSE_UNHANDLED = "jdisc.http.filtering.response.unhandled";
+
     private MetricDefinitions() {}
 }
diff --git a/jdisc_http_service/src/test/java/com/yahoo/jdisc/http/server/jetty/FilterTestCase.java b/jdisc_http_service/src/test/java/com/yahoo/jdisc/http/server/jetty/FilterTestCase.java
@@ -407,10 +407,7 @@ public void requireThatDefaultRequestFilterChainIsRunIfNoOtherFilterChainMatches
                 .setRequestFilterDefaultForPort(defaultFilterId, 0)
                 .build();
         MyRequestHandler requestHandler = new MyRequestHandler();
-        TestDriver testDriver = TestDriver.newInstance(
-                JettyHttpServer.class,
-                requestHandler,
-                newFilterModule(filterBindings));
+        TestDriver testDriver = newDriver(requestHandler, filterBindings);
 
         testDriver.client().get("/status.html");
 
@@ -433,10 +430,7 @@ public void requireThatDefaultResponseFilterChainIsRunIfNoOtherFilterChainMatche
                 .setResponseFilterDefaultForPort(defaultFilterId, 0)
                 .build();
         MyRequestHandler requestHandler = new MyRequestHandler();
-        TestDriver testDriver = TestDriver.newInstance(
-                JettyHttpServer.class,
-                requestHandler,
-                newFilterModule(filterBindings));
+        TestDriver testDriver = newDriver(requestHandler, filterBindings);
 
         testDriver.client().get("/status.html");
 
@@ -459,10 +453,7 @@ public void requireThatRequestFilterWithBindingMatchHasPrecedenceOverDefaultFilt
                 .setRequestFilterDefaultForPort(defaultFilterId, 0)
                 .build();
         MyRequestHandler requestHandler = new MyRequestHandler();
-        TestDriver testDriver = TestDriver.newInstance(
-                JettyHttpServer.class,
-                requestHandler,
-                newFilterModule(filterBindings));
+        TestDriver testDriver = newDriver(requestHandler, filterBindings);
 
         testDriver.client().get("/filtered/status.html");
 
@@ -474,7 +465,7 @@ public void requireThatRequestFilterWithBindingMatchHasPrecedenceOverDefaultFilt
     }
 
     @Test
-    public void requireThatResponsFilterWithBindingMatchHasPrecedenceOverDefaultFilter() throws IOException, InterruptedException {
+    public void requireThatResponseFilterWithBindingMatchHasPrecedenceOverDefaultFilter() throws IOException, InterruptedException {
         ResponseFilter filterWithBinding = mock(ResponseFilter.class);
         ResponseFilter defaultFilter = mock(ResponseFilter.class);
         String defaultFilterId = "default-response-filter";
@@ -485,10 +476,7 @@ public void requireThatResponsFilterWithBindingMatchHasPrecedenceOverDefaultFilt
                 .setResponseFilterDefaultForPort(defaultFilterId, 0)
                 .build();
         MyRequestHandler requestHandler = new MyRequestHandler();
-        TestDriver testDriver = TestDriver.newInstance(
-                JettyHttpServer.class,
-                requestHandler,
-                newFilterModule(filterBindings));
+        TestDriver testDriver = newDriver(requestHandler, filterBindings);
 
         testDriver.client().get("/filtered/status.html");
 
@@ -499,25 +487,54 @@ public void requireThatResponsFilterWithBindingMatchHasPrecedenceOverDefaultFilt
         assertThat(testDriver.close(), is(true));
     }
 
+    @Test
+    public void requireThatMetricAreReported() throws IOException, InterruptedException {
+        FilterBindings filterBindings = new FilterBindings.Builder()
+                .addRequestFilter("my-request-filter", mock(RequestFilter.class))
+                .addRequestFilterBinding("my-request-filter", "http://*/*")
+                .build();
+        MetricConsumerMock metricConsumerMock = new MetricConsumerMock();
+        MyRequestHandler requestHandler = new MyRequestHandler();
+        TestDriver testDriver = newDriver(requestHandler, filterBindings, metricConsumerMock);
+
+        testDriver.client().get("/status.html");
+        assertThat(requestHandler.awaitInvocation(), is(true));
+        verify(metricConsumerMock.mockitoMock())
+                .add(MetricDefinitions.FILTERING_REQUEST_HANDLED, 1L, MetricConsumerMock.STATIC_CONTEXT);
+        verify(metricConsumerMock.mockitoMock(), never())
+                .add(MetricDefinitions.FILTERING_REQUEST_UNHANDLED, 1L, MetricConsumerMock.STATIC_CONTEXT);
+        verify(metricConsumerMock.mockitoMock(), never())
+                .add(MetricDefinitions.FILTERING_RESPONSE_HANDLED, 1L, MetricConsumerMock.STATIC_CONTEXT);
+        verify(metricConsumerMock.mockitoMock())
+                .add(MetricDefinitions.FILTERING_RESPONSE_UNHANDLED, 1L, MetricConsumerMock.STATIC_CONTEXT);
+        assertThat(testDriver.close(), is(true));
+    }
+
     private static TestDriver newDriver(MyRequestHandler requestHandler, FilterBindings filterBindings) {
+        return newDriver(requestHandler, filterBindings, new MetricConsumerMock());
+    }
+
+    private static TestDriver newDriver(MyRequestHandler requestHandler, FilterBindings filterBindings, MetricConsumerMock metricConsumer) {
         return TestDriver.newInstance(
                 JettyHttpServer.class,
                 requestHandler,
-                newFilterModule(filterBindings));
+                newFilterModule(filterBindings, metricConsumer));
     }
 
-    private static com.google.inject.Module newFilterModule(FilterBindings filterBindings) {
+    private static com.google.inject.Module newFilterModule(FilterBindings filterBindings, MetricConsumerMock metricConsumer) {
         return Modules.combine(
                 new AbstractModule() {
                     @Override
                     protected void configure() {
+
                         bind(FilterBindings.class).toInstance(filterBindings);
                         bind(ServerConfig.class).toInstance(new ServerConfig(new ServerConfig.Builder()));
                         bind(ConnectorConfig.class).toInstance(new ConnectorConfig(new ConnectorConfig.Builder()));
                         bind(ServletPathsConfig.class).toInstance(new ServletPathsConfig(new ServletPathsConfig.Builder()));
                     }
                 },
-                new ConnectorFactoryRegistryModule());
+                new ConnectorFactoryRegistryModule(),
+                metricConsumer.asGuiceModule());
     }
 
     private static abstract class RequestFilterMockBase extends AbstractResource implements RequestFilter {}