Fix a hashing bug in MetricsParser, and use record for Dimension.

- XOR does not work well for comparing two distinct pairs of
  duplicate values.

2 files changed, 42 insertions, 8 deletions

diff --git a/metrics-proxy/src/main/java/ai/vespa/metricsproxy/service/MetricsParser.java b/metrics-proxy/src/main/java/ai/vespa/metricsproxy/service/MetricsParser.java
index ea2b35959f7..00960e630b5 100644
--- a/metrics-proxy/src/main/java/ai/vespa/metricsproxy/service/MetricsParser.java
+++ b/metrics-proxy/src/main/java/ai/vespa/metricsproxy/service/MetricsParser.java
@@ -16,6 +16,7 @@ import java.util.HashMap;
 import java.util.List;
 import java.util.Map;
 import java.util.stream.Collectors;
+import java.util.stream.Stream;
 
 import static ai.vespa.metricsproxy.metric.model.DimensionId.toDimensionId;
 
@@ -142,25 +143,42 @@ public class MetricsParser {
 
     private static Map<DimensionId, String> parseDimensions(JsonParser parser,
                                                             Map<Long, Map<DimensionId, String>> uniqueDimensions) throws IOException {
-        List<Map.Entry<String, String>> dims = new ArrayList<>();
-        int keyHash = 0;
-        int valueHash = 0;
+
+        List<Dimension> dimensions = new ArrayList<>();
+
         for (parser.nextToken(); parser.getCurrentToken() != JsonToken.END_OBJECT; parser.nextToken()) {
             String fieldName = parser.getCurrentName();
             JsonToken token = parser.nextToken();
+
             if (token == JsonToken.VALUE_STRING){
                 String value = parser.getValueAsString();
-                dims.add(Map.entry(fieldName, value));
-                keyHash ^= fieldName.hashCode();
-                valueHash ^= value.hashCode();
+                dimensions.add(Dimension.of(fieldName, value));
             } else if (token == JsonToken.VALUE_NULL) {
                 // TODO Should log a warning if this happens
             } else {
                 throw new IllegalArgumentException("Dimension '" + fieldName + "' must be a string");
             }
         }
-        Long uniqueKey = (((long) keyHash) << 32) | (valueHash & 0xffffffffL);
-        return uniqueDimensions.computeIfAbsent(uniqueKey, key -> dims.stream().collect(Collectors.toUnmodifiableMap(e -> toDimensionId(e.getKey()), Map.Entry::getValue)));
+        return uniqueDimensions.computeIfAbsent(dimensionsHashCode(dimensions),
+                                                key -> dimensions.stream().collect(Collectors.toUnmodifiableMap(
+                                                        dim -> toDimensionId(dim.id), dim -> dim.value)));
+    }
+
+    static long dimensionsHashCode(List<Dimension> dimensions) {
+        int keyHash = 0;
+        int valueHash = 0;
+
+        for (Dimension dim : dimensions) {
+            keyHash += dim.id.hashCode();
+            valueHash += dim.value.hashCode();
+        }
+        return (((long) keyHash) << 32) | (valueHash & 0xffffffffL);
+    }
+
+    record Dimension(String id, String value) {
+        static Dimension of(String id, String value) {
+            return new Dimension(id, value);
+        }
     }
 
     private static List<Map.Entry<String, Number>> parseValues(String prefix, JsonParser parser) throws IOException {
diff --git a/metrics-proxy/src/test/java/ai/vespa/metricsproxy/service/MetricsParserTest.java b/metrics-proxy/src/test/java/ai/vespa/metricsproxy/service/MetricsParserTest.java
index 4fc77aac45f..df83a947565 100644
--- a/metrics-proxy/src/test/java/ai/vespa/metricsproxy/service/MetricsParserTest.java
+++ b/metrics-proxy/src/test/java/ai/vespa/metricsproxy/service/MetricsParserTest.java
@@ -7,6 +7,7 @@ import org.junit.Test;
 import java.util.ArrayList;
 import java.util.List;
 
+import static ai.vespa.metricsproxy.service.MetricsParser.dimensionsHashCode;
 import static org.junit.Assert.assertEquals;
 import static org.junit.Assert.assertNotEquals;
 
@@ -25,6 +26,21 @@ public class MetricsParserTest {
     }
 
     @Test
+    public void dimensions_hashcode_is_different_for_distinct_but_duplicate_dimension_values() {
+        var dimensions1 = List.of(
+                new MetricsParser.Dimension("cluster", "CLUSTER-1"),
+                new MetricsParser.Dimension("clusterid", "CLUSTER-1"));
+
+        var dimensions2 = List.of(
+                new MetricsParser.Dimension("cluster", "CLUSTER-2"),
+                new MetricsParser.Dimension("clusterid", "CLUSTER-2"));
+
+        System.out.println(dimensionsHashCode(dimensions1));
+        System.out.println(dimensionsHashCode(dimensions2));
+        assertNotEquals(dimensionsHashCode(dimensions1), dimensionsHashCode(dimensions2));
+    }
+
+    @Test
     public void different_dimension_values_are_not_treated_as_equal() throws Exception {
         var collector = new MetricsConsumer();
         MetricsParser.parse(metricsJson(), collector);