GC xml output of metrics.

7 files changed, 157 insertions, 451 deletions

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 <vespa/metrics/metricmanager.h>
 #include <vespa/metrics/state_api_adapter.h>
 #include <vespa/metrics/textwriter.h>
-#include <vespa/metrics/xmlwriter.h>
 #include <vespa/vespalib/data/slime/slime.h>
 #include <vespa/vespalib/gtest/gtest.h>
 #include <vespa/vespalib/stllike/asciistream.h>
 #include <vespa/vespalib/util/size_literals.h>
-#include <vespa/vespalib/util/xmlstream.h>
 #include <vespa/vespalib/util/time.h>
 #include <vespa/vespalib/data/simple_buffer.h>
 #include <vespa/vespalib/util/atomic.h>
@@ -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<std::string, std::string>(
-            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<std::string, std::string> consumerMatch( \
-                                            getMatchedMetrics(config)); \
+    std::pair<std::string, std::string> 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<uint32_t> periods;
@@ -429,17 +418,15 @@ std::string dumpAllSnapshots(const MetricManager& mm,
     }
     for (uint32_t i=0; i<periods.size(); ++i) {
         MetricLockGuard metricLock(mm.getMetricLock());
-        const MetricSnapshotSet& set(mm.getMetricSnapshotSet(
-                metricLock, periods[i]));
+        const MetricSnapshotSet& set(mm.getMetricSnapshotSet(metricLock, periods[i]));
         ost << set.getName() << "\n";
-        uint32_t count = 0;
-        for (uint32_t j=0; j<2; ++j) {
+        for (uint32_t count=0,j=0; j<2; ++j) {
             if (set.getCount() == 1 && j == 1) continue;
             const MetricSnapshot& snap(set.getSnapshot(j == 1));
             BriefValuePrinter briefValuePrinter;
             mm.visit(metricLock, snap, briefValuePrinter, consumer);
             ost << "  " << count++ << " " << &snap.getMetrics() << ": "
-                    << briefValuePrinter.ost.str() << "\n";
+                << briefValuePrinter.ost.str() << "\n";
         }
     }
     return ost.str();
@@ -452,14 +439,11 @@ std::string dumpAllSnapshots(const MetricManager& mm,
     MetricLockGuard lockGuard(mm.getMetricLock()); \
     BriefValuePrinter briefValuePrinter; \
     if (period == -1) { \
-        mm.visit(lockGuard, mm.getActiveMetrics(lockGuard), \
-                 briefValuePrinter, "snapper"); \
+        mm.visit(lockGuard, mm.getActiveMetrics(lockGuard), briefValuePrinter, "snapper"); \
     } else if (period == 0) { \
-        mm.visit(lockGuard, mm.getTotalMetricSnapshot(lockGuard), \
-                 briefValuePrinter, "snapper"); \
+        mm.visit(lockGuard, mm.getTotalMetricSnapshot(lockGuard), briefValuePrinter, "snapper"); \
     } else { \
-        mm.visit(lockGuard, mm.getMetricSnapshot(lockGuard, period), \
-                 briefValuePrinter, "snapper"); \
+        mm.visit(lockGuard, mm.getMetricSnapshot(lockGuard, period), briefValuePrinter, "snapper"); \
     } \
     EXPECT_EQ(std::string(expected), briefValuePrinter.ost.str()) << dumpAllSnapshots(mm, "snapper"); \
 }
@@ -474,8 +458,8 @@ std::string dumpAllSnapshots(const MetricManager& mm,
 
 TEST_F(MetricManagerTest, test_snapshots)
 {
-    FakeTimer* timer = new FakeTimer(1000);
-    std::unique_ptr<MetricManager::Timer> timerImpl(timer);
+    auto timerImpl = std::make_unique<FakeTimer>(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<MetricManager::Timer> 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(
-            "'<snapshot name=\"5 minute\" from=\"1000\" to=\"1300\" period=\"300\">\n"
-            "  <temp>\n"
-            "    <val6 average=\"2\" last=\"2\" min=\"2\" max=\"2\" count=\"1\"/>\n"
-            "    <sub>\n"
-            "      <val1 average=\"4\" last=\"4\" min=\"4\" max=\"4\" count=\"1\"/>\n"
-            "      <valsum average=\"4\" last=\"4\" min=\"4\" max=\"4\" count=\"1\"/>\n"
-            "    </sub>\n"
-            "    <multisub>\n"
-            "      <count count=\"1\"/>\n"
-            "      <a>\n"
-            "        <val1 average=\"7\" last=\"7\" min=\"7\" max=\"7\" count=\"1\"/>\n"
-            "        <valsum average=\"9\" last=\"9\"/>\n"
-            "      </a>\n"
-            "      <b>\n"
-            "        <val1 average=\"1\" last=\"1\" min=\"1\" max=\"1\" count=\"1\"/>\n"
-            "        <valsum average=\"1\" last=\"1\" min=\"1\" max=\"1\" count=\"1\"/>\n"
-            "      </b>\n"
-            "      <sum>\n"
-            "        <val1 average=\"8\" last=\"8\"/>\n"
-            "        <val2 average=\"2\" last=\"2\" min=\"2\" max=\"2\" count=\"1\"/>\n"
-            "        <valsum average=\"10\" last=\"10\"/>\n"
-            "      </sum>\n"
-            "    </multisub>\n"
-            "  </temp>\n"
-            "</snapshot>'");
-
-    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
     $<TARGET_OBJECTS:metrics_common>
 
     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<Timer>())
+{ }
+
 MetricManager::MetricManager(std::unique_ptr<Timer> timer)
     : _activeMetrics("Active metrics showing updates since last snapshot"),
       _configSubscriber(),
@@ -70,9 +76,7 @@ MetricManager::MetricManager(std::unique_ptr<Timer> timer)
       _config(),
       _consumerConfig(),
       _snapshots(),
-      _totalMetrics(std::make_shared<MetricSnapshot>(
-                "Empty metrics before init", 0, _activeMetrics.getMetrics(),
-                false)),
+      _totalMetrics(std::make_shared<MetricSnapshot>("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<config::ConfigSubscriber>(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> 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::SnapSpec>
 MetricManager::createSnapshotPeriods(const Config& config)
 {
     std::vector<SnapSpec> result;
-    try{
-        for (uint32_t i=0; i<config.snapshot.periods.size(); ++i) {
-            uint32_t length = config.snapshot.periods[i];
+    try {
+        for (auto length : config.snapshot.periods) {
             if (length < 1)
-                throw vespalib::IllegalStateException("Snapshot periods must be positive numbers", VESPA_STRLOC);
+                throw IllegalStateException("Snapshot periods must be positive numbers", VESPA_STRLOC);
             std::ostringstream name;
             if (setSnapshotName(name, "week", length, 60 * 60 * 24 * 7)) {
             } else if (setSnapshotName(name, "day", length, 60 * 60 * 24)) {
@@ -425,16 +429,13 @@ MetricManager::createSnapshotPeriods(const Config& config)
         for (uint32_t i=1; i<result.size(); ++i) {
             if (result[i].first % result[i-1].first != 0) {
                 std::ostringstream ost;
-                ost << "Period " << result[i].first
-                    << " is not a multiplum of period "
+                ost << "Period " << result[i].first << " is not a multiplum of period "
                     << result[i-1].first << " which is needs to be.";
-                throw vespalib::IllegalStateException(
-                        ost.str(), VESPA_STRLOC);
+                throw IllegalStateException(ost.str(), VESPA_STRLOC);
             }
         }
     } catch (vespalib::Exception& e) {
-        LOG(warning, "Invalid snapshot periods specified. Using defaults: %s",
-            e.getMessage().c_str());
+        LOG(warning, "Invalid snapshot periods specified. Using defaults: %s", e.getMessage().c_str());
         result.clear();
     }
     if (result.empty()) {
@@ -454,8 +455,7 @@ MetricManager::configure(const MetricLockGuard & , std::unique_ptr<Config> 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<Config> 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<MetricSnapshotSet>(
@@ -489,9 +483,7 @@ MetricManager::configure(const MetricLockGuard & , std::unique_ptr<Config> confi
         _totalMetrics = std::make_shared<MetricSnapshot>("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<Metric::String> _visitedMetrics;
+    std::set<Metric::String> _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<time_t>::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<uint64_t>(hook->_nextCall), hook->_period, static_cast<uint64_t>(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<uint64_t>(currentTime));
+    LOG(spam, "Worker thread starting to process for time %" PRIu64 ".", static_cast<uint64_t>(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<uint64_t>(_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<uint64_t>(_activeMetrics.getFromTime()), static_cast<uint64_t>(_activeMetrics.getToTime()),
         static_cast<uint64_t>(firstTarget.getFromTime()), static_cast<uint64_t>(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<uint64_t>
-                (_snapshots[i]->getBuilderCount() * _snapshots[i]->getPeriod()
-                 + _snapshots[i]->getFromTime()),
-                _snapshots[i]->getPeriod(),
-                static_cast<uint64_t>(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<uint64_t>(_snapshots[i]->getBuilderCount() * _snapshots[i]->getPeriod() + _snapshots[i]->getFromTime()),
+                _snapshots[i]->getPeriod(), static_cast<uint64_t>(timeToProcess));
             break;
         } else {
             LOG(debug, "Rolled snapshot %s at time %" PRIu64 ".",
-                _snapshots[i]->getName().c_str(),
-                static_cast<uint64_t>(timeToProcess));
+                _snapshots[i]->getName().c_str(), static_cast<uint64_t>(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<MemoryConsumption>();
     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> timer = std::make_unique<Timer>());
+    MetricManager();
+    MetricManager(std::unique_ptr<Timer> 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<uint32_t> 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 <vespa/vespalib/util/xmlstream.h>
-#include <sstream>
-
-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<tags.size(); ++i) {
-            ost << "," << tags[i].key();
-        }
-        _xos << XmlAttribute("tags", ost.str());
-    }
-    if (_verbosity >= 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 <vespa/metrics/metric.h>
-#include <vespa/vespalib/util/xmlserializable.h>
-
-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 <boost/lexical_cast.hpp>
 #include <vespa/metrics/jsonwriter.h>
 #include <vespa/metrics/textwriter.h>
-#include <vespa/metrics/xmlwriter.h>
 #include <vespa/metrics/metricmanager.h>
 #include <vespa/storageapi/messageapi/storagemessage.h>
 #include <vespa/vespalib/stllike/asciistream.h>
@@ -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 << "<?xml version=\"1.0\"?>\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";