Merge pull request #20582 from vespa-engine/balder/refactor-and-speedup-handlevaluev7.518.53

- Refactor and speedup MetricParser.handleValue.

2 files changed, 67 insertions, 64 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 9a601a67eb5..bb21485e0e7 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
@@ -6,17 +6,16 @@ import ai.vespa.metricsproxy.metric.model.DimensionId;
 import ai.vespa.metricsproxy.metric.model.MetricId;
 import com.fasterxml.jackson.core.JsonParser;
 import com.fasterxml.jackson.core.JsonToken;
-import com.fasterxml.jackson.databind.JsonNode;
 import com.fasterxml.jackson.databind.ObjectMapper;
 
 import java.io.IOException;
 import java.io.InputStream;
 import java.time.Instant;
+import java.util.ArrayList;
 import java.util.HashMap;
-import java.util.Iterator;
+import java.util.List;
 import java.util.Map;
-import java.util.logging.Level;
-import java.util.logging.Logger;
+import java.util.stream.Collectors;
 
 import static ai.vespa.metricsproxy.metric.model.DimensionId.toDimensionId;
 
@@ -26,14 +25,13 @@ import static ai.vespa.metricsproxy.metric.model.DimensionId.toDimensionId;
  * @author Jo Kristian Bergum
  */
 public class MetricsParser {
-    private final static Logger log = Logger.getLogger(MetricsParser.class.getName());
     public interface Consumer {
         void consume(Metric metric);
     }
 
     private static final ObjectMapper jsonMapper = new ObjectMapper();
 
-    static void parse(String data, Consumer consumer) throws IOException {
+    public static void parse(String data, Consumer consumer) throws IOException {
         parse(jsonMapper.createParser(data), consumer);
     }
 
@@ -67,7 +65,6 @@ public class MetricsParser {
             JsonToken token = parser.nextToken();
             if (fieldName.equals("to")) {
                 timestamp = Instant.ofEpochSecond(parser.getLongValue());
-                long now = System.currentTimeMillis() / 1000;
                 timestamp = Instant.ofEpochSecond(Metric.adjustTime(timestamp.getEpochSecond(), Instant.now().getEpochSecond()));
             } else {
                 if (token == JsonToken.START_OBJECT || token == JsonToken.START_ARRAY) {
@@ -78,19 +75,14 @@ public class MetricsParser {
         return timestamp;
     }
 
-    static private void parseValues(JsonParser parser, Instant timestamp, Consumer consumer) throws IOException {
+    static private void parseMetricValues(JsonParser parser, Instant timestamp, Consumer consumer) throws IOException {
         if (parser.getCurrentToken() != JsonToken.START_ARRAY) {
             throw new IOException("Expected start of 'metrics:values' array, got " + parser.currentToken());
         }
 
-        Map<String, Map<DimensionId, String>> uniqueDimensions = new HashMap<>();
+        Map<Long, Map<DimensionId, String>> uniqueDimensions = new HashMap<>();
         while (parser.nextToken() == JsonToken.START_OBJECT) {
-            // read everything from this START_OBJECT to the matching END_OBJECT
-            // and return it as a tree model ObjectNode
-            JsonNode value = jsonMapper.readTree(parser);
-            handleValue(value, timestamp, consumer, uniqueDimensions);
-
-            // do whatever you need to do with this object
+            handleValue(parser, timestamp, consumer, uniqueDimensions);
         }
     }
 
@@ -105,7 +97,7 @@ public class MetricsParser {
             if (fieldName.equals("snapshot")) {
                 timestamp = parseSnapshot(parser);
             } else if (fieldName.equals("values")) {
-                parseValues(parser, timestamp, consumer);
+                parseMetricValues(parser, timestamp, consumer);
             } else {
                 if (token == JsonToken.START_OBJECT || token == JsonToken.START_ARRAY) {
                     parser.skipChildren();
@@ -114,58 +106,69 @@ public class MetricsParser {
         }
     }
 
-    static private void handleValue(JsonNode metric, Instant timestamp, Consumer consumer,
-                                    Map<String, Map<DimensionId, String>> uniqueDimensions) {
-        String name = metric.get("name").textValue();
-        String description = "";
-
-        if (metric.has("description")) {
-            description = metric.get("description").textValue();
+    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;
+        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();
+            } else if (token == JsonToken.VALUE_NULL) {
+                // TODO Should log a warning if this happens
+            } else {
+                throw new IllegalArgumentException("Dimension '" + fieldName + "' must be a string");
+            }
         }
-
-        Map<DimensionId, String> dim = Map.of();
-        if (metric.has("dimensions")) {
-            JsonNode dimensions = metric.get("dimensions");
-            StringBuilder sb = new StringBuilder();
-            for (Iterator<?> it = dimensions.fieldNames(); it.hasNext(); ) {
-                String k = (String) it.next();
-                String v = dimensions.get(k).textValue();
-                if (v != null) {
-                    sb.append(toDimensionId(k)).append(v);
-                }
+        Long uniqueKey = (((long) keyHash) << 32) | (valueHash & 0xffffffffL);
+        return uniqueDimensions.computeIfAbsent(uniqueKey, key -> dims.stream().collect(Collectors.toUnmodifiableMap(e -> toDimensionId(e.getKey()), Map.Entry::getValue)));
+    }
+    private static List<Map.Entry<String, Number>> parseValues(String prefix, JsonParser parser) throws IOException {
+        List<Map.Entry<String, Number>> metrics = new ArrayList<>();
+        for (parser.nextToken(); parser.getCurrentToken() != JsonToken.END_OBJECT; parser.nextToken()) {
+            String fieldName = parser.getCurrentName();
+            JsonToken token = parser.nextToken();
+            String metricName = prefix + fieldName;
+            if (token == JsonToken.VALUE_NUMBER_INT) {
+                metrics.add(Map.entry(metricName, parser.getLongValue()));
+            } else if (token == JsonToken.VALUE_NUMBER_FLOAT) {
+                metrics.add(Map.entry(metricName, parser.getValueAsDouble()));
+            } else {
+                throw new IllegalArgumentException("Value for aggregator '" + fieldName + "' is not a number");
             }
-            if ( ! uniqueDimensions.containsKey(sb.toString())) {
-                dim = new HashMap<>();
-                for (Iterator<?> it = dimensions.fieldNames(); it.hasNext(); ) {
-                    String k = (String) it.next();
-                    String v = dimensions.get(k).textValue();
-                    if (v != null) {
-                        dim.put(toDimensionId(k), v);
-                    } else {
-                        // TODO This should never happen, but it has been seen. This should be flagged as warning,
-                        // but will try to find root cause before flooding the log.
-                        log.log(Level.FINE, "Metric '" + name + "': dimension '" + k + "' is null");
-                    }
+        }
+        return metrics;
+    }
+    static private void handleValue(JsonParser parser, Instant timestamp, Consumer consumer,
+                                    Map<Long, Map<DimensionId, String>> uniqueDimensions) throws IOException {
+        String name = "";
+        String description = "";
+        Map<DimensionId, String> dim = Map.of();
+        List<Map.Entry<String, Number>> values = List.of();
+        for (parser.nextToken(); parser.getCurrentToken() != JsonToken.END_OBJECT; parser.nextToken()) {
+            String fieldName = parser.getCurrentName();
+            JsonToken token = parser.nextToken();
+            if (fieldName.equals("name")) {
+                name = parser.getText();
+            } else if (fieldName.equals("description")) {
+                description = parser.getText();
+            } else if (fieldName.equals("dimensions")) {
+                dim = parseDimensions(parser, uniqueDimensions);
+            } else if (fieldName.equals("values")) {
+                values = parseValues(name+".", parser);
+            } else {
+                if (token == JsonToken.START_OBJECT || token == JsonToken.START_ARRAY) {
+                    parser.skipChildren();
                 }
-                uniqueDimensions.put(sb.toString(), Map.copyOf(dim));
             }
-            dim = uniqueDimensions.get(sb.toString());
         }
-
-        JsonNode aggregates = metric.get("values");
-        String prefix = name + ".";
-        for (Iterator<?> it = aggregates.fieldNames(); it.hasNext(); ) {
-            String aggregator = (String) it.next();
-            JsonNode aggregatorValue = aggregates.get(aggregator);
-            if (aggregatorValue == null) {
-                throw new IllegalArgumentException("Value for aggregator '" + aggregator + "' is missing");
-            }
-            Number value = aggregatorValue.numberValue();
-            if (value == null) {
-                throw new IllegalArgumentException("Value for aggregator '" + aggregator + "' is not a number");
-            }
-            String metricName = prefix + aggregator;
-            consumer.consume(new Metric(MetricId.toMetricId(metricName), value, timestamp, dim, description));
+        for (Map.Entry<String, Number> value : values) {
+            consumer.consume(new Metric(MetricId.toMetricId(value.getKey()), value.getValue(), timestamp, dim, description));
         }
     }
 }
diff --git a/metrics-proxy/src/test/java/ai/vespa/metricsproxy/service/MetricsFetcherTest.java b/metrics-proxy/src/test/java/ai/vespa/metricsproxy/service/MetricsFetcherTest.java
index b20126ea23f..5d4512276d4 100644
--- a/metrics-proxy/src/test/java/ai/vespa/metricsproxy/service/MetricsFetcherTest.java
+++ b/metrics-proxy/src/test/java/ai/vespa/metricsproxy/service/MetricsFetcherTest.java
@@ -40,7 +40,7 @@ public class MetricsFetcherTest {
         String jsonData = TestUtil.getFileContents("metrics-state.json");
         Metrics metrics = fetch(jsonData);
         assertEquals(10, metrics.size());
-        assertEquals(28, getMetric("query_hits.count", metrics).getValue());
+        assertEquals(28L, getMetric("query_hits.count", metrics).getValue());
         assertEquals(0.4667, getMetric("queries.rate", metrics).getValue());
         assertEquals(Instant.ofEpochSecond(1334134700L), metrics.getTimeStamp());
     }