Squash commits into a single signed commit: (#242)

- add an integration test that fails
- still working on it...
- always inject X-Datadog-Tags
- uncomment the other integration tests
- document the x-datadog-tags change
- unit tests are a thing
- clang-format-9
- generalize the most recent addition to the integration tests
- address review comments

src/span_buffer.cpp

@@ -183,20 +183,20 @@ OptionalSamplingPriority SpanBuffer::generateSamplingPriorityImpl(const SpanData
   return getSamplingPriorityImpl(span->trace_id);
 }
 
-std::string SpanBuffer::serializeTraceTags(uint64_t trace_id) {
-  std::string result;
+std::unique_ptr<std::string> SpanBuffer::serializeTraceTags(uint64_t trace_id) {
   std::lock_guard<std::mutex> lock{mutex_};
 
   const auto trace_found = traces_.find(trace_id);
   if (trace_found == traces_.end()) {
     logger_->Log(LogLevel::error, trace_id,
                  "Requested trace_id not found in SpanBuffer::serializeTraceTags");
-    return result;
+    return nullptr;
   }
 
   auto& trace = trace_found->second;
 
   trace.applySamplingDecisionToTraceTags();
+  std::string result;
   for (const auto& entry : trace.trace_tags) {
     appendTag(result, entry.first, entry.second);
   }
@@ -209,11 +209,10 @@ std::string SpanBuffer::serializeTraceTags(uint64_t trace_id) {
         << "Serialized trace tags are too large for propagation.  Configured maximum length is "
         << configured_max << ", but the following has length " << result.size() << ": " << result;
     logger_->Log(LogLevel::error, trace_id, message.str());
-    // Return an empty string, which will not be propagated.
-    result.clear();
+    return nullptr;
   }
 
-  return result;
+  return std::unique_ptr<std::string>(new std::string(std::move(result)));
 }
 
 void SpanBuffer::setServiceName(uint64_t trace_id, ot::string_view service_name) {

src/span_buffer.h

@@ -78,12 +78,10 @@ class SpanBuffer {
   OptionalSamplingPriority generateSamplingPriority(const SpanData* span);
 
   // Return the serialization of the trace tags associated with the trace
-  // having the specified `trace_id`, or return an empty string if an error
-  // occurs.  Note that an empty string could mean either that there no tags or
-  // that an error occurred.  If an encoding error occurs, a corresponding
-  // `_dd.propagation_error` tag value will be added to the relevant trace's
-  // local root span.
-  std::string serializeTraceTags(uint64_t trace_id);
+  // having the specified `trace_id`, or return `nullptr` if an error occurs.
+  // If an encoding error occurs, a corresponding `_dd.propagation_error` tag
+  // value will be added to the relevant trace's local root span.
+  std::unique_ptr<std::string> serializeTraceTags(uint64_t trace_id);
 
   // Change the name of the service associated with the trace having the
   // specified `trace_id` to the specified `service_name`.

src/span_context.cpp

@@ -325,9 +325,11 @@ ot::expected<void> SpanContext::serialize(std::ostream &writer,
       j[json_origin_key] = origin_;
     }
   }
-  std::string tags = pending_traces->serializeTraceTags(trace_id_);
-  if (!tags.empty()) {
-    j[json_tags_key] = std::move(tags);
+  auto tags = pending_traces->serializeTraceTags(trace_id_);
+  if (tags) {
+    j[json_tags_key] = *tags;
+  } else {
+    j[json_tags_key] = "";
   }
   j[json_baggage_key] = baggage_;
 
@@ -397,9 +399,14 @@ ot::expected<void> SpanContext::serialize(const ot::TextMapWriter &writer,
     }
   }
 
-  const std::string tags = pending_traces->serializeTraceTags(trace_id_);
-  if (!tags.empty()) {
-    result = writer.Set(headers_impl.tags_header, tags);
+  const auto tags = pending_traces->serializeTraceTags(trace_id_);
+  // Inject the trace tags, even if they're empty of if an error occurred.
+  // This is to work around a quirk of nginx-opentracing.
+  // See <https://github.com/DataDog/dd-opentracing-cpp/issues/241>.
+  if (tags) {
+    result = writer.Set(headers_impl.tags_header, *tags);
+  } else {
+    result = writer.Set(headers_impl.tags_header, "");
   }
   if (!result) {
     return result;

test/integration/nginx/nginx_integration_test.sh

@@ -68,12 +68,12 @@ function wait_for_port() {
 }
 
 function run_nginx() {
-  eval "nginx -g \"daemon off;\" 1>/tmp/nginx_log.txt &"
+  nginx -g "daemon off;" 1>/tmp/nginx_log.txt &
   NGINX_PID=$!
   wait_for_port 80
 }
 
-echo "Test 1: Ensure the right traces sent to the agent."
+echo "======== Test 1: Ensure the right traces sent to the agent. ========"
 # Start wiremock in background
 wiremock --port 8126 >/dev/null 2>&1 &
 # Wait for wiremock to start
@@ -105,7 +105,7 @@ then
 fi
 
 reset_test
-echo "Test 2: Check that libcurl isn't writing to stdout"
+echo "======== Test 2: Check that libcurl isn't writing to stdout ========"
 run_nginx
 curl -s localhost?[1-10000] 1> /tmp/curl_log.txt
 
@@ -118,7 +118,7 @@ then
 fi
 
 reset_test
-echo "Test 3: Check that creating a root span doesn't produce an error"
+echo "======== Test 3: Check that creating a root span doesn't produce an error ========"
 run_nginx
 curl -s localhost?[1-5] 1> /tmp/curl_log.txt
 
@@ -137,7 +137,7 @@ then
 fi
 
 reset_test
-echo "Test 4: Check that priority sampling works."
+echo "======== Test 4: Check that priority sampling works. ========"
 # Start the mock agent
 wiremock --port 8126 >/dev/null 2>&1 & wait_for_port 8126
 curl -s -X POST --data '{ "priority":10, "request": { "method": "ANY", "urlPattern": ".*" }, "response": { "status": 200, "body": "{\"rate_by_service\":{\"service:nginx,env:prod\":0.5, \"service:nginx,env:\":0.2, \"service:wrong,env:\":0.1, \"service:nginx,env:wrong\":0.9}}" }}' http://localhost:8126/__admin/mappings/new
@@ -189,7 +189,7 @@ then
 fi
 
 reset_test
-echo "Test 5: Ensure that NGINX errors are reported to Datadog"
+echo "======== Test 5: Ensure that NGINX errors are reported to Datadog ========"
 wiremock --port 8126 >/dev/null 2>&1 &
 # Wait for wiremock to start
 wait_for_port 8126
@@ -213,7 +213,7 @@ fi
 
 reset_test
 
-echo "Test 6: Origin header is propagated and adds a tag"
+echo "======== Test 6: Origin header is propagated and adds a tag ========"
 wiremock --port 8126 >/dev/null 2>&1 & wait_for_port 8126
 curl -s -X POST --data '{ "priority":10, "request": { "method": "ANY", "urlPattern": ".*" }, "response": { "status": 200, "body": "OK" }}' http://localhost:8126/__admin/mappings/new
 wiremock --port 8080 >/dev/null 2>&1 & wait_for_port 8080
@@ -248,3 +248,37 @@ then
   echo "${DIFF}"
   exit 1
 fi
+
+reset_test
+echo "======== Test 7: Check that extracting a child span without X-Datadog-Tags doesn't produce an error ========"
+# See <https://github.com/DataDog/dd-opentracing-cpp/issues/241>.
+
+wiremock --port 8080 >/dev/null 2>&1 & wait_for_port 8080
+curl -s -X POST --data '{ "priority":10, "request": { "method": "ANY", "urlPattern": ".*" }, "response": { "status": 200, "body": "Hello World" }}' http://localhost:8080/__admin/mappings/new
+
+echo '{
+  "service": "nginx",
+  "operation_name_override": "nginx.handle"
+}' > ${TRACER_CONF_PATH}
+
+run_nginx
+
+curl -s \
+  -H 'X-Datadog-Trace-Id: 1234' \
+  -H 'X-Datadog-Parent-Id: 1234' \
+  -H 'X-Datadog-Sampling-Priority: 0' \
+  http://localhost/proxy/ >/tmp/curl_log.txt
+
+if [ "$(cat ${NGINX_ERROR_LOG} | grep 'no opentracing context value found for span context key ' | wc -l)" != "0" ]
+then
+  echo "Extraction errors in nginx log file:"
+  cat ${NGINX_ERROR_LOG}
+  echo ""
+  exit 1
+elif [ "$(cat ${NGINX_ERROR_LOG})" != "" ]
+then
+  echo "Other errors in nginx log file:"
+  cat ${NGINX_ERROR_LOG}
+  echo ""
+  exit 1
+fi

test/propagation_test.cpp

@@ -900,7 +900,11 @@ TEST_CASE("propagated Datadog tags (x-datadog-tags)") {
       REQUIRE(result);
 
       const auto injected_json = nlohmann::json::parse(injected.str());
-      REQUIRE(injected_json.find("tags") == injected_json.end());
+      const auto tags = injected_json.find("tags");
+      // See <https://github.com/DataDog/dd-opentracing-cpp/issues/241>.
+      // Also, the redundant parentheses are required here because of the way
+      // Catch2 parses predicates (it's the "||").
+      REQUIRE((tags == injected_json.end() || *tags == ""));
 
       // Because the tags were omitted due to being oversized, there will be a
       // specific error tag on the local root span.