Merge pull request #10290 from vespa-engine/ogronnesby/update-deployment-metrics-aggregation

Improve aggregation over .sum()

3 files changed, 52 insertions, 16 deletions

diff --git a/controller-api/src/main/java/com/yahoo/vespa/hosted/controller/api/application/v4/model/ClusterMetrics.java b/controller-api/src/main/java/com/yahoo/vespa/hosted/controller/api/application/v4/model/ClusterMetrics.java
index 1377a333335..28c94960daf 100644
--- a/controller-api/src/main/java/com/yahoo/vespa/hosted/controller/api/application/v4/model/ClusterMetrics.java
+++ b/controller-api/src/main/java/com/yahoo/vespa/hosted/controller/api/application/v4/model/ClusterMetrics.java
@@ -1,15 +1,23 @@
 // Copyright 2019 Oath Inc. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root.
 package com.yahoo.vespa.hosted.controller.api.application.v4.model;
 
-import java.util.Collections;
 import java.util.HashMap;
 import java.util.Map;
+import java.util.Optional;
+import java.util.OptionalDouble;
 
 /**
  * @author olaa
  */
 public class ClusterMetrics {
 
+    // These field names originate from the MetricsResponse class
+    private static final String QUERIES_PER_SECOND = "queriesPerSecond";
+    private static final String FEED_PER_SECOND = "feedPerSecond";
+    private static final String DOCUMENT_COUNT = "documentCount";
+    private static final String FEED_LATENCY = "feedLatency";
+    private static final String QUERY_LATENCY  = "queryLatency";
+
     private final String clusterId;
     private final ClusterType clusterType;
     private final Map<String, Double> metrics;
@@ -28,8 +36,24 @@ public class ClusterMetrics {
         return clusterType;
     }
 
-    public Map<String, Double> getMetrics() {
-        return Collections.unmodifiableMap(metrics);
+    public Optional<Double> queriesPerSecond() {
+        return Optional.ofNullable(metrics.get(QUERIES_PER_SECOND));
+    }
+
+    public Optional<Double> feedPerSecond() {
+        return Optional.ofNullable(metrics.get(FEED_PER_SECOND));
+    }
+
+    public Optional<Double> documentCount() {
+        return Optional.ofNullable(metrics.get(DOCUMENT_COUNT));
+    }
+
+    public Optional<Double> feedLatency() {
+        return Optional.ofNullable(metrics.get(FEED_LATENCY));
+    }
+
+    public Optional<Double> queryLatency() {
+        return Optional.ofNullable(metrics.get(QUERY_LATENCY));
     }
 
     public void addMetric(String name, double value) {
diff --git a/controller-api/src/main/java/com/yahoo/vespa/hosted/controller/api/integration/metrics/ConfigServerMetricsService.java b/controller-api/src/main/java/com/yahoo/vespa/hosted/controller/api/integration/metrics/ConfigServerMetricsService.java
index a47e2165291..2240afee804 100644
--- a/controller-api/src/main/java/com/yahoo/vespa/hosted/controller/api/integration/metrics/ConfigServerMetricsService.java
+++ b/controller-api/src/main/java/com/yahoo/vespa/hosted/controller/api/integration/metrics/ConfigServerMetricsService.java
@@ -10,7 +10,8 @@ import com.yahoo.vespa.hosted.controller.api.integration.routing.RotationStatus;
 
 import java.util.List;
 import java.util.Map;
-import java.util.stream.Stream;
+import java.util.Optional;
+import java.util.function.Function;
 
 /**
  * Retrieves metrics from the configuration server.
@@ -35,17 +36,13 @@ public class ConfigServerMetricsService implements MetricsService {
         var deploymentId = new DeploymentId(application, zone);
         var metrics = configServerClient.getMetrics(deploymentId);
 
-        // TODO(ogronnesby): We probably want something more intelligent than just using .sum(), but it's better to
-        // TODO(ogronnesby): get some values populated and then fix the formula later.
-
         // The field names here come from the MetricsResponse class.
-
         return new DeploymentMetrics(
-                doubleStream(metrics, "queriesPerSecond").mapToDouble(Double::doubleValue).sum(),
-                doubleStream(metrics, "feedPerSecond").mapToDouble(Double::doubleValue).sum(),
-                doubleStream(metrics, "documentCount").mapToLong(Double::longValue).sum(),
-                doubleStream(metrics, "queryLatency").mapToDouble(Double::doubleValue).sum(),
-                doubleStream(metrics, "feedLatency").mapToDouble(Double::doubleValue).sum()
+                metrics.stream().flatMap(m -> m.queriesPerSecond().stream()).mapToDouble(Double::doubleValue).sum(),
+                metrics.stream().flatMap(m -> m.feedPerSecond().stream()).mapToDouble(Double::doubleValue).sum(),
+                metrics.stream().flatMap(m -> m.documentCount().stream()).mapToLong(Double::longValue).sum(),
+                weightedAverageLatency(metrics, ClusterMetrics::queriesPerSecond, ClusterMetrics::queryLatency),
+                weightedAverageLatency(metrics, ClusterMetrics::feedPerSecond, ClusterMetrics::feedLatency)
         );
     }
 
@@ -62,7 +59,22 @@ public class ConfigServerMetricsService implements MetricsService {
         return Map.of();
     }
 
-    private Stream<Double> doubleStream(List<ClusterMetrics> metrics, String name) {
-        return metrics.stream().map(m -> m.getMetrics().getOrDefault(name, 0.0));
+    private double weightedAverageLatency(List<ClusterMetrics> metrics,
+                                          Function<ClusterMetrics, Optional<Double>> rateExtractor,
+                                          Function<ClusterMetrics, Optional<Double>> latencyExtractor)
+    {
+        var rateSum = metrics.stream().flatMap(m -> rateExtractor.apply(m).stream()).mapToDouble(Double::longValue).sum();
+        if (rateSum == 0) {
+            return 0.0;
+        }
+
+        var weightedLatency = metrics.stream()
+                .flatMap(m -> {
+                    return latencyExtractor.apply(m).flatMap(l -> rateExtractor.apply(m).map(r -> l * r)).stream();
+                })
+                .mapToDouble(Double::doubleValue)
+                .sum();
+
+        return weightedLatency / rateSum;
     }
 }
diff --git a/controller-api/src/test/java/com/yahoo/vespa/hosted/controller/api/integration/metrics/ConfigServerMetricsServiceTest.java b/controller-api/src/test/java/com/yahoo/vespa/hosted/controller/api/integration/metrics/ConfigServerMetricsServiceTest.java
index 9c14bc641c7..01998d634ca 100644
--- a/controller-api/src/test/java/com/yahoo/vespa/hosted/controller/api/integration/metrics/ConfigServerMetricsServiceTest.java
+++ b/controller-api/src/test/java/com/yahoo/vespa/hosted/controller/api/integration/metrics/ConfigServerMetricsServiceTest.java
@@ -60,7 +60,7 @@ public class ConfigServerMetricsServiceTest {
         var deploymentMetrics = service.getDeploymentMetrics(applicationId, zoneId);
 
         assertEquals(23.0 + 11.0, deploymentMetrics.queriesPerSecond(), 0.001);
-        assertEquals(1337.0 + 12.0, deploymentMetrics.queryLatencyMillis(), 0.001); // TODO: again, this definition of combined latency makes no sense
+        assertEquals(908.323, deploymentMetrics.queryLatencyMillis(), 0.001);
         assertEquals(0, deploymentMetrics.documentCount());
         assertEquals(0.0, deploymentMetrics.writeLatencyMillis(), 0.001);
         assertEquals(0.0, deploymentMetrics.writesPerSecond(), 0.001);