Add scheme as dimension to response metrics

3 files changed, 112 insertions, 65 deletions

diff --git a/jdisc_http_service/src/main/java/com/yahoo/jdisc/http/server/jetty/HttpResponseStatisticsCollector.java b/jdisc_http_service/src/main/java/com/yahoo/jdisc/http/server/jetty/HttpResponseStatisticsCollector.java
index 1e92fbef967..4239d2120cf 100644
--- a/jdisc_http_service/src/main/java/com/yahoo/jdisc/http/server/jetty/HttpResponseStatisticsCollector.java
+++ b/jdisc_http_service/src/main/java/com/yahoo/jdisc/http/server/jetty/HttpResponseStatisticsCollector.java
@@ -16,10 +16,9 @@ import javax.servlet.AsyncListener;
 import javax.servlet.ServletException;
 import javax.servlet.http.HttpServletRequest;
 import javax.servlet.http.HttpServletResponse;
-
 import java.io.IOException;
-import java.util.HashMap;
-import java.util.Map;
+import java.util.ArrayList;
+import java.util.List;
 import java.util.concurrent.Future;
 import java.util.concurrent.TimeoutException;
 import java.util.concurrent.atomic.AtomicLong;
@@ -40,19 +39,25 @@ public class HttpResponseStatisticsCollector extends HandlerWrapper implements G
         GET, PATCH, POST, PUT, DELETE, OPTIONS, HEAD, OTHER
     }
 
+    public enum HttpScheme {
+        HTTP, HTTPS, OTHER
+    }
+
     private static final String[] HTTP_RESPONSE_GROUPS = { Metrics.RESPONSES_1XX, Metrics.RESPONSES_2XX, Metrics.RESPONSES_3XX,
             Metrics.RESPONSES_4XX, Metrics.RESPONSES_5XX, Metrics.RESPONSES_401, Metrics.RESPONSES_403};
 
     private final AtomicLong inFlight = new AtomicLong();
-    private final LongAdder statistics[][];
+    private final LongAdder statistics[][][];
 
     public HttpResponseStatisticsCollector() {
         super();
-        statistics = new LongAdder[HttpMethod.values().length][];
-        for (int method = 0; method < statistics.length; method++) {
-            statistics[method] = new LongAdder[HTTP_RESPONSE_GROUPS.length];
-            for (int group = 0; group < HTTP_RESPONSE_GROUPS.length; group++) {
-                statistics[method][group] = new LongAdder();
+        statistics = new LongAdder[HttpScheme.values().length][HttpMethod.values().length][];
+        for (int scheme = 0; scheme < HttpScheme.values().length; ++scheme) {
+            for (int method = 0; method < HttpMethod.values().length; method++) {
+                statistics[scheme][method] = new LongAdder[HTTP_RESPONSE_GROUPS.length];
+                for (int group = 0; group < HTTP_RESPONSE_GROUPS.length; group++) {
+                    statistics[scheme][method][group] = new LongAdder();
+                }
             }
         }
     }
@@ -110,10 +115,11 @@ public class HttpResponseStatisticsCollector extends HandlerWrapper implements G
     private void observeEndOfRequest(Request request, HttpServletResponse flushableResponse) throws IOException {
         int group = groupIndex(request);
         if (group >= 0) {
+            HttpScheme scheme = getScheme(request);
             HttpMethod method = getMethod(request);
-            statistics[method.ordinal()][group].increment();
+            statistics[scheme.ordinal()][method.ordinal()][group].increment();
             if (group == 5 || group == 6) { // if 401/403, also increment 4xx
-                statistics[method.ordinal()][3].increment();
+                statistics[scheme.ordinal()][method.ordinal()][3].increment();
             }
         }
 
@@ -146,6 +152,17 @@ public class HttpResponseStatisticsCollector extends HandlerWrapper implements G
         }
     }
 
+    private HttpScheme getScheme(Request request) {
+        switch (request.getScheme()) {
+            case "http":
+                return HttpScheme.HTTP;
+            case "https":
+                return HttpScheme.HTTPS;
+            default:
+                return HttpScheme.OTHER;
+        }
+    }
+
     private HttpMethod getMethod(Request request) {
         switch (request.getMethod()) {
         case "GET":
@@ -167,17 +184,18 @@ public class HttpResponseStatisticsCollector extends HandlerWrapper implements G
         }
     }
 
-    public Map<String, Map<String, Long>> takeStatisticsByMethod() {
-        Map<String, Map<String, Long>> ret = new HashMap<>();
-
-        for (HttpMethod method : HttpMethod.values()) {
-            int methodIndex = method.ordinal();
-            Map<String, Long> methodStats = new HashMap<>();
-            ret.put(method.toString(), methodStats);
-
-            for (int group = 0; group < HTTP_RESPONSE_GROUPS.length; group++) {
-                long value = statistics[methodIndex][group].sumThenReset();
-                methodStats.put(HTTP_RESPONSE_GROUPS[group], value);
+    public List<StatisticsEntry> takeStatistics() {
+        var ret = new ArrayList<StatisticsEntry>();
+        for (HttpScheme scheme : HttpScheme.values()) {
+            int schemeIndex = scheme.ordinal();
+            for (HttpMethod method : HttpMethod.values()) {
+                int methodIndex = method.ordinal();
+                for (int group = 0; group < HTTP_RESPONSE_GROUPS.length; group++) {
+                    long value = statistics[schemeIndex][methodIndex][group].sumThenReset();
+                    if (value > 0) {
+                        ret.add(new StatisticsEntry(scheme.name().toLowerCase(), method.name(), HTTP_RESPONSE_GROUPS[group], value));
+                    }
+                }
             }
         }
         return ret;
@@ -216,4 +234,19 @@ public class HttpResponseStatisticsCollector extends HandlerWrapper implements G
         FutureCallback futureCallback = shutdown.get();
         return futureCallback != null && futureCallback.isDone();
     }
+
+    public static class StatisticsEntry {
+        public final String scheme;
+        public final String method;
+        public final String name;
+        public final long value;
+
+
+        public StatisticsEntry(String scheme, String method, String name, long value) {
+            this.scheme = scheme;
+            this.method = method;
+            this.name = name;
+            this.value = value;
+        }
+    }
 }
diff --git a/jdisc_http_service/src/main/java/com/yahoo/jdisc/http/server/jetty/JettyHttpServer.java b/jdisc_http_service/src/main/java/com/yahoo/jdisc/http/server/jetty/JettyHttpServer.java
index 49ae54f7db3..30a1b1d885c 100644
--- a/jdisc_http_service/src/main/java/com/yahoo/jdisc/http/server/jetty/JettyHttpServer.java
+++ b/jdisc_http_service/src/main/java/com/yahoo/jdisc/http/server/jetty/JettyHttpServer.java
@@ -8,7 +8,6 @@ import com.yahoo.component.ComponentId;
 import com.yahoo.component.provider.ComponentRegistry;
 import com.yahoo.container.logging.AccessLog;
 import com.yahoo.jdisc.Metric;
-import com.yahoo.jdisc.Metric.Context;
 import com.yahoo.jdisc.application.OsgiFramework;
 import com.yahoo.jdisc.http.ServerConfig;
 import com.yahoo.jdisc.http.ServletPathsConfig;
@@ -358,13 +357,12 @@ public class JettyHttpServer extends AbstractServerProvider {
     }
 
     private void addResponseMetrics(HttpResponseStatisticsCollector statisticsCollector) {
-        Map<String, Map<String, Long>> statistics = statisticsCollector.takeStatisticsByMethod();
-        statistics.forEach((httpMethod, statsByResponseType) -> {
+        for (var metricEntry : statisticsCollector.takeStatistics()) {
             Map<String, Object> dimensions = new HashMap<>();
-            dimensions.put(Metrics.METHOD_DIMENSION, httpMethod);
-            Context ctx = metric.createContext(dimensions);
-            statsByResponseType.forEach((group, value) -> metric.add(group, value, ctx));
-        });
+            dimensions.put(Metrics.METHOD_DIMENSION, metricEntry.method);
+            dimensions.put(Metrics.SCHEME_DIMENSION, metricEntry.scheme);
+            metric.add(metricEntry.name, metricEntry.value, metric.createContext(dimensions));
+        }
     }
 
     private void setConnectorMetrics(JDiscServerConnector connector) {
diff --git a/jdisc_http_service/src/test/java/com/yahoo/jdisc/http/server/jetty/HttpResponseStatisticsCollectorTest.java b/jdisc_http_service/src/test/java/com/yahoo/jdisc/http/server/jetty/HttpResponseStatisticsCollectorTest.java
index 3c23a2b0937..df2308f6dd0 100644
--- a/jdisc_http_service/src/test/java/com/yahoo/jdisc/http/server/jetty/HttpResponseStatisticsCollectorTest.java
+++ b/jdisc_http_service/src/test/java/com/yahoo/jdisc/http/server/jetty/HttpResponseStatisticsCollectorTest.java
@@ -1,6 +1,7 @@
 // Copyright 2018 Yahoo Holdings. 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.http.server.jetty.HttpResponseStatisticsCollector.StatisticsEntry;
 import com.yahoo.jdisc.http.server.jetty.JettyHttpServer.Metrics;
 import org.eclipse.jetty.http.HttpFields;
 import org.eclipse.jetty.http.HttpURI;
@@ -22,10 +23,9 @@ import org.testng.annotations.Test;
 import javax.servlet.ServletException;
 import javax.servlet.http.HttpServletRequest;
 import javax.servlet.http.HttpServletResponse;
-
 import java.io.IOException;
 import java.nio.ByteBuffer;
-import java.util.Map;
+import java.util.List;
 
 import static org.hamcrest.MatcherAssert.assertThat;
 import static org.hamcrest.Matchers.equalTo;
@@ -40,55 +40,62 @@ public class HttpResponseStatisticsCollectorTest {
 
     @Test
     public void statistics_are_aggregated_by_category() throws Exception {
-        testRequest(300, "GET");
-        testRequest(301, "GET");
-        testRequest(200, "GET");
+        testRequest("http", 300, "GET");
+        testRequest("http", 301, "GET");
+        testRequest("http", 200, "GET");
 
-        Map<String, Map<String, Long>> stats = collector.takeStatisticsByMethod();
-        assertThat(stats.get("GET").get(Metrics.RESPONSES_2XX), equalTo(1L));
-        assertThat(stats.get("GET").get(Metrics.RESPONSES_3XX), equalTo(2L));
+        var stats = collector.takeStatistics();
+        assertStatisticsEntryPresent(stats, "http", "GET", Metrics.RESPONSES_2XX, 1L);
+        assertStatisticsEntryPresent(stats, "http", "GET", Metrics.RESPONSES_3XX, 2L);
     }
 
     @Test
-    public void statistics_are_grouped_by_http_method() throws Exception {
-        testRequest(200, "GET");
-        testRequest(200, "PUT");
-        testRequest(200, "POST");
-        testRequest(200, "POST");
-        testRequest(404, "GET");
-
-        Map<String, Map<String, Long>> stats = collector.takeStatisticsByMethod();
-        assertThat(stats.get("GET").get(Metrics.RESPONSES_2XX), equalTo(1L));
-        assertThat(stats.get("GET").get(Metrics.RESPONSES_4XX), equalTo(1L));
-        assertThat(stats.get("PUT").get(Metrics.RESPONSES_2XX), equalTo(1L));
-        assertThat(stats.get("POST").get(Metrics.RESPONSES_2XX), equalTo(2L));
+    public void statistics_are_grouped_by_http_method_and_scheme() throws Exception {
+        testRequest("http", 200, "GET");
+        testRequest("http", 200, "PUT");
+        testRequest("http", 200, "POST");
+        testRequest("http", 200, "POST");
+        testRequest("http", 404, "GET");
+        testRequest("https", 404, "GET");
+        testRequest("https", 200, "POST");
+        testRequest("https", 200, "POST");
+        testRequest("https", 200, "POST");
+        testRequest("https", 200, "POST");
+
+        var stats = collector.takeStatistics();
+        assertStatisticsEntryPresent(stats, "http", "GET", Metrics.RESPONSES_2XX, 1L);
+        assertStatisticsEntryPresent(stats, "http", "GET", Metrics.RESPONSES_4XX, 1L);
+        assertStatisticsEntryPresent(stats, "http", "PUT", Metrics.RESPONSES_2XX, 1L);
+        assertStatisticsEntryPresent(stats, "http", "POST", Metrics.RESPONSES_2XX, 2L);
+        assertStatisticsEntryPresent(stats, "https", "GET", Metrics.RESPONSES_4XX, 1L);
+        assertStatisticsEntryPresent(stats, "https", "POST", Metrics.RESPONSES_2XX, 4L);
     }
 
     @Test
     public void statistics_include_grouped_and_single_statuscodes() throws Exception {
-        testRequest(401, "GET");
-        testRequest(404, "GET");
-        testRequest(403, "GET");
+        testRequest("http", 401, "GET");
+        testRequest("http", 404, "GET");
+        testRequest("http", 403, "GET");
 
-        Map<String, Map<String, Long>> stats = collector.takeStatisticsByMethod();
-        assertThat(stats.get("GET").get(Metrics.RESPONSES_4XX), equalTo(3L));
-        assertThat(stats.get("GET").get(Metrics.RESPONSES_401), equalTo(1L));
-        assertThat(stats.get("GET").get(Metrics.RESPONSES_403), equalTo(1L));
+        var stats = collector.takeStatistics();
+        assertStatisticsEntryPresent(stats, "http", "GET", Metrics.RESPONSES_4XX, 3L);
+        assertStatisticsEntryPresent(stats, "http", "GET", Metrics.RESPONSES_401, 1L);
+        assertStatisticsEntryPresent(stats, "http", "GET", Metrics.RESPONSES_403, 1L);
 
     }
 
     @Test
     public void retrieving_statistics_resets_the_counters() throws Exception {
-        testRequest(200, "GET");
-        testRequest(200, "GET");
+        testRequest("http", 200, "GET");
+        testRequest("http", 200, "GET");
 
-        Map<String, Map<String, Long>> stats = collector.takeStatisticsByMethod();
-        assertThat(stats.get("GET").get(Metrics.RESPONSES_2XX), equalTo(2L));
+        var stats = collector.takeStatistics();
+        assertStatisticsEntryPresent(stats, "http", "GET", Metrics.RESPONSES_2XX, 2L);
 
-        testRequest(200, "GET");
+        testRequest("http", 200, "GET");
 
-        stats = collector.takeStatisticsByMethod();
-        assertThat(stats.get("GET").get(Metrics.RESPONSES_2XX), equalTo(1L));
+        stats = collector.takeStatistics();
+        assertStatisticsEntryPresent(stats, "http", "GET", Metrics.RESPONSES_2XX, 1L);
     }
 
     @BeforeTest
@@ -116,9 +123,9 @@ public class HttpResponseStatisticsCollectorTest {
         server.start();
     }
 
-    private Request testRequest(int responseCode, String httpMethod) throws Exception {
+    private Request testRequest(String scheme, int responseCode, String httpMethod) throws Exception {
         HttpChannel channel = new HttpChannel(connector, new HttpConfiguration(), null, new DummyTransport());
-        MetaData.Request metaData = new MetaData.Request(httpMethod, new HttpURI("http://foo/bar"), HttpVersion.HTTP_1_1, new HttpFields());
+        MetaData.Request metaData = new MetaData.Request(httpMethod, new HttpURI(scheme + "://foo/bar"), HttpVersion.HTTP_1_1, new HttpFields());
         Request req = channel.getRequest();
         req.setMetaData(metaData);
 
@@ -127,6 +134,15 @@ public class HttpResponseStatisticsCollectorTest {
         return req;
     }
 
+    private static void assertStatisticsEntryPresent(List<StatisticsEntry> result, String scheme, String method, String name, long expectedValue) {
+        long value = result.stream()
+                .filter(entry -> entry.method.equals(method) && entry.scheme.equals(scheme) && entry.name.equals(name))
+                .mapToLong(entry -> entry.value)
+                .findAny()
+                .orElseThrow(() -> new AssertionError(String.format("Not matching entry in result (scheme=%s, method=%s, name=%s)", scheme, method, name)));
+        assertThat(value, equalTo(expectedValue));
+    }
+
     private final class DummyTransport implements HttpTransport {
         @Override
         public void send(Response info, boolean head, ByteBuffer content, boolean lastContent, Callback callback) {