diff options
author | gjoranv <gv@verizonmedia.com> | 2022-12-16 16:01:28 +0100 |
---|---|---|
committer | gjoranv <gv@verizonmedia.com> | 2022-12-16 16:04:17 +0100 |
commit | 8e0ed067cc0bca39bd0fae8bbb584e9ee4cc09e4 (patch) | |
tree | b422cd7c8414077143d8deaa929aa48b34c2842b /metrics-proxy | |
parent | f86b7f337297a78be031b8fe06c978ee72f68de6 (diff) |
Fix a hashing bug in MetricsParser, and use record for Dimension.
- XOR does not work well for comparing two distinct pairs of
duplicate values.
Diffstat (limited to 'metrics-proxy')
-rw-r--r-- | metrics-proxy/src/main/java/ai/vespa/metricsproxy/service/MetricsParser.java | 34 | ||||
-rw-r--r-- | metrics-proxy/src/test/java/ai/vespa/metricsproxy/service/MetricsParserTest.java | 16 |
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); |