TraceSegment can outlive Tracer (#89)

* TraceSegment can outlive Tracer

* remove comment

It looks like there's a typo at the beginning, and I couldn't think of a
good way to describe `client_id` concisely, so probably best to omit the
comment.

Author: dgoffredo

src/datadog/datadog_agent.cpp

@@ -150,7 +150,8 @@ DatadogAgent::DatadogAgent(
     const FinalizedDatadogAgentConfig& config,
     const std::shared_ptr<TracerTelemetry>& tracer_telemetry,
     const std::shared_ptr<Logger>& logger,
-    const TracerSignature& tracer_signature, ConfigManager& config_manager)
+    const TracerSignature& tracer_signature,
+    const std::shared_ptr<ConfigManager>& config_manager)
     : tracer_telemetry_(tracer_telemetry),
       clock_(config.clock),
       logger_(logger),

src/datadog/datadog_agent.h

@@ -66,7 +66,7 @@ class DatadogAgent : public Collector {
   DatadogAgent(const FinalizedDatadogAgentConfig&,
                const std::shared_ptr<TracerTelemetry>&,
                const std::shared_ptr<Logger>&, const TracerSignature& id,
-               ConfigManager& config_manager);
+               const std::shared_ptr<ConfigManager>& config_manager);
   ~DatadogAgent();
 
   Expected<void> send(

src/datadog/remote_config.cpp

@@ -1,5 +1,6 @@
 #include "remote_config.h"
 
+#include <cassert>
 #include <cstdint>
 #include <type_traits>
 #include <unordered_set>
@@ -64,10 +65,13 @@ ConfigUpdate parse_dynamic_config(const nlohmann::json& j) {
 }  // namespace
 
 RemoteConfigurationManager::RemoteConfigurationManager(
-    const TracerSignature& tracer_signature, ConfigManager& config_manager)
+    const TracerSignature& tracer_signature,
+    const std::shared_ptr<ConfigManager>& config_manager)
     : tracer_signature_(tracer_signature),
       config_manager_(config_manager),
-      client_id_(uuid()) {}
+      client_id_(uuid()) {
+  assert(config_manager_);
+}
 
 bool RemoteConfigurationManager::is_new_config(
     StringView config_path, const nlohmann::json& config_meta) {
@@ -215,11 +219,11 @@ void RemoteConfigurationManager::process_response(const nlohmann::json& json) {
 }
 
 void RemoteConfigurationManager::apply_config(Configuration config) {
-  config_manager_.update(config.content);
+  config_manager_->update(config.content);
 }
 
 void RemoteConfigurationManager::revert_config(Configuration) {
-  config_manager_.reset();
+  config_manager_->reset();
 }
 
 }  // namespace tracing

src/datadog/remote_config.h

@@ -12,6 +12,7 @@
 // It interacts with the `ConfigManager` to seamlessly apply or revert
 // configurations based on responses received from the remote source.
 
+#include <memory>
 #include <string>
 
 #include "config_manager.h"
@@ -44,15 +45,16 @@ class RemoteConfigurationManager {
   };
 
   TracerSignature tracer_signature_;
-  ConfigManager& config_manager_;
-  std::string client_id_;  ///< Identifier a `RemoteConfigurationManager`
+  std::shared_ptr<ConfigManager> config_manager_;
+  std::string client_id_;
 
   State state_;
   std::unordered_map<std::string, Configuration> applied_config_;
 
  public:
-  RemoteConfigurationManager(const TracerSignature& tracer_signature,
-                             ConfigManager& config_manager);
+  RemoteConfigurationManager(
+      const TracerSignature& tracer_signature,
+      const std::shared_ptr<ConfigManager>& config_manager);
 
   // Construct a JSON object representing the payload to be sent in a remote
   // configuration request.

src/datadog/tracer.cpp

@@ -36,6 +36,7 @@ Tracer::Tracer(const FinalizedTracerConfig& config)
 Tracer::Tracer(const FinalizedTracerConfig& config,
                const std::shared_ptr<const IDGenerator>& generator)
     : logger_(config.logger),
+      config_manager_(std::make_shared<ConfigManager>(config)),
       collector_(/* see constructor body */),
       defaults_(std::make_shared<SpanDefaults>(config.defaults)),
       runtime_id_(config.runtime_id ? *config.runtime_id
@@ -53,7 +54,6 @@ Tracer::Tracer(const FinalizedTracerConfig& config,
       extraction_styles_(config.extraction_styles),
       hostname_(config.report_hostname ? get_hostname() : nullopt),
       tags_header_max_size_(config.tags_header_size),
-      config_manager_(config),
       sampling_delegation_enabled_(config.delegate_trace_sampling) {
   if (auto* collector =
           std::get_if<std::shared_ptr<Collector>>(&config.collector)) {
@@ -94,7 +94,7 @@ nlohmann::json Tracer::config_json() const {
   });
   // clang-format on
 
-  config.merge_patch(config_manager_.config_json());
+  config.merge_patch(config_manager_->config_json());
 
   if (hostname_) {
     config["hostname"] = *hostname_;
@@ -122,7 +122,7 @@ Span Tracer::create_span(const SpanConfig& config) {
   tracer_telemetry_->metrics().tracer.trace_segments_created_new.inc();
   const auto segment = std::make_shared<TraceSegment>(
       logger_, collector_, tracer_telemetry_,
-      config_manager_.get_trace_sampler(), span_sampler_, defaults_,
+      config_manager_->get_trace_sampler(), span_sampler_, defaults_,
       runtime_id_, sampling_delegation_enabled_,
       false /* sampling_decision_was_delegated_to_me */, injection_styles_,
       hostname_, nullopt /* origin */, tags_header_max_size_,
@@ -293,7 +293,7 @@ Expected<Span> Tracer::extract_span(const DictReader& reader,
   tracer_telemetry_->metrics().tracer.trace_segments_created_continued.inc();
   const auto segment = std::make_shared<TraceSegment>(
       logger_, collector_, tracer_telemetry_,
-      config_manager_.get_trace_sampler(), span_sampler_, defaults_,
+      config_manager_->get_trace_sampler(), span_sampler_, defaults_,
       runtime_id_, sampling_delegation_enabled_, delegate_sampling_decision,
       injection_styles_, hostname_, std::move(origin), tags_header_max_size_,
       std::move(trace_tags), std::move(sampling_decision),

src/datadog/tracer.h

@@ -10,6 +10,9 @@
 // obtained from a `TracerConfig` via the `finalize_config` function.  See
 // `tracer_config.h`.
 
+#include <cstddef>
+#include <memory>
+
 #include "clock.h"
 #include "config_manager.h"
 #include "error.h"
@@ -32,6 +35,7 @@ class SpanSampler;
 
 class Tracer {
   std::shared_ptr<Logger> logger_;
+  std::shared_ptr<ConfigManager> config_manager_;
   std::shared_ptr<Collector> collector_;
   std::shared_ptr<const SpanDefaults> defaults_;
   RuntimeID runtime_id_;
@@ -44,7 +48,6 @@ class Tracer {
   std::vector<PropagationStyle> extraction_styles_;
   Optional<std::string> hostname_;
   std::size_t tags_header_max_size_;
-  ConfigManager config_manager_;
   bool sampling_delegation_enabled_;
 
  public:

test/test_remote_config.cpp

@@ -28,7 +28,8 @@ REMOTE_CONFIG_TEST("first payload") {
   TracerConfig config;
   config.defaults.service = "testsvc";
   config.defaults.environment = "test";
-  ConfigManager config_manager(*finalize_config(config));
+  const auto config_manager =
+      std::make_shared<ConfigManager>(*finalize_config(config));
 
   RemoteConfigurationManager rc(tracer_signature, config_manager);
 
@@ -62,7 +63,8 @@ REMOTE_CONFIG_TEST("response processing") {
   config.defaults.service = "testsvc";
   config.defaults.environment = "test";
   config.trace_sampler.sample_rate = 1.0;
-  ConfigManager config_manager(*finalize_config(config));
+  const auto config_manager =
+      std::make_shared<ConfigManager>(*finalize_config(config));
 
   RemoteConfigurationManager rc(tracer_signature, config_manager);
 
@@ -177,9 +179,9 @@ REMOTE_CONFIG_TEST("response processing") {
 
     REQUIRE(!response_json.is_discarded());
 
-    const auto old_trace_sampler = config_manager.get_trace_sampler();
+    const auto old_trace_sampler = config_manager->get_trace_sampler();
     rc.process_response(response_json);
-    const auto new_trace_sampler = config_manager.get_trace_sampler();
+    const auto new_trace_sampler = config_manager->get_trace_sampler();
 
     CHECK(new_trace_sampler != old_trace_sampler);
 
@@ -201,7 +203,7 @@ REMOTE_CONFIG_TEST("response processing") {
         REQUIRE(!response_json.is_discarded());
 
         rc.process_response(response_json);
-        const auto current_trace_sampler = config_manager.get_trace_sampler();
+        const auto current_trace_sampler = config_manager->get_trace_sampler();
         CHECK(old_trace_sampler == current_trace_sampler);
       }
 
@@ -227,7 +229,7 @@ REMOTE_CONFIG_TEST("response processing") {
         REQUIRE(!response_json.is_discarded());
 
         rc.process_response(response_json);
-        const auto current_trace_sampler = config_manager.get_trace_sampler();
+        const auto current_trace_sampler = config_manager->get_trace_sampler();
         CHECK(old_trace_sampler == current_trace_sampler);
       }
     }
@@ -270,9 +272,9 @@ REMOTE_CONFIG_TEST("response processing") {
 
     REQUIRE(!response_json.is_discarded());
 
-    const auto old_sampling_rate = config_manager.get_trace_sampler();
+    const auto old_sampling_rate = config_manager->get_trace_sampler();
     rc.process_response(response_json);
-    const auto new_sampling_rate = config_manager.get_trace_sampler();
+    const auto new_sampling_rate = config_manager->get_trace_sampler();
 
     CHECK(new_sampling_rate == old_sampling_rate);
   }

test/test_trace_segment.cpp

@@ -1,3 +1,4 @@
+#include <datadog/optional.h>
 #include <datadog/platform_util.h>
 #include <datadog/rate.h>
 #include <datadog/tags.h>
@@ -446,3 +447,24 @@ TEST_CASE("TraceSegment finalization of spans") {
     }
   }
 }  // span finalizers
+
+TEST_CASE("independent of Tracer") {
+  // This test verifies that a `TraceSegment` (via the `Span`s that refer to it)
+  // can continue to operate even after the `Tracer` that created it is
+  // destroyed.
+  //
+  // Primarily, the test checks that the code doesn't crash in this scenario.
+  TracerConfig config;
+  config.defaults.service = "testsvc";
+  config.defaults.name = "do.thing";
+  config.logger = std::make_shared<NullLogger>();
+
+  auto maybe_tracer = finalize_config(config);
+  REQUIRE(maybe_tracer);
+  Optional<Tracer> tracer{*maybe_tracer};
+
+  Span root = tracer->create_span();
+  Span child = root.create_child();
+
+  tracer.reset();
+}

test/test_tracer.cpp

@@ -23,6 +23,7 @@
 #include <ctime>
 #include <iosfwd>
 #include <stdexcept>
+#include <utility>
 
 #include "matchers.h"
 #include "mocks/collectors.h"
@@ -1702,3 +1703,19 @@ TEST_CASE("heterogeneous extraction") {
 
   REQUIRE(writer.items == test_case.expected_injected_headers);
 }
+
+TEST_CASE("move semantics") {
+  // Verify that `Tracer` can be moved.
+  TracerConfig config;
+  config.defaults.service = "testsvc";
+  config.logger = std::make_shared<NullLogger>();
+  config.collector = std::make_shared<MockCollector>();
+
+  auto finalized_config = finalize_config(config);
+  REQUIRE(finalized_config);
+  Tracer tracer1{*finalized_config};
+
+  // This must compile.
+  Tracer tracer2{std::move(tracer1)};
+  (void)tracer2;
+}