From 280f17e3d62cc1973fc14c0fefdc19c7e6fdc85b Mon Sep 17 00:00:00 2001
From: gjoranv <gv@verizonmedia.com>
Date: Wed, 12 Feb 2020 12:07:57 +0100
Subject: Add test for public default metric set and non-existent set.

+ Fix bug that allowed adding an unknown (null) metric set.
---
 .../monitoring/builder/xml/MetricsBuilder.java     |  7 +++-
 .../MetricsProxyContainerClusterTest.java          | 43 ++++++++++++++++++++++
 2 files changed, 49 insertions(+), 1 deletion(-)

diff --git a/config-model/src/main/java/com/yahoo/vespa/model/admin/monitoring/builder/xml/MetricsBuilder.java b/config-model/src/main/java/com/yahoo/vespa/model/admin/monitoring/builder/xml/MetricsBuilder.java
index b13fa4917e4..f029dad01a9 100644
--- a/config-model/src/main/java/com/yahoo/vespa/model/admin/monitoring/builder/xml/MetricsBuilder.java
+++ b/config-model/src/main/java/com/yahoo/vespa/model/admin/monitoring/builder/xml/MetricsBuilder.java
@@ -62,7 +62,7 @@ public class MetricsBuilder {
                 .collect(Collectors.toCollection(LinkedList::new));
 
         List<MetricSet> metricSets = XML.getChildren(consumerElement, "metric-set").stream()
-                .map(metricSetElement -> availableMetricSets.get(metricSetElement.getAttribute(ID_ATTRIBUTE)))
+                .map(metricSetElement -> getMetricSetOrThrow(metricSetElement.getAttribute(ID_ATTRIBUTE)))
                 .collect(Collectors.toCollection(LinkedList::new));
 
         metricSets.add(defaultVespaMetricSet);
@@ -75,6 +75,11 @@ public class MetricsBuilder {
         return "user-metrics-" + consumerName;
     }
 
+    private MetricSet getMetricSetOrThrow(String id) {
+        if (! availableMetricSets.containsKey(id)) throw new IllegalArgumentException("No such metric-set: " + id);
+        return availableMetricSets.get(id);
+    }
+
     private void throwIfIllegalConsumerId(Metrics metrics, String consumerId) {
         if (consumerId.equalsIgnoreCase(VESPA_CONSUMER_ID) && applicationType != ApplicationType.HOSTED_INFRASTRUCTURE)
             throw new IllegalArgumentException("'Vespa' is not allowed as metrics consumer id (case is ignored.)");
diff --git a/config-model/src/test/java/com/yahoo/vespa/model/admin/metricsproxy/MetricsProxyContainerClusterTest.java b/config-model/src/test/java/com/yahoo/vespa/model/admin/metricsproxy/MetricsProxyContainerClusterTest.java
index de40e557265..9265e4437f1 100644
--- a/config-model/src/test/java/com/yahoo/vespa/model/admin/metricsproxy/MetricsProxyContainerClusterTest.java
+++ b/config-model/src/test/java/com/yahoo/vespa/model/admin/metricsproxy/MetricsProxyContainerClusterTest.java
@@ -67,6 +67,7 @@ import static org.junit.Assert.assertTrue;
  */
 public class MetricsProxyContainerClusterTest {
 
+    private static int numPublicDefaultMetrics = defaultPublicMetricSet.getMetrics().size();
     private static int numDefaultVespaMetrics = defaultVespaMetricSet.getMetrics().size();
     private static int numVespaMetrics = vespaMetricSet.getMetrics().size();
     private static int numSystemMetrics = systemMetricSet.getMetrics().size();
@@ -237,6 +238,25 @@ public class MetricsProxyContainerClusterTest {
         consumersConfigFromXml(services, self_hosted);
     }
 
+    @Test
+    public void non_existent_metric_set_causes_exception() {
+        String services = String.join("\n",
+                                      "<services>",
+                                      "    <admin version='2.0'>",
+                                      "        <adminserver hostalias='node1'/>",
+                                      "        <metrics>",
+                                      "            <consumer id='consumer-with-non-existent-default-set'>",
+                                      "                <metric-set id='non-existent'/>",
+                                      "            </consumer>",
+                                      "        </metrics>",
+                                      "    </admin>",
+                                      "</services>"
+        );
+        thrown.expect(IllegalArgumentException.class);
+        thrown.expectMessage("No such metric-set: non-existent");
+        consumersConfigFromXml(services, self_hosted);
+    }
+
     @Test
     public void consumer_with_no_metric_set_has_its_own_metrics_plus_system_metrics_plus_default_vespa_metrics() {
         String services = String.join("\n",
@@ -262,6 +282,29 @@ public class MetricsProxyContainerClusterTest {
         assertTrue("Did not contain metric: " + customMetric2, checkMetric(consumer, customMetric2));
     }
 
+    @Test
+    public void consumer_with_default_public_metric_set_has_all_public_metrics_plus_all_system_metrics_plus_its_own() {
+        String services = String.join("\n",
+                                      "<services>",
+                                      "    <admin version='2.0'>",
+                                      "        <adminserver hostalias='node1'/>",
+                                      "        <metrics>",
+                                      "            <consumer id='consumer-with-public-default-set'>",
+                                      "                <metric-set id='public'/>",
+                                      "                <metric id='custom.metric'/>",
+                                      "            </consumer>",
+                                      "        </metrics>",
+                                      "    </admin>",
+                                      "</services>"
+        );
+        ConsumersConfig.Consumer consumer = getCustomConsumer(services);
+
+        assertEquals(numPublicDefaultMetrics + numSystemMetrics + 1, consumer.metric().size());
+
+        Metric customMetric = new Metric("custom.metric");
+        assertTrue("Did not contain metric: " + customMetric, checkMetric(consumer, customMetric));
+    }
+
     @Test
     public void consumer_with_vespa_metric_set_has_all_vespa_metrics_plus_all_system_metrics_plus_its_own() {
         String services = String.join("\n",
-- 
cgit v1.2.3