Make metrics retrieval be in the context of an application instance

5 files changed, 94 insertions, 95 deletions

diff --git a/configserver/src/main/java/com/yahoo/vespa/config/server/ApplicationRepository.java b/configserver/src/main/java/com/yahoo/vespa/config/server/ApplicationRepository.java
index 0c7b6d24ed9..a16b9a863d7 100644
--- a/configserver/src/main/java/com/yahoo/vespa/config/server/ApplicationRepository.java
+++ b/configserver/src/main/java/com/yahoo/vespa/config/server/ApplicationRepository.java
@@ -644,7 +644,7 @@ public class ApplicationRepository implements com.yahoo.config.provision.Deploye
 
     // ---------------- Metrics ------------------------------------------------------------------------
 
-    public HttpResponse getMetrics(ApplicationId applicationId) {
+    public MetricsResponse getMetrics(ApplicationId applicationId) {
         var metricsRetriever = new MetricsRetriever();
         var clusters = getClustersOfApplication(applicationId);
         var clusterMetrics = new LinkedHashMap<ClusterInfo, MetricsAggregator>();
diff --git a/configserver/src/main/java/com/yahoo/vespa/config/server/http/v2/ApplicationHandler.java b/configserver/src/main/java/com/yahoo/vespa/config/server/http/v2/ApplicationHandler.java
index 3f790b31813..e18c6ad6c56 100644
--- a/configserver/src/main/java/com/yahoo/vespa/config/server/http/v2/ApplicationHandler.java
+++ b/configserver/src/main/java/com/yahoo/vespa/config/server/http/v2/ApplicationHandler.java
@@ -105,7 +105,7 @@ public class ApplicationHandler extends HttpHandler {
         }
 
         if (isMetricsRequest(request)) {
-            return applicationRepository.getMetrics();
+            return applicationRepository.getMetrics(applicationId);
         }
 
         if (isIsSuspendedRequest(request)) {
diff --git a/configserver/src/main/java/com/yahoo/vespa/config/server/metrics/MetricsAggregator.java b/configserver/src/main/java/com/yahoo/vespa/config/server/metrics/MetricsAggregator.java
index 9c8712c6f1e..c6b2131863d 100644
--- a/configserver/src/main/java/com/yahoo/vespa/config/server/metrics/MetricsAggregator.java
+++ b/configserver/src/main/java/com/yahoo/vespa/config/server/metrics/MetricsAggregator.java
@@ -6,85 +6,81 @@ import java.util.Optional;
 
 /**
  * @author olaa
+ * @author ogronnesby
  */
 public class MetricsAggregator {
 
-    double feedLatencySum;
-    double feedLatencyCount;
-    double qrQueryLatencySum;
-    double qrQueryLatencyCount;
-    double containerQueryLatencySum;
-    double containerQueryLatencyCount;
-    double documentCount;
-    Instant timestamp;
-
-    public void addFeedLatencySum(double feedLatencySum) {
-        this.feedLatencySum += feedLatencySum;
-    }
-
-    public void addFeedLatencyCount(double feedLatencyCount) {
-        this.feedLatencyCount += feedLatencyCount;
-    }
+    private LatencyMetrics feed;
+    private LatencyMetrics qr;
+    private LatencyMetrics container;
+    private Double documentCount;
+    private Instant timestamp;
 
-    public void addQrQueryLatencyCount(double qrQueryLatencyCount) {
-        this.qrQueryLatencyCount += qrQueryLatencyCount;
+    public MetricsAggregator addFeedLatency(double sum, double count) {
+        this.feed = combineLatency(this.feed, sum, count);
+        return this;
     }
 
-    public void addQrQueryLatencySum(double qrQueryLatencySum) {
-        this.qrQueryLatencySum += qrQueryLatencySum;
+    public MetricsAggregator addQrLatency(double sum, double count) {
+        this.qr = combineLatency(this.qr, sum, count);
+        return this;
     }
 
-    public void addContainerQueryLatencyCount(double containerQueryLatencyCount) {
-        this.containerQueryLatencyCount += containerQueryLatencyCount;
+    public MetricsAggregator addContainerLatency(double sum, double count) {
+        this.container = combineLatency(this.container, sum, count);
+        return this;
     }
 
-    public void addContainerQueryLatencySum(double containerQueryLatencySum) {
-        this.containerQueryLatencySum += containerQueryLatencySum;
+    public MetricsAggregator addDocumentCount(double count) {
+        this.documentCount = (this.documentCount == null ? 0.0 : this.documentCount) + count;
+        return this;
     }
 
-    public void addDocumentCount(double documentCount) {
-        this.documentCount += documentCount;
+    public MetricsAggregator setTimestamp(Instant timestamp) {
+        this.timestamp = timestamp;
+        return this;
     }
 
     public Optional<Double> aggregateFeedLatency() {
-        if (isZero(feedLatencySum) || isZero(feedLatencyCount)) return Optional.empty();
-        return Optional.of(feedLatencySum / feedLatencyCount);
+        return Optional.ofNullable(feed).map(m -> m.latencySum / m.latencyCount);
+
     }
 
     public Optional<Double> aggregateFeedRate() {
-        if (isZero(feedLatencyCount)) return Optional.empty();
-        return Optional.of(feedLatencyCount / 60);
+        return Optional.ofNullable(feed).map(m -> m.latencyCount / 60);
     }
 
     public Optional<Double> aggregateQueryLatency() {
-        if (isZero(containerQueryLatencyCount, containerQueryLatencySum) && isZero(qrQueryLatencyCount, qrQueryLatencySum)) return Optional.empty();
-        return Optional.of((containerQueryLatencySum + qrQueryLatencySum) / (containerQueryLatencyCount + qrQueryLatencyCount));
+        if (container == null && qr == null) return Optional.empty();
+        var c = Optional.ofNullable(container).orElseGet(LatencyMetrics::new);
+        var q = Optional.ofNullable(qr).orElseGet(LatencyMetrics::new);
+        return Optional.of((c.latencySum + q.latencySum) / (c.latencyCount + q.latencyCount));
     }
 
     public Optional<Double> aggregateQueryRate() {
-        if (isZero(containerQueryLatencyCount) && isZero(qrQueryLatencyCount)) return Optional.empty();
-        return Optional.of((containerQueryLatencyCount + qrQueryLatencyCount) / 60);
+        if (container == null && qr == null) return Optional.empty();
+        var c = Optional.ofNullable(container).orElseGet(LatencyMetrics::new);
+        var q = Optional.ofNullable(qr).orElseGet(LatencyMetrics::new);
+        return Optional.of((c.latencyCount + q.latencyCount) / 60);
     }
 
     public Optional<Double> aggregateDocumentCount() {
-        if (isZero(documentCount)) return Optional.empty();
-        return Optional.of(documentCount);
-    }
-
-    public void setTimestamp(Instant timestamp) {
-        this.timestamp = timestamp;
+        return Optional.ofNullable(documentCount);
     }
 
     public Instant getTimestamp() {
         return timestamp;
     }
 
-    private boolean isZero(double... values) {
-        boolean isZero = false;
-        for (double value : values) {
-            isZero |= Math.abs(value) < 0.001;
-        }
-        return isZero;
+    private LatencyMetrics combineLatency(LatencyMetrics metricsOrNull, double sum, double count) {
+        var metrics = Optional.ofNullable(metricsOrNull).orElseGet(LatencyMetrics::new);
+        metrics.latencyCount += count;
+        metrics.latencySum += sum;
+        return metrics;
     }
 
+    private static class LatencyMetrics {
+        double latencySum;
+        double latencyCount;
+    }
 }
diff --git a/configserver/src/main/java/com/yahoo/vespa/config/server/metrics/MetricsRetriever.java b/configserver/src/main/java/com/yahoo/vespa/config/server/metrics/MetricsRetriever.java
index 273979cead5..0881d32b21e 100644
--- a/configserver/src/main/java/com/yahoo/vespa/config/server/metrics/MetricsRetriever.java
+++ b/configserver/src/main/java/com/yahoo/vespa/config/server/metrics/MetricsRetriever.java
@@ -66,14 +66,17 @@ public class MetricsRetriever {
             Inspector values = m.field("values");
             switch (serviceName) {
                 case "container":
-                    metrics.addContainerQueryLatencyCount(values.field("query_latency.count").asDouble());
-                    metrics.addContainerQueryLatencySum(values.field("query_latency.sum").asDouble());
-                    metrics.addFeedLatencyCount(values.field("feed_latency.count").asDouble());
-                    metrics.addFeedLatencySum(values.field("feed_latency.sum").asDouble());
+                    metrics.addContainerLatency(
+                            values.field("query_latency.sum").asDouble(),
+                            values.field("query_latency.count").asDouble());
+                    metrics.addFeedLatency(
+                            values.field("feed_latency.sum").asDouble(),
+                            values.field("feed_latency.count").asDouble());
                     break;
                 case "qrserver":
-                    metrics.addQrQueryLatencyCount(values.field("query_latency.count").asDouble());
-                    metrics.addQrQueryLatencySum(values.field("query_latency.sum").asDouble());
+                    metrics.addQrLatency(
+                            values.field("query_latency.sum").asDouble(),
+                            values.field("query_latency.count").asDouble());
                     break;
                 case "distributor":
                     metrics.addDocumentCount(values.field("vds.distributor.docsstored.average").asDouble());
diff --git a/configserver/src/test/java/com/yahoo/vespa/config/server/metrics/MetricsRetrieverTest.java b/configserver/src/test/java/com/yahoo/vespa/config/server/metrics/MetricsRetrieverTest.java
index 798fecaff65..1b878a432c9 100644
--- a/configserver/src/test/java/com/yahoo/vespa/config/server/metrics/MetricsRetrieverTest.java
+++ b/configserver/src/test/java/com/yahoo/vespa/config/server/metrics/MetricsRetrieverTest.java
@@ -1,19 +1,17 @@
 package com.yahoo.vespa.config.server.metrics;
 
 import com.github.tomakehurst.wiremock.junit.WireMockRule;
-import com.yahoo.config.provision.ApplicationId;
-import com.yahoo.vespa.config.server.http.v2.MetricsResponse;
+import junit.framework.AssertionFailedError;
 import org.junit.Rule;
 import org.junit.Test;
 
-import java.io.ByteArrayOutputStream;
 import java.io.IOException;
 import java.net.URI;
 import java.nio.file.Files;
 import java.nio.file.Path;
-import java.util.Collection;
 import java.util.List;
-import java.util.Map;
+import java.util.Optional;
+import java.util.function.BiConsumer;
 
 import static com.github.tomakehurst.wiremock.client.WireMock.aResponse;
 import static com.github.tomakehurst.wiremock.client.WireMock.get;
@@ -33,14 +31,12 @@ public class MetricsRetrieverTest {
 
     @Test
     public void testMetricAggregation() throws IOException {
-        MetricsRetriever metricsRetriever = new MetricsRetriever();
+        var metricsRetriever = new MetricsRetriever();
 
-        ApplicationId applicationId = ApplicationId.from("tenant", "app", "default");
-
-        List<ClusterInfo> clusters = List.of(new ClusterInfo("cluster1", ClusterInfo.ClusterType.content, List.of(URI.create("http://localhost:8080/1"), URI.create("http://localhost:8080/2"))),
-                new ClusterInfo("cluster2", ClusterInfo.ClusterType.container, List.of(URI.create("http://localhost:8080/3"))));
-
-        Map<ApplicationId, Collection<ClusterInfo>> applications = Map.of(applicationId, clusters);
+        var clusters = List.of(
+                new ClusterInfo("cluster1", ClusterInfo.ClusterType.content, List.of(URI.create("http://localhost:8080/1"), URI.create("http://localhost:8080/2"))),
+                new ClusterInfo("cluster2", ClusterInfo.ClusterType.container, List.of(URI.create("http://localhost:8080/3")))
+        );
 
         stubFor(get(urlEqualTo("/1"))
                 .willReturn(aResponse()
@@ -57,36 +53,21 @@ public class MetricsRetrieverTest {
                         .withStatus(200)
                         .withBody(containerMetrics())));
 
-        MetricsResponse metricsResponse = metricsRetriever.retrieveAllMetrics(applications);
-        ByteArrayOutputStream bos = new ByteArrayOutputStream();
-        metricsResponse.render(bos);
-        String expectedResponse = "[\n" +
-                " {\n" +
-                "  \"applicationId\": \"tenant:app:default\",\n" +
-                "  \"clusters\": [\n" +
-                "   {\n" +
-                "    \"clusterId\": \"cluster1\",\n" +
-                "    \"clusterType\": \"content\",\n" +
-                "    \"metrics\": {\n" +
-                "     \"documentCount\": 6000.0\n" +
-                "    }\n" +
-                "   },\n" +
-                "   {\n" +
-                "    \"clusterId\": \"cluster2\",\n" +
-                "    \"clusterType\": \"container\",\n" +
-                "    \"metrics\": {\n" +
-                "     \"queriesPerSecond\": 1.4333333333333333,\n" +
-                "     \"feedPerSecond\": 0.7166666666666667,\n" +
-                "     \"queryLatency\": 93.02325581395348,\n" +
-                "     \"feedLatency\": 69.76744186046511\n" +
-                "    }\n" +
-                "   }\n" +
-                "  ]\n" +
-                " }\n" +
-                "]\n";
-        assertEquals(expectedResponse, bos.toString());
-        wireMock.stop();
+        compareAggregators(
+                new MetricsAggregator().addDocumentCount(6000.0),
+                metricsRetriever.requestMetricsForCluster(clusters.get(0))
+        );
 
+        compareAggregators(
+                new MetricsAggregator()
+                        .addContainerLatency(3000, 43)
+                        .addContainerLatency(2000, 0)
+                        .addQrLatency(3000, 43)
+                        .addFeedLatency(3000, 43),
+                metricsRetriever.requestMetricsForCluster(clusters.get(1))
+        );
+
+        wireMock.stop();
     }
 
     private String containerMetrics() throws IOException {
@@ -96,4 +77,23 @@ public class MetricsRetrieverTest {
     private String contentMetrics() throws IOException {
         return Files.readString(Path.of("src/test/resources/metrics/content_metrics"));
     }
+
+    // Same tolerance value as used internally in MetricsAggregator.isZero
+    private static final double metricsTolerance = 0.001;
+
+    private void compareAggregators(MetricsAggregator expected, MetricsAggregator actual) {
+        BiConsumer<Double, Double> assertDoubles = (a, b) -> assertEquals(a.doubleValue(), b.doubleValue(), metricsTolerance);
+
+        compareOptionals(expected.aggregateDocumentCount(), actual.aggregateDocumentCount(), assertDoubles);
+        compareOptionals(expected.aggregateQueryRate(), actual.aggregateQueryRate(), assertDoubles);
+        compareOptionals(expected.aggregateFeedRate(), actual.aggregateFeedRate(), assertDoubles);
+        compareOptionals(expected.aggregateQueryLatency(), actual.aggregateQueryLatency(), assertDoubles);
+        compareOptionals(expected.aggregateFeedLatency(), actual.aggregateFeedLatency(), assertDoubles);
+    }
+
+    @SuppressWarnings("OptionalUsedAsFieldOrParameterType")
+    private static <T> void compareOptionals(Optional<T> a, Optional<T> b, BiConsumer<T, T> comparer) {
+        if (a.isPresent() != b.isPresent()) throw new AssertionFailedError("Both optionals are not present: " + a + ", " + b);
+        a.ifPresent(x -> b.ifPresent(y -> comparer.accept(x, y)));
+    }
 }
\ No newline at end of file