fix(dnsdist): Move Tracer to shared_ptr

This makes it impossible to create a Tracer object outside of a
shared_ptr as well. Thanks to Remi for the shared_from_this hint.

diff --git a/pdns/dnsdistdist/dnsdist-idstate.hh b/pdns/dnsdistdist/dnsdist-idstate.hh
index dbf700fce2ad4482cc4c7aa03ebcf24a92ad292b..a1e5cdd18d3301782fed35682ad5ed7e627bf158 100644 (file)
--- a/pdns/dnsdistdist/dnsdist-idstate.hh
+++ b/pdns/dnsdistdist/dnsdist-idstate.hh
@@ -117,8 +117,7 @@ struct InternalQueryState
   bool tracingEnabled = false;
 
   // TODO: Do we want to keep some data *without* creating a tracer for each query?
-  // TODO: shard_ptr to work with Tracer::Closer?
-  std::unique_ptr<pdns::trace::dnsdist::Tracer> d_OTTracer{new pdns::trace::dnsdist::Tracer};
+  std::shared_ptr<pdns::trace::dnsdist::Tracer> d_OTTracer{pdns::trace::dnsdist::Tracer::getTracer()};
 
   InternalQueryState()
   {

diff --git a/pdns/dnsdistdist/dnsdist-opentelemetry.cc b/pdns/dnsdistdist/dnsdist-opentelemetry.cc
index 9b8a061ea4e9e5aae4f921e4ab574ae2b9edacac..9cd71478f61914e975605d9a4be3c705c294d58a 100644 (file)
--- a/pdns/dnsdistdist/dnsdist-opentelemetry.cc
+++ b/pdns/dnsdistdist/dnsdist-opentelemetry.cc
@@ -236,7 +236,7 @@ Tracer::Closer Tracer::getCloser([[maybe_unused]] const SpanID& spanid)
 #ifdef DISABLE_PROTOBUF
   return Tracer::Closer();
 #else
-  return {this, spanid};
+  return {shared_from_this(), spanid};
 #endif
 }
 

diff --git a/pdns/dnsdistdist/dnsdist-opentelemetry.hh b/pdns/dnsdistdist/dnsdist-opentelemetry.hh
index c1316ca827384663c17d42acc74215089a166b56..3e6837167818787ad4549a3db3c613a3be9322db 100644 (file)
--- a/pdns/dnsdistdist/dnsdist-opentelemetry.hh
+++ b/pdns/dnsdistdist/dnsdist-opentelemetry.hh
@@ -21,6 +21,7 @@
  */
 #pragma once
 
+#include <memory>
 #include <string>
 #include <vector>
 
@@ -59,16 +60,23 @@ namespace pdns::trace::dnsdist
  * @brief This class holds a single trace instance
  *
  */
-class Tracer
+class Tracer : public std::enable_shared_from_this<Tracer>
 {
 public:
-  Tracer() = default;
   ~Tracer() = default;
   Tracer(const Tracer&) = delete;
   Tracer& operator=(const Tracer) = delete;
   Tracer& operator=(Tracer&&) = delete;
   Tracer(Tracer&&) = delete;
 
+  /**
+   * @brief get a new Tracer
+   */
+  static std::shared_ptr<Tracer> getTracer()
+  {
+    return std::shared_ptr<Tracer>(new Tracer);
+  }
+
   /**
    * @brief Activate the Tracer
    *
@@ -173,11 +181,9 @@ public:
     /**
      * @brief An empty Closer, not really useful
      */
-#ifdef DISABLE_PROTOBUF
     Closer() = default;
-#else
-    Closer() :
-      d_tracer(nullptr), d_spanID(SpanID{}) {};
+
+#ifndef DISABLE_PROTOBUF
     /**
      * @brief Create a Closer
      *
@@ -189,8 +195,8 @@ public:
      * @param tracer A pointer to the Tracer where we want to close a Span
      * @param spanid The SpanID to close in the Tracer
      */
-    Closer(Tracer* tracer, const SpanID& spanid) :
-      d_tracer(tracer), d_spanID(spanid) {};
+    Closer(std::shared_ptr<Tracer> tracer, const SpanID& spanid) :
+      d_tracer(std::move(tracer)), d_spanID(spanid) {};
 #endif
 
     /**
@@ -218,9 +224,8 @@ public:
 
   private:
 #ifndef DISABLE_PROTOBUF
-    // XXX: Should we make this a shared_ptr and force all consumers to Tracer to keep it as a shared_ptr as well?
-    Tracer* d_tracer;
-    SpanID d_spanID;
+    std::shared_ptr<Tracer> d_tracer{nullptr};
+    SpanID d_spanID{};
 #endif
   };
 
@@ -250,6 +255,8 @@ public:
   Closer openSpan(const std::string& name, const SpanID& parentSpanID);
 
 private:
+  Tracer() = default;
+
   /**
    * @brief Create a new Span
    *

diff --git a/pdns/dnsdistdist/test-dnsdist-opentelemetry_cc.cc b/pdns/dnsdistdist/test-dnsdist-opentelemetry_cc.cc
index 4e9c534e1b1ed90838ff40fd76bd8a2ceee879d3..f9501c45f5ac110d4ad93d1f9e41fe33a13c8b04 100644 (file)
--- a/pdns/dnsdistdist/test-dnsdist-opentelemetry_cc.cc
+++ b/pdns/dnsdistdist/test-dnsdist-opentelemetry_cc.cc
@@ -37,51 +37,51 @@ BOOST_AUTO_TEST_SUITE(dnsdistopentelemetry_cc)
 BOOST_AUTO_TEST_CASE(getTraceID)
 {
   // Ensure we always have a TraceID after activation
-  auto tracer = pdns::trace::dnsdist::Tracer();
-  BOOST_CHECK_EQUAL(tracer.getTraceID(), TraceID{});
+  auto tracer = pdns::trace::dnsdist::Tracer::getTracer();
+  BOOST_CHECK_EQUAL(tracer->getTraceID(), TraceID{});
 
   // Ensure we have one *after* activation
-  tracer.activate();
-  auto traceid = tracer.getTraceID();
+  tracer->activate();
+  auto traceid = tracer->getTraceID();
   BOOST_CHECK_NE(traceid, TraceID{});
 
   // Ensure we have the same one *after* deactivation
-  tracer.deactivate();
-  BOOST_CHECK_EQUAL(tracer.getTraceID(), traceid);
+  tracer->deactivate();
+  BOOST_CHECK_EQUAL(tracer->getTraceID(), traceid);
 
   // Ensure we have the same one *after* reactivation
-  tracer.deactivate();
-  BOOST_CHECK_EQUAL(tracer.getTraceID(), traceid);
+  tracer->deactivate();
+  BOOST_CHECK_EQUAL(tracer->getTraceID(), traceid);
 }
 
 BOOST_AUTO_TEST_CASE(getLastSpanID)
 {
-  auto tracer = pdns::trace::dnsdist::Tracer();
+  auto tracer = pdns::trace::dnsdist::Tracer::getTracer();
 
   // Empty SpanID returned when there are no spans
-  auto lastSpanID = tracer.getLastSpanID();
+  auto lastSpanID = tracer->getLastSpanID();
   BOOST_CHECK_EQUAL(lastSpanID, SpanID{});
 
   // Add event before activation
-  auto spanid = tracer.openSpan("myevent").getSpanID();
-  lastSpanID = tracer.getLastSpanID();
+  auto spanid = tracer->openSpan("myevent").getSpanID();
+  lastSpanID = tracer->getLastSpanID();
   BOOST_CHECK_EQUAL(spanid, lastSpanID);
 
   for (auto i = 0; i < 4; i++) {
-    spanid = tracer.openSpan("myevent" + std::to_string(i)).getSpanID();
+    spanid = tracer->openSpan("myevent" + std::to_string(i)).getSpanID();
   }
-  lastSpanID = tracer.getLastSpanID();
+  lastSpanID = tracer->getLastSpanID();
   BOOST_CHECK_EQUAL(spanid, lastSpanID);
 
-  tracer.activate();
-  spanid = tracer.openSpan("post-activation-myevent").getSpanID();
-  lastSpanID = tracer.getLastSpanID();
+  tracer->activate();
+  spanid = tracer->openSpan("post-activation-myevent").getSpanID();
+  lastSpanID = tracer->getLastSpanID();
   BOOST_CHECK_EQUAL(spanid, lastSpanID);
 
   for (auto i = 0; i < 4; i++) {
-    spanid = tracer.openSpan("post-activation-myevent" + std::to_string(i)).getSpanID();
+    spanid = tracer->openSpan("post-activation-myevent" + std::to_string(i)).getSpanID();
   }
-  lastSpanID = tracer.getLastSpanID();
+  lastSpanID = tracer->getLastSpanID();
   BOOST_CHECK_EQUAL(spanid, lastSpanID);
 }
 
@@ -89,61 +89,61 @@ BOOST_AUTO_TEST_CASE(getLastSpanIDForName)
 {
   // We only create spans with the same name
   std::string eventName{"myEvent"};
-  auto tracer = pdns::trace::dnsdist::Tracer();
+  auto tracer = pdns::trace::dnsdist::Tracer::getTracer();
 
   // Empty SpanID returned when there are no spans
-  auto lastSpanID = tracer.getLastSpanIDForName(eventName);
+  auto lastSpanID = tracer->getLastSpanIDForName(eventName);
   BOOST_CHECK_EQUAL(lastSpanID, SpanID{});
 
   // Add event before activation
-  auto spanid = tracer.openSpan(eventName).getSpanID();
-  lastSpanID = tracer.getLastSpanIDForName(eventName);
+  auto spanid = tracer->openSpan(eventName).getSpanID();
+  lastSpanID = tracer->getLastSpanIDForName(eventName);
   BOOST_CHECK_EQUAL(spanid, lastSpanID);
 
   for (auto i = 0; i < 4; i++) {
-    spanid = tracer.openSpan(eventName).getSpanID();
+    spanid = tracer->openSpan(eventName).getSpanID();
   }
-  lastSpanID = tracer.getLastSpanIDForName(eventName);
+  lastSpanID = tracer->getLastSpanIDForName(eventName);
   BOOST_CHECK_EQUAL(spanid, lastSpanID);
   auto preactivationSpanID = spanid;
 
-  tracer.activate();
-  spanid = tracer.openSpan(eventName).getSpanID();
-  lastSpanID = tracer.getLastSpanIDForName(eventName);
+  tracer->activate();
+  spanid = tracer->openSpan(eventName).getSpanID();
+  lastSpanID = tracer->getLastSpanIDForName(eventName);
   BOOST_CHECK_EQUAL(spanid, lastSpanID);
 
   for (auto i = 0; i < 4; i++) {
-    spanid = tracer.openSpan(eventName).getSpanID();
+    spanid = tracer->openSpan(eventName).getSpanID();
   }
-  lastSpanID = tracer.getLastSpanIDForName(eventName);
+  lastSpanID = tracer->getLastSpanIDForName(eventName);
   BOOST_CHECK_EQUAL(spanid, lastSpanID);
 
-  tracer.deactivate();
-  lastSpanID = tracer.getLastSpanIDForName(eventName);
+  tracer->deactivate();
+  lastSpanID = tracer->getLastSpanIDForName(eventName);
   BOOST_CHECK_EQUAL(preactivationSpanID, lastSpanID);
 }
 
 BOOST_AUTO_TEST_CASE(activate)
 {
-  auto tracer = pdns::trace::dnsdist::Tracer();
+  auto tracer = pdns::trace::dnsdist::Tracer::getTracer();
 
   // We don't actually check the internal state, but we infer it from the order
   // of the output.
-  auto preActivationSpanID = tracer.openSpan("pre-activation-event").getSpanID();
-  tracer.activate();
-  auto postActivationSpanID = tracer.openSpan("post-activation-event").getSpanID();
+  auto preActivationSpanID = tracer->openSpan("pre-activation-event").getSpanID();
+  tracer->activate();
+  auto postActivationSpanID = tracer->openSpan("post-activation-event").getSpanID();
 
   // Ensure order is pre1, post1
-  auto trace = tracer.getTracesData();
+  auto trace = tracer->getTracesData();
   BOOST_ASSERT(trace.resource_spans.at(0).scope_spans.at(0).spans.size() == 2);
   BOOST_CHECK_EQUAL(trace.resource_spans.at(0).scope_spans.at(0).spans.at(0).span_id, preActivationSpanID);
   BOOST_CHECK_EQUAL(trace.resource_spans.at(0).scope_spans.at(0).spans.at(1).span_id, postActivationSpanID);
 
   // Now deactivate and check if the order will be pre1, pre2, post1
-  tracer.deactivate();
-  auto preActivationSpanID2 = tracer.openSpan("pre-activation-event2").getSpanID();
+  tracer->deactivate();
+  auto preActivationSpanID2 = tracer->openSpan("pre-activation-event2").getSpanID();
 
-  trace = tracer.getTracesData();
+  trace = tracer->getTracesData();
   BOOST_ASSERT(trace.resource_spans.at(0).scope_spans.at(0).spans.size() == 3);
   BOOST_CHECK_EQUAL(trace.resource_spans.at(0).scope_spans.at(0).spans.at(0).span_id, preActivationSpanID);
   BOOST_CHECK_EQUAL(trace.resource_spans.at(0).scope_spans.at(0).spans.at(1).span_id, preActivationSpanID2);
@@ -152,22 +152,22 @@ BOOST_AUTO_TEST_CASE(activate)
 
 BOOST_AUTO_TEST_CASE(Closer)
 {
-  auto tracer = pdns::trace::dnsdist::Tracer();
+  auto tracer = pdns::trace::dnsdist::Tracer::getTracer();
 
-  auto prespanid = tracer.openSpan("foo").getSpanID();
-  tracer.activate();
-  auto postspanid = tracer.openSpan("bar").getSpanID();
+  auto prespanid = tracer->openSpan("foo").getSpanID();
+  tracer->activate();
+  auto postspanid = tracer->openSpan("bar").getSpanID();
 
   SpanID openeventSpanID;
   SpanID openevent2SpanID;
 
   {
-    auto precloser = tracer.getCloser(prespanid);
-    auto postcloser = tracer.getCloser(postspanid);
+    auto precloser = tracer->getCloser(prespanid);
+    auto postcloser = tracer->getCloser(postspanid);
 
-    auto openEventCloser = tracer.openSpan("openEvent");
+    auto openEventCloser = tracer->openSpan("openEvent");
     openeventSpanID = openEventCloser.getSpanID();
-    auto openEventCloser2 = tracer.openSpan("openEvent2", openeventSpanID);
+    auto openEventCloser2 = tracer->openSpan("openEvent2", openeventSpanID);
     openevent2SpanID = openEventCloser2.getSpanID();
 
     // Make sure the destructor does not segfault when it is empty
@@ -175,7 +175,7 @@ BOOST_AUTO_TEST_CASE(Closer)
   }
 
   // Closer is out of scope, so each event should have a closing time
-  auto trace = tracer.getTracesData();
+  auto trace = tracer->getTracesData();
   BOOST_ASSERT(trace.resource_spans.at(0).scope_spans.at(0).spans.size() == 4);
 
   BOOST_CHECK_EQUAL(trace.resource_spans.at(0).scope_spans.at(0).spans.at(0).span_id, prespanid);
@@ -197,20 +197,20 @@ BOOST_AUTO_TEST_CASE(Closer)
 
 BOOST_AUTO_TEST_CASE(attributes)
 {
-  auto tracer = pdns::trace::dnsdist::Tracer();
-  tracer.setTraceAttribute("foo", AnyValue{"bar"});
+  auto tracer = pdns::trace::dnsdist::Tracer::getTracer();
+  tracer->setTraceAttribute("foo", AnyValue{"bar"});
 
   // Test that no attributes are added when the tracer is not activated
-  auto trace = tracer.getTracesData();
+  auto trace = tracer->getTracesData();
   BOOST_CHECK_EQUAL(trace.resource_spans.at(0).resource.attributes.size(), 1);
   BOOST_CHECK_EQUAL(trace.resource_spans.at(0).resource.attributes.at(0).key, "service.name");
 
   // Now activate and add 2 attributes
-  tracer.activate();
-  tracer.setTraceAttribute("foo", AnyValue{"bar"});
-  tracer.setTraceAttribute("baz", AnyValue{256});
+  tracer->activate();
+  tracer->setTraceAttribute("foo", AnyValue{"bar"});
+  tracer->setTraceAttribute("baz", AnyValue{256});
 
-  trace = tracer.getTracesData();
+  trace = tracer->getTracesData();
 
   BOOST_ASSERT(trace.resource_spans.at(0).resource.attributes.size() == 3);
   BOOST_CHECK_EQUAL(trace.resource_spans.at(0).resource.attributes.at(1).key, "foo");
@@ -220,10 +220,10 @@ BOOST_AUTO_TEST_CASE(attributes)
   BOOST_CHECK_EQUAL(trace.resource_spans.at(0).resource.attributes.at(2).value, AnyValue{256});
 
   // Add a span and some attributes
-  auto spanid = tracer.openSpan("anEvent").getSpanID();
-  tracer.setSpanAttribute(spanid, "spanattr", AnyValue{"exciting"});
+  auto spanid = tracer->openSpan("anEvent").getSpanID();
+  tracer->setSpanAttribute(spanid, "spanattr", AnyValue{"exciting"});
 
-  trace = tracer.getTracesData();
+  trace = tracer->getTracesData();
 
   BOOST_ASSERT(trace.resource_spans.at(0).scope_spans.at(0).spans.at(0).attributes.size() == 1);
   BOOST_CHECK_EQUAL(trace.resource_spans.at(0).scope_spans.at(0).spans.at(0).attributes.front().key, "spanattr");
@@ -232,13 +232,13 @@ BOOST_AUTO_TEST_CASE(attributes)
 
 BOOST_AUTO_TEST_CASE(getOTProtobuf)
 {
-  auto tracer = pdns::trace::dnsdist::Tracer();
-  auto data = tracer.getOTProtobuf();
+  auto tracer = pdns::trace::dnsdist::Tracer::getTracer();
+  auto data = tracer->getOTProtobuf();
   BOOST_TEST(data.size() == 31U);
 
-  tracer.activate();
-  tracer.setTraceAttribute("foo", AnyValue{"bar"});
-  data = tracer.getOTProtobuf();
+  tracer->activate();
+  tracer->setTraceAttribute("foo", AnyValue{"bar"});
+  data = tracer->getOTProtobuf();
   BOOST_TEST(data.size() == 45U);
 }