Simplify metrics reporting using Maintainer

8 files changed, 71 insertions, 102 deletions

diff --git a/node-repository/src/main/config/node-repository.xml b/node-repository/src/main/config/node-repository.xml
index aadca9187b6..90f47e0acc8 100644
--- a/node-repository/src/main/config/node-repository.xml
+++ b/node-repository/src/main/config/node-repository.xml
@@ -3,7 +3,6 @@
 <component id="com.yahoo.vespa.hosted.provision.provisioning.NodeRepositoryProvisioner" bundle="node-repository" />
 <component id="NodeRepository" class="com.yahoo.vespa.hosted.provision.NodeRepository" bundle="node-repository"/>
 <component id="com.yahoo.vespa.hosted.provision.maintenance.NodeRepositoryMaintenance" bundle="node-repository"/>
-<component id="com.yahoo.vespa.hosted.provision.monitoring.ProvisionMetrics" bundle="node-repository" />
 <component id="com.yahoo.config.provision.NodeFlavors" bundle="config-provisioning" />
 
 <rest-api path="hack" jersey2="true">
diff --git a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/maintenance/ApplicationMaintainer.java b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/maintenance/ApplicationMaintainer.java
index 00f212ea1d4..107f5a4b842 100644
--- a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/maintenance/ApplicationMaintainer.java
+++ b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/maintenance/ApplicationMaintainer.java
@@ -29,13 +29,13 @@ public class ApplicationMaintainer extends Maintainer {
     private final Deployer deployer;
     private final Function<NodeRepository, Set<ApplicationId>> activeApplicationsGetter;
 
-    public ApplicationMaintainer(Deployer deployer, NodeRepository nodeRepository, Duration rate) {
-        this(deployer, nodeRepository, rate, ApplicationMaintainer::getActiveApplications);
+    public ApplicationMaintainer(Deployer deployer, NodeRepository nodeRepository, Duration interval) {
+        this(deployer, nodeRepository, interval, ApplicationMaintainer::getActiveApplications);
     }
 
-    ApplicationMaintainer(Deployer deployer, NodeRepository nodeRepository, Duration rate,
+    ApplicationMaintainer(Deployer deployer, NodeRepository nodeRepository, Duration interval,
                           Function<NodeRepository, Set<ApplicationId>> activeApplicationsGetter) {
-        super(nodeRepository, rate);
+        super(nodeRepository, interval);
         this.deployer = deployer;
         this.activeApplicationsGetter = activeApplicationsGetter;
     }
diff --git a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/maintenance/Maintainer.java b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/maintenance/Maintainer.java
index e988780ff3c..04cbdbb5988 100644
--- a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/maintenance/Maintainer.java
+++ b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/maintenance/Maintainer.java
@@ -3,7 +3,6 @@ package com.yahoo.vespa.hosted.provision.maintenance;
 
 import com.yahoo.component.AbstractComponent;
 import com.yahoo.vespa.hosted.provision.NodeRepository;
-import com.yahoo.yolean.Exceptions;
 
 import java.time.Duration;
 import java.util.concurrent.ScheduledExecutorService;
@@ -22,16 +21,16 @@ public abstract class Maintainer extends AbstractComponent implements Runnable {
     protected static final Logger log = Logger.getLogger(Maintainer.class.getName());
 
     private final NodeRepository nodeRepository;
-    private final Duration rate;
+    private final Duration interval;
 
     private final ScheduledExecutorService service;
 
-    public Maintainer(NodeRepository nodeRepository, Duration rate) {
+    public Maintainer(NodeRepository nodeRepository, Duration interval) {
         this.nodeRepository = nodeRepository;
-        this.rate = rate;
+        this.interval = interval;
 
         this.service = new ScheduledThreadPoolExecutor(1);
-        this.service.scheduleAtFixedRate(this, rate.toMillis(), rate.toMillis(), TimeUnit.MILLISECONDS);
+        this.service.scheduleAtFixedRate(this, interval.toMillis(), interval.toMillis(), TimeUnit.MILLISECONDS);
     }
 
     /** Returns the node repository */
@@ -41,8 +40,8 @@ public abstract class Maintainer extends AbstractComponent implements Runnable {
         return a.toMillis() < b.toMillis() ? a : b;
     }
 
-    /** Returns the rate at which this job is set to run */
-    protected Duration rate() { return rate; }
+    /** Returns the interval at which this job is set to run */
+    protected Duration interval() { return interval; }
 
     @Override
     public void run() {
@@ -50,7 +49,7 @@ public abstract class Maintainer extends AbstractComponent implements Runnable {
             maintain();
         }
         catch (RuntimeException e) {
-            log.log(Level.WARNING, this + " failed. Will retry in " + rate.toMinutes() + " minutes", e);
+            log.log(Level.WARNING, this + " failed. Will retry in " + interval.toMinutes() + " minutes", e);
         }
     }
 
diff --git a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/maintenance/MetricsReporter.java b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/maintenance/MetricsReporter.java
new file mode 100644
index 00000000000..fef597ed8b1
--- /dev/null
+++ b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/maintenance/MetricsReporter.java
@@ -0,0 +1,39 @@
+// Copyright 2016 Yahoo Inc. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root.
+package com.yahoo.vespa.hosted.provision.maintenance;
+
+import com.yahoo.component.AbstractComponent;
+import com.yahoo.config.provision.NodeType;
+import com.yahoo.jdisc.Metric;
+import com.yahoo.log.LogLevel;
+import com.yahoo.vespa.hosted.provision.Node;
+import com.yahoo.vespa.hosted.provision.NodeRepository;
+
+import java.time.Duration;
+import java.util.concurrent.ScheduledExecutorService;
+import java.util.concurrent.ScheduledThreadPoolExecutor;
+import java.util.concurrent.TimeUnit;
+import java.util.logging.Logger;
+
+/**
+ * @author oyving
+ */
+public class MetricsReporter extends Maintainer {
+
+    private final Metric metric;
+
+    public MetricsReporter(NodeRepository nodeRepository, Metric metric, Duration interval) {
+        super(nodeRepository, interval);
+        this.metric = metric;
+    }
+
+    @Override
+    public void maintain() {
+        for (Node.State state : Node.State.values())
+            metric.set("hostedVespa." + state.name() + "Hosts", 
+                       nodeRepository().getNodes(NodeType.tenant, state).size(), null);
+    }
+
+    @Override
+    public String toString() { return "Metrics reporter"; }
+    
+}
diff --git a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/maintenance/NodeRebooter.java b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/maintenance/NodeRebooter.java
index 75c1c32478f..b9c53452137 100644
--- a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/maintenance/NodeRebooter.java
+++ b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/maintenance/NodeRebooter.java
@@ -4,7 +4,6 @@ import com.yahoo.config.provision.NodeType;
 import com.yahoo.vespa.hosted.provision.Node;
 import com.yahoo.vespa.hosted.provision.NodeRepository;
 import com.yahoo.vespa.hosted.provision.node.History;
-import com.yahoo.vespa.hosted.provision.node.filter.NodeFilter;
 import com.yahoo.vespa.hosted.provision.node.filter.NodeListFilter;
 
 import java.time.Clock;
@@ -50,7 +49,7 @@ public class NodeRebooter extends Maintainer {
         if (lastReboot.isPresent() && lastReboot.get().at().plus(rebootInterval).isAfter(clock.instant()))
             return false;
         else // schedule with a probability such that reboots of nodes are spread roughly over the reboot interval
-            return random.nextDouble() < (double)rate().getSeconds() / (double)rebootInterval.getSeconds();
+            return random.nextDouble() < (double) interval().getSeconds() / (double)rebootInterval.getSeconds();
     }
 
     @Override
diff --git a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/maintenance/NodeRepositoryMaintenance.java b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/maintenance/NodeRepositoryMaintenance.java
index 6e3a03d4e5e..90b3a8d3ec7 100644
--- a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/maintenance/NodeRepositoryMaintenance.java
+++ b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/maintenance/NodeRepositoryMaintenance.java
@@ -7,6 +7,7 @@ import com.yahoo.config.provision.Deployer;
 import com.yahoo.config.provision.Environment;
 import com.yahoo.config.provision.HostLivenessTracker;
 import com.yahoo.config.provision.Zone;
+import com.yahoo.jdisc.Metric;
 import com.yahoo.vespa.curator.Curator;
 import com.yahoo.vespa.hosted.provision.NodeRepository;
 import com.yahoo.vespa.orchestrator.Orchestrator;
@@ -32,17 +33,18 @@ public class NodeRepositoryMaintenance extends AbstractComponent {
     private final FailedExpirer failedExpirer;
     private final DirtyExpirer dirtyExpirer;
     private final NodeRebooter nodeRebooter;
+    private final MetricsReporter metricsReporter;
 
     @Inject
     public NodeRepositoryMaintenance(NodeRepository nodeRepository, Deployer deployer, Curator curator,
                                      HostLivenessTracker hostLivenessTracker, ServiceMonitor serviceMonitor, 
-                                     Zone zone, Orchestrator orchestrator) {
-        this(nodeRepository, deployer, curator, hostLivenessTracker, serviceMonitor, zone, Clock.systemUTC(), orchestrator);
+                                     Zone zone, Orchestrator orchestrator, Metric metric) {
+        this(nodeRepository, deployer, curator, hostLivenessTracker, serviceMonitor, zone, Clock.systemUTC(), orchestrator, metric);
     }
 
     public NodeRepositoryMaintenance(NodeRepository nodeRepository, Deployer deployer, Curator curator,
                                      HostLivenessTracker hostLivenessTracker, ServiceMonitor serviceMonitor, 
-                                     Zone zone, Clock clock, Orchestrator orchestrator) {
+                                     Zone zone, Clock clock, Orchestrator orchestrator, Metric metric) {
         DefaultTimes defaults = new DefaultTimes(zone.environment());
         nodeFailer = new NodeFailer(deployer, hostLivenessTracker, serviceMonitor, nodeRepository, fromEnv("fail_grace").orElse(defaults.failGrace), clock, orchestrator);
         applicationMaintainer = new ApplicationMaintainer(deployer, nodeRepository, fromEnv("redeploy_frequency").orElse(defaults.redeployFrequency));
@@ -53,6 +55,7 @@ public class NodeRepositoryMaintenance extends AbstractComponent {
         failedExpirer = new FailedExpirer(nodeRepository, zone, clock, fromEnv("failed_expiry").orElse(defaults.failedExpiry));
         dirtyExpirer = new DirtyExpirer(nodeRepository, clock, fromEnv("dirty_expiry").orElse(defaults.dirtyExpiry));
         nodeRebooter = new NodeRebooter(nodeRepository, clock, fromEnv("reboot_interval").orElse(defaults.rebootInterval));
+        metricsReporter = new MetricsReporter(nodeRepository, metric, fromEnv("metrics_interval").orElse(defaults.metricsInterval));
     }
 
     private Optional<Duration> fromEnv(String envVariable) {
@@ -71,6 +74,7 @@ public class NodeRepositoryMaintenance extends AbstractComponent {
         failedExpirer.deconstruct();
         dirtyExpirer.deconstruct();
         nodeRebooter.deconstruct();
+        metricsReporter.deconstruct();
     }
 
     private static class DefaultTimes {
@@ -89,6 +93,7 @@ public class NodeRepositoryMaintenance extends AbstractComponent {
         private final Duration failedExpiry;
         private final Duration dirtyExpiry;
         private final Duration rebootInterval;
+        private final Duration metricsInterval;
 
         DefaultTimes(Environment environment) {
             if (environment.equals(Environment.prod)) {
@@ -102,7 +107,8 @@ public class NodeRepositoryMaintenance extends AbstractComponent {
                 retiredExpiry = Duration.ofDays(4); // enough time to migrate data
                 failedExpiry = Duration.ofDays(4); // enough time to recover data even if it happens friday night
                 dirtyExpiry = Duration.ofHours(2); // enough time to clean the node
-                rebootInterval = Duration.ofDays(30); 
+                rebootInterval = Duration.ofDays(30);
+                metricsInterval = Duration.ofMinutes(1);
             } else {
                 // These values ensure tests and development is not delayed due to nodes staying around
                 // Use non-null values as these also determine the maintenance interval
@@ -115,6 +121,7 @@ public class NodeRepositoryMaintenance extends AbstractComponent {
                 failedExpiry = Duration.ofMinutes(10);
                 dirtyExpiry = Duration.ofMinutes(30);
                 rebootInterval = Duration.ofDays(30);
+                metricsInterval = Duration.ofMinutes(1);
             }
         }
 
diff --git a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/monitoring/ProvisionMetrics.java b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/monitoring/ProvisionMetrics.java
deleted file mode 100644
index 18f4765fa03..00000000000
--- a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/monitoring/ProvisionMetrics.java
+++ /dev/null
@@ -1,61 +0,0 @@
-// Copyright 2016 Yahoo Inc. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root.
-package com.yahoo.vespa.hosted.provision.monitoring;
-
-import com.yahoo.component.AbstractComponent;
-import com.yahoo.config.provision.NodeType;
-import com.yahoo.jdisc.Metric;
-import com.yahoo.log.LogLevel;
-import com.yahoo.vespa.hosted.provision.Node;
-import com.yahoo.vespa.hosted.provision.NodeRepository;
-
-import java.util.concurrent.ScheduledExecutorService;
-import java.util.concurrent.ScheduledThreadPoolExecutor;
-import java.util.concurrent.TimeUnit;
-import java.util.logging.Logger;
-
-/**
- * @author oyving
- */
-public class ProvisionMetrics extends AbstractComponent {
-
-    private static final Logger log = Logger.getLogger(ProvisionMetrics.class.getName());
-    private final ScheduledExecutorService executorService;
-
-    // TODO: make report interval configurable
-    public ProvisionMetrics(Metric metric, NodeRepository nodeRepository) {
-        this.executorService = new ScheduledThreadPoolExecutor(1);
-        this.executorService.scheduleAtFixedRate(
-                new ProvisionMetricsTask(metric, nodeRepository),
-                0, // start immediately
-                1, // report every minute
-                TimeUnit.MINUTES
-        );
-    }
-
-    @Override
-    public void deconstruct() {
-        this.executorService.shutdown();
-    }
-
-    private static class ProvisionMetricsTask implements Runnable {
-        private final Metric metric;
-        private final NodeRepository nodeRepository;
-
-        private ProvisionMetricsTask(Metric metric, NodeRepository nodeRepository) {
-            this.metric = metric;
-            this.nodeRepository = nodeRepository;
-        }
-
-        @Override
-        public void run() {
-            log.log(LogLevel.DEBUG, "Running provision metrics task");
-            try {
-                for (Node.State state : Node.State.values())
-                    metric.set("hostedVespa." + state.name() + "Hosts", nodeRepository.getNodes(NodeType.tenant, state).size(), null);
-            } catch (RuntimeException e) {
-                log.log(LogLevel.INFO, "Failed gathering metrics data: " + e.getMessage());
-            }
-        }
-    }
-
-}
diff --git a/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/monitoring/ProvisionMetricsTest.java b/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/monitoring/MetricsReporterTest.java
index 871af0be5b5..88d68af16de 100644
--- a/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/monitoring/ProvisionMetricsTest.java
+++ b/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/monitoring/MetricsReporterTest.java
@@ -9,11 +9,13 @@ import com.yahoo.vespa.curator.mock.MockCurator;
 import com.yahoo.vespa.hosted.provision.Node;
 import com.yahoo.vespa.hosted.provision.NodeRepository;
 import com.yahoo.config.provision.NodeFlavors;
+import com.yahoo.vespa.hosted.provision.maintenance.MetricsReporter;
 import com.yahoo.vespa.hosted.provision.testutils.FlavorConfigBuilder;
 import com.yahoo.vespa.hosted.provision.testutils.MockNameResolver;
 import org.junit.Test;
 
 import java.time.Clock;
+import java.time.Duration;
 import java.util.Collections;
 import java.util.HashMap;
 import java.util.Map;
@@ -25,14 +27,14 @@ import static org.junit.Assert.assertEquals;
 /**
  * @author oyving
  */
-public class ProvisionMetricsTest {
+public class MetricsReporterTest {
 
-    @Test(timeout = 10_000L)
+    @Test
     public void test_registered_metric() throws InterruptedException {
         NodeFlavors nodeFlavors = FlavorConfigBuilder.createDummies("default");
         Curator curator = new MockCurator();
         NodeRepository nodeRepository = new NodeRepository(nodeFlavors, curator, Clock.systemUTC(), Zone.defaultZone(),
-                new MockNameResolver().mockAnyLookup());
+                                                           new MockNameResolver().mockAnyLookup());
         Node node = nodeRepository.createNode("openStackId", "hostname", Optional.empty(), nodeFlavors.getFlavorOrThrow("default"), NodeType.tenant);
         nodeRepository.addNodes(Collections.singletonList(node));
         Node hostNode = nodeRepository.createNode("openStackId2", "parent", Optional.empty(), nodeFlavors.getFlavorOrThrow("default"), NodeType.host);
@@ -48,36 +50,28 @@ public class ProvisionMetricsTest {
         expectedMetrics.put("hostedVespa.dirtyHosts", 0);
         expectedMetrics.put("hostedVespa.failedHosts", 0);
 
-        TestMetric metric = new TestMetric(expectedMetrics.size());
-        ProvisionMetrics provisionMetrics = new ProvisionMetrics(metric, nodeRepository);
+        TestMetric metric = new TestMetric();
+        MetricsReporter metricsReporter = new MetricsReporter(nodeRepository, metric, Duration.ofMinutes(1));
+        metricsReporter.maintain();
 
-        metric.latch.await();
         assertEquals(expectedMetrics, metric.values);
-
-        provisionMetrics.deconstruct();
     }
 
     private static class TestMetric implements Metric {
-        public CountDownLatch latch;
+
         public Map<String, Number> values = new HashMap<>();
         public Map<String, Context> context = new HashMap<>();
 
-        public TestMetric(int latchNumber) {
-            this.latch = new CountDownLatch(latchNumber);
-        }
-
         @Override
         public void set(String key, Number val, Context ctx) {
             values.put(key, val);
             context.put(key, ctx);
-            countDownAboveZero();
         }
 
         @Override
         public void add(String key, Number val, Context ctx) {
             values.put(key, val);
             context.put(key, ctx);
-            countDownAboveZero();
         }
 
         @Override
@@ -85,13 +79,6 @@ public class ProvisionMetricsTest {
             return null;
         }
 
-        private void countDownAboveZero() {
-            if (latch.getCount() == 0) {
-                throw new AssertionError("Countdown latch too low - check metric.set metric.add calls");
-            }
-
-            latch.countDown();
-        }
     }
 
 }