From e6b82ff717f71b05f4af6df6d96d806f02688d31 Mon Sep 17 00:00:00 2001 From: Gary Brown Date: Tue, 19 Sep 2017 22:18:57 +0100 Subject: [PATCH 1/5] Recover the OT context header from the B3 headers if not propagated Resolves #1364 Signed-off-by: Gary Brown --- source/common/tracing/zipkin/span_context.h | 11 +++++ .../tracing/zipkin/zipkin_tracer_impl.cc | 14 ++++++ .../tracing/zipkin/zipkin_tracer_impl_test.cc | 46 +++++++++++++++++++ 3 files changed, 71 insertions(+) diff --git a/source/common/tracing/zipkin/span_context.h b/source/common/tracing/zipkin/span_context.h index 450b571d4fee..044194187b2d 100644 --- a/source/common/tracing/zipkin/span_context.h +++ b/source/common/tracing/zipkin/span_context.h @@ -19,6 +19,17 @@ class SpanContext { */ SpanContext() : trace_id_(0), id_(0), parent_id_(0), is_initialized_(false) {} + /** + * Constructor that creates a context object from the supplied trace, span + * and parent ids. + * + * @param trace_id The trace id. + * @param id The span id. + * @param parent_id The parent id. + */ + SpanContext(const uint64_t trace_id, const uint64_t id, const uint64_t parent_id) + : trace_id_(trace_id), id_(id), parent_id_(parent_id), is_initialized_(true) {} + /** * Constructor that creates a context object from the given Zipkin span object. * diff --git a/source/common/tracing/zipkin/zipkin_tracer_impl.cc b/source/common/tracing/zipkin/zipkin_tracer_impl.cc index f28bb0663311..b71d756b8a38 100644 --- a/source/common/tracing/zipkin/zipkin_tracer_impl.cc +++ b/source/common/tracing/zipkin/zipkin_tracer_impl.cc @@ -107,6 +107,20 @@ Tracing::SpanPtr Driver::startSpan(const Tracing::Config& config, Http::HeaderMa new_zipkin_span = tracer.startSpan(config, request_headers.Host()->value().c_str(), start_time, context); + + } else if (request_headers.XB3TraceId() && request_headers.XB3SpanId()) { + uint64_t parentId(0); + if (request_headers.XB3ParentSpanId()) { + parentId = std::stoull(request_headers.XB3ParentSpanId()->value().c_str(), nullptr, 16); + } + + SpanContext context(std::stoull(request_headers.XB3TraceId()->value().c_str(), nullptr, 16), + std::stoull(request_headers.XB3SpanId()->value().c_str(), nullptr, 16), + parentId); + + new_zipkin_span = + tracer.startSpan(config, request_headers.Host()->value().c_str(), start_time, context); + } else { // Create a root Zipkin span. No context was found in the headers. new_zipkin_span = tracer.startSpan(config, request_headers.Host()->value().c_str(), start_time); diff --git a/test/common/tracing/zipkin/zipkin_tracer_impl_test.cc b/test/common/tracing/zipkin/zipkin_tracer_impl_test.cc index 1036eae83e29..219d4981ff50 100644 --- a/test/common/tracing/zipkin/zipkin_tracer_impl_test.cc +++ b/test/common/tracing/zipkin/zipkin_tracer_impl_test.cc @@ -344,5 +344,51 @@ TEST_F(ZipkinDriverTest, ZipkinSpanTest) { EXPECT_EQ("key3", zipkin_zipkin_span3.binaryAnnotations()[0].key()); EXPECT_EQ("value3", zipkin_zipkin_span3.binaryAnnotations()[0].value()); } + +TEST_F(ZipkinDriverTest, ZipkinSpanContextFromOtSpanContextHeaderTest) { + setupValidDriver(); + + const std::string trace_id = Hex::uint64ToHex(Util::generateRandom64()); + const std::string span_id = Hex::uint64ToHex(Util::generateRandom64()); + const std::string parent_id = Hex::uint64ToHex(Util::generateRandom64()); + const std::string context = + trace_id + ";" + span_id + ";" + parent_id + ";" + ZipkinCoreConstants::get().CLIENT_SEND; + + request_headers_.insertOtSpanContext().value(context); + + // New span will have an SR annotation - so its span and parent ids will be + // the same as the supplied span context (i.e. shared context) + Tracing::SpanPtr span = + driver_->startSpan(config_, request_headers_, operation_name_, start_time_); + + ZipkinSpanPtr zipkin_span(dynamic_cast(span.release())); + + EXPECT_EQ(trace_id, zipkin_span->span().traceIdAsHexString()); + EXPECT_EQ(span_id, zipkin_span->span().idAsHexString()); + EXPECT_EQ(parent_id, zipkin_span->span().parentIdAsHexString()); +} + +TEST_F(ZipkinDriverTest, ZipkinSpanContextFromB3HeadersTest) { + setupValidDriver(); + + const std::string trace_id = Hex::uint64ToHex(Util::generateRandom64()); + const std::string span_id = Hex::uint64ToHex(Util::generateRandom64()); + const std::string parent_id = Hex::uint64ToHex(Util::generateRandom64()); + + request_headers_.insertXB3TraceId().value(trace_id); + request_headers_.insertXB3SpanId().value(span_id); + request_headers_.insertXB3ParentSpanId().value(parent_id); + + // New span will have an SR annotation - so its span and parent ids will be + // the same as the supplied span context (i.e. shared context) + Tracing::SpanPtr span = + driver_->startSpan(config_, request_headers_, operation_name_, start_time_); + + ZipkinSpanPtr zipkin_span(dynamic_cast(span.release())); + + EXPECT_EQ(trace_id, zipkin_span->span().traceIdAsHexString()); + EXPECT_EQ(span_id, zipkin_span->span().idAsHexString()); + EXPECT_EQ(parent_id, zipkin_span->span().parentIdAsHexString()); +} } // namespace Zipkin } // namespace Envoy From eb237f055c7a651808ba251f87235e8904e3bd81 Mon Sep 17 00:00:00 2001 From: Gary Brown Date: Wed, 20 Sep 2017 19:53:46 +0100 Subject: [PATCH 2/5] Change to use StringUtil and check for errors (with tests) Signed-off-by: Gary Brown --- .../tracing/zipkin/zipkin_tracer_impl.cc | 22 +++++++++---- .../tracing/zipkin/zipkin_tracer_impl_test.cc | 33 +++++++++++++++++++ 2 files changed, 49 insertions(+), 6 deletions(-) diff --git a/source/common/tracing/zipkin/zipkin_tracer_impl.cc b/source/common/tracing/zipkin/zipkin_tracer_impl.cc index b71d756b8a38..41e493d8cd09 100644 --- a/source/common/tracing/zipkin/zipkin_tracer_impl.cc +++ b/source/common/tracing/zipkin/zipkin_tracer_impl.cc @@ -1,6 +1,7 @@ #include "common/tracing/zipkin/zipkin_tracer_impl.h" #include "common/common/enum_to_int.h" +#include "common/common/utility.h" #include "common/http/headers.h" #include "common/http/message_impl.h" #include "common/http/utility.h" @@ -107,20 +108,29 @@ Tracing::SpanPtr Driver::startSpan(const Tracing::Config& config, Http::HeaderMa new_zipkin_span = tracer.startSpan(config, request_headers.Host()->value().c_str(), start_time, context); - } else if (request_headers.XB3TraceId() && request_headers.XB3SpanId()) { + uint64_t traceId(0); + uint64_t spanId(0); uint64_t parentId(0); + if (!StringUtil::atoul(request_headers.XB3TraceId()->value().c_str(), traceId, 16)) { + throw EnvoyException(fmt::format("invalid hex string for XB3TraceId '{}'", + request_headers.XB3TraceId()->value().c_str())); + } + if (!StringUtil::atoul(request_headers.XB3SpanId()->value().c_str(), spanId, 16)) { + throw EnvoyException(fmt::format("invalid hex string for XB3SpanId '{}'", + request_headers.XB3SpanId()->value().c_str())); + } if (request_headers.XB3ParentSpanId()) { - parentId = std::stoull(request_headers.XB3ParentSpanId()->value().c_str(), nullptr, 16); + if (!StringUtil::atoul(request_headers.XB3ParentSpanId()->value().c_str(), parentId, 16)) { + throw EnvoyException(fmt::format("invalid hex string for XB3ParentSpanId '{}'", + request_headers.XB3ParentSpanId()->value().c_str())); + } } - SpanContext context(std::stoull(request_headers.XB3TraceId()->value().c_str(), nullptr, 16), - std::stoull(request_headers.XB3SpanId()->value().c_str(), nullptr, 16), - parentId); + SpanContext context(traceId, spanId, parentId); new_zipkin_span = tracer.startSpan(config, request_headers.Host()->value().c_str(), start_time, context); - } else { // Create a root Zipkin span. No context was found in the headers. new_zipkin_span = tracer.startSpan(config, request_headers.Host()->value().c_str(), start_time); diff --git a/test/common/tracing/zipkin/zipkin_tracer_impl_test.cc b/test/common/tracing/zipkin/zipkin_tracer_impl_test.cc index 219d4981ff50..73a3ec480de1 100644 --- a/test/common/tracing/zipkin/zipkin_tracer_impl_test.cc +++ b/test/common/tracing/zipkin/zipkin_tracer_impl_test.cc @@ -390,5 +390,38 @@ TEST_F(ZipkinDriverTest, ZipkinSpanContextFromB3HeadersTest) { EXPECT_EQ(span_id, zipkin_span->span().idAsHexString()); EXPECT_EQ(parent_id, zipkin_span->span().parentIdAsHexString()); } + +TEST_F(ZipkinDriverTest, ZipkinSpanContextFromInvalidTraceIdB3HeadersTest) { + setupValidDriver(); + + request_headers_.insertXB3TraceId().value(std::string("xyz")); + request_headers_.insertXB3SpanId().value(Hex::uint64ToHex(Util::generateRandom64())); + request_headers_.insertXB3ParentSpanId().value(Hex::uint64ToHex(Util::generateRandom64())); + + EXPECT_THROW(driver_->startSpan(config_, request_headers_, operation_name_, start_time_), + EnvoyException); +} + +TEST_F(ZipkinDriverTest, ZipkinSpanContextFromInvalidSpanIdB3HeadersTest) { + setupValidDriver(); + + request_headers_.insertXB3TraceId().value(Hex::uint64ToHex(Util::generateRandom64())); + request_headers_.insertXB3SpanId().value(std::string("xyz")); + request_headers_.insertXB3ParentSpanId().value(Hex::uint64ToHex(Util::generateRandom64())); + + EXPECT_THROW(driver_->startSpan(config_, request_headers_, operation_name_, start_time_), + EnvoyException); +} + +TEST_F(ZipkinDriverTest, ZipkinSpanContextFromInvalidParentIdB3HeadersTest) { + setupValidDriver(); + + request_headers_.insertXB3TraceId().value(Hex::uint64ToHex(Util::generateRandom64())); + request_headers_.insertXB3SpanId().value(Hex::uint64ToHex(Util::generateRandom64())); + request_headers_.insertXB3ParentSpanId().value(std::string("xyz")); + + EXPECT_THROW(driver_->startSpan(config_, request_headers_, operation_name_, start_time_), + EnvoyException); +} } // namespace Zipkin } // namespace Envoy From 540feffae13cd4cff05f69b2c0fc96b54aa87319 Mon Sep 17 00:00:00 2001 From: Gary Brown Date: Wed, 20 Sep 2017 21:30:38 +0100 Subject: [PATCH 3/5] Change from throwing exception to returning nullptr Signed-off-by: Gary Brown --- .../common/tracing/zipkin/zipkin_tracer_impl.cc | 17 ++++------------- .../tracing/zipkin/zipkin_tracer_impl_test.cc | 9 +++------ 2 files changed, 7 insertions(+), 19 deletions(-) diff --git a/source/common/tracing/zipkin/zipkin_tracer_impl.cc b/source/common/tracing/zipkin/zipkin_tracer_impl.cc index 41e493d8cd09..5f5688c37955 100644 --- a/source/common/tracing/zipkin/zipkin_tracer_impl.cc +++ b/source/common/tracing/zipkin/zipkin_tracer_impl.cc @@ -112,19 +112,10 @@ Tracing::SpanPtr Driver::startSpan(const Tracing::Config& config, Http::HeaderMa uint64_t traceId(0); uint64_t spanId(0); uint64_t parentId(0); - if (!StringUtil::atoul(request_headers.XB3TraceId()->value().c_str(), traceId, 16)) { - throw EnvoyException(fmt::format("invalid hex string for XB3TraceId '{}'", - request_headers.XB3TraceId()->value().c_str())); - } - if (!StringUtil::atoul(request_headers.XB3SpanId()->value().c_str(), spanId, 16)) { - throw EnvoyException(fmt::format("invalid hex string for XB3SpanId '{}'", - request_headers.XB3SpanId()->value().c_str())); - } - if (request_headers.XB3ParentSpanId()) { - if (!StringUtil::atoul(request_headers.XB3ParentSpanId()->value().c_str(), parentId, 16)) { - throw EnvoyException(fmt::format("invalid hex string for XB3ParentSpanId '{}'", - request_headers.XB3ParentSpanId()->value().c_str())); - } + if (!StringUtil::atoul(request_headers.XB3TraceId()->value().c_str(), traceId, 16) + || !StringUtil::atoul(request_headers.XB3SpanId()->value().c_str(), spanId, 16) + || (request_headers.XB3ParentSpanId() && !StringUtil::atoul(request_headers.XB3ParentSpanId()->value().c_str(), parentId, 16))) { + return nullptr; } SpanContext context(traceId, spanId, parentId); diff --git a/test/common/tracing/zipkin/zipkin_tracer_impl_test.cc b/test/common/tracing/zipkin/zipkin_tracer_impl_test.cc index 73a3ec480de1..8b97d8d57b94 100644 --- a/test/common/tracing/zipkin/zipkin_tracer_impl_test.cc +++ b/test/common/tracing/zipkin/zipkin_tracer_impl_test.cc @@ -398,8 +398,7 @@ TEST_F(ZipkinDriverTest, ZipkinSpanContextFromInvalidTraceIdB3HeadersTest) { request_headers_.insertXB3SpanId().value(Hex::uint64ToHex(Util::generateRandom64())); request_headers_.insertXB3ParentSpanId().value(Hex::uint64ToHex(Util::generateRandom64())); - EXPECT_THROW(driver_->startSpan(config_, request_headers_, operation_name_, start_time_), - EnvoyException); + EXPECT_EQ(nullptr, driver_->startSpan(config_, request_headers_, operation_name_, start_time_)); } TEST_F(ZipkinDriverTest, ZipkinSpanContextFromInvalidSpanIdB3HeadersTest) { @@ -409,8 +408,7 @@ TEST_F(ZipkinDriverTest, ZipkinSpanContextFromInvalidSpanIdB3HeadersTest) { request_headers_.insertXB3SpanId().value(std::string("xyz")); request_headers_.insertXB3ParentSpanId().value(Hex::uint64ToHex(Util::generateRandom64())); - EXPECT_THROW(driver_->startSpan(config_, request_headers_, operation_name_, start_time_), - EnvoyException); + EXPECT_EQ(nullptr, driver_->startSpan(config_, request_headers_, operation_name_, start_time_)); } TEST_F(ZipkinDriverTest, ZipkinSpanContextFromInvalidParentIdB3HeadersTest) { @@ -420,8 +418,7 @@ TEST_F(ZipkinDriverTest, ZipkinSpanContextFromInvalidParentIdB3HeadersTest) { request_headers_.insertXB3SpanId().value(Hex::uint64ToHex(Util::generateRandom64())); request_headers_.insertXB3ParentSpanId().value(std::string("xyz")); - EXPECT_THROW(driver_->startSpan(config_, request_headers_, operation_name_, start_time_), - EnvoyException); + EXPECT_EQ(nullptr, driver_->startSpan(config_, request_headers_, operation_name_, start_time_)); } } // namespace Zipkin } // namespace Envoy From 3e5c02907d743b966303a15e129ea933f49c0d66 Mon Sep 17 00:00:00 2001 From: Gary Brown Date: Thu, 21 Sep 2017 15:54:09 +0100 Subject: [PATCH 4/5] Change to use NullSpan instead of nullptr when invalid B3 header detected Signed-off-by: Gary Brown --- source/common/tracing/zipkin/zipkin_tracer_impl.cc | 9 +++++---- .../common/tracing/zipkin/zipkin_tracer_impl_test.cc | 12 +++++++++--- 2 files changed, 14 insertions(+), 7 deletions(-) diff --git a/source/common/tracing/zipkin/zipkin_tracer_impl.cc b/source/common/tracing/zipkin/zipkin_tracer_impl.cc index 5f5688c37955..81af96c970c0 100644 --- a/source/common/tracing/zipkin/zipkin_tracer_impl.cc +++ b/source/common/tracing/zipkin/zipkin_tracer_impl.cc @@ -112,10 +112,11 @@ Tracing::SpanPtr Driver::startSpan(const Tracing::Config& config, Http::HeaderMa uint64_t traceId(0); uint64_t spanId(0); uint64_t parentId(0); - if (!StringUtil::atoul(request_headers.XB3TraceId()->value().c_str(), traceId, 16) - || !StringUtil::atoul(request_headers.XB3SpanId()->value().c_str(), spanId, 16) - || (request_headers.XB3ParentSpanId() && !StringUtil::atoul(request_headers.XB3ParentSpanId()->value().c_str(), parentId, 16))) { - return nullptr; + if (!StringUtil::atoul(request_headers.XB3TraceId()->value().c_str(), traceId, 16) || + !StringUtil::atoul(request_headers.XB3SpanId()->value().c_str(), spanId, 16) || + (request_headers.XB3ParentSpanId() && + !StringUtil::atoul(request_headers.XB3ParentSpanId()->value().c_str(), parentId, 16))) { + return Tracing::SpanPtr(new Tracing::NullSpan()); } SpanContext context(traceId, spanId, parentId); diff --git a/test/common/tracing/zipkin/zipkin_tracer_impl_test.cc b/test/common/tracing/zipkin/zipkin_tracer_impl_test.cc index 8b97d8d57b94..6392f16f5e6f 100644 --- a/test/common/tracing/zipkin/zipkin_tracer_impl_test.cc +++ b/test/common/tracing/zipkin/zipkin_tracer_impl_test.cc @@ -398,7 +398,9 @@ TEST_F(ZipkinDriverTest, ZipkinSpanContextFromInvalidTraceIdB3HeadersTest) { request_headers_.insertXB3SpanId().value(Hex::uint64ToHex(Util::generateRandom64())); request_headers_.insertXB3ParentSpanId().value(Hex::uint64ToHex(Util::generateRandom64())); - EXPECT_EQ(nullptr, driver_->startSpan(config_, request_headers_, operation_name_, start_time_)); + Tracing::SpanPtr span = + driver_->startSpan(config_, request_headers_, operation_name_, start_time_); + EXPECT_NE(nullptr, dynamic_cast(span.get())); } TEST_F(ZipkinDriverTest, ZipkinSpanContextFromInvalidSpanIdB3HeadersTest) { @@ -408,7 +410,9 @@ TEST_F(ZipkinDriverTest, ZipkinSpanContextFromInvalidSpanIdB3HeadersTest) { request_headers_.insertXB3SpanId().value(std::string("xyz")); request_headers_.insertXB3ParentSpanId().value(Hex::uint64ToHex(Util::generateRandom64())); - EXPECT_EQ(nullptr, driver_->startSpan(config_, request_headers_, operation_name_, start_time_)); + Tracing::SpanPtr span = + driver_->startSpan(config_, request_headers_, operation_name_, start_time_); + EXPECT_NE(nullptr, dynamic_cast(span.get())); } TEST_F(ZipkinDriverTest, ZipkinSpanContextFromInvalidParentIdB3HeadersTest) { @@ -418,7 +422,9 @@ TEST_F(ZipkinDriverTest, ZipkinSpanContextFromInvalidParentIdB3HeadersTest) { request_headers_.insertXB3SpanId().value(Hex::uint64ToHex(Util::generateRandom64())); request_headers_.insertXB3ParentSpanId().value(std::string("xyz")); - EXPECT_EQ(nullptr, driver_->startSpan(config_, request_headers_, operation_name_, start_time_)); + Tracing::SpanPtr span = + driver_->startSpan(config_, request_headers_, operation_name_, start_time_); + EXPECT_NE(nullptr, dynamic_cast(span.get())); } } // namespace Zipkin } // namespace Envoy From c1a859d2a76ea20ea7582a289035613f47aff806 Mon Sep 17 00:00:00 2001 From: Gary Brown Date: Thu, 21 Sep 2017 19:34:10 +0100 Subject: [PATCH 5/5] Change to snake_case for variable names Signed-off-by: Gary Brown --- source/common/tracing/zipkin/zipkin_tracer_impl.cc | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/source/common/tracing/zipkin/zipkin_tracer_impl.cc b/source/common/tracing/zipkin/zipkin_tracer_impl.cc index 81af96c970c0..14d04f1ea851 100644 --- a/source/common/tracing/zipkin/zipkin_tracer_impl.cc +++ b/source/common/tracing/zipkin/zipkin_tracer_impl.cc @@ -109,17 +109,17 @@ Tracing::SpanPtr Driver::startSpan(const Tracing::Config& config, Http::HeaderMa new_zipkin_span = tracer.startSpan(config, request_headers.Host()->value().c_str(), start_time, context); } else if (request_headers.XB3TraceId() && request_headers.XB3SpanId()) { - uint64_t traceId(0); - uint64_t spanId(0); - uint64_t parentId(0); - if (!StringUtil::atoul(request_headers.XB3TraceId()->value().c_str(), traceId, 16) || - !StringUtil::atoul(request_headers.XB3SpanId()->value().c_str(), spanId, 16) || + uint64_t trace_id(0); + uint64_t span_id(0); + uint64_t parent_id(0); + if (!StringUtil::atoul(request_headers.XB3TraceId()->value().c_str(), trace_id, 16) || + !StringUtil::atoul(request_headers.XB3SpanId()->value().c_str(), span_id, 16) || (request_headers.XB3ParentSpanId() && - !StringUtil::atoul(request_headers.XB3ParentSpanId()->value().c_str(), parentId, 16))) { + !StringUtil::atoul(request_headers.XB3ParentSpanId()->value().c_str(), parent_id, 16))) { return Tracing::SpanPtr(new Tracing::NullSpan()); } - SpanContext context(traceId, spanId, parentId); + SpanContext context(trace_id, span_id, parent_id); new_zipkin_span = tracer.startSpan(config, request_headers.Host()->value().c_str(), start_time, context);