pass Datadog's internal shared tests for 128-bit trace IDs (#41)

* don't propagate invalid _dd.p.tid

* "_dd.propagation_error:malformed_tid xxxxx"

* 128-bit trace IDs contain unix timestamp

* always include _dd.p.tid with 128-bit trace IDs

* manual.keep and manual.drop are not special

* note when _dd.p.tid disagrees with trace ID

* note when _dd.p.tid cannot be parsed

* work around the poorly named "-Wunused-lambda-capture"

* capture unit test case context

* work around a spurious compiler warning

* Permit unused-lambda-capture on Clang only. It doesn't exist in GCC.

Author: dgoffredo

CMakeLists.txt

@@ -21,6 +21,12 @@ set(CMAKE_CXX_EXTENSIONS OFF)
 # <https://gcc.gnu.org/bugzilla/show_bug.cgi?id=108088>.
 add_compile_options(-Wno-error=free-nonheap-object)
 
+# This warning has a false positive with clang. See
+# <https://stackoverflow.com/questions/52416362>.
+if (CMAKE_CXX_COMPILER_ID STREQUAL "Clang")
+    add_compile_options(-Wno-error=unused-lambda-capture)
+endif()
+
 # If we're building with clang, then use the libc++ version of the standard
 # library instead of libstdc++. Better coverage of build configurations.
 #

src/datadog/id_generator.cpp

@@ -1,6 +1,7 @@
 #include "id_generator.h"
 
 #include <bitset>
+#include <chrono>
 
 #include "random.h"
 
@@ -15,11 +16,17 @@ class DefaultIDGenerator : public IDGenerator {
   explicit DefaultIDGenerator(bool trace_id_128_bit)
       : trace_id_128_bit_(trace_id_128_bit) {}
 
-  TraceID trace_id() const override {
+  TraceID trace_id(const TimePoint& start) const override {
     TraceID result;
     result.low = random_uint64();
     if (trace_id_128_bit_) {
-      result.high = random_uint64();
+      // Highest 32 bits contain a unix timestamp (the trace start time).
+      const auto since_epoch = start.wall.time_since_epoch();
+      const auto seconds =
+          std::chrono::duration_cast<std::chrono::seconds>(since_epoch).count();
+      // The farthest we'll go back is the unix epoch.
+      const std::uint64_t unsigned_seconds = seconds < 0 ? 0 : seconds;
+      result.high = unsigned_seconds << 32;
     } else {
       // In 64-bit mode, zero the most significant bit for compatibility with
       // older tracers that can't accept values above

src/datadog/id_generator.h

@@ -12,6 +12,7 @@
 #include <cstdint>
 #include <memory>
 
+#include "clock.h"
 #include "trace_id.h"
 
 namespace datadog {
@@ -21,8 +22,10 @@ class IDGenerator {
  public:
   virtual ~IDGenerator() = default;
 
+  // Generate a span ID.
   virtual std::uint64_t span_id() const = 0;
-  virtual TraceID trace_id() const = 0;
+  // Generate a trace ID for a trace having the specified `start` time.
+  virtual TraceID trace_id(const TimePoint& start) const = 0;
 };
 
 std::shared_ptr<const IDGenerator> default_id_generator(bool trace_id_128_bit);

src/datadog/tags.cpp

@@ -11,8 +11,6 @@ const std::string service_name = "service.name";
 const std::string span_type = "span.type";
 const std::string operation_name = "operation";
 const std::string resource_name = "resource.name";
-const std::string manual_keep = "manual.keep";
-const std::string manual_drop = "manual.drop";
 const std::string version = "version";
 
 namespace internal {

src/datadog/tags.h

@@ -16,8 +16,6 @@ extern const std::string service_name;
 extern const std::string span_type;
 extern const std::string operation_name;
 extern const std::string resource_name;
-extern const std::string manual_keep;
-extern const std::string manual_drop;
 extern const std::string version;
 
 namespace internal {

src/datadog/tracer.cpp

@@ -54,11 +54,13 @@ void handle_trace_tags(StringView trace_tags, ExtractedData& result,
             error->with_prefix("Unable to parse high bits of the trace ID in "
                                "Datadog style from the "
                                "\"_dd.p.tid\" trace tag: "));
-        span_tags[tags::internal::propagation_error] = "decoding_error";
+        span_tags[tags::internal::propagation_error] = "malformed_tid " + value;
+        continue;
       }
-      // Note that this assumes the lower 64 bits of the trace ID have already
-      // been extracted (i.e. we look for X-Datadog-Trace-ID first).
+
       if (result.trace_id) {
+        // Note that this assumes the lower 64 bits of the trace ID have already
+        // been extracted (i.e. we look for X-Datadog-Trace-ID first).
         result.trace_id->high = *high;
       }
     }
@@ -269,7 +271,7 @@ Span Tracer::create_span(const SpanConfig& config) {
   auto span_data = std::make_unique<SpanData>();
   span_data->apply_config(*defaults_, config, clock_);
   std::vector<std::pair<std::string, std::string>> trace_tags;
-  span_data->trace_id = generator_->trace_id();
+  span_data->trace_id = generator_->trace_id(span_data->start);
   if (span_data->trace_id.high) {
     trace_tags.emplace_back(tags::internal::trace_id_high,
                             hex(span_data->trace_id.high));
@@ -392,6 +394,31 @@ Expected<Span> Tracer::extract_span(const DictReader& reader,
   span_data->trace_id = *trace_id;
   span_data->parent_id = *parent_id;
 
+  if (span_data->trace_id.high) {
+    // The trace ID has some bits set in the higher 64 bits. Set the
+    // corresponding `trace_id_high` tag, so that the Datadog backend is aware
+    // of those bits.
+    //
+    // First, though, if the `trace_id_high` tag is already set and has a bogus
+    // value or a value inconsistent with the trace ID, tag an error.
+    const std::string hex_high = hex(span_data->trace_id.high);
+    const auto extant = std::find_if(
+        trace_tags.begin(), trace_tags.end(), [&](const auto& pair) {
+          return pair.first == tags::internal::trace_id_high;
+        });
+    if (extant == trace_tags.end()) {
+      trace_tags.emplace_back(tags::internal::trace_id_high, hex_high);
+    } else if (!parse_uint64(extant->second, 16)) {
+      span_data->tags[tags::internal::propagation_error] =
+          "malformed_tid " + extant->second;
+      extant->second = hex_high;
+    } else if (extant->second != hex_high) {
+      span_data->tags[tags::internal::propagation_error] =
+          "inconsistent_tid " + extant->second;
+      extant->second = hex_high;
+    }
+  }
+
   Optional<SamplingDecision> sampling_decision;
   if (sampling_priority) {
     SamplingDecision decision;

src/datadog/w3c_propagation.cpp

@@ -214,12 +214,6 @@ void parse_datadog_tracestate(ExtractedData& result, StringView datadog_value) {
       const auto tag_suffix = key.substr(2);
       std::string tag_name = "_dd.p.";
       append(tag_name, tag_suffix);
-      // The "_dd.p.tid" trace tag is ignored in this context, since its value
-      // is better inferred from the higher 64 bits of the trace ID.
-      if (tag_name == "_dd.p.tid") {
-        pair_begin = pair_end == end ? end : pair_end + 1;
-        continue;
-      }
       // The tag value was encoded with all '=' replaced by '~'.  Undo that
       // transformation.
       std::string decoded_value{value};

test/test_span.cpp

@@ -373,7 +373,7 @@ TEST_CASE("injection") {
   struct Generator : public IDGenerator {
     const std::uint64_t id;
     explicit Generator(std::uint64_t id) : id(id) {}
-    TraceID trace_id() const override { return TraceID(id); }
+    TraceID trace_id(const TimePoint&) const override { return TraceID(id); }
     std::uint64_t span_id() const override { return id; }
   };
   Tracer tracer{*finalized_config, std::make_shared<Generator>(42)};
@@ -478,7 +478,7 @@ TEST_CASE("injecting W3C traceparent header") {
     struct Generator : public IDGenerator {
       const std::uint64_t id;
       explicit Generator(std::uint64_t id) : id(id) {}
-      TraceID trace_id() const override { return TraceID(id); }
+      TraceID trace_id(const TimePoint&) const override { return TraceID(id); }
       std::uint64_t span_id() const override { return id; }
     };
     Tracer tracer{*finalized_config,
@@ -515,7 +515,7 @@ TEST_CASE("injecting W3C traceparent header") {
     struct Generator : public IDGenerator {
       const std::uint64_t id;
       explicit Generator(std::uint64_t id) : id(id) {}
-      TraceID trace_id() const override { return TraceID(id); }
+      TraceID trace_id(const TimePoint&) const override { return TraceID(id); }
       std::uint64_t span_id() const override { return id; }
     };
     Tracer tracer{*finalized_config, std::make_shared<Generator>(expected_id)};
@@ -715,7 +715,7 @@ TEST_CASE("128-bit trace ID injection") {
 
    public:
     explicit MockIDGenerator(TraceID trace_id) : trace_id_(trace_id) {}
-    TraceID trace_id() const override { return trace_id_; }
+    TraceID trace_id(const TimePoint&) const override { return trace_id_; }
     // `span_id` won't be called, because root spans use the lower part of
     // `trace_id` for the span ID.
     std::uint64_t span_id() const override { return 42; }