Avoid mixing platform and OS version in metric

2 files changed, 23 insertions, 8 deletions

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 85f45ded6bd..15b4da7703a 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
@@ -55,7 +55,7 @@ public class MetricsReporter extends ControllerMaintainer {
     private final Clock clock;
 
     // Tracks hosts and versions for which we have reported change duration metrics
-    private final ConcurrentHashMap<HostName, Set<Version>> reportedVersions = new ConcurrentHashMap<>();
+    private final ConcurrentHashMap<HostName, Map<String, Set<Version>>> reportedVersions = new ConcurrentHashMap<>();
 
     public MetricsReporter(Controller controller, Metric metric) {
         super(controller, Duration.ofMinutes(1)); // use fixed rate for metrics
@@ -115,8 +115,8 @@ public class MetricsReporter extends ControllerMaintainer {
         changeDurations.forEach((nodeVersion, duration) -> {
             // Zero the metric for past versions because our metrics framework remembers the last value for each unique
             // dimension set for each metric.
-            reportVersion(nodeVersion.hostname(), nodeVersion.currentVersion());
-            for (var reportedVersion : reportedVersions.get(nodeVersion.hostname())) {
+            reportVersion(metricName, nodeVersion.hostname(), nodeVersion.currentVersion());
+            for (var reportedVersion : reportedVersions.getOrDefault(nodeVersion.hostname(), Map.of()).get(metricName)) {
                 if (reportedVersion.equals(nodeVersion.currentVersion())) continue;
                 metric.set(metricName, 0, metric.createContext(dimensions(nodeVersion.hostname(), nodeVersion.zone(), reportedVersion)));
             }
@@ -124,12 +124,12 @@ public class MetricsReporter extends ControllerMaintainer {
         });
     }
 
-    private void reportVersion(HostName hostname, Version version) {
+    private void reportVersion(String metricName, HostName hostname, Version version) {
         reportedVersions.compute(hostname, (ignored, values) -> {
             if (values == null) {
-                values = new HashSet<>();
+                values = new HashMap<>();
             }
-            values.add(version);
+            values.computeIfAbsent(metricName, k -> new HashSet<>()).add(version);
             return values;
         });
     }
@@ -214,5 +214,3 @@ public class MetricsReporter extends ControllerMaintainer {
     }
 
 }
-
-
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 4e4c31c519e..8e7f04f6ecc 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
@@ -22,8 +22,10 @@ import org.junit.Test;
 import java.time.Duration;
 import java.util.Comparator;
 import java.util.List;
+import java.util.Set;
 import java.util.function.UnaryOperator;
 import java.util.stream.Collectors;
+import java.util.stream.Stream;
 
 import static com.yahoo.vespa.hosted.controller.api.integration.deployment.JobType.productionUsWest1;
 import static com.yahoo.vespa.hosted.controller.api.integration.deployment.JobType.stagingTest;
@@ -305,6 +307,7 @@ public class MetricsReporterTest {
         assertOsChangeDuration(Duration.ZERO, hosts, version0);
 
         var targets = List.of(Version.fromString("8.1"), Version.fromString("8.2"));
+        var allVersions = Stream.concat(Stream.of(version0), targets.stream()).collect(Collectors.toSet());
         for (int i = 0; i < targets.size(); i++) {
             var currentVersion = i == 0 ? version0 : targets.get(i - 1);
             var version = targets.get(i);
@@ -363,6 +366,20 @@ public class MetricsReporterTest {
             upgradeOsTo(version, remainingHosts, zone, tester);
             runAll(statusUpdater, reporter);
             assertOsChangeDuration(Duration.ZERO, hosts, version);
+
+            // Dimensions used for OS metric are only known OS versions
+            for (var host : hosts) {
+                Set<Version> versionDimensions = metrics.getMetrics((dimensions) -> host.hostname().value().equals(dimensions.get("host")))
+                                                        .entrySet()
+                                                        .stream()
+                                                        .filter(kv -> kv.getValue().containsKey(MetricsReporter.OS_CHANGE_DURATION))
+                                                        .map(kv -> kv.getKey().getDimensions())
+                                                        .map(dimensions -> dimensions.get("currentVersion"))
+                                                        .map(Version::fromString)
+                                                        .collect(Collectors.toSet());
+                assertTrue("Reports only OS versions", allVersions.containsAll(versionDimensions));
+            }
+
         }
     }