Reduce exposure of TraceNode even further.

14 files changed, 32 insertions, 54 deletions

diff --git a/messagebus/src/tests/trace-roundtrip/trace-roundtrip.cpp b/messagebus/src/tests/trace-roundtrip/trace-roundtrip.cpp
index 344d38c8263..f97fc6a0010 100644
--- a/messagebus/src/tests/trace-roundtrip/trace-roundtrip.cpp
+++ b/messagebus/src/tests/trace-roundtrip/trace-roundtrip.cpp
@@ -120,6 +120,6 @@ Test::Main()
                   .addChild("Server reply")
                   .addChild("Proxy reply")
                   .addChild("Client reply");
-    EXPECT_TRUE(reply->getTrace().getRoot().encode() == t.encode());
+    EXPECT_TRUE(reply->getTrace().encode() == t.encode());
     TEST_DONE();
 }
diff --git a/messagebus/src/vespa/messagebus/network/rpcsendv1.cpp b/messagebus/src/vespa/messagebus/network/rpcsendv1.cpp
index f87a8d9638a..a8f7b47f3e3 100644
--- a/messagebus/src/vespa/messagebus/network/rpcsendv1.cpp
+++ b/messagebus/src/vespa/messagebus/network/rpcsendv1.cpp
@@ -163,7 +163,7 @@ RPCSendV1::createResponse(FRT_Values & ret, const string & version, Reply & repl
     ret.AddString(reply.getProtocol().c_str());
     ret.AddData(std::move(payload.payload()), payload.size());
     if (reply.getTrace().getLevel() > 0) {
-        ret.AddString(reply.getTrace().getRoot().encode().c_str());
+        ret.AddString(reply.getTrace().encode().c_str());
     } else {
         ret.AddString("");
     }
diff --git a/messagebus/src/vespa/messagebus/network/rpcsendv2.cpp b/messagebus/src/vespa/messagebus/network/rpcsendv2.cpp
index 9f5aad920b4..16175fea1c2 100644
--- a/messagebus/src/vespa/messagebus/network/rpcsendv2.cpp
+++ b/messagebus/src/vespa/messagebus/network/rpcsendv2.cpp
@@ -240,7 +240,7 @@ RPCSendV2::createResponse(FRT_Values & ret, const string & version, Reply & repl
     root.setString(PROTOCOL_F, reply.getProtocol());
     root.setData(BLOB_F, vespalib::Memory(payload.data(), payload.size()));
     if (reply.getTrace().getLevel() > 0) {
-        root.setString(TRACE_F, reply.getTrace().getRoot().encode());
+        root.setString(TRACE_F, reply.getTrace().encode());
     }
 
     if (reply.getNumErrors() > 0) {
diff --git a/messagebus_test/src/tests/trace/trace.cpp b/messagebus_test/src/tests/trace/trace.cpp
index ae051c4ecab..1ab30303d2c 100644
--- a/messagebus_test/src/tests/trace/trace.cpp
+++ b/messagebus_test/src/tests/trace/trace.cpp
@@ -107,7 +107,7 @@ Test::Main()
         if (reply) {
             reply->getTrace().normalize();
             // resending breaks the trace, so retry until it has expected form
-            if (!reply->hasErrors() && reply->getTrace().getRoot().encode() == expect.encode()) {
+            if (!reply->hasErrors() && reply->getTrace().encode() == expect.encode()) {
                 break;
             }
         }
@@ -116,7 +116,7 @@ Test::Main()
     }
 
     EXPECT_TRUE(!reply->hasErrors());
-    EXPECT_EQUAL(reply->getTrace().getRoot().encode(), expect.encode());
+    EXPECT_EQUAL(reply->getTrace().encode(), expect.encode());
     EXPECT_TRUE(system((ctl_script + " stop all").c_str()) == 0);
     TEST_DONE();
 }
diff --git a/storage/src/tests/visiting/memory_bounded_trace_test.cpp b/storage/src/tests/visiting/memory_bounded_trace_test.cpp
index d0ea1172e11..e43fa702fb5 100644
--- a/storage/src/tests/visiting/memory_bounded_trace_test.cpp
+++ b/storage/src/tests/visiting/memory_bounded_trace_test.cpp
@@ -55,7 +55,7 @@ TEST(MemoryBoundedTraceTest, moved_trace_tree_is_marked_as_strict) {
     mbus::Trace target;
     trace.moveTraceTo(target);
     EXPECT_EQ(1, target.getNumChildren());
-    EXPECT_TRUE(target.getRoot().getChild(0).isStrict());
+    EXPECT_TRUE(target.getChild(0).isStrict());
 }
 
 TEST(MemoryBoundedTraceTest, can_not_add_more_nodes_when_memory_used_exceeds_upper_bound) {
diff --git a/storage/src/vespa/storage/distributor/operations/external/getoperation.cpp b/storage/src/vespa/storage/distributor/operations/external/getoperation.cpp
index 8f47798848a..3adc8d731ee 100644
--- a/storage/src/vespa/storage/distributor/operations/external/getoperation.cpp
+++ b/storage/src/vespa/storage/distributor/operations/external/getoperation.cpp
@@ -144,9 +144,7 @@ GetOperation::onReceive(DistributorMessageSender& sender, const std::shared_ptr<
 
     LOG(debug, "Received %s", msg->toString(true).c_str());
 
-    if ( ! getreply->getTrace().isEmpty()) {
-        _msg->getTrace().addChild(std::move(getreply->getTrace()));
-    }
+    _msg->getTrace().addChild(std::move(getreply->getTrace()));
     bool allDone = true;
     for (auto& response : _responses) {
         for (uint32_t i = 0; i < response.second.size(); i++) {
diff --git a/storage/src/vespa/storage/distributor/operations/external/visitoroperation.h b/storage/src/vespa/storage/distributor/operations/external/visitoroperation.h
index 42b0bd56b9e..24c7157b481 100644
--- a/storage/src/vespa/storage/distributor/operations/external/visitoroperation.h
+++ b/storage/src/vespa/storage/distributor/operations/external/visitoroperation.h
@@ -38,7 +38,7 @@ public:
                      const Config& config,
                      VisitorMetricSet& metrics);
 
-    ~VisitorOperation();
+    ~VisitorOperation() override;
 
     void onClose(DistributorMessageSender& sender) override;
     void onStart(DistributorMessageSender& sender) override;
diff --git a/storage/src/vespa/storage/persistence/persistenceutil.cpp b/storage/src/vespa/storage/persistence/persistenceutil.cpp
index 96a24134e5f..d41ec4e88d6 100644
--- a/storage/src/vespa/storage/persistence/persistenceutil.cpp
+++ b/storage/src/vespa/storage/persistence/persistenceutil.cpp
@@ -93,9 +93,7 @@ MessageTracker::sendReply() {
               _msg->toString(true).c_str(), vespalib::to_s(duration));
     }
     if (hasReply()) {
-        if ( ! _context.getTrace().isEmpty()) {
-            getReply().getTrace().addChild(std::move(_context.getTrace()));
-        }
+        getReply().getTrace().addChild(std::move(_context.getTrace()));
         if (_updateBucketInfo) {
             if (getReply().getResult().success()) {
                 _env.setBucketInfo(*this, _bucketLock->getBucket());
@@ -107,9 +105,7 @@ MessageTracker::sendReply() {
         LOG(spam, "Sending reply up: %s %" PRIu64, getReply().toString().c_str(), getReply().getMsgId());
         _replySender.sendReplyDirectly(std::move(_reply));
     } else {
-        if ( ! _context.getTrace().isEmpty()) {
-            _msg->getTrace().addChild(std::move(_context.getTrace()));
-        }
+        _msg->getTrace().addChild(std::move(_context.getTrace()));
     }
 }
 
diff --git a/storage/src/vespa/storage/storageserver/rpc/storage_api_rpc_service.cpp b/storage/src/vespa/storage/storageserver/rpc/storage_api_rpc_service.cpp
index eaba3d54c27..c465315a5a6 100644
--- a/storage/src/vespa/storage/storageserver/rpc/storage_api_rpc_service.cpp
+++ b/storage/src/vespa/storage/storageserver/rpc/storage_api_rpc_service.cpp
@@ -208,7 +208,7 @@ void StorageApiRpcService::encode_rpc_v1_response(FRT_RPCRequest& request, api::
     // TODO skip encoding header altogether if no relevant fields set?
     protobuf::ResponseHeader hdr;
     if (reply.getTrace().getLevel() > 0) {
-        hdr.set_trace_payload(reply.getTrace().getRoot().encode());
+        hdr.set_trace_payload(reply.getTrace().encode());
     }
     // TODO consistent naming...
     encode_header_into_rpc_params(hdr, *ret);
diff --git a/storage/src/vespa/storage/visiting/memory_bounded_trace.cpp b/storage/src/vespa/storage/visiting/memory_bounded_trace.cpp
index bfa4c2aa07d..d93a461f217 100644
--- a/storage/src/vespa/storage/visiting/memory_bounded_trace.cpp
+++ b/storage/src/vespa/storage/visiting/memory_bounded_trace.cpp
@@ -33,8 +33,10 @@ computeTraceTreeMemoryUsage(const mbus::TraceNode& node)
 } // anon ns
 
 bool
-MemoryBoundedTrace::add(const mbus::TraceNode& node)
+MemoryBoundedTrace::add(const mbus::Trace& trace)
 {
+    if (trace.isEmpty()) return false;
+    const vespalib::TraceNode & node = trace.getRoot();
     const size_t nodeFootprint = computeTraceTreeMemoryUsage(node);
 
     if (_currentMemoryUsed >= _softMemoryUpperBound) {
diff --git a/storage/src/vespa/storage/visiting/memory_bounded_trace.h b/storage/src/vespa/storage/visiting/memory_bounded_trace.h
index a7f8a00d6a9..f376b41a4f5 100644
--- a/storage/src/vespa/storage/visiting/memory_bounded_trace.h
+++ b/storage/src/vespa/storage/visiting/memory_bounded_trace.h
@@ -22,13 +22,7 @@ public:
      * Returns true if `node` was added to internal trace state, false
      * otherwise.
      */
-    bool add(const mbus::TraceNode& node);
-    bool add(const mbus::Trace& trace) {
-        if (!trace.isEmpty()) {
-            return add(trace.getRoot());
-        }
-        return false;
-    }
+    bool add(const mbus::Trace& node);
 
     /**
      * Append current trace tree to the output trace node and clear internal
diff --git a/storage/src/vespa/storage/visiting/visitor.cpp b/storage/src/vespa/storage/visiting/visitor.cpp
index 26efaf323e0..c7825cd2e6f 100644
--- a/storage/src/vespa/storage/visiting/visitor.cpp
+++ b/storage/src/vespa/storage/visiting/visitor.cpp
@@ -601,7 +601,7 @@ bool
 Visitor::addBoundedTrace(uint32_t level, const vespalib::string &message) {
     mbus::Trace tempTrace;
     tempTrace.trace(level, message);
-    return _trace.add(tempTrace.getRoot());
+    return _trace.add(tempTrace);
 }
 
 void
diff --git a/vespalib/src/vespa/vespalib/trace/trace.cpp b/vespalib/src/vespa/vespalib/trace/trace.cpp
index ea93f574ed0..df9b5111d70 100644
--- a/vespalib/src/vespa/vespalib/trace/trace.cpp
+++ b/vespalib/src/vespa/vespalib/trace/trace.cpp
@@ -36,6 +36,11 @@ Trace::toString() const {
     return _root ? _root->toString(31337) : "";
 }
 
+string
+Trace::encode() const {
+    return isEmpty() ? "" : getRoot().encode();
+}
+
 void
 Trace::clear() {
     _level = 0;
diff --git a/vespalib/src/vespa/vespalib/trace/trace.h b/vespalib/src/vespa/vespalib/trace/trace.h
index 1300f121404..4b3752638f1 100644
--- a/vespalib/src/vespa/vespalib/trace/trace.h
+++ b/vespalib/src/vespa/vespalib/trace/trace.h
@@ -50,22 +50,10 @@ public:
         return *this;
     }
 
-    /**
-     * Set the trace level. 0 means no tracing, 9 means enable all tracing.
-     *
-     * @param level The level to set.
-     * @return This, to allow chaining.
-     */
-    Trace &setLevel(uint32_t level) {
+    void setLevel(uint32_t level) {
         _level = std::min(level, 9u);
-        return *this;
     }
 
-    /**
-     * Returns the trace level.
-     *
-     * @return The trace level.
-     */
     uint32_t getLevel() const { return _level; }
 
     /**
@@ -98,36 +86,31 @@ public:
         }
     }
 
-    /**
-     * Adds a child node to this.
-     *
-     * @param child The child to add.
-     * @return This, to allow chaining.
-     */
-    void addChild(TraceNode && child) {
-        ensureRoot().addChild(std::move(child));
-    }
-
     void setStrict(bool strict) {
         ensureRoot().setStrict(strict);
     }
+    void addChild(TraceNode && child) {
+        ensureRoot().addChild(std::move(child));
+    }
     void addChild(Trace && child) {
         if (!child.isEmpty()) {
             addChild(std::move(*child._root));
         }
     }
+    //TODO This one should go away as we should prefer moving
+    void addChild(const Trace & child) {
+        if (!child.isEmpty()) {
+            addChild(TraceNode(*child._root));
+        }
+    }
 
-    /**
-     * Returns a const reference to the root of the trace tree.
-     *
-     * @return The root.
-     */
     const TraceNode &getRoot() const { return *_root; }
 
     bool isEmpty() const { return !_root || _root->isEmpty(); }
 
     uint32_t getNumChildren() const { return _root ? _root->getNumChildren() : 0; }
     const TraceNode & getChild(uint32_t child) const { return getRoot().getChild(child); }
+    string encode() const;
 
     /**
      * Returns a string representation of the contained trace tree. This is a