first pass at a review of cgilmour/telemetry-api:

dgoffredo · cgilmour · commit e13e5d1087f8 · 2023-10-03T03:57:10.000Z
- mention new files in bazel build
- don't store FinalizedTracerConfig
- consistent spacing_style forMemberFunctions
- make config_json() available to send_app_started()
- fix unrelated pet peeve in use of log_startup
- remove dev noise, which fixed all but one of the broken unit tests
diff --git a/src/datadog/datadog_agent.cpp b/src/datadog/datadog_agent.cpp
@@ -150,13 +150,12 @@ DatadogAgent::DatadogAgent(
             n++;
             tracer_telemetry_->capture_metrics();
             if (n % 6 == 0) {
-              sendHeartbeatAndTelemetry();
+              send_heartbeat_and_telemetry();
             }
           })),
       flush_interval_(config.flush_interval) {
   assert(logger_);
   assert(tracer_telemetry_);
-  sendAppStarted();
 }
 
 DatadogAgent::~DatadogAgent() {
@@ -298,8 +297,8 @@ void DatadogAgent::flush() {
   }
 }
 
-void DatadogAgent::sendAppStarted() {
-  auto payload = tracer_telemetry_->app_started();
+void DatadogAgent::send_app_started(nlohmann::json&& tracer_config) {
+  auto payload = tracer_telemetry_->app_started(std::move(tracer_config));
   auto set_request_headers = [&](DictWriter& headers) {
     headers.set("Content-Type", "application/json");
   };
@@ -314,13 +313,6 @@ void DatadogAgent::sendAppStarted() {
                << " with body (starts on next line):\n"
                << response_body;
       });
-      return;
-    } else {
-      logger->log_error([&](auto& stream) {
-        stream << "Successful telemetry submission with response status "
-               << response_status << " and body (starts on next line):\n"
-               << response_body;
-      });
     }
   };
 
@@ -338,7 +330,7 @@ void DatadogAgent::sendAppStarted() {
   }
 }
 
-void DatadogAgent::sendHeartbeatAndTelemetry() {
+void DatadogAgent::send_heartbeat_and_telemetry() {
   auto payload = tracer_telemetry_->heartbeat_and_telemetry();
   auto set_request_headers = [&](DictWriter& headers) {
     headers.set("Content-Type", "application/json");
@@ -354,13 +346,6 @@ void DatadogAgent::sendHeartbeatAndTelemetry() {
                << " with body (starts on next line):\n"
                << response_body;
       });
-      return;
-    } else {
-      logger->log_error([&](auto& stream) {
-        stream << "Successful telemetry submission with response status "
-               << response_status << " and body (starts on next line):\n"
-               << response_body;
-      });
     }
   };
 
diff --git a/src/datadog/datadog_agent.h b/src/datadog/datadog_agent.h
@@ -47,6 +47,7 @@ class DatadogAgent : public Collector {
   std::chrono::steady_clock::duration flush_interval_;
 
   void flush();
+  void send_heartbeat_and_telemetry();
 
  public:
   DatadogAgent(const FinalizedDatadogAgentConfig&,
@@ -57,8 +58,8 @@ class DatadogAgent : public Collector {
   Expected<void> send(
       std::vector<std::unique_ptr<SpanData>>&& spans,
       const std::shared_ptr<TraceSampler>& response_handler) override;
-  void sendAppStarted();
-  void sendHeartbeatAndTelemetry();
+
+  void send_app_started(nlohmann::json&& tracer_config);
 
   nlohmann::json config_json() const override;
 };
diff --git a/src/datadog/tracer.cpp b/src/datadog/tracer.cpp
@@ -243,13 +243,14 @@ Tracer::Tracer(const FinalizedTracerConfig& config,
                const Clock& clock)
     : logger_(config.logger),
       collector_(/* see constructor body */),
-      tracer_telemetry_(std::make_shared<TracerTelemetry>(clock, config)),
+      defaults_(std::make_shared<SpanDefaults>(config.defaults)),
+      tracer_telemetry_(
+          std::make_shared<TracerTelemetry>(clock, logger_, defaults_)),
       trace_sampler_(
           std::make_shared<TraceSampler>(config.trace_sampler, clock)),
       span_sampler_(std::make_shared<SpanSampler>(config.span_sampler, clock)),
       generator_(generator),
       clock_(clock),
-      defaults_(std::make_shared<SpanDefaults>(config.defaults)),
       injection_styles_(config.injection_styles),
       extraction_styles_(config.extraction_styles),
       hostname_(config.report_hostname ? get_hostname() : nullopt),
@@ -260,14 +261,15 @@ Tracer::Tracer(const FinalizedTracerConfig& config,
   } else {
     auto& agent_config =
         std::get<FinalizedDatadogAgentConfig>(config.collector);
-    collector_ = std::make_shared<DatadogAgent>(agent_config, tracer_telemetry_,
+    auto agent = std::make_shared<DatadogAgent>(agent_config, tracer_telemetry_,
                                                 clock, config.logger);
+    collector_ = agent;
+    agent->send_app_started(config_json());
   }
 
   if (config.log_on_startup) {
-    auto json = config_json();
-    logger_->log_startup([&json](std::ostream& log) {
-      log << "DATADOG TRACER CONFIGURATION - " << json;
+    logger_->log_startup([this](std::ostream& log) {
+      log << "DATADOG TRACER CONFIGURATION - " << config_json();
     });
   }
 }
diff --git a/src/datadog/tracer.h b/src/datadog/tracer.h
@@ -31,12 +31,12 @@ class SpanSampler;
 class Tracer {
   std::shared_ptr<Logger> logger_;
   std::shared_ptr<Collector> collector_;
+  std::shared_ptr<const SpanDefaults> defaults_;
   std::shared_ptr<TracerTelemetry> tracer_telemetry_;
   std::shared_ptr<TraceSampler> trace_sampler_;
   std::shared_ptr<SpanSampler> span_sampler_;
   std::shared_ptr<const IDGenerator> generator_;
   Clock clock_;
-  std::shared_ptr<const SpanDefaults> defaults_;
   std::vector<PropagationStyle> injection_styles_;
   std::vector<PropagationStyle> extraction_styles_;
   Optional<std::string> hostname_;
diff --git a/src/datadog/tracer_telemetry.cpp b/src/datadog/tracer_telemetry.cpp
@@ -3,14 +3,16 @@
 #include "json.hpp"
 #include "logger.h"
 #include "platform_util.h"
+#include "span_defaults.h"
 #include "version.h"
 
 namespace datadog {
 namespace tracing {
 
-TracerTelemetry::TracerTelemetry(const Clock& clock,
-                                 const FinalizedTracerConfig& config)
-    : clock_(clock), config_(config) {
+TracerTelemetry::TracerTelemetry(
+    const Clock& clock, const std::shared_ptr<Logger>& logger,
+    const std::shared_ptr<const SpanDefaults>& span_defaults)
+    : clock_(clock), logger_(logger), span_defaults_(span_defaults) {
   metrics_snapshots_.emplace_back(metrics_.tracer.spans_created,
                                   MetricSnapshot{});
   metrics_snapshots_.emplace_back(metrics_.tracer.spans_finished,
@@ -41,12 +43,11 @@ TracerTelemetry::TracerTelemetry(const Clock& clock,
                                   MetricSnapshot{});
 }
 
-std::string TracerTelemetry::app_started() {
+std::string TracerTelemetry::app_started(nlohmann::json&& tracer_config) {
   time_t tracer_time = std::chrono::duration_cast<std::chrono::seconds>(
                            clock_().wall.time_since_epoch())
                            .count();
-  std::string hostname = get_hostname().value_or("hostname-unavailable");
-
+  
   seq_id++;
   auto payload =
       nlohmann::json::object(
@@ -59,15 +60,16 @@ std::string TracerTelemetry::app_started() {
               {"debug", true},
               {"application",
                nlohmann::json::object({
-                   {"service_name", config_.defaults.service},
-                   {"env", config_.defaults.environment},
+                   {"service_name", span_defaults_->service},
+                   {"env", span_defaults_->environment},
                    {"tracer_version", tracer_version_string},
                    {"language_name", "cpp"},
                    {"language_version", std::to_string(__cplusplus)},
                })},
-              // TODO: host information (hostname, os, os_version, kernel, etc)
+              // TODO: host information (os, os_version, kernel, etc)
               {"host", nlohmann::json::object({
-                           {"hostname", hostname},
+                           {"hostname",
+                            get_hostname().value_or("hostname-unavailable")},
                        })},
               {"payload",
                nlohmann::json::object({
@@ -77,6 +79,13 @@ std::string TracerTelemetry::app_started() {
                                      })},
 
                })},
+              // TODO: Until we figure out "configuration", above, include a
+              // JSON dump of the tracer configuration as "additional_payload".
+              {"additional_payload",
+               nlohmann::json::array({nlohmann::json::object({
+                   {"name", "tracer_config_json"},
+                   {"value", tracer_config.dump()},
+               })})},
           })
           .dump();
 
@@ -144,8 +153,8 @@ std::string TracerTelemetry::heartbeat_and_telemetry() {
               {"debug", true},
               {"application",
                nlohmann::json::object({
-                   {"service_name", config_.defaults.service},
-                   {"env", config_.defaults.environment},
+                   {"service_name", span_defaults_->service},
+                   {"env", span_defaults_->environment},
                    {"tracer_version", tracer_version_string},
                    {"language_name", "cpp"},
                    {"language_version", std::to_string(__cplusplus)},
diff --git a/src/datadog/tracer_telemetry.h b/src/datadog/tracer_telemetry.h
@@ -1,15 +1,21 @@
 #pragma once
+#include <memory>
 #include <vector>
 
 #include "clock.h"
+#include "json_fwd.hpp"
 #include "metrics.h"
-#include "tracer_config.h"
 
 namespace datadog {
 namespace tracing {
+
+class Logger;
+class SpanDefaults;
+
 class TracerTelemetry {
   Clock clock_;
-  FinalizedTracerConfig config_;
+  std::shared_ptr<Logger> logger_;
+  std::shared_ptr<const SpanDefaults> span_defaults_;
   uint64_t seq_id = 0;
   using MetricSnapshot = std::vector<std::pair<time_t, uint64_t>>;
   // This uses a reference_wrapper so references to internal metric values can
@@ -56,9 +62,10 @@ class TracerTelemetry {
   } metrics_;
 
  public:
-  TracerTelemetry(const Clock& clock, const FinalizedTracerConfig& config);
+  TracerTelemetry(const Clock& clock, const std::shared_ptr<Logger>& logger,
+                  const std::shared_ptr<const SpanDefaults>& span_defaults);
   auto& metrics() { return metrics_; };
-  std::string app_started();
+  std::string app_started(nlohmann::json&& tracer_config);
   void capture_metrics();
   std::string heartbeat_and_telemetry();
 };
diff --git a/test/test_datadog_agent.cpp b/test/test_datadog_agent.cpp
@@ -140,6 +140,7 @@ TEST_CASE("CollectorResponse") {
       (void)span;
     }
     REQUIRE(event_scheduler->cancelled);
+    CAPTURE(logger->entries);
     REQUIRE(logger->error_count() == 1);
     REQUIRE(logger->first_error().code == error.code);
   }
@@ -156,6 +157,7 @@ TEST_CASE("CollectorResponse") {
       (void)span;
     }
     REQUIRE(event_scheduler->cancelled);
+    // REVIEW: this fails since the addition of telemetry
     REQUIRE(logger->error_count() == 1);
     REQUIRE(logger->first_error().code == error.code);
   }

Original file line number	Diff line number	Diff line change
`@@ -140,6 +140,7 @@ TEST_CASE("CollectorResponse") {`
`140`	`140`	`(void)span;`
`141`	`141`	`}`
`142`	`142`	`REQUIRE(event_scheduler->cancelled);`
	`143`	`+ CAPTURE(logger->entries);`
`143`	`144`	`REQUIRE(logger->error_count() == 1);`
`144`	`145`	`REQUIRE(logger->first_error().code == error.code);`
`145`	`146`	`}`
`@@ -156,6 +157,7 @@ TEST_CASE("CollectorResponse") {`
`156`	`157`	`(void)span;`
`157`	`158`	`}`
`158`	`159`	`REQUIRE(event_scheduler->cancelled);`
	`160`	`+ // REVIEW: this fails since the addition of telemetry`
`159`	`161`	`REQUIRE(logger->error_count() == 1);`
`160`	`162`	`REQUIRE(logger->first_error().code == error.code);`
`161`	`163`	`}`