Merge pull request #9974 from vespa-engine/gjoranv/use-application-from-setExtraMetrics-as-service

Stop replacing service/application id from setExtraMetrics packets

5 files changed, 116 insertions, 53 deletions

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 64ede137e8e..017b2c57370 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
@@ -35,12 +35,13 @@ import static java.util.stream.Collectors.toCollection;
 public class ExternalMetrics {
     private static final Logger log = Logger.getLogger(ExternalMetrics.class.getName());
 
+    // NOTE: node service id must be kept in sync with the same constant _value_ used in docker-api:Metrics.java
+    public static final ServiceId VESPA_NODE_SERVICE_ID = toServiceId("vespa.node");
+
     public static final DimensionId ROLE_DIMENSION = toDimensionId("role");
     public static final DimensionId STATE_DIMENSION = toDimensionId("state");
     public static final DimensionId ORCHESTRATOR_STATE_DIMENSION = toDimensionId("orchestratorState");
 
-    public static final ServiceId VESPA_NODE_SERVICE_ID = toServiceId("vespa.node");
-
     private volatile List<MetricsPacket.Builder> metrics = new ArrayList<>();
     private final MetricsConsumers consumers;
 
@@ -58,7 +59,6 @@ public class ExternalMetrics {
         log.log(DEBUG, () -> "Setting new external metrics with " + externalPackets.size() + " metrics packets.");
         externalPackets.forEach(packet -> {
             packet.addConsumers(consumers.getAllConsumers())
-                    .service(VESPA_NODE_SERVICE_ID)
                     .retainMetrics(metricsToRetain())
                     .applyOutputNames(outputNamesById());
         });
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 e441c353292..bc83712ac70 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
@@ -15,6 +15,7 @@ import ai.vespa.metricsproxy.metric.dimensions.NodeDimensions;
 import ai.vespa.metricsproxy.metric.dimensions.NodeDimensionsConfig;
 import ai.vespa.metricsproxy.metric.model.DimensionId;
 import ai.vespa.metricsproxy.metric.model.MetricsPacket;
+import ai.vespa.metricsproxy.metric.model.ServiceId;
 import ai.vespa.metricsproxy.service.DownService;
 import ai.vespa.metricsproxy.service.DummyService;
 import ai.vespa.metricsproxy.service.VespaService;
@@ -38,6 +39,7 @@ import static ai.vespa.metricsproxy.metric.model.ServiceId.toServiceId;
 import static org.hamcrest.CoreMatchers.containsString;
 import static org.hamcrest.CoreMatchers.is;
 import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertNotNull;
 import static org.junit.Assert.assertThat;
 import static org.junit.Assert.assertTrue;
 
@@ -162,6 +164,21 @@ public class MetricsManagerTest {
     }
 
     @Test
+    public void application_from_extra_metrics_packets_is_used_as_service_in_result_packets() {
+        final ServiceId serviceId = toServiceId("custom-service");
+        metricsManager.setExtraMetrics(ImmutableList.of(
+                new MetricsPacket.Builder(serviceId)
+                        .putMetrics(ImmutableList.of(new Metric(WHITELISTED_METRIC_ID, 0)))));
+
+        List<MetricsPacket> packets = metricsManager.getMetrics(testServices, Instant.EPOCH);
+        MetricsPacket extraPacket = null;
+        for (MetricsPacket packet : packets) {
+            if (packet.service.equals(serviceId)) extraPacket = packet;
+        }
+        assertNotNull(extraPacket);
+    }
+
+    @Test
     public void extra_dimensions_are_added_to_metrics_packets_that_do_not_have_those_dimensions() {
         metricsManager.setExtraMetrics(ImmutableList.of(
                 new MetricsPacket.Builder(toServiceId("foo"))
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 29ab8c66694..dc89e5bb9f2 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
@@ -37,6 +37,7 @@ import java.util.concurrent.Executors;
 
 import static ai.vespa.metricsproxy.core.VespaMetrics.INSTANCE_DIMENSION_ID;
 import static ai.vespa.metricsproxy.http.GenericMetricsHandler.DEFAULT_PUBLIC_CONSUMER_ID;
+import static ai.vespa.metricsproxy.metric.ExternalMetrics.VESPA_NODE_SERVICE_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;
@@ -74,7 +75,7 @@ public class GenericMetricsHandlerTest {
     public static void setup() {
         MetricsManager metricsManager = TestUtil.createMetricsManager(vespaServices, getMetricsConsumers(), getApplicationDimensions(), getNodeDimensions());
         metricsManager.setExtraMetrics(ImmutableList.of(
-                new MetricsPacket.Builder(toServiceId("foo"))
+                new MetricsPacket.Builder(VESPA_NODE_SERVICE_ID)
                         .timestamp(Instant.now().getEpochSecond())
                         .putMetrics(ImmutableList.of(new Metric(CPU_METRIC, 12.345)))));
         GenericMetricsHandler handler = new GenericMetricsHandler(Executors.newSingleThreadExecutor(), metricsManager, vespaServices, getMetricsConsumers());
diff --git a/metrics-proxy/src/test/java/ai/vespa/metricsproxy/metric/ExternalMetricsTest.java b/metrics-proxy/src/test/java/ai/vespa/metricsproxy/metric/ExternalMetricsTest.java
index 11c271d46e4..2cce2f66039 100644
--- a/metrics-proxy/src/test/java/ai/vespa/metricsproxy/metric/ExternalMetricsTest.java
+++ b/metrics-proxy/src/test/java/ai/vespa/metricsproxy/metric/ExternalMetricsTest.java
@@ -8,6 +8,7 @@ import ai.vespa.metricsproxy.core.ConsumersConfig;
 import ai.vespa.metricsproxy.core.MetricsConsumers;
 import ai.vespa.metricsproxy.metric.model.ConsumerId;
 import ai.vespa.metricsproxy.metric.model.MetricsPacket;
+import ai.vespa.metricsproxy.metric.model.ServiceId;
 import com.google.common.collect.ImmutableList;
 import org.junit.Test;
 
@@ -38,15 +39,17 @@ public class ExternalMetricsTest {
     }
 
     @Test
-    public void service_id_is_set_to_vespa_node_id() {
+    public void service_id_from_extra_packets_is_not_replaced() {
+        final ServiceId SERVICE_ID = toServiceId("do-not-replace");
+
         MetricsConsumers noConsumers = new MetricsConsumers(new ConsumersConfig.Builder().build());
         ExternalMetrics externalMetrics = new ExternalMetrics(noConsumers);
         externalMetrics.setExtraMetrics(ImmutableList.of(
-                new MetricsPacket.Builder(toServiceId("replace_with_vespa_node_id"))));
+                new MetricsPacket.Builder(SERVICE_ID)));
 
         List<MetricsPacket.Builder> packets = externalMetrics.getMetrics();
         assertEquals(1, packets.size());
-        assertEquals(VESPA_NODE_SERVICE_ID, packets.get(0).build().service);
+        assertEquals(SERVICE_ID, packets.get(0).build().service);
     }
 
     @Test
diff --git a/metrics-proxy/src/test/java/ai/vespa/metricsproxy/rpc/RpcMetricsTest.java b/metrics-proxy/src/test/java/ai/vespa/metricsproxy/rpc/RpcMetricsTest.java
index d4777618546..d6084e3e03a 100644
--- a/metrics-proxy/src/test/java/ai/vespa/metricsproxy/rpc/RpcMetricsTest.java
+++ b/metrics-proxy/src/test/java/ai/vespa/metricsproxy/rpc/RpcMetricsTest.java
@@ -17,7 +17,9 @@ import com.yahoo.jrt.Transport;
 import org.json.JSONArray;
 import org.json.JSONException;
 import org.json.JSONObject;
+import org.junit.Rule;
 import org.junit.Test;
+import org.junit.rules.ExpectedException;
 
 import java.util.List;
 
@@ -34,6 +36,8 @@ import static org.hamcrest.CoreMatchers.notNullValue;
 import static org.junit.Assert.assertFalse;
 import static org.junit.Assert.assertNotNull;
 import static org.junit.Assert.assertThat;
+import static org.junit.Assert.assertTrue;
+import static org.junit.Assert.fail;
 
 /**
  * @author jobergum
@@ -41,13 +45,60 @@ import static org.junit.Assert.assertThat;
  */
 public class RpcMetricsTest {
 
-    private static final String METRICS_RESPONSE_CCL =
-            getFileContents("metrics-storage-simple.json").trim();
+    private static final String METRICS_RESPONSE = getFileContents("metrics-storage-simple.json").trim();
+    private static final String EXTRA_APP = "extra";
+
+    private static class RpcClient implements AutoCloseable {
+        private final Supervisor supervisor;
+        private final Target target;
+
+        RpcClient(int port) {
+            supervisor = new Supervisor(new Transport());
+            target = supervisor.connect(new Spec("localhost", port));
+        }
+
+        @Override
+        public void close() {
+            target.close();
+            supervisor.transport().shutdown().join();
+        }
+    }
+
+    @Test
+    public void extra_metrics_are_added_to_output() throws Exception {
+        String extraMetricsPayload = "{\"timestamp\":1557754772,\"application\":\"" + EXTRA_APP +
+                "\",\"metrics\":{\"foo.count\":3},\"dimensions\":{\"role\":\"extra-role\"}}";
+
+        try (IntegrationTester tester = new IntegrationTester()) {
+            try (RpcClient rpcClient = new RpcClient(tester.rpcPort())) {
+                Request req = new Request("setExtraMetrics");
+                req.parameters().add(new StringValue(extraMetricsPayload));
+                invoke(req, rpcClient, false);
+                String allServicesResponse = getMetricsForYamas(ALL_SERVICES, rpcClient).trim();
+
+                // Verify that application is used as serviceId, and that metric exists.
+                JSONObject extraMetrics = findExtraMetricsObject(allServicesResponse);
+                assertThat(extraMetrics.getJSONObject("metrics").getInt("foo.count"), is(3));
+                assertThat(extraMetrics.getJSONObject("dimensions").getString("role"), is("extra-role"));
+            }
+        }
+    }
+
+    private JSONObject findExtraMetricsObject(String jsonResponse) throws JSONException {
+        JSONArray metrics = new JSONObject(jsonResponse).getJSONArray("metrics");
+        for (int i = 0; i <  metrics.length(); i++) {
+            JSONObject jsonObject = metrics.getJSONObject(i);
+            assertTrue(jsonObject.has("application"));
+            if (jsonObject.getString("application").equals(EXTRA_APP)) return jsonObject;
+        }
+        fail("Metrics from setExtraMetrics was missing.");
+        throw new RuntimeException();
+    }
 
     @Test
     public void testGetMetrics() throws Exception {
         try (IntegrationTester tester = new IntegrationTester()) {
-            tester.httpServer().setResponse(METRICS_RESPONSE_CCL);
+            tester.httpServer().setResponse(METRICS_RESPONSE);
             List<VespaService> services = tester.vespaServices().getInstancesById(SERVICE_1_CONFIG_ID);
 
             assertThat("#Services should be 1 for config id " + SERVICE_1_CONFIG_ID, services.size(), is(1));
@@ -62,34 +113,29 @@ public class RpcMetricsTest {
             Metric m2 = metrics.getMetric("bar.count");
             assertNotNull("Did not find expected metric with name 'bar.count'", m2);
 
-            // Setup RPC client
-            Supervisor supervisor = new Supervisor(new Transport());
-            Target target = supervisor.connect(new Spec("localhost", tester.rpcPort()));
+            try (RpcClient rpcClient = new RpcClient(tester.rpcPort())) {
+                verifyMetricsFromRpcRequest(qrserver, rpcClient);
 
-            verifyMetricsFromRpcRequest(qrserver, target);
+                services = tester.vespaServices().getInstancesById(SERVICE_2_CONFIG_ID);
+                assertThat("#Services should be 1 for config id " + SERVICE_2_CONFIG_ID, services.size(), is(1));
 
-            services = tester.vespaServices().getInstancesById(SERVICE_2_CONFIG_ID);
-            assertThat("#Services should be 1 for config id " + SERVICE_2_CONFIG_ID, services.size(), is(1));
+                VespaService storageService = services.get(0);
+                verfiyMetricsFromServiceObject(storageService);
 
-            VespaService storageService = services.get(0);
-            verfiyMetricsFromServiceObject(storageService);
+                String metricsById = getMetricsById(storageService.getConfigId(), rpcClient);
+                assertThat(metricsById, is("'storage.cluster.storage.storage.0'.foo_count=1 "));
 
-            String metricsById = getMetricsById(storageService.getConfigId(), target);
-            assertThat(metricsById, is("'storage.cluster.storage.storage.0'.foo_count=1 "));
+                String jsonResponse = getMetricsForYamas("non-existing", rpcClient).trim();
+                assertThat(jsonResponse, is("105: No service with name 'non-existing'"));
 
-            String jsonResponse = getMetricsForYamas("non-existing", target).trim();
-            assertThat(jsonResponse, is("105: No service with name 'non-existing'"));
+                verifyMetricsFromRpcRequestForAllServices(rpcClient);
 
-            verifyMetricsFromRpcRequestForAllServices(target);
-
-            // Shutdown RPC
-            target.close();
-            supervisor.transport().shutdown().join();
+            }
         }
     }
 
-    private static void verifyMetricsFromRpcRequest(VespaService service, Target target) throws JSONException {
-        String jsonResponse = getMetricsForYamas(service.getMonitoringName(), target).trim();
+    private static void verifyMetricsFromRpcRequest(VespaService service, RpcClient client) throws JSONException {
+        String jsonResponse = getMetricsForYamas(service.getMonitoringName(), client).trim();
         JSONArray metrics = new JSONObject(jsonResponse).getJSONArray("metrics");
         assertThat("Expected 3 metric messages", metrics.length(), is(3));
         for (int i = 0; i < metrics.length() - 1; i++) { // The last "metric message" contains only status code/message
@@ -124,18 +170,18 @@ public class RpcMetricsTest {
         assertThat("Metric foo did not contain correct dimension for key = bar", foo.getDimensions().get(toDimensionId("bar")), is("foo"));
     }
 
-    private void verifyMetricsFromRpcRequestForAllServices(Target target) throws JSONException {
+    private void verifyMetricsFromRpcRequestForAllServices(RpcClient client) throws JSONException {
         // Verify that metrics for all services can be retrieved in one request.
-        String allServicesResponse = getMetricsForYamas(ALL_SERVICES, target).trim();
+        String allServicesResponse = getMetricsForYamas(ALL_SERVICES, client).trim();
         JSONArray allServicesMetrics = new JSONObject(allServicesResponse).getJSONArray("metrics");
         assertThat(allServicesMetrics.length(), is(5));
     }
 
     @Test
-    public void testGetAllMetricNames() {
+    public void testGetAllMetricNames() throws Exception {
         try (IntegrationTester tester = new IntegrationTester()) {
 
-            tester.httpServer().setResponse(METRICS_RESPONSE_CCL);
+            tester.httpServer().setResponse(METRICS_RESPONSE);
             List<VespaService> services = tester.vespaServices().getInstancesById(SERVICE_1_CONFIG_ID);
 
             assertThat(services.size(), is(1));
@@ -144,52 +190,48 @@ public class RpcMetricsTest {
             Metric m = metrics.getMetric("foo.count");
             assertNotNull("Did not find expected metric with name 'foo.count'", m);
 
-
             Metric m2 = metrics.getMetric("bar.count");
             assertNotNull("Did not find expected metric with name 'bar'", m2);
 
-            // Setup RPC
-            Supervisor supervisor = new Supervisor(new Transport());
-            Target target = supervisor.connect(new Spec("localhost", tester.rpcPort()));
-
-            String response = getAllMetricNamesForService(services.get(0).getMonitoringName(), VESPA_CONSUMER_ID, target);
-            assertThat(response, is("foo.count=ON;output-name=foo_count,bar.count=OFF,"));
-
-            // Shutdown RPC
-            target.close();
-            supervisor.transport().shutdown().join();
+            try (RpcClient rpcClient = new RpcClient(tester.rpcPort())) {
+                String response = getAllMetricNamesForService(services.get(0).getMonitoringName(), VESPA_CONSUMER_ID, rpcClient);
+                assertThat(response, is("foo.count=ON;output-name=foo_count,bar.count=OFF,"));
+            }
         }
     }
 
-    private static String getMetricsForYamas(String service, Target target) {
+    private static String getMetricsForYamas(String service, RpcClient client) {
         Request req = new Request("getMetricsForYamas");
         req.parameters().add(new StringValue(service));
-        return invoke(req, target);
+        return invoke(req, client, true);
     }
 
-    private String getMetricsById(String service, Target target) {
+    private String getMetricsById(String service, RpcClient client) {
         Request req = new Request("getMetricsById");
         req.parameters().add(new StringValue(service));
-        return invoke(req, target);
+        return invoke(req, client, true);
     }
 
-    private String getAllMetricNamesForService(String service, ConsumerId consumer, Target target) {
+    private String getAllMetricNamesForService(String service, ConsumerId consumer, RpcClient client) {
         Request req = new Request("getAllMetricNamesForService");
         req.parameters().add(new StringValue(service));
         req.parameters().add(new StringValue(consumer.id));
-        return invoke(req, target);
+        return invoke(req, client, true);
     }
 
-    private static String invoke(Request req, Target target) {
+    private static String invoke(Request req, RpcClient client, boolean expectReturnValue) {
         String returnValue;
-        target.invokeSync(req, 20.0);
+        client.target.invokeSync(req, 20.0);
         if (req.checkReturnTypes("s")) {
             returnValue = req.returnValues().get(0).asString();
-        } else {
+        } else if (expectReturnValue) {
             System.out.println(req.methodName() + " from rpcserver - Invocation failed "
                                        + req.errorCode() + ": " + req.errorMessage());
             returnValue = req.errorCode() + ": " + req.errorMessage();
         }
+        else {
+            return "";
+        }
         return returnValue;
     }