From 0df29582f6d9d5e373cca7a5b71b2fd1e5d75179 Mon Sep 17 00:00:00 2001 From: Henning Baldersheim Date: Tue, 28 Feb 2023 11:10:14 +0000 Subject: GC xml output of metrics. --- metrics/src/tests/metricmanagertest.cpp | 153 +++--------- metrics/src/vespa/metrics/CMakeLists.txt | 1 - metrics/src/vespa/metrics/metricmanager.cpp | 277 +++++++++------------ metrics/src/vespa/metrics/metricmanager.h | 21 +- metrics/src/vespa/metrics/xmlwriter.cpp | 112 --------- metrics/src/vespa/metrics/xmlwriter.h | 30 --- .../vespa/storage/common/statusmetricconsumer.cpp | 14 +- 7 files changed, 157 insertions(+), 451 deletions(-) delete mode 100644 metrics/src/vespa/metrics/xmlwriter.cpp delete mode 100644 metrics/src/vespa/metrics/xmlwriter.h diff --git a/metrics/src/tests/metricmanagertest.cpp b/metrics/src/tests/metricmanagertest.cpp index 98d03514de0..1ba1cad9463 100644 --- a/metrics/src/tests/metricmanagertest.cpp +++ b/metrics/src/tests/metricmanagertest.cpp @@ -5,12 +5,10 @@ #include #include #include -#include #include #include #include #include -#include #include #include #include @@ -57,7 +55,7 @@ SubMetricSet::SubMetricSet(const Metric::String & name, MetricSet* owner) valsum.addMetricToSum(val1); valsum.addMetricToSum(val2); } -SubMetricSet::~SubMetricSet() { } +SubMetricSet::~SubMetricSet() = default; struct MultiSubMetricSet { @@ -167,20 +165,15 @@ getMatchedMetrics(const vespalib::string& config) MetricLockGuard g(mm.getMetricLock()); mm.visit(g, mm.getActiveMetrics(g), visitor, "consumer"); - MetricManager::ConsumerSpec::SP consumerSpec( - mm.getConsumerSpec(g, "consumer")); - return std::pair( - visitor.toString(), - consumerSpec.get() ? consumerSpec->toString() - : "Non-existing consumer"); + MetricManager::ConsumerSpec::SP consumerSpec(mm.getConsumerSpec(g, "consumer")); + return { visitor.toString(), consumerSpec ? consumerSpec->toString() : "Non-existing consumer" }; } } #define ASSERT_CONSUMER_MATCH(name, expected, config) \ { \ - std::pair consumerMatch( \ - getMatchedMetrics(config)); \ + std::pair consumerMatch(getMatchedMetrics(config)); \ EXPECT_EQ("\n" + expected, "\n" + consumerMatch.first) << (name + std::string(": ") + consumerMatch.second); \ } @@ -391,8 +384,7 @@ struct BriefValuePrinter : public MetricVisitor { } }; -bool waitForTimeProcessed(const MetricManager& mm, - time_t processtime, uint32_t timeout = 120) +bool waitForTimeProcessed(const MetricManager& mm, time_t processtime, uint32_t timeout = 120) { uint32_t lastchance = time(0) + timeout; while (time(0) < lastchance) { @@ -403,23 +395,20 @@ bool waitForTimeProcessed(const MetricManager& mm, return false; } -std::string dumpAllSnapshots(const MetricManager& mm, - const std::string& consumer) +std::string dumpAllSnapshots(const MetricManager& mm, const std::string& consumer) { std::ostringstream ost; ost << "\n"; { MetricLockGuard metricLock(mm.getMetricLock()); BriefValuePrinter briefValuePrinter; - mm.visit(metricLock, mm.getActiveMetrics(metricLock), - briefValuePrinter, consumer); + mm.visit(metricLock, mm.getActiveMetrics(metricLock), briefValuePrinter, consumer); ost << "Current: " << briefValuePrinter.ost.str() << "\n"; } { MetricLockGuard metricLock(mm.getMetricLock()); BriefValuePrinter briefValuePrinter; - mm.visit(metricLock, mm.getTotalMetricSnapshot(metricLock), - briefValuePrinter, consumer); + mm.visit(metricLock, mm.getTotalMetricSnapshot(metricLock), briefValuePrinter, consumer); ost << "Total: " << briefValuePrinter.ost.str() << "\n"; } std::vector periods; @@ -429,17 +418,15 @@ std::string dumpAllSnapshots(const MetricManager& mm, } for (uint32_t i=0; i timerImpl(timer); + auto timerImpl = std::make_unique(1000); + FakeTimer & timer = *timerImpl; TestMetricSet mySet; MetricManager mm(std::move(timerImpl)); { @@ -522,7 +506,7 @@ TEST_F(MetricManagerTest, test_snapshots) mySet.val10.a.val1.addValue(7); mySet.val10.a.val2.addValue(2); mySet.val10.b.val1.addValue(1); - timer->add_time(5 * 60); + timer.add_time(5 * 60); ASSERT_PROCESS_TIME(mm, 1000 + 5 * 60); ASSERT_VALUES(mm, 5 * 60, "2,4,4,1,7,9,1,1,8,2,10"); ASSERT_VALUES(mm, 60 * 60, ""); @@ -536,7 +520,7 @@ TEST_F(MetricManagerTest, test_snapshots) mySet.val10.a.val1.addValue(8); mySet.val10.a.val2.addValue(3); mySet.val10.b.val1.addValue(2); - timer->add_time(5 * 60); + timer.add_time(5 * 60); ASSERT_PROCESS_TIME(mm, 1000 + 5 * 60 * 2); ASSERT_VALUES(mm, 5 * 60, "4,5,5,1,8,11,2,2,10,3,13"); ASSERT_VALUES(mm, 60 * 60, ""); @@ -544,7 +528,7 @@ TEST_F(MetricManagerTest, test_snapshots) // Adding another five minute period where nothing have happened. // Metric for last 5 minutes should be 0. - timer->add_time(5 * 60); + timer.add_time(5 * 60); ASSERT_PROCESS_TIME(mm, 1000 + 5 * 60 * 3); ASSERT_VALUES(mm, 5 * 60, "0,0,0,0,0,0,0,0,0,0,0"); ASSERT_VALUES(mm, 60 * 60, ""); @@ -555,7 +539,7 @@ TEST_F(MetricManagerTest, test_snapshots) mySet.val6.addValue(6); for (uint32_t i=0; i<9; ++i) { // 9 x 5 minutes. Avoid snapshot bumping // due to taking snapshots in the past - timer->add_time(5 * 60); + timer.add_time(5 * 60); ASSERT_PROCESS_TIME(mm, 1000 + 5 * 60 * (4 + i)); } ASSERT_VALUES(mm, 5 * 60, "0,0,0,0,0,0,0,0,0,0,0"); @@ -570,82 +554,6 @@ TEST_F(MetricManagerTest, test_snapshots) ASSERT_VALUES(mm, 0 * 60, "0,0,0,0,0,0,0,0,0,0,0"); } -TEST_F(MetricManagerTest, test_xml_output) -{ - FakeTimer* timer = new FakeTimer(1000); - std::unique_ptr timerImpl(timer); - MetricManager mm(std::move(timerImpl)); - TestMetricSet mySet; - { - MetricLockGuard lockGuard(mm.getMetricLock()); - mm.registerMetric(lockGuard, mySet.set); - } - - // Initialize metric manager to get snapshots created. - mm.init(ConfigUri("raw:" - "consumer[2]\n" - "consumer[0].name snapper\n" - "consumer[0].tags[1]\n" - "consumer[0].tags[0] snaptest\n" - "consumer[1].name log\n" - "consumer[1].tags[1]\n" - "consumer[1].tags[0] snaptest\n")); - - takeSnapshots(mm, 1000); - - // Adding metrics to have some values in them - mySet.val6.addValue(2); - mySet.val9.val1.addValue(4); - mySet.val10.count.inc(); - mySet.val10.a.val1.addValue(7); - mySet.val10.a.val2.addValue(2); - mySet.val10.b.val1.addValue(1); - - timer->set_time(1300); - takeSnapshots(mm, 1300); - - std::string expected( - "'\n" - " \n" - " \n" - " \n" - " \n" - " \n" - " \n" - " \n" - " \n" - " \n" - " \n" - " \n" - " \n" - " \n" - " \n" - " \n" - " \n" - " \n" - " \n" - " \n" - " \n" - " \n" - " \n" - " \n" - "'"); - - std::ostringstream ost; - vespalib::XmlOutputStream xos(ost, " "); - XmlWriter writer(xos, 300, 0); - { - MetricLockGuard lockGuard(mm.getMetricLock()); - mm.visit(lockGuard, mm.getMetricSnapshot(lockGuard, 300, false), - writer, "snapper"); - } - std::string actual(ost.str()); - // Not bothering to match all the nitty gritty details as it will test - // more than it needs to. Just be here in order to check on XML output - // easily if needed. - EXPECT_EQ(expected, "'" + actual + "'"); -} - TEST_F(MetricManagerTest, test_json_output) { FakeTimer* timer = new FakeTimer(1000); @@ -683,8 +591,7 @@ TEST_F(MetricManagerTest, test_json_output) JsonWriter writer(jsonStream); { MetricLockGuard lockGuard(mm.getMetricLock()); - mm.visit(lockGuard, mm.getMetricSnapshot(lockGuard, 300, false), - writer, "snapper"); + mm.visit(lockGuard, mm.getMetricSnapshot(lockGuard, 300, false), writer, "snapper"); } jsonStream.finalize(); std::string jsonData = as.str(); @@ -773,22 +680,19 @@ struct MetricSnapshotTestFixture JsonWriter writer(jsonStream); { MetricLockGuard lockGuard(manager.getMetricLock()); - manager.visit(lockGuard, manager.getMetricSnapshot(lockGuard, 300, false), - writer, "snapper"); + manager.visit(lockGuard, manager.getMetricSnapshot(lockGuard, 300, false), writer, "snapper"); } jsonStream.finalize(); return as.str(); } - std::string renderLastSnapshotAsText( - const std::string& matchPattern = ".*") const + std::string renderLastSnapshotAsText(const std::string& matchPattern = ".*") const { std::ostringstream ss; TextWriter writer(ss, 300, matchPattern, true); { MetricLockGuard lockGuard(manager.getMetricLock()); - manager.visit(lockGuard, manager.getMetricSnapshot(lockGuard, 300, false), - writer, "snapper"); + manager.visit(lockGuard, manager.getMetricSnapshot(lockGuard, 300, false), writer, "snapper"); } return ss.str(); } @@ -819,8 +723,7 @@ public: } std::string nthMetricDimension(size_t metricIndex, const std::string& key) { - return nthMetric(metricIndex)["dimensions"][key] - .asString().make_string(); + return nthMetric(metricIndex)["dimensions"][key].asString().make_string(); } // Verify that the nth metric has the given name and the given set of @@ -843,7 +746,7 @@ JsonMetricWrapper::JsonMetricWrapper(const std::string& jsonText) { vespalib::slime::JsonFormat::decode(vespalib::Memory(jsonText), _tree); } -JsonMetricWrapper::~JsonMetricWrapper() { } +JsonMetricWrapper::~JsonMetricWrapper() = default; struct DimensionTestMetricSet : MetricSet { @@ -851,7 +754,7 @@ struct DimensionTestMetricSet : MetricSet LongCountMetric val2; DimensionTestMetricSet(MetricSet* owner = nullptr); - ~DimensionTestMetricSet(); + ~DimensionTestMetricSet() override; }; DimensionTestMetricSet::DimensionTestMetricSet(MetricSet* owner) @@ -859,7 +762,7 @@ DimensionTestMetricSet::DimensionTestMetricSet(MetricSet* owner) val1("val1", {{"tag1"}}, "val1 desc", this), val2("val2", {{"baz", "superbaz"}}, "val2 desc", this) { } -DimensionTestMetricSet::~DimensionTestMetricSet() { } +DimensionTestMetricSet::~DimensionTestMetricSet() = default; } diff --git a/metrics/src/vespa/metrics/CMakeLists.txt b/metrics/src/vespa/metrics/CMakeLists.txt index 2441bf95c0b..62254a8d620 100644 --- a/metrics/src/vespa/metrics/CMakeLists.txt +++ b/metrics/src/vespa/metrics/CMakeLists.txt @@ -18,7 +18,6 @@ vespa_add_library(metrics updatehook.cpp valuemetric.cpp valuemetricvalues.cpp - xmlwriter.cpp $ INSTALL lib64 diff --git a/metrics/src/vespa/metrics/metricmanager.cpp b/metrics/src/vespa/metrics/metricmanager.cpp index a0e44ddbeac..da7b8f71fdf 100644 --- a/metrics/src/vespa/metrics/metricmanager.cpp +++ b/metrics/src/vespa/metrics/metricmanager.cpp @@ -22,6 +22,9 @@ LOG_SETUP(".metrics.manager"); namespace metrics { using Config = MetricsmanagerConfig; +using vespalib::IllegalStateException; +using vespalib::IllegalArgumentException; +using vespalib::make_string_short::fmt; MetricManager::ConsumerSpec::ConsumerSpec() = default; MetricManager::ConsumerSpec::~ConsumerSpec() = default; @@ -34,13 +37,12 @@ MetricManager::Timer::getTime() const { void MetricManager::assertMetricLockLocked(const MetricLockGuard& g) const { if ( ! g.owns(_waiter)) { - throw vespalib::IllegalArgumentException("Given lock does not lock the metric lock.", VESPA_STRLOC); + throw IllegalArgumentException("Given lock does not lock the metric lock.", VESPA_STRLOC); } } void -MetricManager::ConsumerSpec::print(std::ostream& out, bool verbose, - const std::string& indent) const +MetricManager::ConsumerSpec::print(std::ostream& out, bool verbose, const std::string& indent) const { (void) verbose; out << "ConsumerSpec("; @@ -63,6 +65,10 @@ MetricManager::ConsumerSpec::addMemoryUsage(MemoryConsumption& mc) const } } +MetricManager::MetricManager() + : MetricManager(std::make_unique()) +{ } + MetricManager::MetricManager(std::unique_ptr timer) : _activeMetrics("Active metrics showing updates since last snapshot"), _configSubscriber(), @@ -70,9 +76,7 @@ MetricManager::MetricManager(std::unique_ptr timer) _config(), _consumerConfig(), _snapshots(), - _totalMetrics(std::make_shared( - "Empty metrics before init", 0, _activeMetrics.getMetrics(), - false)), + _totalMetrics(std::make_shared("Empty metrics before init", 0, _activeMetrics.getMetrics(), false)), _timer(std::move(timer)), _lastProcessedTime(0), _snapshotUnsetMetrics(false), @@ -165,9 +169,8 @@ void MetricManager::init(const config::ConfigUri & uri, bool startThread) { if (isInitialized()) { - throw vespalib::IllegalStateException( - "The metric manager have already been initialized. " - "It can only be initialized once.", VESPA_STRLOC); + throw IllegalStateException("The metric manager have already been initialized. " + "It can only be initialized once.", VESPA_STRLOC); } LOG(debug, "Initializing metric manager."); _configSubscriber = std::make_unique(uri.getContext()); @@ -233,8 +236,10 @@ struct ConsumerMetricBuilder : public MetricVisitor { bool nameRemoved; uint32_t metricCount; - Result() : tagAdded(false), tagRemoved(false), - nameAdded(false), nameRemoved(false), metricCount(0) {} + Result() + : tagAdded(false), tagRemoved(false), + nameAdded(false), nameRemoved(false), metricCount(0) + {} }; std::list result; @@ -353,9 +358,7 @@ ConsumerMetricBuilder::~ConsumerMetricBuilder() = default; void MetricManager::checkMetricsAltered(const MetricLockGuard & guard) { - if (_activeMetrics.getMetrics().isRegistrationAltered() - || _consumerConfigChanged) - { + if (_activeMetrics.getMetrics().isRegistrationAltered() || _consumerConfigChanged) { handleMetricsAltered(guard); } } @@ -385,8 +388,8 @@ MetricManager::handleMetricsAltered(const MetricLockGuard & guard) } LOG(debug, "Recreating snapshots to include altered metrics"); _totalMetrics->recreateSnapshot(_activeMetrics.getMetrics(), _snapshotUnsetMetrics); - for (uint32_t i=0; i<_snapshots.size(); ++i) { - _snapshots[i]->recreateSnapshot(_activeMetrics.getMetrics(), _snapshotUnsetMetrics); + for (const auto & snapshot: _snapshots) { + snapshot->recreateSnapshot(_activeMetrics.getMetrics(), _snapshotUnsetMetrics); } LOG(debug, "Setting new consumer config. Clearing dirty flag"); _consumerConfig.swap(configMap); @@ -394,24 +397,25 @@ MetricManager::handleMetricsAltered(const MetricLockGuard & guard) } namespace { - bool setSnapshotName(std::ostream& out, const char* name, uint32_t length, uint32_t period) - { - if (length % period != 0) return false; - out << (length / period) << ' ' << name; - if (length / period != 1) out << "s"; - return true; - } + +bool +setSnapshotName(std::ostream& out, const char* name, uint32_t length, uint32_t period) { + if (length % period != 0) return false; + out << (length / period) << ' ' << name; + if (length / period != 1) out << "s"; + return true; +} + } std::vector MetricManager::createSnapshotPeriods(const Config& config) { std::vector result; - try{ - for (uint32_t i=0; i confi std::ostringstream ost; config::OstreamConfigWriter w(ost); w.write(*config); - LOG(debug, "Received new config for metric manager: %s", - ost.str().c_str()); + LOG(debug, "Received new config for metric manager: %s", ost.str().c_str()); } if (_snapshots.empty()) { LOG(debug, "Initializing snapshots as this is first configure call"); @@ -466,18 +466,12 @@ MetricManager::configure(const MetricLockGuard & , std::unique_ptr confi time_t currentTime(_timer->getTime()); _activeMetrics.setFromTime(currentTime); uint32_t count = 1; - for (uint32_t i = 0; i< snapshotPeriods.size(); ++i) - { + for (uint32_t i = 0; i< snapshotPeriods.size(); ++i) { uint32_t nextCount = 1; if (i + 1 < snapshotPeriods.size()) { - nextCount = snapshotPeriods[i + 1].first - / snapshotPeriods[i].first; - if (snapshotPeriods[i + 1].first - % snapshotPeriods[i].first != 0) - { - throw vespalib::IllegalStateException( - "Snapshot periods must be multiplum of each other", - VESPA_STRLOC); + nextCount = snapshotPeriods[i + 1].first / snapshotPeriods[i].first; + if ((snapshotPeriods[i + 1].first % snapshotPeriods[i].first) != 0) { + throw IllegalStateException("Snapshot periods must be multiplum of each other",VESPA_STRLOC); } } _snapshots.push_back(std::make_shared( @@ -489,9 +483,7 @@ MetricManager::configure(const MetricLockGuard & , std::unique_ptr confi _totalMetrics = std::make_shared("All time snapshot", 0, _activeMetrics.getMetrics(), _snapshotUnsetMetrics); _totalMetrics->reset(currentTime); } - if (_config.get() == 0 - || _config->consumer.size() != config->consumer.size()) - { + if (_config.get() == 0 || (_config->consumer.size() != config->consumer.size())) { _consumerConfigChanged = true; } else { for (uint32_t i=0; i<_config->consumer.size(); ++i) { @@ -519,61 +511,55 @@ MetricManager::getConsumerSpec(const MetricLockGuard &, const Metric::String& co namespace { - struct ConsumerMetricVisitor : public MetricVisitor { - const MetricManager::ConsumerSpec& _metricsToMatch; - MetricVisitor& _client; +struct ConsumerMetricVisitor : public MetricVisitor { + const MetricManager::ConsumerSpec& _metricsToMatch; + MetricVisitor& _client; #ifdef VERIFY_ALL_METRICS_VISITED - std::set _visitedMetrics; + std::set _visitedMetrics; #endif - ConsumerMetricVisitor(const MetricManager::ConsumerSpec& spec, - MetricVisitor& clientVisitor) - : _metricsToMatch(spec), _client(clientVisitor) {} + ConsumerMetricVisitor(const MetricManager::ConsumerSpec& spec, MetricVisitor& clientVisitor) + : _metricsToMatch(spec), _client(clientVisitor) + {} - bool visitMetricSet(const MetricSet& metricSet, - bool autoGenerated) override - { - if (metricSet.isTopSet()) return true; - return (_metricsToMatch.contains(metricSet) - && _client.visitMetricSet(metricSet, autoGenerated)); - } - void doneVisitingMetricSet(const MetricSet& metricSet) override { - if (!metricSet.isTopSet()) { + bool visitMetricSet(const MetricSet& metricSet, bool autoGenerated) override { + if (metricSet.isTopSet()) return true; + return (_metricsToMatch.contains(metricSet) + && _client.visitMetricSet(metricSet, autoGenerated)); + } + void doneVisitingMetricSet(const MetricSet& metricSet) override { + if (!metricSet.isTopSet()) { #ifdef VERIFY_ALL_METRICS_VISITED - _visitedMetrics.insert(metricSet.getPath()); + _visitedMetrics.insert(metricSet.getPath()); #endif - _client.doneVisitingMetricSet(metricSet); - } + _client.doneVisitingMetricSet(metricSet); } - bool visitCountMetric(const AbstractCountMetric& metric, - bool autoGenerated) override - { - if (_metricsToMatch.contains(metric)) { + } + bool visitCountMetric(const AbstractCountMetric& metric, bool autoGenerated) override { + if (_metricsToMatch.contains(metric)) { #ifdef VERIFY_ALL_METRICS_VISITED - _visitedMetrics.insert(metric.getPath()); + _visitedMetrics.insert(metric.getPath()); #endif - return _client.visitCountMetric(metric, autoGenerated); - } - return true; + return _client.visitCountMetric(metric, autoGenerated); } - bool visitValueMetric(const AbstractValueMetric& metric, - bool autoGenerated) override - { - if (_metricsToMatch.contains(metric)) { + return true; + } + bool visitValueMetric(const AbstractValueMetric& metric, bool autoGenerated) override { + if (_metricsToMatch.contains(metric)) { #ifdef VERIFY_ALL_METRICS_VISITED - _visitedMetrics.insert(metric.getPath()); + _visitedMetrics.insert(metric.getPath()); #endif - return _client.visitValueMetric(metric, autoGenerated); - } - return true; + return _client.visitValueMetric(metric, autoGenerated); } - }; + return true; + } +}; } void -MetricManager::visit(const MetricLockGuard & guard, const MetricSnapshot& snapshot, MetricVisitor& visitor, - const std::string& consumer) const +MetricManager::visit(const MetricLockGuard & guard, const MetricSnapshot& snapshot, + MetricVisitor& visitor, const std::string& consumer) const { if (visitor.visitSnapshot(snapshot)) { if (consumer == "") { @@ -593,9 +579,7 @@ MetricManager::visit(const MetricLockGuard & guard, const MetricSnapshot& snapsh } #endif } else { - LOGBP(debug, - "Requested metrics for non-defined consumer '%s'.", - consumer.c_str()); + LOGBP(debug, "Requested metrics for non-defined consumer '%s'.", consumer.c_str()); } } visitor.doneVisitingSnapshot(snapshot); @@ -616,39 +600,31 @@ MetricManager::getSnapshotPeriods(const MetricLockGuard& l) const // Client should have grabbed metrics lock before doing this const MetricSnapshot& -MetricManager::getMetricSnapshot(const MetricLockGuard& l, - uint32_t period, bool getInProgressSet) const +MetricManager::getMetricSnapshot(const MetricLockGuard& l, uint32_t period, bool getInProgressSet) const { assertMetricLockLocked(l); - for (uint32_t i=0; i<_snapshots.size(); ++i) { - if (_snapshots[i]->getPeriod() == period) { - if (_snapshots[i]->getCount() == 1 && getInProgressSet) { - throw vespalib::IllegalStateException( - "No temporary snapshot for set " - + _snapshots[i]->getName(), VESPA_STRLOC); + for (const auto & snapshot : _snapshots) { + if (snapshot->getPeriod() == period) { + if (snapshot->getCount() == 1 && getInProgressSet) { + throw IllegalStateException("No temporary snapshot for set " + snapshot->getName(), VESPA_STRLOC); } - return _snapshots[i]->getSnapshot(getInProgressSet); + return snapshot->getSnapshot(getInProgressSet); } } - std::ostringstream ost; - ost << "No snapshot for period of length " << period << " exist."; - throw vespalib::IllegalArgumentException(ost.str(), VESPA_STRLOC); + throw IllegalArgumentException(fmt("No snapshot for period of length %u exist.", period), VESPA_STRLOC); } // Client should have grabbed metrics lock before doing this const MetricSnapshotSet& -MetricManager::getMetricSnapshotSet(const MetricLockGuard& l, - uint32_t period) const +MetricManager::getMetricSnapshotSet(const MetricLockGuard& l, uint32_t period) const { assertMetricLockLocked(l); - for (uint32_t i=0; i<_snapshots.size(); ++i) { - if (_snapshots[i]->getPeriod() == period) { - return *_snapshots[i]; + for (const auto & snapshot : _snapshots) { + if (snapshot->getPeriod() == period) { + return *snapshot; } } - std::ostringstream ost; - ost << "No snapshot set for period of length " << period << " exist."; - throw vespalib::IllegalArgumentException(ost.str(), VESPA_STRLOC); + throw IllegalArgumentException(fmt("No snapshot set for period of length %u exist.", period), VESPA_STRLOC); } void @@ -661,9 +637,8 @@ MetricManager::timeChangedNotification() const void MetricManager::updateMetrics(bool includeSnapshotOnlyHooks) { - LOG(debug, "Calling metric update hooks%s.", - includeSnapshotOnlyHooks ? ", including snapshot hooks" : ""); - // Ensure we're not in the way of the background thread + LOG(debug, "Calling metric update hooks%s.", includeSnapshotOnlyHooks ? ", including snapshot hooks" : ""); + // Ensure we're not in the way of the background thread MetricLockGuard sync(_waiter); LOG(debug, "Giving %zu periodic update hooks.", _periodicUpdateHooks.size()); updatePeriodicMetrics(sync, 0, true); @@ -677,6 +652,7 @@ MetricManager::updateMetrics(bool includeSnapshotOnlyHooks) time_t MetricManager::updatePeriodicMetrics(const MetricLockGuard & guard, time_t updateTime, bool outOfSchedule) { + assertMetricLockLocked(guard); time_t nextUpdateTime = std::numeric_limits::max(); time_t preTime = _timer->getTimeInMilliSecs(); for (auto hook : _periodicUpdateHooks) { @@ -684,10 +660,8 @@ MetricManager::updatePeriodicMetrics(const MetricLockGuard & guard, time_t updat hook->updateMetrics(guard); if (hook->_nextCall + hook->_period < updateTime) { if (hook->_nextCall != 0) { - LOG(debug, "Updated hook %s at time %" PRIu64 ", but next " - "run in %u seconds have already passed as time" - " is %" PRIu64 ". Bumping next call to current " - "time + period.", + LOG(debug, "Updated hook %s at time %" PRIu64 ", but next run in %u seconds have already passed as " + "time is %" PRIu64 ". Bumping next call to current time + period.", hook->_name, static_cast(hook->_nextCall), hook->_period, static_cast(updateTime)); } hook->_nextCall = updateTime + hook->_period; @@ -712,9 +686,10 @@ MetricManager::updatePeriodicMetrics(const MetricLockGuard & guard, time_t updat void MetricManager::updateSnapshotMetrics(const MetricLockGuard & guard) { + assertMetricLockLocked(guard); time_t preTime = _timer->getTimeInMilliSecs(); - for (auto it = _snapshotUpdateHooks.begin(); it != _snapshotUpdateHooks.end(); ++it) { - (**it).updateMetrics(guard); + for (const auto & hook : _snapshotUpdateHooks) { + hook->updateMetrics(guard); time_t postTime = _timer->getTimeInMilliSecs(); _snapshotHookLatency.addValue(postTime - preTime); preTime = postTime; @@ -738,8 +713,8 @@ MetricManager::reset(time_t currentTime) // to avoid conflict with adding/removal of metrics std::lock_guard waiterLock(_waiter); _activeMetrics.reset(currentTime); - for (uint32_t i=0; i<_snapshots.size(); ++i) { - _snapshots[i]->reset(currentTime); + for (const auto & snapshot : _snapshots) { + snapshot->reset(currentTime); } _totalMetrics->reset(currentTime); time_t postTime = _timer->getTimeInMilliSecs(); @@ -780,11 +755,10 @@ MetricManager::run() time_t MetricManager::tick(const MetricLockGuard & guard, time_t currentTime) { - LOG(spam, "Worker thread starting to process for time %" PRIu64 ".", - static_cast(currentTime)); + LOG(spam, "Worker thread starting to process for time %" PRIu64 ".", static_cast(currentTime)); // Check for new config and reconfigure - if (_configSubscriber.get() && _configSubscriber->nextConfigNow()) { + if (_configSubscriber && _configSubscriber->nextConfigNow()) { configure(guard, _configHandle->getConfig()); } @@ -811,8 +785,7 @@ MetricManager::tick(const MetricLockGuard & guard, time_t currentTime) // Do snapshotting if it is time if (nextWorkTime <= currentTime) takeSnapshots(guard, nextWorkTime); - _lastProcessedTime.store(nextWorkTime <= currentTime ? nextWorkTime : currentTime, - std::memory_order_relaxed); + _lastProcessedTime.store(nextWorkTime <= currentTime ? nextWorkTime : currentTime, std::memory_order_relaxed); LOG(spam, "Worker thread done with processing for time %" PRIu64 ".", static_cast(_lastProcessedTime.load(std::memory_order_relaxed))); time_t next = _snapshots[0]->getPeriod() + _snapshots[0]->getToTime(); @@ -821,8 +794,9 @@ MetricManager::tick(const MetricLockGuard & guard, time_t currentTime) } void -MetricManager::takeSnapshots(const MetricLockGuard &, time_t timeToProcess) +MetricManager::takeSnapshots(const MetricLockGuard & guard, time_t timeToProcess) { + assertMetricLockLocked(guard); // If not time to do dump data from active snapshot yet, nothing to do if (!_snapshots[0]->timeForAnotherSnapshot(timeToProcess)) { LOG(spam, "Not time to process snapshot %s at time %" PRIu64 ". Current " @@ -840,67 +814,54 @@ MetricManager::takeSnapshots(const MetricLockGuard &, time_t timeToProcess) _activeMetrics.addToSnapshot(firstTarget, false, timeToProcess); _activeMetrics.addToSnapshot(*_totalMetrics, false, timeToProcess); _activeMetrics.reset(timeToProcess); - LOG(debug, "After snapshotting, " - "active metrics goes from %" PRIu64 " to %" PRIu64", " + LOG(debug, "After snapshotting, active metrics goes from %" PRIu64 " to %" PRIu64", " "and 5 minute metrics goes from %" PRIu64 " to %" PRIu64".", static_cast(_activeMetrics.getFromTime()), static_cast(_activeMetrics.getToTime()), static_cast(firstTarget.getFromTime()), static_cast(firstTarget.getToTime())); // Update later snapshots if it is time for it for (uint32_t i=1; i<_snapshots.size(); ++i) { - LOG(debug, "Adding data from last snapshot to building snapshot of " - "next period snapshot %s.", + LOG(debug, "Adding data from last snapshot to building snapshot of next period snapshot %s.", _snapshots[i]->getName().c_str()); MetricSnapshot& target(_snapshots[i]->getNextTarget()); - _snapshots[i-1]->getSnapshot().addToSnapshot( - target, false, timeToProcess); + _snapshots[i-1]->getSnapshot().addToSnapshot(target, false, timeToProcess); target.setToTime(timeToProcess); if (!_snapshots[i]->haveCompletedNewPeriod(timeToProcess)) { - LOG(debug, "Not time to roll snapshot %s yet. %u of %u snapshot " - "taken at time %" PRIu64 ", and period of %u is not up " - "yet as we're currently processing for time %" PRIu64 ".", - _snapshots[i]->getName().c_str(), - _snapshots[i]->getBuilderCount(), - _snapshots[i]->getCount(), - static_cast - (_snapshots[i]->getBuilderCount() * _snapshots[i]->getPeriod() - + _snapshots[i]->getFromTime()), - _snapshots[i]->getPeriod(), - static_cast(timeToProcess)); + LOG(debug, "Not time to roll snapshot %s yet. %u of %u snapshot taken at time %" PRIu64 ", and period of %u " + "is not up yet as we're currently processing for time %" PRIu64 ".", + _snapshots[i]->getName().c_str(), _snapshots[i]->getBuilderCount(), _snapshots[i]->getCount(), + static_cast(_snapshots[i]->getBuilderCount() * _snapshots[i]->getPeriod() + _snapshots[i]->getFromTime()), + _snapshots[i]->getPeriod(), static_cast(timeToProcess)); break; } else { LOG(debug, "Rolled snapshot %s at time %" PRIu64 ".", - _snapshots[i]->getName().c_str(), - static_cast(timeToProcess)); + _snapshots[i]->getName().c_str(), static_cast(timeToProcess)); } } time_t postTime = _timer->getTimeInMilliSecs(); - _snapshotLatency.addValue(postTime - preTime); + _snapshotLatency.addValue(postTime - preTime); } MemoryConsumption::UP MetricManager::getMemoryConsumption(const MetricLockGuard & guard) const { - (void) guard; - MemoryConsumption::UP mc(new MemoryConsumption); + assertMetricLockLocked(guard); + auto mc = std::make_unique(); mc->_consumerCount += _consumerConfig.size(); - mc->_consumerMeta += (sizeof(ConsumerSpec::SP) + sizeof(ConsumerSpec)) - * _consumerConfig.size(); - for (auto it = _consumerConfig.begin(); it != _consumerConfig.end(); ++it) { - mc->_consumerId += mc->getStringMemoryUsage( - it->first, mc->_consumerIdUnique) - + sizeof(Metric::String); - it->second->addMemoryUsage(*mc); + mc->_consumerMeta += (sizeof(ConsumerSpec::SP) + sizeof(ConsumerSpec)) * _consumerConfig.size(); + for (const auto & consumer : _consumerConfig) { + mc->_consumerId += mc->getStringMemoryUsage(consumer.first, mc->_consumerIdUnique) + sizeof(Metric::String); + consumer.second->addMemoryUsage(*mc); } uint32_t preTotal = mc->getTotalMemoryUsage(); _activeMetrics.addMemoryUsage(*mc); uint32_t postTotal = mc->getTotalMemoryUsage(); mc->addSnapShotUsage("active", postTotal - preTotal); preTotal = postTotal; - for (uint32_t i=0; i<_snapshots.size(); ++i) { - _snapshots[i]->addMemoryUsage(*mc); + for (const auto & snapshot : _snapshots) { + snapshot->addMemoryUsage(*mc); postTotal = mc->getTotalMemoryUsage(); - mc->addSnapShotUsage(_snapshots[i]->getName(), postTotal - preTotal); + mc->addSnapShotUsage(snapshot->getName(), postTotal - preTotal); preTotal = postTotal; } _totalMetrics->addMemoryUsage(*mc); diff --git a/metrics/src/vespa/metrics/metricmanager.h b/metrics/src/vespa/metrics/metricmanager.h index 7a34f894282..2c595186ea4 100644 --- a/metrics/src/vespa/metrics/metricmanager.h +++ b/metrics/src/vespa/metrics/metricmanager.h @@ -88,8 +88,7 @@ public: return (includedMetrics.find(m.getPath()) != includedMetrics.end()); } - void print(std::ostream& out, bool verbose, - const std::string& indent) const override; + void print(std::ostream& out, bool verbose, const std::string& indent) const override; void addMemoryUsage(MemoryConsumption&) const; }; @@ -126,7 +125,8 @@ private: bool stop_requested() const { return _stop_requested.load(std::memory_order_relaxed); } public: - MetricManager(std::unique_ptr timer = std::make_unique()); + MetricManager(); + MetricManager(std::unique_ptr timer); ~MetricManager(); void stop(); @@ -232,19 +232,16 @@ public: } /** While accessing the total metrics you should have the metric lock. */ - const MetricSnapshot& getTotalMetricSnapshot(const MetricLockGuard& l) const - { + const MetricSnapshot& getTotalMetricSnapshot(const MetricLockGuard& l) const { assertMetricLockLocked(l); return *_totalMetrics; } /** While accessing snapshots you should have the metric lock. */ - const MetricSnapshot& getMetricSnapshot( - const MetricLockGuard&, - uint32_t period, bool getInProgressSet = false) const; - const MetricSnapshotSet& getMetricSnapshotSet( - const MetricLockGuard&, uint32_t period) const; - bool hasTemporarySnapshot(const MetricLockGuard& l, uint32_t period) const - { return getMetricSnapshotSet(l, period).hasTemporarySnapshot(); } + const MetricSnapshot& getMetricSnapshot( const MetricLockGuard& guard, uint32_t period) const { + return getMetricSnapshot(guard, period, false); + } + const MetricSnapshot& getMetricSnapshot( const MetricLockGuard&, uint32_t period, bool getInProgressSet) const; + const MetricSnapshotSet& getMetricSnapshotSet(const MetricLockGuard&, uint32_t period) const; std::vector getSnapshotPeriods(const MetricLockGuard& l) const; diff --git a/metrics/src/vespa/metrics/xmlwriter.cpp b/metrics/src/vespa/metrics/xmlwriter.cpp deleted file mode 100644 index 11cb450e64d..00000000000 --- a/metrics/src/vespa/metrics/xmlwriter.cpp +++ /dev/null @@ -1,112 +0,0 @@ -// Copyright Yahoo. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root. - -#include "xmlwriter.h" -#include "countmetric.h" -#include "metricset.h" -#include "metricsnapshot.h" -#include "valuemetric.h" -#include -#include - -namespace metrics { - -XmlWriter::XmlWriter(vespalib::xml::XmlOutputStream& xos, - [[maybe_unused]] uint32_t period, int verbosity) - : _xos(xos), _verbosity(verbosity) {} - -bool -XmlWriter::visitSnapshot(const MetricSnapshot& snapshot) -{ - using namespace vespalib::xml; - _xos << XmlTag("snapshot") << XmlAttribute("name", snapshot.getName()) - << XmlAttribute("from", snapshot.getFromTime()) - << XmlAttribute("to", snapshot.getToTime()) - << XmlAttribute("period", snapshot.getPeriod()); - return true; -} - -void -XmlWriter::doneVisitingSnapshot(const MetricSnapshot&) -{ - using namespace vespalib::xml; - _xos << XmlEndTag(); -} - -bool -XmlWriter::visitMetricSet(const MetricSet& set, bool) -{ - using namespace vespalib::xml; - if (set.used() || _verbosity >= 2) { - _xos << XmlTag(set.getName(), XmlTagFlags::CONVERT_ILLEGAL_CHARACTERS); - printCommonXmlParts(set); - return true; - } - return false; -} -void -XmlWriter::doneVisitingMetricSet(const MetricSet&) { - using namespace vespalib::xml; - _xos << XmlEndTag(); -} - -bool -XmlWriter::visitCountMetric(const AbstractCountMetric& metric, bool) -{ - MetricValueClass::UP values(metric.getValues()); - if (!metric.inUse(*values) && _verbosity < 2) return true; - using namespace vespalib::xml; - std::ostringstream ost; - _xos << XmlTag(metric.getName(), XmlTagFlags::CONVERT_ILLEGAL_CHARACTERS) - << XmlAttribute(metric.sumOnAdd() - ? "count" : "value", values->toString("count")); - printCommonXmlParts(metric); - _xos << XmlEndTag(); - return true; -} - -bool -XmlWriter::visitValueMetric(const AbstractValueMetric& metric, bool) -{ - MetricValueClass::UP values(metric.getValues()); - if (!metric.inUse(*values) && _verbosity < 2) return true; - using namespace vespalib::xml; - _xos << XmlTag(metric.getName(), XmlTagFlags::CONVERT_ILLEGAL_CHARACTERS) - << XmlAttribute("average", values->getLongValue("count") == 0 - ? 0 : values->getDoubleValue("total") - / values->getDoubleValue("count")) - << XmlAttribute("last", values->toString("last")); - if (!metric.summedAverage()) { - if (values->getLongValue("count") > 0) { - _xos << XmlAttribute("min", values->toString("min")) - << XmlAttribute("max", values->toString("max")); - } - _xos << XmlAttribute("count", values->getLongValue("count")); - if (_verbosity >= 2) { - _xos << XmlAttribute("total", values->toString("total")); - } - } - printCommonXmlParts(metric); - _xos << XmlEndTag(); - return true; -} - -void -XmlWriter::printCommonXmlParts(const Metric& metric) const -{ - using namespace vespalib::xml; - const Metric::Tags& tags(metric.getTags()); - if (_verbosity >= 3 && tags.size() > 0) { - std::ostringstream ost; - // XXX print tag values as well - ost << tags[0].key(); - for (uint32_t i=1; i= 1 && !metric.getDescription().empty()) { - _xos << XmlAttribute("description", metric.getDescription()); - } -} - -} // metrics diff --git a/metrics/src/vespa/metrics/xmlwriter.h b/metrics/src/vespa/metrics/xmlwriter.h deleted file mode 100644 index a0e8a3efeea..00000000000 --- a/metrics/src/vespa/metrics/xmlwriter.h +++ /dev/null @@ -1,30 +0,0 @@ -// Copyright Yahoo. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root. - -#pragma once - -#include -#include - -namespace metrics { - -class XmlWriter : public MetricVisitor { - vespalib::xml::XmlOutputStream& _xos; - int _verbosity; - -public: - XmlWriter(vespalib::xml::XmlOutputStream& xos, - uint32_t period, int verbosity); - - bool visitSnapshot(const MetricSnapshot&) override; - void doneVisitingSnapshot(const MetricSnapshot&) override; - bool visitMetricSet(const MetricSet& set, bool) override; - void doneVisitingMetricSet(const MetricSet&) override; - bool visitCountMetric(const AbstractCountMetric&, bool autoGenerated) override; - bool visitValueMetric(const AbstractValueMetric&, bool autoGenerated) override; - -private: - void printCommonXmlParts(const Metric& metric) const; -}; - -} - diff --git a/storage/src/vespa/storage/common/statusmetricconsumer.cpp b/storage/src/vespa/storage/common/statusmetricconsumer.cpp index 8eb3e9f3ab6..c6f73540605 100644 --- a/storage/src/vespa/storage/common/statusmetricconsumer.cpp +++ b/storage/src/vespa/storage/common/statusmetricconsumer.cpp @@ -5,7 +5,6 @@ #include #include #include -#include #include #include #include @@ -37,10 +36,6 @@ StatusMetricConsumer::getReportContentType(const framework::HttpUrlPath& path) c return "text/plain"; } - if (path.getAttribute("format") == "xml") { - return "application/xml"; - } - if (path.getAttribute("format") == "text") { return "text/plain"; } @@ -67,7 +62,6 @@ StatusMetricConsumer::reportStatus(std::ostream& out, LOG(debug, "Not calling update hooks as dontcallupdatehooks option has been given"); } int64_t currentTimeS(vespalib::count_s(_component.getClock().getMonotonicTime().time_since_epoch())); - bool xml = (path.getAttribute("format") == "xml"); bool json = (path.getAttribute("format") == "json"); int verbosity(path.get("verbosity", 0)); @@ -131,13 +125,7 @@ StatusMetricConsumer::reportStatus(std::ostream& out, } std::string consumer = path.getAttribute("consumer", ""); - if (xml) { - out << "\n"; - vespalib::XmlOutputStream xos(out); - metrics::XmlWriter xmlWriter(xos, snapshot->getPeriod(), verbosity); - _manager.visit(metricLock, *snapshot, xmlWriter, consumer); - out << "\n"; - } else if (json) { + if (json) { vespalib::asciistream jsonStreamData; vespalib::JsonStream stream(jsonStreamData, true); stream << Object() << "metrics"; -- cgit v1.2.3