10BC0 Squash commits into a single signed commit: (#242) · DataDog/dd-opentracing-cpp@c07cf6c · GitHub
[go: up one dir, main page]
Skip to content

Commit c07cf6c

Browse files
dgoffredodgoffredo
authored
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
1 parent dfa7727 commit c07cf6c

File tree

5 files changed

+68
-26
lines changed

5 files changed

+68
-26
lines changed

‎src/span_buffer.cpp‎

Lines changed: 5 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -183,20 +183,20 @@ OptionalSamplingPriority SpanBuffer::generateSamplingPriorityImpl(const SpanData
183183
return getSamplingPriorityImpl(span->trace_id);
184184
}
185185

186-
std::string SpanBuffer::serializeTraceTags(uint64_t trace_id) {
187-
std::string result;
186+
std::unique_ptr<std::string> SpanBuffer::serializeTraceTags(uint64_t trace_id) {
188187
std::lock_guard<std::mutex> lock{mutex_};
189188

190189
const auto trace_found = traces_.find(trace_id);
191190
if (trace_found == traces_.end()) {
192191
logger_->Log(LogLevel::error, trace_id,
193192
"Requested trace_id not found in SpanBuffer::serializeTraceTags");
194-
return result;
193+
return nullptr;
195194
}
196195

197196
auto& trace = trace_found->second;
198197

199198
trace.applySamplingDecisionToTraceTags();
199+
std::string result;
200200
for (const auto& entry : trace.trace_tags) {
201201
appendTag(result, entry.first, entry.second);
202202
}
@@ -209,11 +209,10 @@ std::string SpanBuffer::serializeTraceTags(uint64_t trace_id) {
209209
<< "Serialized trace tags are too large for propagation. Configured maximum length is "
210210
<< configured_max << ", but the following has length " << result.size() << ": " << result;
211211
logger_->Log(LogLevel::error, trace_id, message.str());
212-
// Return an empty string, which will not be propagated.
213-
result.clear();
212+
return nullptr;
214213
}
215214

216-
return result;
215+
return std::unique_ptr<std::string>(new std::string(std::move(result)));
217216
}
218217

219218
void SpanBuffer::setServiceName(uint64_t trace_id, ot::string_view service_name) {

‎src/span_buffer.h‎

Lines changed: 4 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -78,12 +78,10 @@ class SpanBuffer {
7878
OptionalSamplingPriority generateSamplingPriority(const SpanData* span);
7979

8080
// Return the serialization of the trace tags associated with the trace
81-
// having the specified `trace_id`, or return an empty string if an error
82-
// occurs. Note that an empty string could mean either that there no tags or
83-
// that an error occurred. If an encoding error occurs, a corresponding
84-
// `_dd.propagation_error` tag value will be added to the relevant trace's
85-
// local root span.
86-
std::string serializeTraceTags(uint64_t trace_id);
81+
// having the specified `trace_id`, or return `nullptr` if an error occurs.
82+
// If an encoding error occurs, a corresponding `_dd.propagation_error` tag
83+
// value will be added to the relevant trace's local root span.
84+
std::unique_ptr<std::string> serializeTraceTags(uint64_t trace_id);
8785

8886
// Change the name of the service associated with the trace having the
8987
// specified `trace_id` to the specified `service_name`.

‎src/span_context.cpp‎

Lines changed: 13 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -325,9 +325,11 @@ ot::expected<void> SpanContext::serialize(std::ostream &writer,
325325
j[json_origin_key] = origin_;
326326
}
327327
}
328-
std::string tags = pending_traces->serializeTraceTags(trace_id_);
329-
if (!tags.empty()) {
330-
j[json_tags_key] = std::move(tags);
328+
auto tags = pending_traces->serializeTraceTags(trace_id_);
329+
if (tags) {
330+
j[json_tags_key] = *tags;
331+
} else {
332+
j[json_tags_key] = "";
331333
}
332334
j[json_baggage_key] = baggage_;
333335

@@ -397,9 +399,14 @@ ot::expected<void> SpanContext::serialize(const ot::TextMapWriter &writer,
397399
}
398400
}
399401

400-
const std::string tags = pending_traces->serializeTraceTags(trace_id_);
401-
if (!tags.empty()) {
402-
result = writer.Set(headers_impl.tags_header, tags);
402+
const auto tags = pending_traces->serializeTraceTags(trace_id_);
403+
// Inject the trace tags, even if they're empty of if an error occurred.
404+
// This is to work around a quirk of nginx-opentracing.
405+
// See <https://github.com/DataDog/dd-opentracing-cpp/issues/241>.
406+
if (tags) {
407+
result = writer.Set(headers_impl.tags_header, *tags);
408+
} e 8B92 lse {
409+
result = writer.Set(headers_impl.tags_header, "");
403410
}
404411
if (!result) {
405412
return result;

‎test/integration/nginx/nginx_integration_test.sh‎

Lines changed: 41 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -68,12 +68,12 @@ function wait_for_port() {
6868
}
6969

7070
function run_nginx() {
71-
eval "nginx -g \"daemon off;\" 1>/tmp/nginx_log.txt &"
71+
nginx -g "daemon off;" 1>/tmp/nginx_log.txt &
7272
NGINX_PID=$!
7373
wait_for_port 80
7474
}
7575

76-
echo "Test 1: Ensure the right traces sent to the agent."
76+
echo "======== Test 1: Ensure the right traces sent to the agent. ========"
7777
# Start wiremock in background
7878
wiremock --port 8126 >/dev/null 2>&1 &
7979
# Wait for wiremock to start
@@ -105,7 +105,7 @@ then
105105
fi
106106

107107
reset_test
108-
echo "Test 2: Check that libcurl isn't writing to stdout"
108+
echo "======== Test 2: Check that libcurl isn't writing to stdout ========"
109109
run_nginx
110110
curl -s localhost?[1-10000] 1> /tmp/curl_log.txt
111111

@@ -118,7 +118,7 @@ then
118118
fi
119119

120120
reset_test
121-
echo "Test 3: Check that creating a root span doesn't produce an error"
121+
echo "======== Test 3: Check that creating a root span doesn't produce an error ========"
122122
run_nginx
123123
curl -s localhost?[1-5] 1> /tmp/curl_log.txt
124124

@@ -137,7 +137,7 @@ then
137137
fi
138138

139139
reset_test
140-
echo "Test 4: Check that priority sampling works."
140+
echo 8B92 "======== Test 4: Check that priority sampling works. ========"
141141
# Start the mock agent
142142
wiremock --port 8126 >/dev/null 2>&1 & wait_for_port 8126
143143
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
189189
fi
190190

191191
reset_test
192-
echo "Test 5: Ensure that NGINX errors are reported to Datadog"
192+
echo "======== Test 5: Ensure that NGINX errors are reported to Datadog ========"
193193
wiremock --port 8126 >/dev/null 2>&1 &
194194
# Wait for wiremock to start
195195
wait_for_port 8126
@@ -213,7 +213,7 @@ fi
213213

214214
reset_test
215215

216-
echo "Test 6: Origin header is propagated and adds a tag"
216+
echo "======== Test 6: Origin header is propagated and adds a tag ========"
217217
wiremock --port 8126 >/dev/null 2>&1 & wait_for_port 8126
218218
curl -s -X POST --data '{ "priority":10, "request": { "method": "ANY", "urlPattern": ".*" }, "response": { "status": 200, "body": "OK" }}' http://localhost:8126/__admin/mappings/new
219219
wiremock --port 8080 >/dev/null 2>&1 & wait_for_port 8080
@@ -248,3 +248,37 @@ then
248248
echo "${DIFF}"
249249
exit 1
250250
fi
251+
252+
reset_test
253+
echo "======== Test 7: Check that extracting a child span without X-Datadog-Tags doesn't produce an error ========"
254+
# See <https://github.com/DataDog/dd-opentracing-cpp/issues/241>.
255+
256+
wiremock --port 8080 >/dev/null 2>&1 & wait_for_port 8080
257+
curl -s -X POST --data '{ "priority":10, "request": { "method": "ANY", "urlPattern": ".*" }, "response": { "status": 200, "body": "Hello World" }}' http://localhost:8080/__admin/mappings/new
258+
259+
echo '{
260+
"service": "nginx",
261+
"operation_name_override": "nginx.handle"
262+
}' > ${TRACER_CONF_PATH}
263+
264+
run_nginx
265+
266+
curl -s \
267+
-H 'X-Datadog-Trace-Id: 1234' \
268+
-H 'X-Datadog-Parent-Id: 1234' \
269+
-H 'X-Datadog-Sampling-Priority: 0' \
270+
http://localhost/proxy/ >/tmp/curl_log.txt
271+
272+
if [ "$(cat ${NGINX_ERROR_LOG} | grep 'no opentracing context value found for span context key ' | wc -l)" != "0" ]
273+
then
274+
echo "Extraction errors in nginx log file:"
275+
cat ${NGINX_ERROR_LOG}
276+
echo ""
277+
exit 1
278+
elif [ "$(cat ${NGINX_ERROR_LOG})" != "" ]
279+
then
280+
echo "Other errors in nginx log file:"
281+
cat ${NGINX_ERROR_LOG}
282+
echo ""
283+
exit 1
284+
fi

‎test/propagation_test.cpp‎

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -900,7 +900,11 @@ TEST_CASE("propagated Datadog tags (x-datadog-tags)") {
900900
REQUIRE(result);
901901

902902
const auto injected_json = nlohmann::json::parse(injected.str());
903-
REQUIRE(injected_json.find("tags") == injected_json.end());
903+
const auto tags = injected_json.find("tags");
904+
// See <https://github.com/DataDog/dd-opentracing-cpp/issues/241>.
905+
// Also, the redundant parentheses are required here because of the way
906+
// Catch2 parses predicates (it's the "||").
907+
REQUIRE((tags == injected_json.end() || *tags == ""));
904908

905909
// Because the tags were omitted due to being oversized, there will be a
906910
// specific error tag on the local root span.

0 commit comments

Comments
 (0)
0