From af53c523787cca108ae9f458ea5c962e48187a36 Mon Sep 17 00:00:00 2001
From: Caleb Gilmour <caleb.gilmour@datadoghq.com>
Date: Wed, 28 Jul 2021 08:51:06 +1200
Subject: [PATCH] Fix handling of origin header without sampling priority
 (#191)

---
 src/propagation.cpp       | 17 +++++------------
 test/propagation_test.cpp | 29 +++++++++++++----------------
 2 files changed, 18 insertions(+), 28 deletions(-)

diff --git a/src/propagation.cpp b/src/propagation.cpp
index ebd54b82..471e7d11 100644
--- a/src/propagation.cpp
+++ b/src/propagation.cpp
@@ -101,7 +101,7 @@ bool has_prefix(const std::string &str, const std::string &prefix) {
 // whether the corresponding tag is set.  Note that `std::unique_ptr` is here
 // used as a substitute for `std::optional`.
 std::unique_ptr<ot::expected<std::unique_ptr<ot::SpanContext>>> enforce_tag_presence_policy(
-    bool trace_id_set, bool parent_id_set, bool sampling_priority_set, bool origin_set) {
+    bool trace_id_set, bool parent_id_set, bool origin_set) {
   using Result = ot::expected<std::unique_ptr<ot::SpanContext>>;
 
   if (!trace_id_set && !parent_id_set) {
@@ -116,10 +116,6 @@ std::unique_ptr<ot::expected<std::unique_ptr<ot::SpanContext>>> enforce_tag_pres
     // Parent ID is required, except when origin is set.
     return std::make_unique<Result>(ot::make_unexpected(ot::span_context_corrupted_error));
   }
-  if (origin_set && !sampling_priority_set) {
-    // Origin should only be set if sampling priority is also set.
-    return std::make_unique<Result>(ot::make_unexpected(ot::span_context_corrupted_error));
-  }
   return nullptr;
 }
 
@@ -412,9 +408,9 @@ ot::expected<std::unique_ptr<ot::SpanContext>> SpanContext::deserialize(
 
   reader >> j;
 
-  if (const auto result = enforce_tag_presence_policy(
-          j.count(json_trace_id_key), j.count(json_parent_id_key),
-          j.count(json_sampling_priority_key), j.count(json_origin_key))) {
+  if (const auto result = enforce_tag_presence_policy(j.contains(json_trace_id_key),
+                                                      j.contains(json_parent_id_key),
+                                                      j.contains(json_origin_key))) {
     return std::move(*result);
   }
 
@@ -481,7 +477,6 @@ ot::expected<std::unique_ptr<ot::SpanContext>> SpanContext::deserialize(
   std::string origin;
   bool trace_id_set = false;
   bool parent_id_set = false;
-  bool sampling_priority_set = false;
   bool origin_set = false;
   std::unordered_map<std::string, std::string> baggage;
   auto result =
@@ -501,7 +496,6 @@ ot::expected<std::unique_ptr<ot::SpanContext>> SpanContext::deserialize(
                         << std::endl;
               return ot::make_unexpected(ot::span_context_corrupted_error);
             }
-            sampling_priority_set = true;
           } else if (headers_impl.origin_header != nullptr &&
                      equals_ignore_case(key, headers_impl.origin_header)) {
             origin = value;
@@ -520,8 +514,7 @@ ot::expected<std::unique_ptr<ot::SpanContext>> SpanContext::deserialize(
   if (!result) {  // "if unexpected", hence "return {}" from above is fine.
     return ot::make_unexpected(result.error());
   }
-  if (const auto result = enforce_tag_presence_policy(trace_id_set, parent_id_set,
-                                                      sampling_priority_set, origin_set)) {
+  if (const auto result = enforce_tag_presence_policy(trace_id_set, parent_id_set, origin_set)) {
     return std::move(*result);
   }
   auto context =
diff --git a/test/propagation_test.cpp b/test/propagation_test.cpp
index a062b108..737deb3b 100644
--- a/test/propagation_test.cpp
+++ b/test/propagation_test.cpp
@@ -196,15 +196,6 @@ TEST_CASE("deserialize fails") {
     REQUIRE(!err);
     REQUIRE(err.error() == ot::span_context_corrupted_error);
   }
-
-  SECTION("when origin provided without sampling priority") {
-    carrier.Set(test_case.x_datadog_trace_id, "123");
-    carrier.Set(test_case.x_datadog_parent_id, "456");
-    carrier.Set(test_case.x_datadog_origin, "madeuporigin");
-    auto err = SpanContext::deserialize(logger, carrier, test_case.styles);
-    REQUIRE(!err);
-    REQUIRE(err.error() == ot::span_context_corrupted_error);
-  }
 }
 
 TEST_CASE("SamplingPriority values are clamped apropriately for b3") {
@@ -342,13 +333,6 @@ TEST_CASE("Binary Span Context") {
       REQUIRE(err.error() == ot::span_context_corrupted_error);
     }
 
-    SECTION("when sampling priority is missing but origin is set") {
-      carrier << "{ \"trace_id\": \"123\", \"parent_id\": \"420\", \"origin\": \"synthetics\" }";
-      auto err = SpanContext::deserialize(logger, carrier);
-      REQUIRE(!err);
-      REQUIRE(err.error() == ot::span_context_corrupted_error);
-    }
-
     SECTION("when given invalid json data") {
       carrier << "something that isn't JSON";
       auto err = SpanContext::deserialize(logger, carrier);
@@ -660,4 +644,17 @@ TEST_CASE("origin header propagation") {
     meta = spans->at(1)->meta;
     REQUIRE(meta.find("_dd.origin") != meta.end());
   }
+
+  SECTION("only trace id and origin headers are required") {
+    MockTextMapCarrier tmc;
+    tmc.text_map["x-datadog-trace-id"] = "321";
+    tmc.text_map["x-datadog-origin"] = "madeuporigin";
+
+    auto span_context_maybe = tracer->Extract(tmc);
+    REQUIRE(span_context_maybe);
+
+    auto sc = dynamic_cast<SpanContext*>(span_context_maybe->get());
+    REQUIRE(sc->traceId() == 321);
+    REQUIRE(sc->origin() == "madeuporigin");
+  }
 }