Merge pull request #13154 from vespa-engine/mpolden/version-dimension

Add currentVersion dimension to change duration metric

4 files changed, 22 insertions, 16 deletions

diff --git a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/Controller.java b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/Controller.java
index f46b40b7668..297f3fcf218 100644
--- a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/Controller.java
+++ b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/Controller.java
@@ -231,7 +231,7 @@ public class Controller extends AbstractComponent {
                 Set<Version> newVersions = newStatus.versionsIn(cloud);
                 if (currentStatus.versionsIn(cloud).size() > 1 && newVersions.size() == 1) {
                     log.info("All nodes in " + cloud + " cloud upgraded to OS version " +
-                             newVersions.iterator().next());
+                             newVersions.iterator().next().toFullString());
                 }
             }
             curator.writeOsVersionStatus(newStatus);
diff --git a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/maintenance/MetricsReporter.java b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/maintenance/MetricsReporter.java
index a745cf47b42..95b1066b7c9 100644
--- a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/maintenance/MetricsReporter.java
+++ b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/maintenance/MetricsReporter.java
@@ -1,7 +1,6 @@
 // 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.maintenance;
 
-import com.yahoo.concurrent.maintenance.JobControl;
 import com.yahoo.config.provision.ApplicationId;
 import com.yahoo.jdisc.Metric;
 import com.yahoo.vespa.hosted.controller.Application;
@@ -168,7 +167,7 @@ public class MetricsReporter extends ControllerMaintainer {
     private static Map<ApplicationId, Integer> deploymentWarnings(DeploymentStatusList statuses) {
         return statuses.asList().stream()
                        .flatMap(status -> status.application().instances().values().stream())
-                        .collect(Collectors.toMap(Instance::id, a -> maxWarningCountOf(a.deployments().values())));
+                       .collect(Collectors.toMap(Instance::id, a -> maxWarningCountOf(a.deployments().values())));
     }
 
     private static int maxWarningCountOf(Collection<Deployment> deployments) {
@@ -188,7 +187,8 @@ public class MetricsReporter extends ControllerMaintainer {
 
     private static Map<String, String> dimensions(NodeVersion nodeVersion) {
         return Map.of("host", nodeVersion.hostname().value(),
-                      "zone", nodeVersion.zone().value());
+                      "zone", nodeVersion.zone().value(),
+                      "currentVersion", nodeVersion.currentVersion().toFullString());
     }
 
 }
diff --git a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/integration/MetricsMock.java b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/integration/MetricsMock.java
index e2f598340be..61c98258549 100644
--- a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/integration/MetricsMock.java
+++ b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/integration/MetricsMock.java
@@ -6,6 +6,8 @@ import com.yahoo.jdisc.Metric;
 
 import java.util.Collections;
 import java.util.HashMap;
+import java.util.LinkedHashMap;
+import java.util.List;
 import java.util.Map;
 import java.util.Objects;
 import java.util.Optional;
@@ -17,7 +19,7 @@ import java.util.stream.Collectors;
  */
 public class MetricsMock implements Metric {
 
-    private final Map<Context, Map<String, Number>> metrics = new HashMap<>();
+    private final LinkedHashMap<Context, Map<String, Number>> metrics = new LinkedHashMap<>();
 
     @Override
     public void set(String key, Number val, Context ctx) {
@@ -56,16 +58,20 @@ public class MetricsMock implements Metric {
         return metrics.entrySet()
                       .stream()
                       .filter(context -> dimensionMatcher.test(((MapContext) context.getKey()).getDimensions()))
-                      .collect(Collectors.toMap(entry -> (MapContext) entry.getKey(), Map.Entry::getValue));
+                      .collect(Collectors.toMap(kv -> (MapContext) kv.getKey(),
+                                                Map.Entry::getValue,
+                                                (v1, v2) -> { throw new IllegalStateException("Duplicate keys for values '" + v1 + "' and '" + v2 + "'."); },
+                                                LinkedHashMap::new));
     }
 
-    /** Returns metric filtered by dimension and name */
+    /** Returns the most recently added metric matching given dimension and name */
     public Optional<Number> getMetric(Predicate<Map<String, String>> dimensionMatcher, String name) {
-        Map<String, Number> metrics = getMetrics(dimensionMatcher).values()
-                                                                  .stream()
-                                                                  .findFirst()
-                                                                  .orElseGet(Collections::emptyMap);
-        return Optional.ofNullable(metrics.get(name));
+        var metrics = List.copyOf(getMetrics(dimensionMatcher).values());
+        for (int i = metrics.size() - 1; i >= 0; i--) {
+            var metric = metrics.get(i).get(name);
+            if (metric != null) return Optional.of(metric);
+        }
+        return Optional.empty();
     }
 
     public static class MapContext implements Context {
diff --git a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/maintenance/MetricsReporterTest.java b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/maintenance/MetricsReporterTest.java
index 3c60d467342..31f25cd17ca 100644
--- a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/maintenance/MetricsReporterTest.java
+++ b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/maintenance/MetricsReporterTest.java
@@ -226,7 +226,7 @@ public class MetricsReporterTest {
     }
 
     @Test
-    public void nodes_failing_system_upgrade() {
+    public void platform_change_duration() {
         var tester = new ControllerTester();
         var reporter = createReporter(tester.controller());
         var zone = ZoneId.from("prod.eu-west-1");
@@ -279,7 +279,7 @@ public class MetricsReporterTest {
     }
 
     @Test
-    public void nodes_failing_os_upgrade() {
+    public void os_change_duration() {
         var tester = new ControllerTester();
         var reporter = createReporter(tester.controller());
         var zone = ZoneId.from("prod.eu-west-1");
@@ -420,13 +420,13 @@ public class MetricsReporterTest {
 
     private void assertPlatformChangeDuration(Duration duration, List<Node> nodes) {
         for (var node : nodes) {
-            assertEquals(duration, getChangeDuration(MetricsReporter.PLATFORM_CHANGE_DURATION, node.hostname()));
+            assertEquals("Platform change duration of " + node.hostname(), duration, getChangeDuration(MetricsReporter.PLATFORM_CHANGE_DURATION, node.hostname()));
         }
     }
 
     private void assertOsChangeDuration(Duration duration, List<Node> nodes) {
         for (var node : nodes) {
-            assertEquals(duration, getChangeDuration(MetricsReporter.OS_CHANGE_DURATION, node.hostname()));
+            assertEquals("OS change duration of " + node.hostname(), duration, getChangeDuration(MetricsReporter.OS_CHANGE_DURATION, node.hostname()));
         }
     }