In order to avoid possible deadlock when calling yourself for metrics when TTL expires, we only fetch metrics in the background.

If it is present use it, if not it will be there next time.

3 files changed, 34 insertions, 25 deletions

diff --git a/metrics-proxy/src/test/java/ai/vespa/metricsproxy/http/application/ApplicationMetricsHandlerTest.java b/metrics-proxy/src/test/java/ai/vespa/metricsproxy/http/application/ApplicationMetricsHandlerTest.java
index cf1eac3c691..563ecab1346 100644
--- a/metrics-proxy/src/test/java/ai/vespa/metricsproxy/http/application/ApplicationMetricsHandlerTest.java
+++ b/metrics-proxy/src/test/java/ai/vespa/metricsproxy/http/application/ApplicationMetricsHandlerTest.java
@@ -4,6 +4,7 @@ package ai.vespa.metricsproxy.http.application;
 import ai.vespa.metricsproxy.core.ConsumersConfig;
 import ai.vespa.metricsproxy.core.MetricsConsumers;
 import ai.vespa.metricsproxy.metric.dimensions.PublicDimensions;
+import ai.vespa.metricsproxy.metric.model.ConsumerId;
 import ai.vespa.metricsproxy.metric.model.json.GenericApplicationModel;
 import ai.vespa.metricsproxy.metric.model.json.GenericJsonModel;
 import ai.vespa.metricsproxy.metric.model.json.GenericMetrics;
@@ -84,6 +85,8 @@ public class ApplicationMetricsHandlerTest {
         ApplicationMetricsHandler handler = new ApplicationMetricsHandler(Executors.newSingleThreadExecutor(),
                                                                           applicationMetricsRetriever,
                                                                           getMetricsConsumers());
+        applicationMetricsRetriever.getMetrics(ConsumerId.toConsumerId(CUSTOM_CONSUMER));
+        applicationMetricsRetriever.startPollAnwWait();
         testDriver = new RequestHandlerTestDriver(handler);
     }
 
diff --git a/metrics-proxy/src/test/java/ai/vespa/metricsproxy/http/application/ApplicationMetricsRetrieverTest.java b/metrics-proxy/src/test/java/ai/vespa/metricsproxy/http/application/ApplicationMetricsRetrieverTest.java
index a74989f7af6..d01001b9e93 100644
--- a/metrics-proxy/src/test/java/ai/vespa/metricsproxy/http/application/ApplicationMetricsRetrieverTest.java
+++ b/metrics-proxy/src/test/java/ai/vespa/metricsproxy/http/application/ApplicationMetricsRetrieverTest.java
@@ -1,6 +1,7 @@
 // Copyright 2020 Oath Inc. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root.
 package ai.vespa.metricsproxy.http.application;
 
+import ai.vespa.metricsproxy.metric.model.MetricsPacket;
 import com.github.tomakehurst.wiremock.junit.WireMockRule;
 import org.junit.Before;
 import org.junit.Rule;
@@ -8,6 +9,7 @@ import org.junit.Test;
 
 import java.time.Duration;
 import java.util.Arrays;
+import java.util.List;
 import java.util.concurrent.TimeoutException;
 
 import static ai.vespa.metricsproxy.TestUtil.getFileContents;
@@ -56,6 +58,7 @@ public class ApplicationMetricsRetrieverTest {
                                      .willReturn(aResponse().withBody(RESPONSE)));
 
         ApplicationMetricsRetriever retriever = new ApplicationMetricsRetriever(config);
+        retriever.startPollAnwWait();
         var metricsByNode = retriever.getMetrics();
         assertEquals(1, metricsByNode.size());
         assertEquals(4, metricsByNode.get(node).size());
@@ -73,6 +76,7 @@ public class ApplicationMetricsRetrieverTest {
                                      .willReturn(aResponse().withBody(RESPONSE)));
 
         ApplicationMetricsRetriever retriever = new ApplicationMetricsRetriever(config);
+        retriever.startPollAnwWait();
         var metricsByNode = retriever.getMetrics();
         assertEquals(2, metricsByNode.size());
         assertEquals(4, metricsByNode.get(node0).size());
@@ -101,6 +105,7 @@ public class ApplicationMetricsRetrieverTest {
                                      .willReturn(aResponse().withBody(RESPONSE)));
 
         ApplicationMetricsRetriever retriever = new ApplicationMetricsRetriever(config);
+        retriever.startPollAnwWait();
         var metricsByNode = retriever.getMetrics();
         assertEquals(2, metricsByNode.size());
         assertEquals(0, metricsByNode.get(node0).size());
@@ -110,7 +115,7 @@ public class ApplicationMetricsRetrieverTest {
     @Test
     public void an_exception_is_thrown_when_retrieving_times_out() {
         var config = nodesConfig("/node0");
-
+        Node node = new Node(config.node(0));
         wireMockRule.stubFor(get(urlPathEqualTo(config.node(0).metricsPath()))
                                      .willReturn(aResponse()
                                                          .withBody(RESPONSE)
@@ -118,13 +123,9 @@ public class ApplicationMetricsRetrieverTest {
 
         ApplicationMetricsRetriever retriever = new ApplicationMetricsRetriever(config);
         retriever.setTaskTimeout(Duration.ofMillis(1));
+        retriever.startPollAnwWait();
+        assertTrue(retriever.getMetrics().get(node).isEmpty());
 
-        try {
-            retriever.getMetrics();
-            fail("Did not get expected exception");
-        } catch (ApplicationMetricsException expected) {
-            assertTrue(expected.getCause() instanceof TimeoutException);
-        }
     }
 
     @Test
@@ -139,12 +140,8 @@ public class ApplicationMetricsRetrieverTest {
 
         ApplicationMetricsRetriever retriever = new ApplicationMetricsRetriever(config);
         retriever.setTaskTimeout(Duration.ofMillis(1));
-        try {
-            retriever.getMetrics();
-            fail("Did not get expected exception");
-        } catch (ApplicationMetricsException expected) {
-        }
-
+        retriever.startPollAnwWait();
+        assertTrue(retriever.getMetrics().get(node).isEmpty());
         // Verify successful retrieving
         wireMockRule.removeStubMapping(delayedStub);
         verifyRetrievingMetricsFromSingleNode(config, node);
diff --git a/metrics-proxy/src/test/java/ai/vespa/metricsproxy/http/application/NodeMetricsClientTest.java b/metrics-proxy/src/test/java/ai/vespa/metricsproxy/http/application/NodeMetricsClientTest.java
index ab84a4edcde..6644579147f 100644
--- a/metrics-proxy/src/test/java/ai/vespa/metricsproxy/http/application/NodeMetricsClientTest.java
+++ b/metrics-proxy/src/test/java/ai/vespa/metricsproxy/http/application/NodeMetricsClientTest.java
@@ -14,6 +14,7 @@ import org.junit.ClassRule;
 import org.junit.Test;
 
 import java.net.URI;
+import java.time.Duration;
 import java.util.List;
 
 import static ai.vespa.metricsproxy.TestUtil.getFileContents;
@@ -45,6 +46,8 @@ public class NodeMetricsClientTest {
     private static final String CPU_METRIC = "cpu.util";
     private static final String REPLACED_CPU_METRIC = "replaced_cpu_util";
     private static final String CUSTOM_CONSUMER = "custom-consumer";
+    private static final Duration TTL = Duration.ofSeconds(30);
+
 
     private static Node node;
 
@@ -85,38 +88,44 @@ public class NodeMetricsClientTest {
     }
 
     @Test
-    public void metrics_are_retrieved_upon_first_request() {
+    public void metrics_are_retrieved_upon_first_update() {
+        assertEquals(0, nodeMetricsClient.getMetrics(defaultMetricsConsumerId).size());
+        assertEquals(0, nodeMetricsClient.snapshotsRetrieved());
+        assertTrue(nodeMetricsClient.updateSnapshots(defaultMetricsConsumerId, TTL));
+        assertEquals(1, nodeMetricsClient.snapshotsRetrieved());
         List<MetricsPacket> metrics = nodeMetricsClient.getMetrics(defaultMetricsConsumerId);
         assertEquals(1, nodeMetricsClient.snapshotsRetrieved());
         assertEquals(4, metrics.size());
    }
 
     @Test
-    public void cached_metrics_are_used_when_ttl_has_not_expired() {
-        nodeMetricsClient.getMetrics(defaultMetricsConsumerId);
-        assertEquals(1, nodeMetricsClient.snapshotsRetrieved());
-
-        clock.advance(NodeMetricsClient.METRICS_TTL.minusMillis(1));
-        nodeMetricsClient.getMetrics(defaultMetricsConsumerId);
+    public void metrics_are_refreshed_on_every_update() {
+        assertEquals(0, nodeMetricsClient.snapshotsRetrieved());
+        assertTrue(nodeMetricsClient.updateSnapshots(defaultMetricsConsumerId, TTL));
         assertEquals(1, nodeMetricsClient.snapshotsRetrieved());
+        assertTrue(nodeMetricsClient.updateSnapshots(defaultMetricsConsumerId, Duration.ZERO));
+        assertEquals(2, nodeMetricsClient.snapshotsRetrieved());
     }
 
     @Test
-    public void metrics_are_refreshed_when_ttl_has_expired() {
-        nodeMetricsClient.getMetrics(defaultMetricsConsumerId);
+    public void metrics_are_not_refreshed_if_ttl_not_expired() {
+        assertEquals(0, nodeMetricsClient.snapshotsRetrieved());
+        assertTrue(nodeMetricsClient.updateSnapshots(defaultMetricsConsumerId, TTL));
         assertEquals(1, nodeMetricsClient.snapshotsRetrieved());
-
-        clock.advance(NodeMetricsClient.METRICS_TTL.plusMillis(1));
-        nodeMetricsClient.getMetrics(defaultMetricsConsumerId);
+        assertTrue(nodeMetricsClient.updateSnapshots(defaultMetricsConsumerId, TTL));
+        assertEquals(1, nodeMetricsClient.snapshotsRetrieved());
+        assertTrue(nodeMetricsClient.updateSnapshots(defaultMetricsConsumerId, Duration.ZERO));
         assertEquals(2, nodeMetricsClient.snapshotsRetrieved());
     }
 
     @Test
     public void metrics_for_different_consumers_are_cached_separately() {
+        assertTrue(nodeMetricsClient.updateSnapshots(defaultMetricsConsumerId,TTL));
         List<MetricsPacket> defaultMetrics = nodeMetricsClient.getMetrics(defaultMetricsConsumerId);
         assertEquals(1, nodeMetricsClient.snapshotsRetrieved());
         assertEquals(4, defaultMetrics.size());
 
+        assertTrue(nodeMetricsClient.updateSnapshots(toConsumerId(CUSTOM_CONSUMER), TTL));
         List<MetricsPacket> customMetrics = nodeMetricsClient.getMetrics(toConsumerId(CUSTOM_CONSUMER));
         assertEquals(2, nodeMetricsClient.snapshotsRetrieved());
         assertEquals(4, customMetrics.size());