diff options
author | Harald Musum <musum@verizonmedia.com> | 2021-12-19 21:00:41 +0100 |
---|---|---|
committer | GitHub <noreply@github.com> | 2021-12-19 21:00:41 +0100 |
commit | f987d5642c69a0134e1fc8b2b87f0142160a68b0 (patch) | |
tree | a248840c93ff3e23af1299a24d9aba22b4bf0937 | |
parent | 6df122c97a324461194e0ca7d9e71fd1f77f325d (diff) | |
parent | 42bd7c2591c34c578d2a825de8225cdaa0489407 (diff) |
Merge pull request #20582 from vespa-engine/balder/refactor-and-speedup-handlevaluev7.518.53
- Refactor and speedup MetricParser.handleValue.
-rw-r--r-- | metrics-proxy/src/main/java/ai/vespa/metricsproxy/service/MetricsParser.java | 129 | ||||
-rw-r--r-- | metrics-proxy/src/test/java/ai/vespa/metricsproxy/service/MetricsFetcherTest.java | 2 |
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()); } |