Handle that metrics can arrive with a dimension set to null. Skip and log it as debug.

Let exceptions through in the tests so that we do not hide errors there.

3 files changed, 41 insertions, 20 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 f9443c46bad..701af8730d7 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
@@ -14,6 +14,8 @@ import java.io.InputStream;
 import java.util.HashMap;
 import java.util.Iterator;
 import java.util.Map;
+import java.util.logging.Level;
+import java.util.logging.Logger;
 
 import static ai.vespa.metricsproxy.metric.model.DimensionId.toDimensionId;
 
@@ -23,6 +25,7 @@ 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);
     }
@@ -128,15 +131,23 @@ public class MetricsParser {
             StringBuilder sb = new StringBuilder();
             for (Iterator<?> it = dimensions.fieldNames(); it.hasNext(); ) {
                 String k = (String) it.next();
-                String v = dimensions.get(k).asText();
-                sb.append(toDimensionId(k)).append(v);
+                String v = dimensions.get(k).textValue();
+                if (v != null) {
+                    sb.append(toDimensionId(k)).append(v);
+                }
             }
             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();
-                    dim.put(toDimensionId(k), v);
+                    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");
+                    }
                 }
                 uniqueDimensions.put(sb.toString(), Map.copyOf(dim));
             }
diff --git a/metrics-proxy/src/main/java/ai/vespa/metricsproxy/service/RemoteMetricsFetcher.java b/metrics-proxy/src/main/java/ai/vespa/metricsproxy/service/RemoteMetricsFetcher.java
index f375fd1586f..5ca9e6fd950 100644
--- a/metrics-proxy/src/main/java/ai/vespa/metricsproxy/service/RemoteMetricsFetcher.java
+++ b/metrics-proxy/src/main/java/ai/vespa/metricsproxy/service/RemoteMetricsFetcher.java
@@ -28,12 +28,8 @@ public class RemoteMetricsFetcher extends HttpMetricFetcher {
         }
     }
 
-    void createMetrics(String data, MetricsParser.Consumer consumer, int fetchCount) {
-        try {
-            MetricsParser.parse(data, consumer);
-        } catch (Exception e) {
-            handleException(e, data, fetchCount);
-        }
+    void createMetrics(String data, MetricsParser.Consumer consumer, int fetchCount) throws IOException {
+        MetricsParser.parse(data, consumer);
     }
     private void createMetrics(InputStream data, MetricsParser.Consumer consumer, int fetchCount) throws IOException {
         try {
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 b25d3af2c20..56e78e3930c 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
@@ -6,24 +6,27 @@ import ai.vespa.metricsproxy.metric.Metric;
 import ai.vespa.metricsproxy.metric.Metrics;
 import org.junit.Test;
 
+import java.io.IOException;
+
 import static ai.vespa.metricsproxy.metric.model.MetricId.toMetricId;
 import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertNull;
+import static org.junit.Assert.fail;
 
 /**
  */
 public class MetricsFetcherTest {
 
     private static final int port = 9;  //port number is not used in this test
-    private static final double EPSILON = 0.00000000001;
 
-    private class MetricsConsumer implements MetricsParser.Consumer {
+    private static class MetricsConsumer implements MetricsParser.Consumer {
         Metrics metrics = new Metrics();
         @Override
         public void consume(Metric metric) {
             metrics.add(metric);
         }
     }
-    Metrics fetch(String data) {
+    Metrics fetch(String data) throws IOException {
         RemoteMetricsFetcher fetcher = new RemoteMetricsFetcher(new DummyService(0, "dummy/id/0"), port);
         MetricsConsumer consumer = new MetricsConsumer();
         fetcher.createMetrics(data, consumer, 0);
@@ -31,7 +34,7 @@ public class MetricsFetcherTest {
     }
 
     @Test
-    public void testStateFormatMetricsParse() {
+    public void testStateFormatMetricsParse() throws IOException {
         String jsonData = TestUtil.getFileContents("metrics-state.json");
         Metrics metrics = fetch(jsonData);
         assertEquals(10, metrics.size());
@@ -41,20 +44,25 @@ public class MetricsFetcherTest {
     }
 
     @Test
-    public void testEmptyJson() {
+    public void testEmptyJson() throws IOException {
         String  jsonData = "{}";
         Metrics metrics = fetch(jsonData);
         assertEquals(0, metrics.size());
     }
 
     @Test
-    public void testErrors() {
+    public void testErrors() throws IOException {
         String jsonData;
-        Metrics metrics;
+        Metrics metrics = null;
 
         jsonData = "";
-        metrics = fetch(jsonData);
-        assertEquals(0, metrics.size());
+        try {
+            metrics = fetch(jsonData);
+            fail("Should have an IOException instead");
+        } catch (IOException e) {
+            assertEquals("Expected start of object, got null", e.getMessage());
+        }
+        assertNull(metrics);
 
         jsonData = "{\n" +
                 "\"status\" : {\n" +
@@ -91,8 +99,14 @@ public class MetricsFetcherTest {
                 "}\n" +
                 "}";
 
-        metrics = fetch(jsonData);
-        assertEquals(0, metrics.size());
+        metrics = null;
+        try {
+            metrics = fetch(jsonData);
+            fail("Should have an IOException instead");
+        } catch (IllegalArgumentException e) {
+            assertEquals("Value for aggregator 'count' is not a number", e.getMessage());
+        }
+        assertNull(metrics);
     }
 
     public Metric getMetric(String metric, Metrics metrics) {