From 22f954fafad86a2bd04955bda99d1b8f312f9d81 Mon Sep 17 00:00:00 2001 From: Henning Baldersheim Date: Sat, 18 Dec 2021 22:42:11 +0100 Subject: - Refactor and speedup MetricParser.handleValue. - Only use streaming jackson parsing - Aggregate hash codes to make unique dimension keys. --- .../vespa/metricsproxy/service/MetricsParser.java | 125 +++++++++++---------- .../metricsproxy/service/MetricsFetcherTest.java | 2 +- 2 files changed, 66 insertions(+), 61 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..070275ee177 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,17 @@ 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; @@ -67,7 +67,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 +77,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> uniqueDimensions = new HashMap<>(); + Map> 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 +99,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 +108,69 @@ public class MetricsParser { } } - static private void handleValue(JsonNode metric, Instant timestamp, Consumer consumer, - Map> uniqueDimensions) { - String name = metric.get("name").textValue(); - String description = ""; - - if (metric.has("description")) { - description = metric.get("description").textValue(); + private static Map parseDimensions(JsonParser parser, + Map> uniqueDimensions) throws IOException { + List> 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 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> parseValues(String prefix, JsonParser parser) throws IOException { + List> 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> uniqueDimensions) throws IOException { + String name = ""; + String description = ""; + Map dim = Map.of(); + List> 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 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()); } -- cgit v1.2.3 From a68a73a0517982cfe5f477e6133fa1fe5cca03e4 Mon Sep 17 00:00:00 2001 From: Henning Baldersheim Date: Sun, 19 Dec 2021 20:10:33 +0100 Subject: GC unused logger --- .../src/main/java/ai/vespa/metricsproxy/service/MetricsParser.java | 2 -- 1 file changed, 2 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 070275ee177..4f6373b6145 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 @@ -15,7 +15,6 @@ import java.util.ArrayList; import java.util.HashMap; import java.util.List; import java.util.Map; -import java.util.logging.Logger; import java.util.stream.Collectors; import static ai.vespa.metricsproxy.metric.model.DimensionId.toDimensionId; @@ -26,7 +25,6 @@ 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); } -- cgit v1.2.3 From 42bd7c2591c34c578d2a825de8225cdaa0489407 Mon Sep 17 00:00:00 2001 From: Henning Baldersheim Date: Sun, 19 Dec 2021 20:33:13 +0100 Subject: Make parse method public to enable testing. --- .../src/main/java/ai/vespa/metricsproxy/service/MetricsParser.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) 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 4f6373b6145..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 @@ -31,7 +31,7 @@ public class MetricsParser { 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); } -- cgit v1.2.3