From c07cf6c095a6baefa766503712e8a88affb28ece Mon Sep 17 00:00:00 2001 From: David Goffredo Date: Sat, 10 Sep 2022 08:26:50 -0400 Subject: [PATCH] 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 | 11 ++--- src/span_buffer.h | 10 ++-- src/span_context.cpp | 19 +++++--- .../nginx/nginx_integration_test.sh | 48 ++++++++++++++++--- test/propagation_test.cpp | 6 ++- 5 files changed, 68 insertions(+), 26 deletions(-) diff --git a/src/span_buffer.cpp b/src/span_buffer.cpp index 073e71b8..c80c1e05 100644 --- a/src/span_buffer.cpp +++ b/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 SpanBuffer::serializeTraceTags(uint64_t trace_id) { std::lock_guard 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(new std::string(std::move(result))); } void SpanBuffer::setServiceName(uint64_t trace_id, ot::string_view service_name) { diff --git a/src/span_buffer.h b/src/span_buffer.h index bb942e08..b7a5109e 100644 --- a/src/span_buffer.h +++ b/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 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`. diff --git a/src/span_context.cpp b/src/span_context.cpp index b720ffe6..d4bebfaa 100644 --- a/src/span_context.cpp +++ b/src/span_context.cpp @@ -325,9 +325,11 @@ ot::expected 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 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 . + if (tags) { + result = writer.Set(headers_impl.tags_header, *tags); + } else { + result = writer.Set(headers_impl.tags_header, ""); } if (!result) { return result; diff --git a/test/integration/nginx/nginx_integration_test.sh b/test/integration/nginx/nginx_integration_test.sh index b98e025e..6c2a4759 100755 --- a/test/integration/nginx/nginx_integration_test.sh +++ b/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 . + +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 diff --git a/test/propagation_test.cpp b/test/propagation_test.cpp index ebc51497..d9a542d2 100644 --- a/test/propagation_test.cpp +++ b/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 . + // 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.