From e17dd4258a0ace82728b0a60038437bb19a6d760 Mon Sep 17 00:00:00 2001 From: Roman Dzhabarov Date: Thu, 26 Jan 2017 23:40:30 -0800 Subject: [PATCH 1/7] initial --- include/envoy/http/header_map.h | 3 +- include/envoy/tracing/http_tracer.h | 12 ++++- source/common/http/conn_manager_impl.cc | 1 + source/common/http/headers.h | 1 + source/common/tracing/http_tracer_impl.cc | 51 +++++++++++++++++--- source/common/tracing/http_tracer_impl.h | 12 ++++- test/common/http/conn_manager_impl_test.cc | 4 ++ test/common/tracing/http_tracer_impl_test.cc | 50 ++++++++++++++----- test/mocks/tracing/mocks.h | 10 ++-- 9 files changed, 120 insertions(+), 24 deletions(-) diff --git a/include/envoy/http/header_map.h b/include/envoy/http/header_map.h index f99ff61a27c43..bcd287039ebbb 100644 --- a/include/envoy/http/header_map.h +++ b/include/envoy/http/header_map.h @@ -213,7 +213,8 @@ class HeaderEntry { HEADER_FUNC(Status) \ HEADER_FUNC(TransferEncoding) \ HEADER_FUNC(Upgrade) \ - HEADER_FUNC(UserAgent) + HEADER_FUNC(UserAgent) \ + HEADER_FUNC(OtSpanContext) /** * The following functions are defined for each inline header above. E.g., for ContentLength we diff --git a/include/envoy/tracing/http_tracer.h b/include/envoy/tracing/http_tracer.h index de9910da20827..bf33f91f0617e 100644 --- a/include/envoy/tracing/http_tracer.h +++ b/include/envoy/tracing/http_tracer.h @@ -36,7 +36,16 @@ class Driver { public: virtual ~Driver() {} - virtual SpanPtr startSpan(const std::string& operation_name, SystemTime start_time) PURE; + /** + * Start driver specific span. + */ + virtual SpanPtr startSpan(const std::string& parent_context, const std::string& operation_name, + SystemTime start_time) PURE; + + /** + * Inject span context into HTTP carrier. + */ + virtual void inject(Span* active_span, Http::HeaderMap& headers) PURE; }; typedef std::unique_ptr DriverPtr; @@ -51,6 +60,7 @@ class HttpTracer { virtual SpanPtr startSpan(const Config& config, const Http::HeaderMap& request_headers, const Http::AccessLog::RequestInfo& request_info) PURE; + virtual void inject(Span* active_span, Http::HeaderMap& request_headers) PURE; }; typedef std::unique_ptr HttpTracerPtr; diff --git a/source/common/http/conn_manager_impl.cc b/source/common/http/conn_manager_impl.cc index 3016cf4887f62..682e37de69f93 100644 --- a/source/common/http/conn_manager_impl.cc +++ b/source/common/http/conn_manager_impl.cc @@ -431,6 +431,7 @@ void ConnectionManagerImpl::ActiveStream::decodeHeaders(HeaderMapPtr&& headers, if (tracing_decision.is_tracing) { active_span_ = connection_manager_.tracer_.startSpan(*this, *request_headers_, request_info_); + connection_manager_.tracer_.inject(active_span_.get(), *request_headers_); } // Set the trusted address for the connection by taking the last address in XFF. diff --git a/source/common/http/headers.h b/source/common/http/headers.h index 9c14d5542a23a..5dacad7d34be9 100644 --- a/source/common/http/headers.h +++ b/source/common/http/headers.h @@ -51,6 +51,7 @@ class Headers { const LowerCaseString TransferEncoding{"transfer-encoding"}; const LowerCaseString Upgrade{"upgrade"}; const LowerCaseString UserAgent{"user-agent"}; + const LowerCaseString OtSpanContext{"x-ot-span-context"}; struct { const std::string Close{"close"}; diff --git a/source/common/tracing/http_tracer_impl.cc b/source/common/tracing/http_tracer_impl.cc index 0c531dd8e55c7..f7331e9cd7573 100644 --- a/source/common/tracing/http_tracer_impl.cc +++ b/source/common/tracing/http_tracer_impl.cc @@ -137,7 +137,13 @@ HttpTracerImpl::HttpTracerImpl(DriverPtr&& driver, const LocalInfo::LocalInfo& l SpanPtr HttpTracerImpl::startSpan(const Config& config, const Http::HeaderMap& request_headers, const Http::AccessLog::RequestInfo& request_info) { - SpanPtr active_span = driver_->startSpan(config.operationName(), request_info.startTime()); + std::string parent_context; + if (request_headers.OtSpanContext()) { + parent_context = request_headers.OtSpanContext()->value().c_str(); + } + + SpanPtr active_span = + driver_->startSpan(parent_context, config.operationName(), request_info.startTime()); if (active_span) { HttpTracerUtility::populateSpan(*active_span, local_info_.nodeName(), request_headers, request_info); @@ -146,7 +152,13 @@ SpanPtr HttpTracerImpl::startSpan(const Config& config, const Http::HeaderMap& r return active_span; } -LightStepSpan::LightStepSpan(lightstep::Span& span) : span_(span) {} +void HttpTracerImpl::inject(Span* active_span, Http::HeaderMap& request_headers) { + if (active_span) { + driver_->inject(active_span, request_headers); + } +} + +LightStepSpan::LightStepSpan(lightstep::Span span) : span_(span) {} void LightStepSpan::finishSpan() { span_.Finish(); } @@ -244,11 +256,38 @@ LightStepDriver::LightStepDriver(const Json::Object& config, }); } -SpanPtr LightStepDriver::startSpan(const std::string& operation_name, SystemTime start_time) { - lightstep::Span span = tls_.getTyped(tls_slot_).tracer_.StartSpan( - operation_name, {lightstep::StartTimestamp(start_time)}); +SpanPtr LightStepDriver::startSpan(const std::string& parent_context, + const std::string& operation_name, SystemTime start_time) { + lightstep::Tracer& tracer = tls_.getTyped(tls_slot_).tracer_; + + SpanPtr active_span; + + if (!parent_context.empty()) { + lightstep::envoy::CarrierStruct ctx; + ctx.ParseFromString(parent_context); + lightstep::SpanContext parent_span_ctx = tracer.Extract( + lightstep::CarrierFormat::EnvoyProtoCarrier, lightstep::envoy::ProtoReader(ctx)); + + active_span.reset(new LightStepSpan( + tracer.StartSpan(operation_name, {lightstep::ChildOf(parent_span_ctx), + lightstep::StartTimestamp(start_time)}))); + } else { + active_span.reset(new LightStepSpan( + tracer.StartSpan(operation_name, {lightstep::StartTimestamp(start_time)}))); + } + + return active_span; +} + +void LightStepDriver::inject(Span* active_span, Http::HeaderMap& headers) { + lightstep::Tracer& tracer = tls_.getTyped(tls_slot_).tracer_; + lightstep::envoy::CarrierStruct ctx; + + LightStepSpan* ligthstep_span = dynamic_cast(active_span); + tracer.Inject(ligthstep_span->context(), lightstep::CarrierFormat::EnvoyProtoCarrier, + lightstep::envoy::ProtoWriter(&ctx)); - return SpanPtr{new LightStepSpan(span)}; + headers.insertOtSpanContext().value(ctx.SerializeAsString()); } void LightStepRecorder::onFailure(Http::AsyncClient::FailureReason) { diff --git a/source/common/tracing/http_tracer_impl.h b/source/common/tracing/http_tracer_impl.h index a601b246af8b3..2cce7db748e16 100644 --- a/source/common/tracing/http_tracer_impl.h +++ b/source/common/tracing/http_tracer_impl.h @@ -10,6 +10,7 @@ #include "common/json/json_loader.h" #include "lightstep/tracer.h" +#include "lightstep/envoy.h" namespace Tracing { @@ -71,6 +72,8 @@ class HttpNullTracer : public HttpTracer { const Http::AccessLog::RequestInfo&) override { return nullptr; } + + void inject(Span*, Http::HeaderMap&) override {} }; class HttpTracerImpl : public HttpTracer { @@ -80,6 +83,7 @@ class HttpTracerImpl : public HttpTracer { // Tracing::HttpTracer SpanPtr startSpan(const Config& config, const Http::HeaderMap& request_headers, const Http::AccessLog::RequestInfo& request_info) override; + void inject(Span* active_span, Http::HeaderMap& request_headers) override; private: DriverPtr driver_; @@ -88,12 +92,14 @@ class HttpTracerImpl : public HttpTracer { class LightStepSpan : public Span { public: - LightStepSpan(lightstep::Span& span); + LightStepSpan(lightstep::Span span); // Tracing::Span void finishSpan() override; void setTag(const std::string& name, const std::string& value) override; + lightstep::SpanContext context() { return span_.context(); } + private: lightstep::Span span_; }; @@ -111,7 +117,9 @@ class LightStepDriver : public Driver { std::unique_ptr options); // Tracer::TracingDriver - SpanPtr startSpan(const std::string& operation_name, SystemTime start_time) override; + SpanPtr startSpan(const std::string& parent_context, const std::string& operation_name, + SystemTime start_time) override; + void inject(Span* active_span, Http::HeaderMap& headers) override; Upstream::ClusterManager& clusterManager() { return cm_; } Upstream::ClusterInfoPtr cluster() { return cluster_; } diff --git a/test/common/http/conn_manager_impl_test.cc b/test/common/http/conn_manager_impl_test.cc index 8fe39e781d20e..face76849286e 100644 --- a/test/common/http/conn_manager_impl_test.cc +++ b/test/common/http/conn_manager_impl_test.cc @@ -201,6 +201,7 @@ TEST_F(HttpConnectionManagerImplTest, StartAndFinishSpanNormalFlow) { NiceMock* span = new NiceMock(); EXPECT_CALL(tracer_, startSpan_(_, _, _)).WillOnce(Return(span)); + EXPECT_CALL(tracer_, inject(span, _)); EXPECT_CALL(*span, finishSpan()); EXPECT_CALL(runtime_.snapshot_, featureEnabled("tracing.global_enabled", 100, _)) .WillOnce(Return(true)); @@ -238,8 +239,11 @@ TEST_F(HttpConnectionManagerImplTest, StartSpanOnlyHealthCheckRequest) { setup(false, ""); NiceMock* span = new NiceMock(); + EXPECT_CALL(tracer_, startSpan_(_, _, _)).WillOnce(Return(span)); + EXPECT_CALL(tracer_, inject(span, _)); EXPECT_CALL(*span, finishSpan()).Times(0); + EXPECT_CALL(runtime_.snapshot_, featureEnabled("tracing.global_enabled", 100, _)) .WillOnce(Return(true)); diff --git a/test/common/tracing/http_tracer_impl_test.cc b/test/common/tracing/http_tracer_impl_test.cc index aa0e2bccc4193..a0486a825bf20 100644 --- a/test/common/tracing/http_tracer_impl_test.cc +++ b/test/common/tracing/http_tracer_impl_test.cc @@ -355,6 +355,7 @@ class LightStepDriverTest : public Test { const Http::TestHeaderMapImpl request_headers_{ {":path", "/"}, {":method", "GET"}, {"x-request-id", "foo"}}; const Http::TestHeaderMapImpl response_headers_{{":status", "500"}}; + SystemTime start_time_; std::unique_ptr driver_; NiceMock* timer_; @@ -447,7 +448,7 @@ TEST(HttpTracerImplTest, BasicFunctionalityNullSpan) { HttpTracerImpl tracer(std::move(driver_ptr), local_info); - EXPECT_CALL(*driver, startSpan_(operation_name, time)).WillOnce(Return(nullptr)); + EXPECT_CALL(*driver, startSpan_("", operation_name, time)).WillOnce(Return(nullptr)); tracer.startSpan(config, request_headers, request_info); } @@ -471,7 +472,7 @@ TEST(HttpTracerImplTest, BasicFunctionalityNodeSet) { HttpTracerImpl tracer(std::move(driver_ptr), local_info); NiceMock* span = new NiceMock(); - EXPECT_CALL(*driver, startSpan_(operation_name, time)).WillOnce(Return(span)); + EXPECT_CALL(*driver, startSpan_("", operation_name, time)).WillOnce(Return(span)); EXPECT_CALL(*span, setTag(_, _)).Times(testing::AnyNumber()); EXPECT_CALL(*span, setTag("node_id", "node_name")); @@ -499,16 +500,19 @@ TEST_F(LightStepDriverTest, FlushSeveralSpans) { return &request; })); - SystemTime start_time; EXPECT_CALL(runtime_.snapshot_, getInteger("tracing.lightstep.min_flush_spans", 5)) .Times(2) .WillRepeatedly(Return(2)); EXPECT_CALL(runtime_.snapshot_, getInteger("tracing.lightstep.request_timeout", 5000U)) .WillOnce(Return(5000U)); - SpanPtr first_span = driver_->startSpan(operation_name_, start_time); + SpanPtr first_span = driver_->startSpan("", operation_name_, start_time_); + Http::TestHeaderMapImpl headers; + // Simulate inject. + driver_->inject(first_span.get(), headers); first_span->finishSpan(); - SpanPtr second_span = driver_->startSpan(operation_name_, start_time); + + SpanPtr second_span = driver_->startSpan("", operation_name_, start_time_); second_span->finishSpan(); Http::MessagePtr msg(new Http::ResponseMessageImpl( @@ -539,11 +543,10 @@ TEST_F(LightStepDriverTest, FlushSpansTimer) { const Optional timeout(std::chrono::seconds(5)); EXPECT_CALL(cm_.async_client_, send_(_, _, timeout)); - SystemTime start_time; EXPECT_CALL(runtime_.snapshot_, getInteger("tracing.lightstep.min_flush_spans", 5)) .WillOnce(Return(5)); - SpanPtr span = driver_->startSpan(operation_name_, start_time); + SpanPtr span = driver_->startSpan("", operation_name_, start_time_); span->finishSpan(); // Timer should be re-enabled. @@ -579,15 +582,12 @@ TEST_F(LightStepDriverTest, FlushOneSpanGrpcFailure) { return &request; })); - - SystemTime start_time; - EXPECT_CALL(runtime_.snapshot_, getInteger("tracing.lightstep.min_flush_spans", 5)) .WillOnce(Return(1)); EXPECT_CALL(runtime_.snapshot_, getInteger("tracing.lightstep.request_timeout", 5000U)) .WillOnce(Return(5000U)); - SpanPtr span = driver_->startSpan(operation_name_, start_time); + SpanPtr span = driver_->startSpan("", operation_name_, start_time_); span->finishSpan(); Http::MessagePtr msg(new Http::ResponseMessageImpl( @@ -605,4 +605,32 @@ TEST_F(LightStepDriverTest, FlushOneSpanGrpcFailure) { EXPECT_EQ(1U, stats_.counter("tracing.lightstep.spans_sent").value()); } +TEST_F(LightStepDriverTest, SerializeAndDeserializeContext) { + setupValidDriver(); + + // Supply empty context. + SpanPtr span = driver_->startSpan("", operation_name_, start_time_); + + Http::TestHeaderMapImpl carrier; + driver_->inject(span.get(), carrier); + + std::string injected_ctx = carrier.OtSpanContext()->value().c_str(); + EXPECT_FALSE(injected_ctx.empty()); + + // Inject again, there should be the same context. + driver_->inject(span.get(), carrier); + std::string same_ctx = carrier.OtSpanContext()->value().c_str(); + EXPECT_EQ(same_ctx, injected_ctx); + + // Context can be parsed fine. + lightstep::envoy::CarrierStruct ctx; + ctx.ParseFromString(injected_ctx); + + // Supply parent context. + SpanPtr span_with_parent = driver_->startSpan(injected_ctx, operation_name_, start_time_); + driver_->inject(span_with_parent.get(), carrier); + injected_ctx = carrier.OtSpanContext()->value().c_str(); + EXPECT_FALSE(injected_ctx.empty()); +} + } // Tracing diff --git a/test/mocks/tracing/mocks.h b/test/mocks/tracing/mocks.h index 6c1f5db25bed9..dbebd698bb99e 100644 --- a/test/mocks/tracing/mocks.h +++ b/test/mocks/tracing/mocks.h @@ -33,6 +33,7 @@ class MockHttpTracer : public HttpTracer { MOCK_METHOD3(startSpan_, Span*(const Config& config, const Http::HeaderMap& request_headers, const Http::AccessLog::RequestInfo& request_info)); + MOCK_METHOD2(inject, void(Span* active_span, Http::HeaderMap& request_headers)); }; class MockDriver : public Driver { @@ -40,11 +41,14 @@ class MockDriver : public Driver { MockDriver(); ~MockDriver(); - SpanPtr startSpan(const std::string& operation_name, SystemTime start_time) override { - return SpanPtr{startSpan_(operation_name, start_time)}; + SpanPtr startSpan(const std::string& parent_context, const std::string& operation_name, + SystemTime start_time) override { + return SpanPtr{startSpan_(parent_context, operation_name, start_time)}; } - MOCK_METHOD2(startSpan_, Span*(const std::string& operation_name, SystemTime start_time)); + MOCK_METHOD3(startSpan_, Span*(const std::string& parent_context, + const std::string& operation_name, SystemTime start_time)); + MOCK_METHOD2(inject, void(Span* active_span, Http::HeaderMap& request_headers)); }; } // Tracing \ No newline at end of file From a6cda5f792a6d80401bbf640841f07f8d22321d2 Mon Sep 17 00:00:00 2001 From: Roman Dzhabarov Date: Fri, 27 Jan 2017 14:50:31 -0800 Subject: [PATCH 2/7] move. --- source/common/tracing/http_tracer_impl.cc | 1 - 1 file changed, 1 deletion(-) diff --git a/source/common/tracing/http_tracer_impl.cc b/source/common/tracing/http_tracer_impl.cc index f7331e9cd7573..7c5e83594b9c3 100644 --- a/source/common/tracing/http_tracer_impl.cc +++ b/source/common/tracing/http_tracer_impl.cc @@ -259,7 +259,6 @@ LightStepDriver::LightStepDriver(const Json::Object& config, SpanPtr LightStepDriver::startSpan(const std::string& parent_context, const std::string& operation_name, SystemTime start_time) { lightstep::Tracer& tracer = tls_.getTyped(tls_slot_).tracer_; - SpanPtr active_span; if (!parent_context.empty()) { From bd7b4953aa8f3f52bf8af72fa1d0b987cce75d4c Mon Sep 17 00:00:00 2001 From: Roman Dzhabarov Date: Mon, 30 Jan 2017 10:59:28 -0800 Subject: [PATCH 3/7] comments. --- include/envoy/http/header_map.h | 4 +- include/envoy/tracing/http_tracer.h | 10 +---- source/common/http/conn_manager_impl.cc | 1 - source/common/http/headers.h | 2 +- source/common/tracing/http_tracer_impl.cc | 44 +++++++++---------- source/common/tracing/http_tracer_impl.h | 13 +++--- test/common/http/conn_manager_impl_test.cc | 2 - test/common/tracing/http_tracer_impl_test.cc | 45 ++++++++++---------- test/mocks/tracing/mocks.h | 12 +++--- 9 files changed, 56 insertions(+), 77 deletions(-) diff --git a/include/envoy/http/header_map.h b/include/envoy/http/header_map.h index bcd287039ebbb..c514570208d31 100644 --- a/include/envoy/http/header_map.h +++ b/include/envoy/http/header_map.h @@ -205,6 +205,7 @@ class HeaderEntry { HEADER_FUNC(Host) \ HEADER_FUNC(KeepAlive) \ HEADER_FUNC(Method) \ + HEADER_FUNC(OtSpanContext) \ HEADER_FUNC(Path) \ HEADER_FUNC(ProxyConnection) \ HEADER_FUNC(RequestId) \ @@ -213,8 +214,7 @@ class HeaderEntry { HEADER_FUNC(Status) \ HEADER_FUNC(TransferEncoding) \ HEADER_FUNC(Upgrade) \ - HEADER_FUNC(UserAgent) \ - HEADER_FUNC(OtSpanContext) + HEADER_FUNC(UserAgent) /** * The following functions are defined for each inline header above. E.g., for ContentLength we diff --git a/include/envoy/tracing/http_tracer.h b/include/envoy/tracing/http_tracer.h index bf33f91f0617e..0268222f27e33 100644 --- a/include/envoy/tracing/http_tracer.h +++ b/include/envoy/tracing/http_tracer.h @@ -39,13 +39,8 @@ class Driver { /** * Start driver specific span. */ - virtual SpanPtr startSpan(const std::string& parent_context, const std::string& operation_name, + virtual SpanPtr startSpan(Http::HeaderMap& request_headers, const std::string& operation_name, SystemTime start_time) PURE; - - /** - * Inject span context into HTTP carrier. - */ - virtual void inject(Span* active_span, Http::HeaderMap& headers) PURE; }; typedef std::unique_ptr DriverPtr; @@ -58,9 +53,8 @@ class HttpTracer { public: virtual ~HttpTracer() {} - virtual SpanPtr startSpan(const Config& config, const Http::HeaderMap& request_headers, + virtual SpanPtr startSpan(const Config& config, Http::HeaderMap& request_headers, const Http::AccessLog::RequestInfo& request_info) PURE; - virtual void inject(Span* active_span, Http::HeaderMap& request_headers) PURE; }; typedef std::unique_ptr HttpTracerPtr; diff --git a/source/common/http/conn_manager_impl.cc b/source/common/http/conn_manager_impl.cc index 682e37de69f93..3016cf4887f62 100644 --- a/source/common/http/conn_manager_impl.cc +++ b/source/common/http/conn_manager_impl.cc @@ -431,7 +431,6 @@ void ConnectionManagerImpl::ActiveStream::decodeHeaders(HeaderMapPtr&& headers, if (tracing_decision.is_tracing) { active_span_ = connection_manager_.tracer_.startSpan(*this, *request_headers_, request_info_); - connection_manager_.tracer_.inject(active_span_.get(), *request_headers_); } // Set the trusted address for the connection by taking the last address in XFF. diff --git a/source/common/http/headers.h b/source/common/http/headers.h index 5dacad7d34be9..1e3a1e1de037b 100644 --- a/source/common/http/headers.h +++ b/source/common/http/headers.h @@ -42,6 +42,7 @@ class Headers { const LowerCaseString KeepAlive{"keep-alive"}; const LowerCaseString Location{"location"}; const LowerCaseString Method{":method"}; + const LowerCaseString OtSpanContext{"x-ot-span-context"}; const LowerCaseString Path{":path"}; const LowerCaseString ProxyConnection{"proxy-connection"}; const LowerCaseString RequestId{"x-request-id"}; @@ -51,7 +52,6 @@ class Headers { const LowerCaseString TransferEncoding{"transfer-encoding"}; const LowerCaseString Upgrade{"upgrade"}; const LowerCaseString UserAgent{"user-agent"}; - const LowerCaseString OtSpanContext{"x-ot-span-context"}; struct { const std::string Close{"close"}; diff --git a/source/common/tracing/http_tracer_impl.cc b/source/common/tracing/http_tracer_impl.cc index 7c5e83594b9c3..3c91496b0da8a 100644 --- a/source/common/tracing/http_tracer_impl.cc +++ b/source/common/tracing/http_tracer_impl.cc @@ -135,15 +135,14 @@ void HttpTracerUtility::finalizeSpan(Span& active_span, HttpTracerImpl::HttpTracerImpl(DriverPtr&& driver, const LocalInfo::LocalInfo& local_info) : driver_(std::move(driver)), local_info_(local_info) {} -SpanPtr HttpTracerImpl::startSpan(const Config& config, const Http::HeaderMap& request_headers, +SpanPtr HttpTracerImpl::startSpan(const Config& config, Http::HeaderMap& request_headers, const Http::AccessLog::RequestInfo& request_info) { std::string parent_context; if (request_headers.OtSpanContext()) { - parent_context = request_headers.OtSpanContext()->value().c_str(); } SpanPtr active_span = - driver_->startSpan(parent_context, config.operationName(), request_info.startTime()); + driver_->startSpan(request_headers, config.operationName(), request_info.startTime()); if (active_span) { HttpTracerUtility::populateSpan(*active_span, local_info_.nodeName(), request_headers, request_info); @@ -152,12 +151,6 @@ SpanPtr HttpTracerImpl::startSpan(const Config& config, const Http::HeaderMap& r return active_span; } -void HttpTracerImpl::inject(Span* active_span, Http::HeaderMap& request_headers) { - if (active_span) { - driver_->inject(active_span, request_headers); - } -} - LightStepSpan::LightStepSpan(lightstep::Span span) : span_(span) {} void LightStepSpan::finishSpan() { span_.Finish(); } @@ -256,37 +249,38 @@ LightStepDriver::LightStepDriver(const Json::Object& config, }); } -SpanPtr LightStepDriver::startSpan(const std::string& parent_context, +SpanPtr LightStepDriver::startSpan(Http::HeaderMap& request_headers, const std::string& operation_name, SystemTime start_time) { lightstep::Tracer& tracer = tls_.getTyped(tls_slot_).tracer_; - SpanPtr active_span; + LightStepSpanPtr active_span; + + // Extract downstream context from HTTP carrier. + const std::string parent_context = + request_headers.OtSpanContext() ? request_headers.OtSpanContext()->value().c_str() : ""; if (!parent_context.empty()) { lightstep::envoy::CarrierStruct ctx; ctx.ParseFromString(parent_context); + lightstep::SpanContext parent_span_ctx = tracer.Extract( lightstep::CarrierFormat::EnvoyProtoCarrier, lightstep::envoy::ProtoReader(ctx)); - - active_span.reset(new LightStepSpan( + lightstep::Span ls_span = tracer.StartSpan(operation_name, {lightstep::ChildOf(parent_span_ctx), - lightstep::StartTimestamp(start_time)}))); + lightstep::StartTimestamp(start_time)}); + active_span.reset(new LightStepSpan(ls_span)); } else { - active_span.reset(new LightStepSpan( - tracer.StartSpan(operation_name, {lightstep::StartTimestamp(start_time)}))); + lightstep::Span ls_span = + tracer.StartSpan(operation_name, {lightstep::StartTimestamp(start_time)}); + active_span.reset(new LightStepSpan(ls_span)); } - return active_span; -} - -void LightStepDriver::inject(Span* active_span, Http::HeaderMap& headers) { - lightstep::Tracer& tracer = tls_.getTyped(tls_slot_).tracer_; + // Inject newly created span context into HTTP carrier. lightstep::envoy::CarrierStruct ctx; - - LightStepSpan* ligthstep_span = dynamic_cast(active_span); - tracer.Inject(ligthstep_span->context(), lightstep::CarrierFormat::EnvoyProtoCarrier, + tracer.Inject(active_span->context(), lightstep::CarrierFormat::EnvoyProtoCarrier, lightstep::envoy::ProtoWriter(&ctx)); + request_headers.insertOtSpanContext().value(ctx.SerializeAsString()); - headers.insertOtSpanContext().value(ctx.SerializeAsString()); + return std::move(active_span); } void LightStepRecorder::onFailure(Http::AsyncClient::FailureReason) { diff --git a/source/common/tracing/http_tracer_impl.h b/source/common/tracing/http_tracer_impl.h index 2cce7db748e16..2597ccb832e85 100644 --- a/source/common/tracing/http_tracer_impl.h +++ b/source/common/tracing/http_tracer_impl.h @@ -68,12 +68,9 @@ class HttpTracerUtility { class HttpNullTracer : public HttpTracer { public: // Tracing::HttpTracer - SpanPtr startSpan(const Config&, const Http::HeaderMap&, - const Http::AccessLog::RequestInfo&) override { + SpanPtr startSpan(const Config&, Http::HeaderMap&, const Http::AccessLog::RequestInfo&) override { return nullptr; } - - void inject(Span*, Http::HeaderMap&) override {} }; class HttpTracerImpl : public HttpTracer { @@ -81,9 +78,8 @@ class HttpTracerImpl : public HttpTracer { HttpTracerImpl(DriverPtr&& driver, const LocalInfo::LocalInfo& local_info); // Tracing::HttpTracer - SpanPtr startSpan(const Config& config, const Http::HeaderMap& request_headers, + SpanPtr startSpan(const Config& config, Http::HeaderMap& request_headers, const Http::AccessLog::RequestInfo& request_info) override; - void inject(Span* active_span, Http::HeaderMap& request_headers) override; private: DriverPtr driver_; @@ -104,6 +100,8 @@ class LightStepSpan : public Span { lightstep::Span span_; }; +typedef std::unique_ptr LightStepSpanPtr; + /** * LightStep (http://lightstep.com/) provides tracing capabilities, aggregation, visualization of * application trace data. @@ -117,9 +115,8 @@ class LightStepDriver : public Driver { std::unique_ptr options); // Tracer::TracingDriver - SpanPtr startSpan(const std::string& parent_context, const std::string& operation_name, + SpanPtr startSpan(Http::HeaderMap& request_headers, const std::string& operation_name, SystemTime start_time) override; - void inject(Span* active_span, Http::HeaderMap& headers) override; Upstream::ClusterManager& clusterManager() { return cm_; } Upstream::ClusterInfoPtr cluster() { return cluster_; } diff --git a/test/common/http/conn_manager_impl_test.cc b/test/common/http/conn_manager_impl_test.cc index face76849286e..6685d4a273366 100644 --- a/test/common/http/conn_manager_impl_test.cc +++ b/test/common/http/conn_manager_impl_test.cc @@ -201,7 +201,6 @@ TEST_F(HttpConnectionManagerImplTest, StartAndFinishSpanNormalFlow) { NiceMock* span = new NiceMock(); EXPECT_CALL(tracer_, startSpan_(_, _, _)).WillOnce(Return(span)); - EXPECT_CALL(tracer_, inject(span, _)); EXPECT_CALL(*span, finishSpan()); EXPECT_CALL(runtime_.snapshot_, featureEnabled("tracing.global_enabled", 100, _)) .WillOnce(Return(true)); @@ -241,7 +240,6 @@ TEST_F(HttpConnectionManagerImplTest, StartSpanOnlyHealthCheckRequest) { NiceMock* span = new NiceMock(); EXPECT_CALL(tracer_, startSpan_(_, _, _)).WillOnce(Return(span)); - EXPECT_CALL(tracer_, inject(span, _)); EXPECT_CALL(*span, finishSpan()).Times(0); EXPECT_CALL(runtime_.snapshot_, featureEnabled("tracing.global_enabled", 100, _)) diff --git a/test/common/tracing/http_tracer_impl_test.cc b/test/common/tracing/http_tracer_impl_test.cc index a0486a825bf20..994ae2ea7616f 100644 --- a/test/common/tracing/http_tracer_impl_test.cc +++ b/test/common/tracing/http_tracer_impl_test.cc @@ -352,7 +352,7 @@ class LightStepDriverTest : public Test { } const std::string operation_name_{"test"}; - const Http::TestHeaderMapImpl request_headers_{ + Http::TestHeaderMapImpl request_headers_{ {":path", "/"}, {":method", "GET"}, {"x-request-id", "foo"}}; const Http::TestHeaderMapImpl response_headers_{{":status", "500"}}; SystemTime start_time_; @@ -448,7 +448,7 @@ TEST(HttpTracerImplTest, BasicFunctionalityNullSpan) { HttpTracerImpl tracer(std::move(driver_ptr), local_info); - EXPECT_CALL(*driver, startSpan_("", operation_name, time)).WillOnce(Return(nullptr)); + EXPECT_CALL(*driver, startSpan_(_, operation_name, time)).WillOnce(Return(nullptr)); tracer.startSpan(config, request_headers, request_info); } @@ -472,7 +472,7 @@ TEST(HttpTracerImplTest, BasicFunctionalityNodeSet) { HttpTracerImpl tracer(std::move(driver_ptr), local_info); NiceMock* span = new NiceMock(); - EXPECT_CALL(*driver, startSpan_("", operation_name, time)).WillOnce(Return(span)); + EXPECT_CALL(*driver, startSpan_(_, operation_name, time)).WillOnce(Return(span)); EXPECT_CALL(*span, setTag(_, _)).Times(testing::AnyNumber()); EXPECT_CALL(*span, setTag("node_id", "node_name")); @@ -506,13 +506,11 @@ TEST_F(LightStepDriverTest, FlushSeveralSpans) { EXPECT_CALL(runtime_.snapshot_, getInteger("tracing.lightstep.request_timeout", 5000U)) .WillOnce(Return(5000U)); - SpanPtr first_span = driver_->startSpan("", operation_name_, start_time_); Http::TestHeaderMapImpl headers; - // Simulate inject. - driver_->inject(first_span.get(), headers); + SpanPtr first_span = driver_->startSpan(headers, operation_name_, start_time_); first_span->finishSpan(); - SpanPtr second_span = driver_->startSpan("", operation_name_, start_time_); + SpanPtr second_span = driver_->startSpan(headers, operation_name_, start_time_); second_span->finishSpan(); Http::MessagePtr msg(new Http::ResponseMessageImpl( @@ -546,7 +544,7 @@ TEST_F(LightStepDriverTest, FlushSpansTimer) { EXPECT_CALL(runtime_.snapshot_, getInteger("tracing.lightstep.min_flush_spans", 5)) .WillOnce(Return(5)); - SpanPtr span = driver_->startSpan("", operation_name_, start_time_); + SpanPtr span = driver_->startSpan(request_headers_, operation_name_, start_time_); span->finishSpan(); // Timer should be re-enabled. @@ -587,7 +585,8 @@ TEST_F(LightStepDriverTest, FlushOneSpanGrpcFailure) { EXPECT_CALL(runtime_.snapshot_, getInteger("tracing.lightstep.request_timeout", 5000U)) .WillOnce(Return(5000U)); - SpanPtr span = driver_->startSpan("", operation_name_, start_time_); + Http::TestHeaderMapImpl headers; + SpanPtr span = driver_->startSpan(headers, operation_name_, start_time_); span->finishSpan(); Http::MessagePtr msg(new Http::ResponseMessageImpl( @@ -608,28 +607,28 @@ TEST_F(LightStepDriverTest, FlushOneSpanGrpcFailure) { TEST_F(LightStepDriverTest, SerializeAndDeserializeContext) { setupValidDriver(); - // Supply empty context. - SpanPtr span = driver_->startSpan("", operation_name_, start_time_); - - Http::TestHeaderMapImpl carrier; - driver_->inject(span.get(), carrier); + // Supply bogus context. + const std::string invalid_context = "not valid context"; + request_headers_.insertOtSpanContext().value(invalid_context); + driver_->startSpan(request_headers_, operation_name_, start_time_); - std::string injected_ctx = carrier.OtSpanContext()->value().c_str(); + std::string injected_ctx = request_headers_.OtSpanContext()->value().c_str(); EXPECT_FALSE(injected_ctx.empty()); - // Inject again, there should be the same context. - driver_->inject(span.get(), carrier); - std::string same_ctx = carrier.OtSpanContext()->value().c_str(); - EXPECT_EQ(same_ctx, injected_ctx); + // Supply empty context. + request_headers_.removeOtSpanContext(); + SpanPtr span = driver_->startSpan(request_headers_, operation_name_, start_time_); + + injected_ctx = request_headers_.OtSpanContext()->value().c_str(); + EXPECT_FALSE(injected_ctx.empty()); // Context can be parsed fine. lightstep::envoy::CarrierStruct ctx; ctx.ParseFromString(injected_ctx); - // Supply parent context. - SpanPtr span_with_parent = driver_->startSpan(injected_ctx, operation_name_, start_time_); - driver_->inject(span_with_parent.get(), carrier); - injected_ctx = carrier.OtSpanContext()->value().c_str(); + // Supply parent context, request_headers has properly populated x-ot-span-context. + SpanPtr span_with_parent = driver_->startSpan(request_headers_, operation_name_, start_time_); + injected_ctx = request_headers_.OtSpanContext()->value().c_str(); EXPECT_FALSE(injected_ctx.empty()); } diff --git a/test/mocks/tracing/mocks.h b/test/mocks/tracing/mocks.h index dbebd698bb99e..d9d21e4abaf79 100644 --- a/test/mocks/tracing/mocks.h +++ b/test/mocks/tracing/mocks.h @@ -26,14 +26,13 @@ class MockHttpTracer : public HttpTracer { MockHttpTracer(); ~MockHttpTracer(); - SpanPtr startSpan(const Config& config, const Http::HeaderMap& request_headers, + SpanPtr startSpan(const Config& config, Http::HeaderMap& request_headers, const Http::AccessLog::RequestInfo& request_info) override { return SpanPtr{startSpan_(config, request_headers, request_info)}; } - MOCK_METHOD3(startSpan_, Span*(const Config& config, const Http::HeaderMap& request_headers, + MOCK_METHOD3(startSpan_, Span*(const Config& config, Http::HeaderMap& request_headers, const Http::AccessLog::RequestInfo& request_info)); - MOCK_METHOD2(inject, void(Span* active_span, Http::HeaderMap& request_headers)); }; class MockDriver : public Driver { @@ -41,14 +40,13 @@ class MockDriver : public Driver { MockDriver(); ~MockDriver(); - SpanPtr startSpan(const std::string& parent_context, const std::string& operation_name, + SpanPtr startSpan(Http::HeaderMap& request_headers, const std::string& operation_name, SystemTime start_time) override { - return SpanPtr{startSpan_(parent_context, operation_name, start_time)}; + return SpanPtr{startSpan_(request_headers, operation_name, start_time)}; } - MOCK_METHOD3(startSpan_, Span*(const std::string& parent_context, + MOCK_METHOD3(startSpan_, Span*(Http::HeaderMap& request_headers, const std::string& operation_name, SystemTime start_time)); - MOCK_METHOD2(inject, void(Span* active_span, Http::HeaderMap& request_headers)); }; } // Tracing \ No newline at end of file From 5f916126e092f1d018b5cd738a7b4b806a90a658 Mon Sep 17 00:00:00 2001 From: Roman Dzhabarov Date: Mon, 30 Jan 2017 11:01:29 -0800 Subject: [PATCH 4/7] revert this. --- source/common/tracing/http_tracer_impl.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/source/common/tracing/http_tracer_impl.h b/source/common/tracing/http_tracer_impl.h index 2597ccb832e85..01c99e1217053 100644 --- a/source/common/tracing/http_tracer_impl.h +++ b/source/common/tracing/http_tracer_impl.h @@ -88,7 +88,7 @@ class HttpTracerImpl : public HttpTracer { class LightStepSpan : public Span { public: - LightStepSpan(lightstep::Span span); + LightStepSpan(lightstep::Span& span); // Tracing::Span void finishSpan() override; From dd9bb0a8fc34df5c4230fd5a74566cdd3f1def26 Mon Sep 17 00:00:00 2001 From: Roman Dzhabarov Date: Mon, 30 Jan 2017 11:09:15 -0800 Subject: [PATCH 5/7] revert. --- source/common/tracing/http_tracer_impl.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/source/common/tracing/http_tracer_impl.cc b/source/common/tracing/http_tracer_impl.cc index 3c91496b0da8a..dbf2afb362eb5 100644 --- a/source/common/tracing/http_tracer_impl.cc +++ b/source/common/tracing/http_tracer_impl.cc @@ -151,7 +151,7 @@ SpanPtr HttpTracerImpl::startSpan(const Config& config, Http::HeaderMap& request return active_span; } -LightStepSpan::LightStepSpan(lightstep::Span span) : span_(span) {} +LightStepSpan::LightStepSpan(lightstep::Span& span) : span_(span) {} void LightStepSpan::finishSpan() { span_.Finish(); } From d255d1131150af0f1432c4b6183ac9ec51c7c8ee Mon Sep 17 00:00:00 2001 From: Roman Dzhabarov Date: Mon, 30 Jan 2017 11:19:39 -0800 Subject: [PATCH 6/7] nit. --- source/common/tracing/http_tracer_impl.cc | 4 ---- source/common/tracing/http_tracer_impl.h | 2 +- test/common/tracing/http_tracer_impl_test.cc | 2 +- 3 files changed, 2 insertions(+), 6 deletions(-) diff --git a/source/common/tracing/http_tracer_impl.cc b/source/common/tracing/http_tracer_impl.cc index dbf2afb362eb5..e137aa3182569 100644 --- a/source/common/tracing/http_tracer_impl.cc +++ b/source/common/tracing/http_tracer_impl.cc @@ -137,10 +137,6 @@ HttpTracerImpl::HttpTracerImpl(DriverPtr&& driver, const LocalInfo::LocalInfo& l SpanPtr HttpTracerImpl::startSpan(const Config& config, Http::HeaderMap& request_headers, const Http::AccessLog::RequestInfo& request_info) { - std::string parent_context; - if (request_headers.OtSpanContext()) { - } - SpanPtr active_span = driver_->startSpan(request_headers, config.operationName(), request_info.startTime()); if (active_span) { diff --git a/source/common/tracing/http_tracer_impl.h b/source/common/tracing/http_tracer_impl.h index 01c99e1217053..bc5782a250901 100644 --- a/source/common/tracing/http_tracer_impl.h +++ b/source/common/tracing/http_tracer_impl.h @@ -9,8 +9,8 @@ #include "common/http/header_map_impl.h" #include "common/json/json_loader.h" -#include "lightstep/tracer.h" #include "lightstep/envoy.h" +#include "lightstep/tracer.h" namespace Tracing { diff --git a/test/common/tracing/http_tracer_impl_test.cc b/test/common/tracing/http_tracer_impl_test.cc index 994ae2ea7616f..c7f4e46ae44be 100644 --- a/test/common/tracing/http_tracer_impl_test.cc +++ b/test/common/tracing/http_tracer_impl_test.cc @@ -607,7 +607,7 @@ TEST_F(LightStepDriverTest, FlushOneSpanGrpcFailure) { TEST_F(LightStepDriverTest, SerializeAndDeserializeContext) { setupValidDriver(); - // Supply bogus context. + // Supply bogus context, that will be simply ignored. const std::string invalid_context = "not valid context"; request_headers_.insertOtSpanContext().value(invalid_context); driver_->startSpan(request_headers_, operation_name_, start_time_); From 0d54ad4d44f5d270edabe730335b4e9b09d79354 Mon Sep 17 00:00:00 2001 From: Roman Dzhabarov Date: Mon, 30 Jan 2017 13:32:41 -0800 Subject: [PATCH 7/7] nit --- source/common/tracing/http_tracer_impl.cc | 9 +++------ 1 file changed, 3 insertions(+), 6 deletions(-) diff --git a/source/common/tracing/http_tracer_impl.cc b/source/common/tracing/http_tracer_impl.cc index e137aa3182569..0942cfc05d4e2 100644 --- a/source/common/tracing/http_tracer_impl.cc +++ b/source/common/tracing/http_tracer_impl.cc @@ -250,13 +250,10 @@ SpanPtr LightStepDriver::startSpan(Http::HeaderMap& request_headers, lightstep::Tracer& tracer = tls_.getTyped(tls_slot_).tracer_; LightStepSpanPtr active_span; - // Extract downstream context from HTTP carrier. - const std::string parent_context = - request_headers.OtSpanContext() ? request_headers.OtSpanContext()->value().c_str() : ""; - - if (!parent_context.empty()) { + if (request_headers.OtSpanContext()) { + // Extract downstream context from HTTP carrier. lightstep::envoy::CarrierStruct ctx; - ctx.ParseFromString(parent_context); + ctx.ParseFromString(request_headers.OtSpanContext()->value().c_str()); lightstep::SpanContext parent_span_ctx = tracer.Extract( lightstep::CarrierFormat::EnvoyProtoCarrier, lightstep::envoy::ProtoReader(ctx));