Fix distributor visitor latency metrics wiring

Also move to floating point latency measuring.

8 files changed, 232 insertions, 276 deletions

diff --git a/storage/src/tests/distributor/visitoroperationtest.cpp b/storage/src/tests/distributor/visitoroperationtest.cpp
index 3d00ae18b5f..9b674029959 100644
--- a/storage/src/tests/distributor/visitoroperationtest.cpp
+++ b/storage/src/tests/distributor/visitoroperationtest.cpp
@@ -188,15 +188,30 @@ private:
         return ost.str();
     }
 
+    std::unique_ptr<VisitorOperation> createOpWithConfig(
+            api::CreateVisitorCommand::SP msg,
+            const VisitorOperation::Config& config)
+    {
+        return std::make_unique<VisitorOperation>(
+                getExternalOperationHandler(),
+                msg,
+                config,
+                getDistributor().getMetrics().visits[msg->getLoadType()]);
+    }
+
+    std::unique_ptr<VisitorOperation> createOpWithDefaultConfig(
+            api::CreateVisitorCommand::SP msg)
+    {
+        return createOpWithConfig(std::move(msg), defaultConfig);
+    }
+
     /**
-       Starts a visitor where we expect no createVisitorCommands to be sent
-       to storage, either due to error or due to no data actually stored.
-    */
+     * Starts a visitor where we expect no createVisitorCommands to be sent
+     * to storage, either due to error or due to no data actually stored.
+     */
     std::string runEmptyVisitor(api::CreateVisitorCommand::SP msg) {
-        VisitorOperation op(getExternalOperationHandler(),
-                            msg,
-                           defaultConfig);
-        op.start(_sender, framework::MilliSecTime(0));
+        auto op = createOpWithDefaultConfig(std::move(msg));
+        op->start(_sender, framework::MilliSecTime(0));
         return _sender.getLastReply();
     }
 
@@ -260,11 +275,9 @@ VisitorOperationTest::doStandardVisitTest(const std::string& clusterState)
     msg->setTimeout(1234);
     msg->getTrace().setLevel(7);
 
-    VisitorOperation op(getExternalOperationHandler(),
-                        msg,
-                        defaultConfig);
+    auto op = createOpWithDefaultConfig(std::move(msg));
 
-    op.start(_sender, framework::MilliSecTime(0));
+    op->start(_sender, framework::MilliSecTime(0));
 
     CPPUNIT_ASSERT_EQUAL(std::string("Visitor Create => 0"),
                          _sender.getCommands(true));
@@ -288,7 +301,7 @@ VisitorOperationTest::doStandardVisitTest(const std::string& clusterState)
     CPPUNIT_ASSERT_EQUAL(uint32_t(1234), cvc->getTimeout());
     CPPUNIT_ASSERT_EQUAL(uint32_t(7), cvc->getTrace().getLevel());
 
-    sendReply(op);
+    sendReply(*op);
 
     CPPUNIT_ASSERT_EQUAL(std::string("CreateVisitorReply("
                                      "last=BucketId(0x000000007fffffff)) "
@@ -316,14 +329,14 @@ VisitorOperationTest::testShutdown()
     msg->addBucketToBeVisited(id);
     msg->addBucketToBeVisited(nullId);
 
-    VisitorOperation op(getExternalOperationHandler(), msg, defaultConfig);
+    auto op = createOpWithDefaultConfig(std::move(msg));
 
-    op.start(_sender, framework::MilliSecTime(0));
+    op->start(_sender, framework::MilliSecTime(0));
 
     CPPUNIT_ASSERT_EQUAL(std::string("Visitor Create => 0"),
                          _sender.getCommands(true));
 
-    op.onClose(_sender); // This will fail the visitor
+    op->onClose(_sender); // This will fail the visitor
 
     CPPUNIT_ASSERT_EQUAL(
             std::string("CreateVisitorReply(last=BucketId(0x0000000000000000)) "
@@ -381,19 +394,17 @@ VisitorOperationTest::testNoResendAfterTimeoutPassed()
     _distributor->enableClusterState(ClusterState("distributor:1 storage:2"));
     addNodesToBucketDB(id, "0=1/1/1/t,1=1/1/1/t");
 
-    VisitorOperation op(
-            getExternalOperationHandler(),
-            createVisitorCommand("lowtimeoutbusy", id, nullId, 8, 20),
-            defaultConfig);
+    auto op = createOpWithDefaultConfig(
+            createVisitorCommand("lowtimeoutbusy", id, nullId, 8, 20));
 
-    op.start(_sender, framework::MilliSecTime(0));
+    op->start(_sender, framework::MilliSecTime(0));
 
     CPPUNIT_ASSERT_EQUAL(std::string("Visitor Create => 0"),
                          _sender.getCommands(true));
 
     getClock().addMilliSecondsToTime(22);
 
-    sendReply(op, -1, api::ReturnCode::BUSY);
+    sendReply(*op, -1, api::ReturnCode::BUSY);
 
     CPPUNIT_ASSERT_EQUAL(
             std::string(
@@ -465,25 +476,25 @@ VisitorOperationTest::testUserSingleBucket()
 
     addNodesToBucketDB(id, "0=1/1/1/t");
 
-    VisitorOperation op(getExternalOperationHandler(),
-                        createVisitorCommand("userSingleBucket",
-                                userid,
-                                nullId,
-                                8,
-                                500,
-                                false,
-                                false,
-                                "dumpvisitor",
-                                document::OrderingSpecification::ASCENDING,
-                                "true"),
-                        defaultConfig);
-
-    op.start(_sender, framework::MilliSecTime(0));
+    auto op = createOpWithDefaultConfig(
+        createVisitorCommand(
+            "userSingleBucket",
+            userid,
+            nullId,
+            8,
+            500,
+            false,
+            false,
+            "dumpvisitor",
+            document::OrderingSpecification::ASCENDING,
+            "true"));
+
+    op->start(_sender, framework::MilliSecTime(0));
 
     CPPUNIT_ASSERT_EQUAL_MSG(_sender.getLastReply(),
                              std::string("Visitor Create => 0"),
                              _sender.getCommands(true));
-    sendReply(op);
+    sendReply(*op);
     CPPUNIT_ASSERT_EQUAL(
             std::string("CreateVisitorReply(last=BucketId(0x000000007fffffff)) "
                         "ReturnCode(NONE)"),
@@ -495,22 +506,21 @@ VisitorOperationTest::runVisitor(document::BucketId id,
                                                document::BucketId lastId,
                                                uint32_t maxBuckets)
 {
-    VisitorOperation op(getExternalOperationHandler(),
-                        createVisitorCommand("inconsistentSplit",
-                                id,
-                                lastId,
-                                maxBuckets,
-                                500,
-                                false,
-                                false,
-                                "dumpvisitor",
-                                document::OrderingSpecification::ASCENDING,
-                                "true"),
-                        defaultConfig);
-
-    op.start(_sender, framework::MilliSecTime(0));
-
-    sendReply(op);
+    auto op = createOpWithDefaultConfig(
+            createVisitorCommand("inconsistentSplit",
+                id,
+                lastId,
+                maxBuckets,
+                500,
+                false,
+                false,
+                "dumpvisitor",
+                document::OrderingSpecification::ASCENDING,
+                "true"));
+
+    op->start(_sender, framework::MilliSecTime(0));
+
+    sendReply(*op);
 
     std::pair<std::string, std::string> retVal =
         std::make_pair(serializeVisitorCommand(), _sender.getLastReply());
@@ -577,20 +587,17 @@ VisitorOperationTest::testBucketRemovedWhileVisitorPending()
 
     addNodesToBucketDB(id, "0=1/1/1/t");
 
-    VisitorOperation op(getExternalOperationHandler(),
-                        createVisitorCommand("removefrombucketdb",
-                                id,
-                                nullId),
-                        defaultConfig);
+    auto op = createOpWithDefaultConfig(
+            createVisitorCommand("removefrombucketdb", id, nullId));
 
-    op.start(_sender, framework::MilliSecTime(0));
+    op->start(_sender, framework::MilliSecTime(0));
 
     CPPUNIT_ASSERT_EQUAL(std::string("Visitor Create => 0"),
                          _sender.getCommands(true));
 
     removeFromBucketDB(id);
 
-    sendReply(op, -1, api::ReturnCode::NOT_CONNECTED);
+    sendReply(*op, -1, api::ReturnCode::NOT_CONNECTED);
 
     CPPUNIT_ASSERT_EQUAL(
             std::string("CreateVisitorReply(last=BucketId(0x0000000000000000)) "
@@ -605,17 +612,16 @@ VisitorOperationTest::testEmptyBucketsVisitedWhenVisitingRemoves()
     document::BucketId id(uint64_t(0x400000000000007b));
     addNodesToBucketDB(id, "0=0/0/0/1/2/t");
 
-    VisitorOperation op(getExternalOperationHandler(),
-                        createVisitorCommand("emptybucket",
-                                id,
-                                nullId,
-                                8,
-                                500,
-                                false,
-                                true),
-                        defaultConfig);
+    auto op = createOpWithDefaultConfig(
+            createVisitorCommand("emptybucket",
+                id,
+                nullId,
+                8,
+                500,
+                false,
+                true));
 
-    op.start(_sender, framework::MilliSecTime(0));
+    op->start(_sender, framework::MilliSecTime(0));
 
     // Since visitRemoves is true, the empty bucket will be visited
     CPPUNIT_ASSERT_EQUAL(std::string("Visitor Create => 0"),
@@ -630,18 +636,15 @@ VisitorOperationTest::testResendToOtherStorageNodeOnFailure()
 
     addNodesToBucketDB(id, "0=1/1/1/t,1=1/1/1/t");
 
-    VisitorOperation op(getExternalOperationHandler(),
-                        createVisitorCommand("emptyinconsistent",
-                                id,
-                                nullId),
-                        defaultConfig);
+    auto op = createOpWithDefaultConfig(
+            createVisitorCommand("emptyinconsistent", id, nullId));
 
-    op.start(_sender, framework::MilliSecTime(0));
+    op->start(_sender, framework::MilliSecTime(0));
 
     CPPUNIT_ASSERT_EQUAL(std::string("Visitor Create => 0"),
                          _sender.getCommands(true));
 
-    sendReply(op, -1, api::ReturnCode::NOT_CONNECTED);
+    sendReply(*op, -1, api::ReturnCode::NOT_CONNECTED);
     CPPUNIT_ASSERT_EQUAL(""s, _sender.getReplies());
 
     CPPUNIT_ASSERT_EQUAL(std::string("Visitor Create => 0,Visitor Create => 1"),
@@ -661,24 +664,23 @@ VisitorOperationTest::testTimeoutOnlyAfterReplyFromAllStorageNodes()
     addNodesToBucketDB(document::BucketId(17, 0x00001), "0=1/1/1/t");
     addNodesToBucketDB(document::BucketId(17, 0x10001), "1=1/1/1/t");
 
-    VisitorOperation op(getExternalOperationHandler(),
-                        createVisitorCommand("timeout2bucketson2nodes",
-                                document::BucketId(16, 1),
-                                nullId,
-                                8),
-                        defaultConfig);
+    auto op = createOpWithDefaultConfig(
+            createVisitorCommand("timeout2bucketson2nodes",
+                document::BucketId(16, 1),
+                nullId,
+                8));
 
-    op.start(_sender, framework::MilliSecTime(0));
+    op->start(_sender, framework::MilliSecTime(0));
 
     CPPUNIT_ASSERT_EQUAL("Visitor Create => 0,Visitor Create => 1"s,
                          _sender.getCommands(true));
 
     getClock().addMilliSecondsToTime(501);
 
-    sendReply(op, 0);
+    sendReply(*op, 0);
     CPPUNIT_ASSERT_EQUAL(""s, _sender.getReplies()); // No reply yet.
 
-    sendReply(op, 1, api::ReturnCode::BUSY);
+    sendReply(*op, 1, api::ReturnCode::BUSY);
 
     CPPUNIT_ASSERT_EQUAL(
             "CreateVisitorReply(last=BucketId(0x4400000000000001)) "
@@ -701,24 +703,23 @@ VisitorOperationTest::testTimeoutDoesNotOverrideCriticalError()
     addNodesToBucketDB(document::BucketId(17, 0x00001), "0=1/1/1/t");
     addNodesToBucketDB(document::BucketId(17, 0x10001), "1=1/1/1/t");
 
-    VisitorOperation op(getExternalOperationHandler(),
-                        createVisitorCommand("timeout2bucketson2nodes",
-                                document::BucketId(16, 1),
-                                nullId,
-                                8,
-                                500), // ms timeout
-                        defaultConfig);
+    auto op = createOpWithDefaultConfig(
+            createVisitorCommand("timeout2bucketson2nodes",
+                document::BucketId(16, 1),
+                nullId,
+                8,
+                500)); // ms timeout
 
-    op.start(_sender, framework::MilliSecTime(0));
+    op->start(_sender, framework::MilliSecTime(0));
     CPPUNIT_ASSERT_EQUAL("Visitor Create => 0,Visitor Create => 1"s,
                          _sender.getCommands(true));
 
     getClock().addMilliSecondsToTime(501);
     // Technically has timed out at this point, but should still report the
     // critical failure.
-    sendReply(op, 0, api::ReturnCode::INTERNAL_FAILURE);
+    sendReply(*op, 0, api::ReturnCode::INTERNAL_FAILURE);
     CPPUNIT_ASSERT_EQUAL(""s, _sender.getReplies());
-    sendReply(op, 1, api::ReturnCode::BUSY);
+    sendReply(*op, 1, api::ReturnCode::BUSY);
 
     CPPUNIT_ASSERT_EQUAL(
             "CreateVisitorReply(last=BucketId(0x0000000000000000)) "
@@ -786,20 +787,19 @@ VisitorOperationTest::testBucketHighBitCount()
                         "ReturnCode(WRONG_DISTRIBUTION, distributor:1 storage:1)"),
             runEmptyVisitor(createVisitorCommand("buckethigbit", id, nullId)));
 
-    VisitorOperation op(getExternalOperationHandler(),
-                        createVisitorCommand("buckethighbitcount",
-                                id,
-                                nullId,
-                                8,
-                                500,
-                                false,
-                                false,
-                                "dumpvisitor",
-                                document::OrderingSpecification::ASCENDING,
-                                "true"),
-                        defaultConfig);
-
-    op.start(_sender, framework::MilliSecTime(0));
+    auto op = createOpWithDefaultConfig(
+            createVisitorCommand("buckethighbitcount",
+                id,
+                nullId,
+                8,
+                500,
+                false,
+                false,
+                "dumpvisitor",
+                document::OrderingSpecification::ASCENDING,
+                "true"));
+
+    op->start(_sender, framework::MilliSecTime(0));
 
     CPPUNIT_ASSERT_EQUAL(std::string("Visitor Create => 0"),
                          _sender.getCommands(true));
@@ -818,20 +818,19 @@ VisitorOperationTest::testBucketLowBitCount()
                         "ReturnCode(WRONG_DISTRIBUTION, distributor:1 storage:1)"),
             runEmptyVisitor(createVisitorCommand("bucketlowbit", id, nullId)));
 
-    VisitorOperation op(getExternalOperationHandler(),
-                        createVisitorCommand("buckethighbitcount",
-                                id,
-                                nullId,
-                                8,
-                                500,
-                                false,
-                                false,
-                                "dumpvisitor",
-                                document::OrderingSpecification::ASCENDING,
-                                "true"),
-                        defaultConfig);
-
-    op.start(_sender, framework::MilliSecTime(0));
+    auto op = createOpWithDefaultConfig(
+            createVisitorCommand("buckethighbitcount",
+                id,
+                nullId,
+                8,
+                500,
+                false,
+                false,
+                "dumpvisitor",
+                document::OrderingSpecification::ASCENDING,
+                "true"));
+
+    op->start(_sender, framework::MilliSecTime(0));
     CPPUNIT_ASSERT_EQUAL(
             std::string("CreateVisitorReply(last=BucketId(0x0000000000000000)) "
                         "ReturnCode(WRONG_DISTRIBUTION, distributor:1 storage:1)"),
@@ -851,17 +850,11 @@ VisitorOperationTest::testParallelVisitorsToOneStorageNode()
 
     document::BucketId id(16, 1);
 
-    VisitorOperation op(getExternalOperationHandler(),
-                        createVisitorCommand("multiplebuckets",
-                                id,
-                                nullId,
-                                31),
-                        VisitorOperation::Config(
-                                framework::MilliSecTime(0),
-                                1,
-                                4));
+    auto op = createOpWithConfig(
+            createVisitorCommand("multiplebuckets", id, nullId, 31),
+            VisitorOperation::Config(framework::MilliSecTime(0), 1, 4));
 
-    op.start(_sender, framework::MilliSecTime(0));
+    op->start(_sender, framework::MilliSecTime(0));
 
     CPPUNIT_ASSERT_EQUAL(std::string("Visitor Create => 0,Visitor Create => 0,"
                                      "Visitor Create => 0,Visitor Create => 0"),
@@ -897,7 +890,7 @@ VisitorOperationTest::testParallelVisitorsToOneStorageNode()
             serializeVisitorCommand(3));
 
     for (uint32_t i = 0; i < 4; ++i) {
-        sendReply(op, i);
+        sendReply(*op, i);
     }
 
     CPPUNIT_ASSERT_EQUAL(
@@ -909,22 +902,22 @@ VisitorOperationTest::testParallelVisitorsToOneStorageNode()
 
     uint32_t minBucketsPerVisitor = 1;
     uint32_t maxVisitorsPerNode = 4;
-    VisitorOperation op2(getExternalOperationHandler(),
-                        createVisitorCommand("multiplebuckets",
-                                id,
-                                document::BucketId(0x54000000000f0001),
-                                31),
-                        VisitorOperation::Config(
-                                framework::MilliSecTime(0),
-                                minBucketsPerVisitor,
-                                maxVisitorsPerNode));
-
-    op2.start(_sender, framework::MilliSecTime(0));
+    auto op2 = createOpWithConfig(
+            createVisitorCommand("multiplebuckets",
+                id,
+                document::BucketId(0x54000000000f0001),
+                31),
+            VisitorOperation::Config(
+                framework::MilliSecTime(0),
+                minBucketsPerVisitor,
+                maxVisitorsPerNode));
+
+    op2->start(_sender, framework::MilliSecTime(0));
 
     CPPUNIT_ASSERT_EQUAL(std::string("Visitor Create => 0"),
                          _sender.getCommands(true));
 
-    sendReply(op2);
+    sendReply(*op2);
 
     CPPUNIT_ASSERT_EQUAL(
             std::string("CreateVisitorReply(last=BucketId(0x000000007fffffff)) "
@@ -947,24 +940,21 @@ VisitorOperationTest::testParallelVisitorsResendOnlyFailing()
 
     uint32_t minBucketsPerVisitor = 5;
     uint32_t maxVisitorsPerNode = 4;
-    VisitorOperation op(getExternalOperationHandler(),
-                        createVisitorCommand("multiplebuckets",
-                                id,
-                                nullId,
-                                31),
-                        VisitorOperation::Config(
-                                framework::MilliSecTime(0),
-                                minBucketsPerVisitor,
-                                maxVisitorsPerNode));
-
-    op.start(_sender, framework::MilliSecTime(0));
+    auto op = createOpWithConfig(
+            createVisitorCommand("multiplebuckets", id, nullId, 31),
+            VisitorOperation::Config(
+                framework::MilliSecTime(0),
+                minBucketsPerVisitor,
+                maxVisitorsPerNode));
+
+    op->start(_sender, framework::MilliSecTime(0));
 
     CPPUNIT_ASSERT_EQUAL(std::string("Visitor Create => 0,Visitor Create => 0,"
                                      "Visitor Create => 0,Visitor Create => 0"),
                          _sender.getCommands(true));
 
     for (uint32_t i = 0; i < 2; ++i) {
-        sendReply(op, i, api::ReturnCode::NOT_CONNECTED);
+        sendReply(*op, i, api::ReturnCode::NOT_CONNECTED);
     }
 
     CPPUNIT_ASSERT_EQUAL(std::string("Visitor Create => 0,Visitor Create => 0,"
@@ -973,7 +963,7 @@ VisitorOperationTest::testParallelVisitorsResendOnlyFailing()
                          _sender.getCommands(true));
 
     for (uint32_t i = 2; i < 6; ++i) {
-        sendReply(op, i);
+        sendReply(*op, i);
     }
 
     CPPUNIT_ASSERT_EQUAL(
@@ -995,16 +985,11 @@ VisitorOperationTest::testParallelVisitorsToOneStorageNodeOneSuperBucket()
 
     document::BucketId id(16, 0x2b6a);
 
-    VisitorOperation op(getExternalOperationHandler(),
-                        createVisitorCommand("multiplebucketsonesuper",
-                                id,
-                                nullId),
-                        VisitorOperation::Config(
-                                framework::MilliSecTime(0),
-                                5,
-                                4));
+    auto op = createOpWithConfig(
+            createVisitorCommand("multiplebucketsonesuper", id, nullId),
+            VisitorOperation::Config(framework::MilliSecTime(0), 5, 4));
 
-    op.start(_sender, framework::MilliSecTime(0));
+    op->start(_sender, framework::MilliSecTime(0));
 
     CPPUNIT_ASSERT_EQUAL(std::string("Visitor Create => 0"),
                          _sender.getCommands(true));
@@ -1017,7 +1002,7 @@ VisitorOperationTest::testParallelVisitorsToOneStorageNodeOneSuperBucket()
                         "BucketId(0x8c000003e3362b6a) BucketId(0x8c000007e3362b6a) ]"),
             serializeVisitorCommand(0));
 
-    sendReply(op);
+    sendReply(*op);
     
     CPPUNIT_ASSERT_EQUAL(
             std::string("CreateVisitorReply(last=BucketId(0x000000007fffffff)) "
@@ -1076,24 +1061,21 @@ VisitorOperationTest::testInconsistencyHandling()
                                                nullId)));
     _sender.clear();
 
-    VisitorOperation op(getExternalOperationHandler(),
-                        createVisitorCommand("multiplebucketsonesuper",
-                                id,
-                                nullId,
-                                8,
-                                500,
-                                true),
-                        VisitorOperation::Config(
-                                framework::MilliSecTime(0),
-                                5,
-                                4));
+    auto op = createOpWithConfig(
+            createVisitorCommand("multiplebucketsonesuper",
+                id,
+                nullId,
+                8,
+                500,
+                true),
+            VisitorOperation::Config(framework::MilliSecTime(0), 5, 4));
 
-    op.start(_sender, framework::MilliSecTime(0));
+    op->start(_sender, framework::MilliSecTime(0));
 
     CPPUNIT_ASSERT_EQUAL(std::string("Visitor Create => 1"),
                          _sender.getCommands(true));
 
-    sendReply(op);
+    sendReply(*op);
 
     CPPUNIT_ASSERT_EQUAL(
             std::string("CreateVisitorReply(last=BucketId(0x000000007fffffff)) "
@@ -1114,14 +1096,10 @@ VisitorOperationTest::testVisitIdealNode()
     }
 
     document::BucketId id(16, 1);
-    VisitorOperation op(getExternalOperationHandler(),
-                        createVisitorCommand("multinode",
-                                id,
-                                nullId,
-                                8),
-                        defaultConfig);
+    auto op = createOpWithDefaultConfig(
+            createVisitorCommand("multinode", id, nullId, 8));
 
-    op.start(_sender, framework::MilliSecTime(0));
+    op->start(_sender, framework::MilliSecTime(0));
 
     CPPUNIT_ASSERT_EQUAL(std::string("Visitor Create => 0"),
                          _sender.getCommands(true));
@@ -1134,7 +1112,7 @@ VisitorOperationTest::testVisitIdealNode()
                         "BucketId(0x54000000000c0001) BucketId(0x54000000001c0001) ]"),
             serializeVisitorCommand(0));
 
-    sendReply(op);
+    sendReply(*op);
 
     CPPUNIT_ASSERT_EQUAL(
             std::string("CreateVisitorReply(last=BucketId(0x54000000001c0001)) "
@@ -1155,19 +1133,15 @@ VisitorOperationTest::testNoResendingOnCriticalFailure()
     }
 
     document::BucketId id(16, 1);
-    VisitorOperation op(getExternalOperationHandler(),
-                        createVisitorCommand("multinodefailurecritical",
-                                id,
-                                nullId,
-                                8),
-                        defaultConfig);
+    auto op = createOpWithDefaultConfig(
+            createVisitorCommand("multinodefailurecritical", id, nullId, 8));
 
-    op.start(_sender, framework::MilliSecTime(0));
+    op->start(_sender, framework::MilliSecTime(0));
 
     CPPUNIT_ASSERT_EQUAL(std::string("Visitor Create => 0"),
                          _sender.getCommands(true));
 
-    sendReply(op, -1, api::ReturnCode::ILLEGAL_PARAMETERS);
+    sendReply(*op, -1, api::ReturnCode::ILLEGAL_PARAMETERS);
 
     CPPUNIT_ASSERT_EQUAL(
             "CreateVisitorReply(last=BucketId(0x0000000000000000)) "
@@ -1188,24 +1162,20 @@ VisitorOperationTest::testFailureOnAllNodes()
     }
 
     document::BucketId id(16, 1);
-    VisitorOperation op(getExternalOperationHandler(),
-                        createVisitorCommand("multinodefailurecritical",
-                                id,
-                                nullId,
-                                8),
-                        defaultConfig);
+    auto op = createOpWithDefaultConfig(
+            createVisitorCommand("multinodefailurecritical", id, nullId, 8));
 
-    op.start(_sender, framework::MilliSecTime(0));
+    op->start(_sender, framework::MilliSecTime(0));
 
     CPPUNIT_ASSERT_EQUAL(std::string("Visitor Create => 0"),
                          _sender.getCommands(true));
 
-    sendReply(op, -1, api::ReturnCode::NOT_CONNECTED);
+    sendReply(*op, -1, api::ReturnCode::NOT_CONNECTED);
 
     CPPUNIT_ASSERT_EQUAL(std::string("Visitor Create => 0,Visitor Create => 1"),
                          _sender.getCommands(true));
 
-    sendReply(op, -1, api::ReturnCode::NOT_CONNECTED);
+    sendReply(*op, -1, api::ReturnCode::NOT_CONNECTED);
 
     CPPUNIT_ASSERT_EQUAL(
             std::string("CreateVisitorReply(last=BucketId(0x0000000000000000)) "
@@ -1439,22 +1409,21 @@ VisitorOperationTest::doOrderedVisitor(document::BucketId startBucket)
     while (true) {
         _sender.clear();
 
-        VisitorOperation op(getExternalOperationHandler(),
-                            createVisitorCommand(
-                                    "uservisitororder",
-                                    startBucket,
-                                    buckets.size() ? buckets[buckets.size() - 1] :
-                                    nullId,
-                                    1,
-                                    500,
-                                    false,
-                                    false,
-                                    "dumpvisitor",
-                                    document::OrderingSpecification::DESCENDING,
-                                    "id.order(6,2)<= 20"),
-                            defaultConfig);
-
-        op.start(_sender, framework::MilliSecTime(0));
+        auto op = createOpWithDefaultConfig(
+                createVisitorCommand(
+                    "uservisitororder",
+                    startBucket,
+                    buckets.size() ? buckets[buckets.size() - 1] :
+                    nullId,
+                    1,
+                    500,
+                    false,
+                    false,
+                    "dumpvisitor",
+                    document::OrderingSpecification::DESCENDING,
+                    "id.order(6,2)<= 20"));
+
+        op->start(_sender, framework::MilliSecTime(0));
 
         CPPUNIT_ASSERT_EQUAL(std::string("Visitor Create => 0"),
                              _sender.getCommands(true));
@@ -1469,7 +1438,7 @@ VisitorOperationTest::doOrderedVisitor(document::BucketId startBucket)
             }
         }
 
-        sendReply(op);
+        sendReply(*op);
 
         CPPUNIT_ASSERT_EQUAL(1, (int)_sender.replies.size());
 
@@ -1560,16 +1529,14 @@ VisitorOperationTest::startOperationWith2StorageNodeVisitors(bool inconsistent)
                        "1=1/1/1/t");
 
     document::BucketId id(16, 1);
-    auto op = std::make_unique<VisitorOperation>(
-            getExternalOperationHandler(),
+    auto op = createOpWithDefaultConfig(
             createVisitorCommand(
-                    "multinodefailurecritical",
-                    id,
-                    nullId,
-                    8,
-                    500,
-                    inconsistent),
-            defaultConfig);
+                "multinodefailurecritical",
+                id,
+                nullId,
+                8,
+                500,
+                inconsistent));
 
     op->start(_sender, framework::MilliSecTime(0));
 
@@ -1628,12 +1595,10 @@ VisitorOperationTest::testQueueTimeoutIsFactorOfTotalTimeout()
     _distributor->enableClusterState(ClusterState("distributor:1 storage:2"));
     addNodesToBucketDB(id, "0=1/1/1/t,1=1/1/1/t");
 
-    VisitorOperation op(
-            getExternalOperationHandler(),
-            createVisitorCommand("foo", id, nullId, 8, 10000),
-            defaultConfig);
+    auto op = createOpWithDefaultConfig(
+            createVisitorCommand("foo", id, nullId, 8, 10000));
 
-    op.start(_sender, framework::MilliSecTime(0));
+    op->start(_sender, framework::MilliSecTime(0));
     CPPUNIT_ASSERT_EQUAL(std::string("Visitor Create => 0"),
                          _sender.getCommands(true));
 
diff --git a/storage/src/vespa/storage/distributor/distributormetricsset.cpp b/storage/src/vespa/storage/distributor/distributormetricsset.cpp
index b57ac81909e..0760ab6223d 100644
--- a/storage/src/vespa/storage/distributor/distributormetricsset.cpp
+++ b/storage/src/vespa/storage/distributor/distributormetricsset.cpp
@@ -68,15 +68,16 @@ PersistenceOperationMetricSet::clone(std::vector<Metric::LP>& ownerList, CopyTyp
 
 DistributorMetricSet::DistributorMetricSet(const metrics::LoadTypeSet& lt)
     : MetricSet("distributor", "distributor", ""),
-      puts(lt, *&PersistenceOperationMetricSet("puts"), this),
-      updates(lt, *&PersistenceOperationMetricSet("updates"), this),
-      update_puts(lt, *&PersistenceOperationMetricSet("update_puts"), this),
-      update_gets(lt, *&PersistenceOperationMetricSet("update_gets"), this),
-      removes(lt, *&PersistenceOperationMetricSet("removes"), this),
-      removelocations(lt, *&PersistenceOperationMetricSet("removelocations"), this),
-      gets(lt, *&PersistenceOperationMetricSet("gets"), this),
-      stats(lt, *&PersistenceOperationMetricSet("stats"), this),
-      multioperations(lt, *&PersistenceOperationMetricSet("multioperations"), this),
+      puts(lt, PersistenceOperationMetricSet("puts"), this),
+      updates(lt, PersistenceOperationMetricSet("updates"), this),
+      update_puts(lt, PersistenceOperationMetricSet("update_puts"), this),
+      update_gets(lt, PersistenceOperationMetricSet("update_gets"), this),
+      removes(lt, PersistenceOperationMetricSet("removes"), this),
+      removelocations(lt, PersistenceOperationMetricSet("removelocations"), this),
+      gets(lt, PersistenceOperationMetricSet("gets"), this),
+      stats(lt, PersistenceOperationMetricSet("stats"), this),
+      multioperations(lt, PersistenceOperationMetricSet("multioperations"), this),
+      visits(lt, VisitorMetricSet(), this),
       recoveryModeTime("recoverymodeschedulingtime", "",
                        "Time spent scheduling operations in recovery mode "
                        "after receiving new cluster state", this),
diff --git a/storage/src/vespa/storage/distributor/distributormetricsset.h b/storage/src/vespa/storage/distributor/distributormetricsset.h
index 392a1068f2c..b8458d119e7 100644
--- a/storage/src/vespa/storage/distributor/distributormetricsset.h
+++ b/storage/src/vespa/storage/distributor/distributormetricsset.h
@@ -1,6 +1,7 @@
 // Copyright 2016 Yahoo Inc. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root.
 #pragma once
 
+#include "visitormetricsset.h"
 #include <vespa/metrics/metrics.h>
 #include <vespa/documentapi/loadtypes/loadtypeset.h>
 
@@ -24,7 +25,6 @@ public:
 
     MetricSet * clone(std::vector<Metric::LP>& ownerList, CopyType copyType,
                       metrics::MetricSet* owner, bool includeUnused) const;
-    PersistenceFailuresMetricSet* operator&() { return this; }
 };
 
 class PersistenceOperationMetricSet : public metrics::MetricSet
@@ -39,7 +39,6 @@ public:
 
     MetricSet * clone(std::vector<Metric::LP>& ownerList, CopyType copyType,
                       metrics::MetricSet* owner, bool includeUnused) const override;
-    PersistenceOperationMetricSet* operator&() { return this; }
 };
 
 class DistributorMetricSet : public metrics::MetricSet
@@ -54,6 +53,7 @@ public:
     metrics::LoadMetric<PersistenceOperationMetricSet> gets;
     metrics::LoadMetric<PersistenceOperationMetricSet> stats;
     metrics::LoadMetric<PersistenceOperationMetricSet> multioperations;
+    metrics::LoadMetric<VisitorMetricSet> visits;
     metrics::DoubleAverageMetric recoveryModeTime;
     metrics::LongValueMetric docsStored;
     metrics::LongValueMetric bytesStored;
diff --git a/storage/src/vespa/storage/distributor/externaloperationhandler.cpp b/storage/src/vespa/storage/distributor/externaloperationhandler.cpp
index 28f5aa91c2c..30ff11fc85f 100644
--- a/storage/src/vespa/storage/distributor/externaloperationhandler.cpp
+++ b/storage/src/vespa/storage/distributor/externaloperationhandler.cpp
@@ -44,8 +44,6 @@ ExternalOperationHandler::ExternalOperationHandler(
         DistributorComponentRegister& compReg)
     : ManagedBucketSpaceComponent(owner, bucketSpace, compReg,
                                   "External operation handler"),
-      _visitorMetrics(getLoadTypes()->getMetricLoadTypes(),
-                      *&VisitorMetricSet(NULL)),
       _operationGenerator(gen),
       _rejectFeedBeforeTimeReached() // At epoch
 {
@@ -264,7 +262,7 @@ IMPL_MSG_COMMAND_H(ExternalOperationHandler, CreateVisitor)
                                 *this,
                                 cmd,
                                 visitorConfig,
-                                &_visitorMetrics[cmd->getLoadType()]));
+                                getMetrics().visits[cmd->getLoadType()]));
     return true;
 }
 
diff --git a/storage/src/vespa/storage/distributor/externaloperationhandler.h b/storage/src/vespa/storage/distributor/externaloperationhandler.h
index dac4b8ce146..d8f35e7e9e5 100644
--- a/storage/src/vespa/storage/distributor/externaloperationhandler.h
+++ b/storage/src/vespa/storage/distributor/externaloperationhandler.h
@@ -5,7 +5,6 @@
 #include <vespa/document/bucket/bucketidfactory.h>
 #include <vespa/vdslib/state/clusterstate.h>
 #include <vespa/storage/distributor/distributorcomponent.h>
-#include <vespa/storage/distributor/visitormetricsset.h>
 #include <vespa/storage/distributor/managed_bucket_space_component.h>
 #include <vespa/storageapi/messageapi/messagehandler.h>
 #include <vespa/storageframework/storageframework.h>
@@ -53,7 +52,6 @@ public:
     }
 
 private:
-    metrics::LoadMetric<VisitorMetricSet> _visitorMetrics;
     const MaintenanceOperationGenerator& _operationGenerator;
     Operation::SP _op;
     TimePoint _rejectFeedBeforeTimeReached;
diff --git a/storage/src/vespa/storage/distributor/operations/external/visitoroperation.cpp b/storage/src/vespa/storage/distributor/operations/external/visitoroperation.cpp
index 90b074779c7..7bb99ea4b43 100644
--- a/storage/src/vespa/storage/distributor/operations/external/visitoroperation.cpp
+++ b/storage/src/vespa/storage/distributor/operations/external/visitoroperation.cpp
@@ -12,6 +12,7 @@
 #include <vespa/storage/distributor/distributor.h>
 #include <vespa/storage/distributor/bucketownership.h>
 #include <vespa/storage/distributor/operations/external/visitororder.h>
+#include <vespa/storage/distributor/visitormetricsset.h>
 
 namespace storage {
 
@@ -48,14 +49,15 @@ VisitorOperation::VisitorOperation(
         DistributorComponent& owner,
         const api::CreateVisitorCommand::SP& m,
         const Config& config,
-        VisitorMetricSet* metric)
+        VisitorMetricSet& metrics)
     : Operation(),
       _owner(owner),
       _msg(m),
       _sentReply(false),
       _config(config),
-      _metrics(metric),
-      _trace(TRACE_SOFT_MEMORY_LIMIT)
+      _metrics(metrics),
+      _trace(TRACE_SOFT_MEMORY_LIMIT),
+      _operationTimer(owner.getClock())
 {
     const std::vector<document::BucketId>& buckets = m->getBuckets();
 
@@ -72,8 +74,6 @@ VisitorOperation::VisitorOperation(
     if (_toTime == 0) {
         _toTime = owner.getUniqueTimestamp();
     }
-
-    _startVisitorTime = owner.getClock().getTimeInMillis();
 }
 
 VisitorOperation::~VisitorOperation()
@@ -124,14 +124,12 @@ VisitorOperation::getLastBucketVisited()
 uint64_t
 VisitorOperation::timeLeft() const noexcept
 {
-    framework::MilliSecTime now = _owner.getClock().getTimeInMillis();
-    framework::MilliSecTime timeSpent = now - _startVisitorTime;
+    const auto elapsed = _operationTimer.getElapsedTime();
+    framework::MilliSecTime timeSpent(
+            std::chrono::duration_cast<std::chrono::milliseconds>(elapsed).count());
 
     LOG(spam,
-        "Checking if visitor has timed out: now=%zu, start=%zu, "
-        "diff=%zu, timeout=%u",
-        now.getTime(),
-        _startVisitorTime.getTime(),
+        "Checking if visitor has timed out: elapsed=%zu ms, timeout=%u ms",
         timeSpent.getTime(),
         _msg->getTimeout());
 
@@ -987,10 +985,7 @@ VisitorOperation::sendReply(const api::ReturnCode& code, DistributorMessageSende
 
         sender.sendReply(reply);
 
-        if (_metrics) {
-            framework::MilliSecTime timeNow(_owner.getClock().getTimeInMillis());
-            _metrics->latency.addValue((timeNow - _startVisitorTime).getTime());
-        }
+        _metrics.latency.addValue(_operationTimer.getElapsedTimeAsDouble());
         _sentReply = true;
     }
 }
diff --git a/storage/src/vespa/storage/distributor/operations/external/visitoroperation.h b/storage/src/vespa/storage/distributor/operations/external/visitoroperation.h
index a0aac7918e6..1824610355d 100644
--- a/storage/src/vespa/storage/distributor/operations/external/visitoroperation.h
+++ b/storage/src/vespa/storage/distributor/operations/external/visitoroperation.h
@@ -5,7 +5,6 @@
 #include <vespa/storage/distributor/operations/operation.h>
 #include <vespa/storageapi/messageapi/storagemessage.h>
 #include <vespa/storageapi/message/visitor.h>
-#include <vespa/storage/distributor/visitormetricsset.h>
 #include <vespa/storage/bucketdb/bucketdatabase.h>
 #include <vespa/storage/visiting/memory_bounded_trace.h>
 
@@ -15,6 +14,8 @@ class Document;
 
 namespace storage {
 
+class VisitorMetricSet;
+
 namespace distributor {
 
 class DistributorComponent;
@@ -38,7 +39,7 @@ public:
     VisitorOperation(DistributorComponent& manager,
                      const std::shared_ptr<api::CreateVisitorCommand> & msg,
                      const Config& config,
-                     VisitorMetricSet* metrics = NULL);
+                     VisitorMetricSet& metrics);
 
     ~VisitorOperation();
 
@@ -170,11 +171,10 @@ private:
     std::vector<uint32_t> _activeNodes;
     uint32_t _bucketCount;
 
-    framework::MilliSecTime _startVisitorTime;
     vdslib::VisitorStatistics _visitorStatistics;
 
     Config _config;
-    VisitorMetricSet* _metrics;
+    VisitorMetricSet& _metrics;
     MemoryBoundedTrace _trace;
 
     static constexpr size_t TRACE_SOFT_MEMORY_LIMIT = 65536;
@@ -183,6 +183,7 @@ private:
     bool hasNoPendingMessages();
     document::BucketId getLastBucketVisited();
     mbus::TraceNode trace;
+    framework::MilliSecTimer _operationTimer;
 };
 
 }
diff --git a/storage/src/vespa/storage/distributor/visitormetricsset.h b/storage/src/vespa/storage/distributor/visitormetricsset.h
index 53981ec3834..24a3bbcce2f 100644
--- a/storage/src/vespa/storage/distributor/visitormetricsset.h
+++ b/storage/src/vespa/storage/distributor/visitormetricsset.h
@@ -10,13 +10,11 @@ struct VisitorMetricSet : public metrics::MetricSet {
     metrics::DoubleAverageMetric latency;
     metrics::LongCountMetric failed;
 
-    VisitorMetricSet(MetricSet* owner = 0);
+    VisitorMetricSet(MetricSet* owner = nullptr);
     ~VisitorMetricSet();
 
     MetricSet * clone(std::vector<Metric::LP>& ownerList, CopyType copyType,
                       MetricSet* owner, bool includeUnused) const override;
-
-    VisitorMetricSet* operator&() { return this; }
 };
 
 } // storage