diff options
8 files changed, 45 insertions, 46 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; }; } diff --git a/searchcore/src/vespa/searchcore/proton/metrics/metrics_engine.cpp b/searchcore/src/vespa/searchcore/proton/metrics/metrics_engine.cpp index 1ccb3956fc2..4f7e0e66d9f 100644 --- a/searchcore/src/vespa/searchcore/proton/metrics/metrics_engine.cpp +++ b/searchcore/src/vespa/searchcore/proton/metrics/metrics_engine.cpp @@ -42,7 +42,7 @@ MetricsEngine::start(const config::ConfigUri &) void MetricsEngine::addMetricsHook(metrics::UpdateHook &hook) { - _manager->addMetricUpdateHook(hook, 5); + _manager->addMetricUpdateHook(hook); } void diff --git a/searchcore/src/vespa/searchcore/proton/server/documentdb.cpp b/searchcore/src/vespa/searchcore/proton/server/documentdb.cpp index 9a84383c2f4..3f28f75c521 100644 --- a/searchcore/src/vespa/searchcore/proton/server/documentdb.cpp +++ b/searchcore/src/vespa/searchcore/proton/server/documentdb.cpp @@ -94,7 +94,7 @@ class MetricsUpdateHook : public metrics::UpdateHook { DocumentDB &_db; public: explicit MetricsUpdateHook(DocumentDB &s) - : metrics::UpdateHook("documentdb-hook"), + : metrics::UpdateHook("documentdb-hook", 5s), _db(s) {} void updateMetrics(const MetricLockGuard & guard) override { diff --git a/searchcore/src/vespa/searchcore/proton/server/proton.cpp b/searchcore/src/vespa/searchcore/proton/server/proton.cpp index c508706ad28..8ec904760cf 100644 --- a/searchcore/src/vespa/searchcore/proton/server/proton.cpp +++ b/searchcore/src/vespa/searchcore/proton/server/proton.cpp @@ -136,7 +136,7 @@ struct MetricsUpdateHook : metrics::UpdateHook { Proton &self; explicit MetricsUpdateHook(Proton &s) - : metrics::UpdateHook("proton-hook"), + : metrics::UpdateHook("proton-hook", 5s), self(s) {} void updateMetrics(const MetricLockGuard &guard) override { diff --git a/storage/src/vespa/storageframework/defaultimplementation/component/componentregisterimpl.cpp b/storage/src/vespa/storageframework/defaultimplementation/component/componentregisterimpl.cpp index 40c2cc3b111..74d58244636 100644 --- a/storage/src/vespa/storageframework/defaultimplementation/component/componentregisterimpl.cpp +++ b/storage/src/vespa/storageframework/defaultimplementation/component/componentregisterimpl.cpp @@ -126,8 +126,8 @@ namespace { struct MetricHookWrapper : public metrics::UpdateHook { MetricUpdateHook& _hook; - MetricHookWrapper(vespalib::stringref name, MetricUpdateHook& hook) - : metrics::UpdateHook(name.data()), // Expected to point to static name + MetricHookWrapper(vespalib::stringref name, MetricUpdateHook& hook, vespalib::duration period) + : metrics::UpdateHook(name.data(), period), // Expected to point to static name _hook(hook) { } @@ -142,8 +142,8 @@ ComponentRegisterImpl::registerUpdateHook(vespalib::stringref name, vespalib::duration period) { std::lock_guard lock(_componentLock); - auto hookPtr = std::make_unique<MetricHookWrapper>(name, hook); - _metricManager->addMetricUpdateHook(*hookPtr, vespalib::to_s(period)); + auto hookPtr = std::make_unique<MetricHookWrapper>(name, hook, period); + _metricManager->addMetricUpdateHook(*hookPtr); _hooks.emplace_back(std::move(hookPtr)); } |