Revert "Revert "Gjoranv/add default public metrics consumer" MERGEOK"

5 files changed, 138 insertions, 36 deletions

diff --git a/metrics-proxy/src/main/java/ai/vespa/metricsproxy/core/VespaMetrics.java b/metrics-proxy/src/main/java/ai/vespa/metricsproxy/core/VespaMetrics.java
index 2ca24dad1e2..ab9f4f6c9c4 100644
--- a/metrics-proxy/src/main/java/ai/vespa/metricsproxy/core/VespaMetrics.java
+++ b/metrics-proxy/src/main/java/ai/vespa/metricsproxy/core/VespaMetrics.java
@@ -117,7 +117,8 @@ public class VespaMetrics {
                 .statusCode(health.getStatus().ordinal())  // TODO: MetricsPacket should use StatusCode instead of int
                 .statusMessage(health.getMessage())
                 .putDimensions(service.getDimensions())
-                .putDimension(INSTANCE_DIMENSION_ID, service.getInstanceName());
+                .putDimension(INSTANCE_DIMENSION_ID, service.getInstanceName())
+                .addConsumers(metricsConsumers.getAllConsumers());
     }
 
     /**
diff --git a/metrics-proxy/src/main/java/ai/vespa/metricsproxy/http/GenericMetricsHandler.java b/metrics-proxy/src/main/java/ai/vespa/metricsproxy/http/GenericMetricsHandler.java
index cadfc053b94..f61a96917a9 100644
--- a/metrics-proxy/src/main/java/ai/vespa/metricsproxy/http/GenericMetricsHandler.java
+++ b/metrics-proxy/src/main/java/ai/vespa/metricsproxy/http/GenericMetricsHandler.java
@@ -4,7 +4,9 @@
 
 package ai.vespa.metricsproxy.http;
 
+import ai.vespa.metricsproxy.core.MetricsConsumers;
 import ai.vespa.metricsproxy.core.MetricsManager;
+import ai.vespa.metricsproxy.metric.model.ConsumerId;
 import ai.vespa.metricsproxy.metric.model.MetricsPacket;
 import ai.vespa.metricsproxy.metric.model.json.JsonRenderingException;
 import ai.vespa.metricsproxy.service.VespaServices;
@@ -19,22 +21,33 @@ import java.nio.charset.Charset;
 import java.time.Instant;
 import java.util.List;
 import java.util.concurrent.Executor;
+import java.util.logging.Logger;
+import java.util.stream.Collectors;
 
+import static ai.vespa.metricsproxy.metric.model.ConsumerId.toConsumerId;
 import static ai.vespa.metricsproxy.metric.model.json.GenericJsonUtil.toGenericJsonModel;
 
 /**
- * Handler exposing the generic metrics format via http.
+ * Http handler that exposes the generic metrics format.
  *
  * @author gjoranv
  */
 public class GenericMetricsHandler extends ThreadedHttpRequestHandler {
+    private static final Logger log = Logger.getLogger(GenericMetricsHandler.class.getName());
 
+    public static final ConsumerId DEFAULT_PUBLIC_CONSUMER_ID = toConsumerId("default");
+
+    private final MetricsConsumers metricsConsumers;
     private final MetricsManager metricsManager;
     private final VespaServices vespaServices;
 
     @Inject
-    public GenericMetricsHandler(Executor executor, MetricsManager metricsManager, VespaServices vespaServices) {
+    public GenericMetricsHandler(Executor executor,
+                                 MetricsManager metricsManager,
+                                 VespaServices vespaServices,
+                                 MetricsConsumers metricsConsumers) {
         super(executor);
+        this.metricsConsumers = metricsConsumers;
         this.metricsManager = metricsManager;
         this.vespaServices = vespaServices;
     }
@@ -42,13 +55,29 @@ public class GenericMetricsHandler extends ThreadedHttpRequestHandler {
     @Override
     public HttpResponse handle(HttpRequest request) {
         try {
-            List<MetricsPacket> metrics = metricsManager.getMetrics(vespaServices.getVespaServices(), Instant.now());
+            ConsumerId consumer = getConsumerOrDefault(request.getProperty("consumer"));
+
+            List<MetricsPacket> metrics = metricsManager.getMetrics(vespaServices.getVespaServices(), Instant.now())
+                    .stream()
+                    .filter(metricsPacket -> metricsPacket.consumers().contains(consumer))
+                    .collect(Collectors.toList());
             return new Response(200, toGenericJsonModel(metrics).serialize());
         } catch (JsonRenderingException e) {
             return new Response(500, e.getMessageAsJson());
         }
     }
 
+    private ConsumerId getConsumerOrDefault(String consumer) {
+        if (consumer == null) return DEFAULT_PUBLIC_CONSUMER_ID;
+
+        ConsumerId consumerId = toConsumerId(consumer);
+        if (! metricsConsumers.getAllConsumers().contains(consumerId)) {
+            log.info("No consumer with id '" + consumer + "' - using the default consumer instead.");
+            return DEFAULT_PUBLIC_CONSUMER_ID;
+        }
+        return consumerId;
+    }
+
     private static class Response extends HttpResponse {
         private final byte[] data;
 
diff --git a/metrics-proxy/src/main/java/ai/vespa/metricsproxy/metric/ExternalMetrics.java b/metrics-proxy/src/main/java/ai/vespa/metricsproxy/metric/ExternalMetrics.java
index 26ae177d767..64ede137e8e 100644
--- a/metrics-proxy/src/main/java/ai/vespa/metricsproxy/metric/ExternalMetrics.java
+++ b/metrics-proxy/src/main/java/ai/vespa/metricsproxy/metric/ExternalMetrics.java
@@ -53,6 +53,8 @@ public class ExternalMetrics {
     }
 
     public void setExtraMetrics(List<MetricsPacket.Builder> externalPackets) {
+        // TODO: Metrics filtering per consumer is not yet implemented.
+        //       Split each packet per metric, and re-aggregate based on the metrics each consumer wants. Then filter out all packages with no consumers.
         log.log(DEBUG, () -> "Setting new external metrics with " + externalPackets.size() + " metrics packets.");
         externalPackets.forEach(packet -> {
             packet.addConsumers(consumers.getAllConsumers())
diff --git a/metrics-proxy/src/test/java/ai/vespa/metricsproxy/core/MetricsManagerTest.java b/metrics-proxy/src/test/java/ai/vespa/metricsproxy/core/MetricsManagerTest.java
index eb620fd37be..e441c353292 100644
--- a/metrics-proxy/src/test/java/ai/vespa/metricsproxy/core/MetricsManagerTest.java
+++ b/metrics-proxy/src/test/java/ai/vespa/metricsproxy/core/MetricsManagerTest.java
@@ -140,6 +140,7 @@ public class MetricsManagerTest {
         service0.setSystemMetrics(oldSystemMetrics);
     }
 
+    // TODO: test that non-whitelisted metrics are filtered out, but this is currently not the case, see ExternalMetrics.setExtraMetrics
     @Test
     public void extra_metrics_packets_containing_whitelisted_metrics_are_added() {
         metricsManager.setExtraMetrics(ImmutableList.of(
diff --git a/metrics-proxy/src/test/java/ai/vespa/metricsproxy/http/GenericMetricsHandlerTest.java b/metrics-proxy/src/test/java/ai/vespa/metricsproxy/http/GenericMetricsHandlerTest.java
index 301dbf56c3f..29ab8c66694 100644
--- a/metrics-proxy/src/test/java/ai/vespa/metricsproxy/http/GenericMetricsHandlerTest.java
+++ b/metrics-proxy/src/test/java/ai/vespa/metricsproxy/http/GenericMetricsHandlerTest.java
@@ -6,6 +6,7 @@ package ai.vespa.metricsproxy.http;
 
 import ai.vespa.metricsproxy.TestUtil;
 import ai.vespa.metricsproxy.core.ConsumersConfig;
+import ai.vespa.metricsproxy.core.ConsumersConfig.Consumer;
 import ai.vespa.metricsproxy.core.MetricsConsumers;
 import ai.vespa.metricsproxy.core.MetricsManager;
 import ai.vespa.metricsproxy.metric.HealthMetric;
@@ -29,12 +30,13 @@ import org.junit.BeforeClass;
 import org.junit.Ignore;
 import org.junit.Test;
 
+import java.io.IOException;
 import java.time.Instant;
 import java.util.List;
 import java.util.concurrent.Executors;
 
 import static ai.vespa.metricsproxy.core.VespaMetrics.INSTANCE_DIMENSION_ID;
-import static ai.vespa.metricsproxy.core.VespaMetrics.VESPA_CONSUMER_ID;
+import static ai.vespa.metricsproxy.http.GenericMetricsHandler.DEFAULT_PUBLIC_CONSUMER_ID;
 import static ai.vespa.metricsproxy.metric.model.ServiceId.toServiceId;
 import static ai.vespa.metricsproxy.metric.model.StatusCode.DOWN;
 import static ai.vespa.metricsproxy.metric.model.json.JacksonUtil.createObjectMapper;
@@ -43,6 +45,7 @@ import static org.junit.Assert.assertEquals;
 import static org.junit.Assert.assertFalse;
 import static org.junit.Assert.assertNotEquals;
 import static org.junit.Assert.assertNotNull;
+import static org.junit.Assert.fail;
 
 /**
  * @author gjoranv
@@ -55,38 +58,69 @@ public class GenericMetricsHandlerTest {
             new DummyService(1, ""),
             new DownService(HealthMetric.getDown("No response")));
 
+    private static final VespaServices vespaServices = new VespaServices(testServices);
+
+    private static final String DEFAULT_CONSUMER = "default";
+    private static final String CUSTOM_CONSUMER = "custom-consumer";
+
     private static final String CPU_METRIC = "cpu";
 
     private static final String URI = "http://localhost/metrics/v1/values";
 
-    private static final VespaServices vespaServices = new VespaServices(testServices);
 
     private static RequestHandlerTestDriver testDriver;
 
     @BeforeClass
-    public static void setupMetricsManager() {
+    public static void setup() {
         MetricsManager metricsManager = TestUtil.createMetricsManager(vespaServices, getMetricsConsumers(), getApplicationDimensions(), getNodeDimensions());
         metricsManager.setExtraMetrics(ImmutableList.of(
                 new MetricsPacket.Builder(toServiceId("foo"))
                         .timestamp(Instant.now().getEpochSecond())
                         .putMetrics(ImmutableList.of(new Metric(CPU_METRIC, 12.345)))));
-        GenericMetricsHandler handler = new GenericMetricsHandler(Executors.newSingleThreadExecutor(), metricsManager, vespaServices);
+        GenericMetricsHandler handler = new GenericMetricsHandler(Executors.newSingleThreadExecutor(), metricsManager, vespaServices, getMetricsConsumers());
         testDriver = new RequestHandlerTestDriver(handler);
     }
 
+    private GenericJsonModel getResponseAsJsonModel(String consumer) {
+        String response = testDriver.sendRequest(URI + "?consumer=" + consumer).readAll();
+        try {
+            return createObjectMapper().readValue(response, GenericJsonModel.class);
+        } catch (IOException e) {
+            fail("Failed to create json model: " + e.getMessage());
+            throw new RuntimeException(e);
+        }
+    }
+
     @Ignore
     @Test
     public void visually_inspect_response() throws Exception{
-        String response = testDriver.sendRequest(URI).readAll();
+        String response = testDriver.sendRequest(URI+ "?consumer=default").readAll();
         ObjectMapper mapper = createObjectMapper();
         var jsonModel = mapper.readValue(response, GenericJsonModel.class);
         System.out.println(mapper.writerWithDefaultPrettyPrinter().writeValueAsString(jsonModel));
     }
 
     @Test
-    public void response_contains_node_metrics() throws Exception {
+    public void no_explicit_consumer_gives_the_default_consumer() {
+        String responseDefaultConsumer = testDriver.sendRequest(URI + "?consumer=default").readAll();
+        String responseNoConsumer = testDriver.sendRequest(URI).readAll();
+        assertEqualsExceptTimestamps(responseDefaultConsumer, responseNoConsumer);
+    }
+
+    @Test
+    public void unknown_consumer_gives_the_default_consumer() {
         String response = testDriver.sendRequest(URI).readAll();
-        var jsonModel = createObjectMapper().readValue(response, GenericJsonModel.class);
+        String responseUnknownConsumer = testDriver.sendRequest(URI + "?consumer=not_defined").readAll();
+        assertEqualsExceptTimestamps(response, responseUnknownConsumer);
+    }
+
+    private void assertEqualsExceptTimestamps(String s1, String s2) {
+        assertEquals(replaceTimestamps(s1), replaceTimestamps(s2));
+    }
+
+    @Test
+    public void response_contains_node_metrics() {
+        GenericJsonModel jsonModel = getResponseAsJsonModel(DEFAULT_CONSUMER);
 
         assertNotNull(jsonModel.node);
         assertEquals(1, jsonModel.node.metrics.size());
@@ -94,9 +128,8 @@ public class GenericMetricsHandlerTest {
     }
 
     @Test
-    public void response_contains_service_metrics() throws Exception {
-        String response = testDriver.sendRequest(URI).readAll();
-        var jsonModel = createObjectMapper().readValue(response, GenericJsonModel.class);
+    public void response_contains_service_metrics() {
+        GenericJsonModel jsonModel = getResponseAsJsonModel(DEFAULT_CONSUMER);
 
         assertEquals(2, jsonModel.services.size());
         GenericService dummyService = jsonModel.services.get(0);
@@ -104,17 +137,50 @@ public class GenericMetricsHandlerTest {
 
         GenericMetrics dummy0Metrics = getMetricsForInstance("dummy0", dummyService);
         assertEquals(1L, dummy0Metrics.values.get(METRIC_1).longValue());
-        assertEquals("metric-dim", dummy0Metrics.dimensions.get("dim0"));
+        assertEquals("default-val", dummy0Metrics.dimensions.get("consumer-dim"));
 
         GenericMetrics dummy1Metrics = getMetricsForInstance("dummy1", dummyService);
         assertEquals(6L, dummy1Metrics.values.get(METRIC_1).longValue());
-        assertEquals("metric-dim", dummy1Metrics.dimensions.get("dim0"));
+        assertEquals("default-val", dummy1Metrics.dimensions.get("consumer-dim"));
     }
 
     @Test
-    public void response_contains_health_from_service_that_is_down() throws Exception {
-        String response = testDriver.sendRequest(URI).readAll();
-        var jsonModel = createObjectMapper().readValue(response, GenericJsonModel.class);
+    public void all_consumers_get_health_from_service_that_is_down() {
+        assertDownServiceHealth(DEFAULT_CONSUMER);
+        assertDownServiceHealth(CUSTOM_CONSUMER);
+    }
+
+    @Test
+    public void all_timestamps_are_equal_and_non_zero() {
+        GenericJsonModel jsonModel = getResponseAsJsonModel(DEFAULT_CONSUMER);
+
+        Long nodeTimestamp = jsonModel.node.timestamp;
+        assertNotEquals(0L, (long) nodeTimestamp);
+        for (var service : jsonModel.services)
+            assertEquals(nodeTimestamp, service.timestamp);
+    }
+
+    @Test
+    public void custom_consumer_gets_only_its_whitelisted_metrics() {
+        GenericJsonModel jsonModel = getResponseAsJsonModel(CUSTOM_CONSUMER);
+
+        assertNotNull(jsonModel.node);
+        // TODO: see comment in ExternalMetrics.setExtraMetrics
+        // assertEquals(0, jsonModel.node.metrics.size());
+
+        assertEquals(2, jsonModel.services.size());
+        GenericService dummyService = jsonModel.services.get(0);
+        assertEquals(2, dummyService.metrics.size());
+
+        GenericMetrics dummy0Metrics = getMetricsForInstance("dummy0", dummyService);
+        assertEquals("custom-val", dummy0Metrics.dimensions.get("consumer-dim"));
+
+        GenericMetrics dummy1Metrics = getMetricsForInstance("dummy1", dummyService);
+        assertEquals("custom-val", dummy1Metrics.dimensions.get("consumer-dim"));
+    }
+
+    private void assertDownServiceHealth(String consumer) {
+        GenericJsonModel jsonModel = getResponseAsJsonModel(consumer);
 
         GenericService downService = jsonModel.services.get(1);
         assertEquals(DOWN.status, downService.status.code);
@@ -127,15 +193,8 @@ public class GenericMetricsHandlerTest {
         assertEquals(DownService.NAME, downService.metrics.get(0).dimensions.get(INSTANCE_DIMENSION_ID.id));
     }
 
-    @Test
-    public void all_timestamps_are_equal_and_non_zero() throws Exception {
-        String response = testDriver.sendRequest(URI).readAll();
-        var jsonModel = createObjectMapper().readValue(response, GenericJsonModel.class);
-
-        Long nodeTimestamp = jsonModel.node.timestamp;
-        assertNotEquals(0L, (long) nodeTimestamp);
-        for (var service : jsonModel.services)
-            assertEquals(nodeTimestamp, service.timestamp);
+    private String replaceTimestamps(String s) {
+        return s.replaceAll("timestamp\":\\d+,", "timestamp\":1,");
     }
 
     private static GenericMetrics getMetricsForInstance(String instance, GenericService service) {
@@ -143,23 +202,33 @@ public class GenericMetricsHandlerTest {
             if (metrics.dimensions.get(INSTANCE_DIMENSION_ID.id).equals(instance))
                 return metrics;
         }
-        throw new RuntimeException("Could not find metrics for service instance " + instance);
+        fail("Could not find metrics for service instance " + instance);
+        throw new RuntimeException();
     }
 
     private static MetricsConsumers getMetricsConsumers() {
-        ConsumersConfig.Consumer.Metric.Dimension.Builder metricDimension = new ConsumersConfig.Consumer.Metric.Dimension.Builder()
-                .key("dim0").value("metric-dim");
+        var defaultConsumerDimension = new Consumer.Metric.Dimension.Builder()
+                .key("consumer-dim").value("default-val");
+
+        var customConsumerDimension = new Consumer.Metric.Dimension.Builder()
+                .key("consumer-dim").value("custom-val");
 
         return new MetricsConsumers(new ConsumersConfig.Builder()
-                                            .consumer(new ConsumersConfig.Consumer.Builder()
-                                                              .name(VESPA_CONSUMER_ID.id)
-                                                              .metric(new ConsumersConfig.Consumer.Metric.Builder()
+                                            .consumer(new Consumer.Builder()
+                                                              .name(DEFAULT_PUBLIC_CONSUMER_ID.id)
+                                                              .metric(new Consumer.Metric.Builder()
                                                                               .name(CPU_METRIC)
                                                                               .outputname(CPU_METRIC))
-                                                              .metric(new ConsumersConfig.Consumer.Metric.Builder()
+                                                              .metric(new Consumer.Metric.Builder()
+                                                                              .name(METRIC_1)
+                                                                              .outputname(METRIC_1)
+                                                                              .dimension(defaultConsumerDimension)))
+                                            .consumer(new Consumer.Builder()
+                                                    .name(CUSTOM_CONSUMER)
+                                                              .metric(new Consumer.Metric.Builder()
                                                                               .name(METRIC_1)
                                                                               .outputname(METRIC_1)
-                                                                              .dimension(metricDimension)))
+                                                                              .dimension(customConsumerDimension)))
                                             .build());
     }