Set period in constructor only

4 files changed, 38 insertions, 39 deletions

diff --git a/metrics/src/tests/metricmanagertest.cpp b/metrics/src/tests/metricmanagertest.cpp
index e398e252a36..6674da98452 100644
--- a/metrics/src/tests/metricmanagertest.cpp
+++ b/metrics/src/tests/metricmanagertest.cpp
@@ -950,8 +950,8 @@ namespace {
         std::mutex&         _output_mutex;
         FakeTimer&          _timer;
 
-        MyUpdateHook(std::ostringstream& output, std::mutex& output_mutex, const char* name, FakeTimer& timer)
-            : UpdateHook(name),
+        MyUpdateHook(std::ostringstream& output, std::mutex& output_mutex, const char* name, vespalib::duration period, FakeTimer& timer)
+            : UpdateHook(name, period),
               _output(output),
               _output_mutex(output_mutex),
               _timer(timer)
@@ -979,12 +979,12 @@ TEST_F(MetricManagerTest, test_update_hooks)
         mm.registerMetric(lockGuard, mySet.set);
     }
 
-    MyUpdateHook preInitShort(output, output_mutex, "BIS", timer);
-    MyUpdateHook preInitLong(output, output_mutex, "BIL", timer);
-    MyUpdateHook preInitInfinite(output, output_mutex, "BII", timer);
-    mm.addMetricUpdateHook(preInitShort, 5);
-    mm.addMetricUpdateHook(preInitLong, 300);
-    mm.addMetricUpdateHook(preInitInfinite, 0);
+    MyUpdateHook preInitShort(output, output_mutex, "BIS", 5s, timer);
+    MyUpdateHook preInitLong(output, output_mutex, "BIL", 300s, timer);
+    MyUpdateHook preInitInfinite(output, output_mutex, "BII", 0s, timer);
+    mm.addMetricUpdateHook(preInitShort);
+    mm.addMetricUpdateHook(preInitLong);
+    mm.addMetricUpdateHook(preInitInfinite);
 
     // All hooks should get called during initialization
 
@@ -1000,12 +1000,12 @@ TEST_F(MetricManagerTest, test_update_hooks)
                       "consumer[1].tags[0] snaptest\n"));
     output << "Init done\n";
 
-    MyUpdateHook postInitShort(output, output_mutex, "AIS", timer);
-    MyUpdateHook postInitLong(output, output_mutex, "AIL", timer);
-    MyUpdateHook postInitInfinite(output, output_mutex, "AII", timer);
-    mm.addMetricUpdateHook(postInitShort, 5);
-    mm.addMetricUpdateHook(postInitLong, 400);
-    mm.addMetricUpdateHook(postInitInfinite, 0);
+    MyUpdateHook postInitShort(output, output_mutex, "AIS", 5s, timer);
+    MyUpdateHook postInitLong(output, output_mutex, "AIL", 400s, timer);
+    MyUpdateHook postInitInfinite(output, output_mutex, "AII", 0s, timer);
+    mm.addMetricUpdateHook(postInitShort);
+    mm.addMetricUpdateHook(postInitLong);
+    mm.addMetricUpdateHook(postInitInfinite);
 
     // After 5 seconds the short ones should get another.
 
diff --git a/metrics/src/vespa/metrics/metricmanager.cpp b/metrics/src/vespa/metrics/metricmanager.cpp
index f6601e710fc..0102d98e9c7 100644
--- a/metrics/src/vespa/metrics/metricmanager.cpp
+++ b/metrics/src/vespa/metrics/metricmanager.cpp
@@ -115,29 +115,26 @@ MetricManager::stop()
 }
 
 void
-MetricManager::addMetricUpdateHook(UpdateHook& hook, uint32_t period)
+MetricManager::addMetricUpdateHook(UpdateHook& hook)
 {
-    hook.setPeriod(vespalib::from_s(period));
     hook.updateNextCall(_timer->getTime());
     std::lock_guard sync(_waiter);
-        // If we've already initialized manager, log period has been set.
-        // In this case. Call first time after period
-    if (period == 0) {
-        for (UpdateHook * sHook : _snapshotUpdateHooks) {
-            if (sHook == &hook) {
+    if ( hook.is_periodic() ) {
+        for (UpdateHook * pHook : _periodicUpdateHooks) {
+            if (pHook == &hook) {
                 LOG(warning, "Update hook already registered");
                 return;
             }
         }
-        _snapshotUpdateHooks.push_back(&hook);
+        _periodicUpdateHooks.push_back(&hook);
     } else {
-        for (UpdateHook * pHook : _periodicUpdateHooks) {
-            if (pHook == &hook) {
+        for (UpdateHook * sHook : _snapshotUpdateHooks) {
+            if (sHook == &hook) {
                 LOG(warning, "Update hook already registered");
                 return;
             }
         }
-        _periodicUpdateHooks.push_back(&hook);
+        _snapshotUpdateHooks.push_back(&hook);
     }
 }
 
@@ -146,16 +143,16 @@ MetricManager::removeMetricUpdateHook(UpdateHook& hook)
 {
     std::lock_guard sync(_waiter);
     if (hook.is_periodic()) {
-        for (auto it = _snapshotUpdateHooks.begin(); it != _snapshotUpdateHooks.end(); it++) {
+        for (auto it = _periodicUpdateHooks.begin(); it != _periodicUpdateHooks.end(); it++) {
             if (*it == &hook) {
-                _snapshotUpdateHooks.erase(it);
+                _periodicUpdateHooks.erase(it);
                 return;
             }
         }
     } else {
-        for (auto it = _periodicUpdateHooks.begin(); it != _periodicUpdateHooks.end(); it++) {
+        for (auto it = _snapshotUpdateHooks.begin(); it != _snapshotUpdateHooks.end(); it++) {
             if (*it == &hook) {
-                _periodicUpdateHooks.erase(it);
+                _snapshotUpdateHooks.erase(it);
                 return;
             }
         }
diff --git a/metrics/src/vespa/metrics/metricmanager.h b/metrics/src/vespa/metrics/metricmanager.h
index db5bb5090f7..a53bad72641 100644
--- a/metrics/src/vespa/metrics/metricmanager.h
+++ b/metrics/src/vespa/metrics/metricmanager.h
@@ -136,7 +136,7 @@ public:
      * snapshotting and metric logging, to make the metrics the best as they can
      * be at those occasions.
      *
-     * @param period Period in seconds for how often callback should be called.
+     * @param period Period for how often callback should be called.
      *               The default value of 0, means only before snapshotting or
      *               logging, while another value will give callbacks each
      *               period seconds. Expensive metrics to calculate will
@@ -145,7 +145,7 @@ public:
      *               seconds or so. Any value of period >= the smallest snapshot
      *               time will behave identically as if period is set to 0.
      */
-    void addMetricUpdateHook(UpdateHook&, uint32_t period);
+    void addMetricUpdateHook(UpdateHook&);
 
     /** Remove a metric update hook so it won't get any more updates. */
     void removeMetricUpdateHook(UpdateHook&);
diff --git a/metrics/src/vespa/metrics/updatehook.h b/metrics/src/vespa/metrics/updatehook.h
index a8f158f713e..997bdf0b5a4 100644
--- a/metrics/src/vespa/metrics/updatehook.h
+++ b/metrics/src/vespa/metrics/updatehook.h
@@ -28,24 +28,26 @@ class MetricManager;
 class UpdateHook {
 public:
     using MetricLockGuard = metrics::MetricLockGuard;
-    UpdateHook(const char* name) : _name(name), _nextCall(), _period(vespalib::duration::zero()) {}
+    UpdateHook(const char* name, vespalib::duration period)
+        : _name(name),
+          _period(period),
+          _nextCall()
+    {}
     virtual ~UpdateHook() = default;
     virtual void updateMetrics(const MetricLockGuard & guard) = 0;
     const char* getName() const { return _name; }
     void updateNextCall() { updateNextCall(_nextCall); }
-    void updateNextCall(time_point now) { _nextCall = now + _period; }
-    bool is_periodic() const noexcept { return _period == vespalib::duration::zero(); }
+    void updateNextCall(time_point now) { setNextCall(now + _period); }
+    bool is_periodic() const noexcept { return _period != vespalib::duration::zero(); }
     bool expired(time_point now) { return _nextCall <= now; }
     bool has_valid_expiry() const noexcept { return _nextCall != time_point(); }
     vespalib::duration getPeriod() const noexcept { return _period; }
     time_point getNextCall() const noexcept { return _nextCall; }
-    // This should be moved to constructor
-    void setPeriod(vespalib::duration period) { _period = period; }
     void setNextCall(time_point now) { _nextCall = now; }
 private:
-    const char*         _name;
-    time_point          _nextCall;
-    vespalib::duration  _period;
+    const char*               _name;
+    const vespalib::duration  _period;
+    time_point                _nextCall;
 };
 
 }