diff --git a/include/envoy/tracing/http_tracer.h b/include/envoy/tracing/http_tracer.h index b55c8c4c492c..109debdf5145 100644 --- a/include/envoy/tracing/http_tracer.h +++ b/include/envoy/tracing/http_tracer.h @@ -31,13 +31,27 @@ class Config { virtual const std::vector& requestHeadersForTags() const PURE; }; -/* - * Basic abstraction for span. - */ class Span; typedef std::unique_ptr SpanPtr; +/* + * Interface to perform context-specific tasks when completing a Span. + */ +class SpanFinalizer { +public: + virtual ~SpanFinalizer() {} + + /** + * Finalize the Span. + * @param span the Span to be finalized + */ + virtual void finalize(Span& span) PURE; +}; + +/* + * Basic abstraction for span. + */ class Span { public: virtual ~Span() {} @@ -52,8 +66,9 @@ class Span { /** * Capture the final duration for this Span and carry out any work necessary to complete it. * Once this method is called, the Span may be safely discarded. + * @param finalizer callback for context-specific work to complete the Span */ - virtual void finishSpan() PURE; + virtual void finishSpan(SpanFinalizer& finalizer) PURE; /** * Mutate the provided headers with the context necessary to propagate this diff --git a/source/common/http/conn_manager_impl.cc b/source/common/http/conn_manager_impl.cc index 1c9d790492be..d5c5f3aff66f 100644 --- a/source/common/http/conn_manager_impl.cc +++ b/source/common/http/conn_manager_impl.cc @@ -336,13 +336,11 @@ ConnectionManagerImpl::ActiveStream::~ActiveStream() { log_handler->log(request_headers_.get(), response_headers_.get(), request_info_); } - if (active_span_) { - if (request_info_.healthCheck()) { - connection_manager_.config_.tracingStats().health_check_.inc(); - } else { - Tracing::HttpTracerUtility::finalizeSpan(*active_span_, *request_headers_, request_info_, - *this); - } + if (request_info_.healthCheck()) { + connection_manager_.config_.tracingStats().health_check_.inc(); + } else { + Tracing::HttpConnManFinalizerImpl finalizer(request_headers_.get(), request_info_, *this); + active_span_->finishSpan(finalizer); } ASSERT(state_.filter_call_state_ == 0); diff --git a/source/common/http/conn_manager_impl.h b/source/common/http/conn_manager_impl.h index 2926f0c1b82b..e059c6b28535 100644 --- a/source/common/http/conn_manager_impl.h +++ b/source/common/http/conn_manager_impl.h @@ -445,7 +445,7 @@ class ConnectionManagerImpl : Logger::Loggable, ConnectionManagerImpl& connection_manager_; Router::ConfigConstSharedPtr snapped_route_config_; - Tracing::SpanPtr active_span_; + Tracing::SpanPtr active_span_{new Tracing::NullSpan()}; const uint64_t stream_id_; StreamEncoder* response_encoder_{}; HeaderMapPtr response_headers_; diff --git a/source/common/tracing/http_tracer_impl.cc b/source/common/tracing/http_tracer_impl.cc index d7329dfd6e39..59a47307ac7b 100644 --- a/source/common/tracing/http_tracer_impl.cc +++ b/source/common/tracing/http_tracer_impl.cc @@ -116,44 +116,46 @@ Decision HttpTracerUtility::isTracing(const Http::AccessLog::RequestInfo& reques NOT_REACHED; } -void HttpTracerUtility::finalizeSpan(Span& active_span, const Http::HeaderMap& request_headers, - const Http::AccessLog::RequestInfo& request_info, - const Config& config) { +HttpConnManFinalizerImpl::HttpConnManFinalizerImpl(Http::HeaderMap* request_headers, + Http::AccessLog::RequestInfo& request_info, + Config& tracing_config) + : request_headers_(request_headers), request_info_(request_info), + tracing_config_(tracing_config) {} + +void HttpConnManFinalizerImpl::finalize(Span& span) { // Pre response data. - active_span.setTag("guid:x-request-id", - std::string(request_headers.RequestId()->value().c_str())); - active_span.setTag("request_line", buildRequestLine(request_headers, request_info)); - active_span.setTag("request_size", std::to_string(request_info.bytesReceived())); - active_span.setTag("host_header", valueOrDefault(request_headers.Host(), "-")); - active_span.setTag("downstream_cluster", - valueOrDefault(request_headers.EnvoyDownstreamServiceCluster(), "-")); - active_span.setTag("user_agent", valueOrDefault(request_headers.UserAgent(), "-")); - - if (request_headers.ClientTraceId()) { - active_span.setTag("guid:x-client-trace-id", - std::string(request_headers.ClientTraceId()->value().c_str())); - } + if (request_headers_) { + span.setTag("guid:x-request-id", std::string(request_headers_->RequestId()->value().c_str())); + span.setTag("request_line", buildRequestLine(*request_headers_, request_info_)); + span.setTag("host_header", valueOrDefault(request_headers_->Host(), "-")); + span.setTag("downstream_cluster", + valueOrDefault(request_headers_->EnvoyDownstreamServiceCluster(), "-")); + span.setTag("user_agent", valueOrDefault(request_headers_->UserAgent(), "-")); + + if (request_headers_->ClientTraceId()) { + span.setTag("guid:x-client-trace-id", + std::string(request_headers_->ClientTraceId()->value().c_str())); + } - // Build tags based on the custom headers. - for (const Http::LowerCaseString& header : config.requestHeadersForTags()) { - const Http::HeaderEntry* entry = request_headers.get(header); - if (entry) { - active_span.setTag(header.get(), entry->value().c_str()); + // Build tags based on the custom headers. + for (const Http::LowerCaseString& header : tracing_config_.requestHeadersForTags()) { + const Http::HeaderEntry* entry = request_headers_->get(header); + if (entry) { + span.setTag(header.get(), entry->value().c_str()); + } } } + span.setTag("request_size", std::to_string(request_info_.bytesReceived())); // Post response data. - active_span.setTag("response_code", buildResponseCode(request_info)); - active_span.setTag("response_size", std::to_string(request_info.bytesSent())); - active_span.setTag("response_flags", - Http::AccessLog::ResponseFlagUtils::toShortString(request_info)); - - if (request_info.responseCode().valid() && - Http::CodeUtility::is5xx(request_info.responseCode().value())) { - active_span.setTag("error", "true"); - } + span.setTag("response_code", buildResponseCode(request_info_)); + span.setTag("response_size", std::to_string(request_info_.bytesSent())); + span.setTag("response_flags", Http::AccessLog::ResponseFlagUtils::toShortString(request_info_)); - active_span.finishSpan(); + if (request_info_.responseCode().valid() && + Http::CodeUtility::is5xx(request_info_.responseCode().value())) { + span.setTag("error", "true"); + } } HttpTracerImpl::HttpTracerImpl(DriverPtr&& driver, const LocalInfo::LocalInfo& local_info) diff --git a/source/common/tracing/http_tracer_impl.h b/source/common/tracing/http_tracer_impl.h index 0f74bebbea5c..61ebcb09cf39 100644 --- a/source/common/tracing/http_tracer_impl.h +++ b/source/common/tracing/http_tracer_impl.h @@ -62,11 +62,42 @@ class HttpTracerUtility { static const std::string EGRESS_OPERATION; }; +class NullSpan : public Span { +public: + // Tracing::Span + void setTag(const std::string&, const std::string&) override {} + void finishSpan(SpanFinalizer&) override {} + void injectContext(Http::HeaderMap&) override {} + SpanPtr spawnChild(const std::string&, SystemTime) override { return SpanPtr{new NullSpan()}; } +}; + +class NullFinalizer : public SpanFinalizer { +public: + // Tracing::SpanFinalizer + void finalize(Span&) override {} +}; + +/** + * Finalizer for Spans covering standard request ingress. + */ +class HttpConnManFinalizerImpl : public SpanFinalizer { +public: + HttpConnManFinalizerImpl(Http::HeaderMap* request_headers, + Http::AccessLog::RequestInfo& request_info, Config& tracing_config); + + void finalize(Span& span) override; + +private: + Http::HeaderMap* request_headers_; + Http::AccessLog::RequestInfo& request_info_; + Config& tracing_config_; +}; + class HttpNullTracer : public HttpTracer { public: // Tracing::HttpTracer SpanPtr startSpan(const Config&, Http::HeaderMap&, const Http::AccessLog::RequestInfo&) override { - return nullptr; + return SpanPtr{new NullSpan()}; } }; @@ -83,14 +114,5 @@ class HttpTracerImpl : public HttpTracer { const LocalInfo::LocalInfo& local_info_; }; -class NullSpan : public Tracing::Span { -public: - // Tracing::Span - void setTag(const std::string&, const std::string&) override {} - void finishSpan() override {} - void injectContext(Http::HeaderMap&) override {} - SpanPtr spawnChild(const std::string&, SystemTime) override { return SpanPtr{new NullSpan()}; } -}; - } // Tracing } // Envoy diff --git a/source/common/tracing/lightstep_tracer_impl.cc b/source/common/tracing/lightstep_tracer_impl.cc index d2bf732969c2..0a998d73f6e9 100644 --- a/source/common/tracing/lightstep_tracer_impl.cc +++ b/source/common/tracing/lightstep_tracer_impl.cc @@ -18,7 +18,10 @@ namespace Tracing { LightStepSpan::LightStepSpan(lightstep::Span& span, lightstep::Tracer& tracer) : span_(span), tracer_(tracer) {} -void LightStepSpan::finishSpan() { span_.Finish(); } +void LightStepSpan::finishSpan(SpanFinalizer& finalizer) { + finalizer.finalize(*this); + span_.Finish(); +} void LightStepSpan::setTag(const std::string& name, const std::string& value) { span_.SetTag(name, value); diff --git a/source/common/tracing/lightstep_tracer_impl.h b/source/common/tracing/lightstep_tracer_impl.h index 11fe912438c9..3f9b4521acca 100644 --- a/source/common/tracing/lightstep_tracer_impl.h +++ b/source/common/tracing/lightstep_tracer_impl.h @@ -32,7 +32,7 @@ class LightStepSpan : public Span { LightStepSpan(lightstep::Span& span, lightstep::Tracer& tracer); // Tracing::Span - void finishSpan() override; + void finishSpan(SpanFinalizer& finalizer) override; void setTag(const std::string& name, const std::string& value) override; void injectContext(Http::HeaderMap& request_headers) override; SpanPtr spawnChild(const std::string& name, SystemTime start_time) override; diff --git a/source/common/tracing/zipkin/zipkin_tracer_impl.cc b/source/common/tracing/zipkin/zipkin_tracer_impl.cc index 30dfcc658a10..576d5f43c03a 100644 --- a/source/common/tracing/zipkin/zipkin_tracer_impl.cc +++ b/source/common/tracing/zipkin/zipkin_tracer_impl.cc @@ -14,7 +14,10 @@ namespace Zipkin { ZipkinSpan::ZipkinSpan(Zipkin::Span& span, Zipkin::Tracer& tracer) : span_(span), tracer_(tracer) {} -void ZipkinSpan::finishSpan() { span_.finish(); } +void ZipkinSpan::finishSpan(Tracing::SpanFinalizer& finalizer) { + finalizer.finalize(*this); + span_.finish(); +} void ZipkinSpan::setTag(const std::string& name, const std::string& value) { if (this->hasCSAnnotation()) { diff --git a/source/common/tracing/zipkin/zipkin_tracer_impl.h b/source/common/tracing/zipkin/zipkin_tracer_impl.h index 0e643805d81d..7cbe01df461e 100644 --- a/source/common/tracing/zipkin/zipkin_tracer_impl.h +++ b/source/common/tracing/zipkin/zipkin_tracer_impl.h @@ -40,8 +40,10 @@ class ZipkinSpan : public Tracing::Span { /** * Calls Zipkin::Span::finishSpan() to perform all actions needed to finalize the span. * This function is called by Tracing::HttpTracerUtility::finalizeSpan(). + * + * @param finalizer SpanFinalizer to be called to complete this Span */ - void finishSpan() override; + void finishSpan(Tracing::SpanFinalizer& finalizer) override; /** * This function adds a Zipkin "string" binary annotation to this span. diff --git a/test/common/http/conn_manager_impl_test.cc b/test/common/http/conn_manager_impl_test.cc index 6d7bba15cb50..d7bee10f1a03 100644 --- a/test/common/http/conn_manager_impl_test.cc +++ b/test/common/http/conn_manager_impl_test.cc @@ -293,7 +293,9 @@ TEST_F(HttpConnectionManagerImplTest, StartAndFinishSpanNormalFlow) { return span; })); - EXPECT_CALL(*span, finishSpan()); + EXPECT_CALL(*span, finishSpan(_)) + .WillOnce( + Invoke([span](Tracing::SpanFinalizer& finalizer) -> void { finalizer.finalize(*span); })); EXPECT_CALL(*span, setTag(_, _)).Times(testing::AnyNumber()); // Verify tag is set based on the request headers. EXPECT_CALL(*span, setTag(":method", "GET")); @@ -422,7 +424,7 @@ TEST_F(HttpConnectionManagerImplTest, StartSpanOnlyHealthCheckRequest) { NiceMock* span = new NiceMock(); EXPECT_CALL(tracer_, startSpan_(_, _, _)).WillOnce(Return(span)); - EXPECT_CALL(*span, finishSpan()).Times(0); + 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 90d128e705db..6bfa704b0dea 100644 --- a/test/common/tracing/http_tracer_impl_test.cc +++ b/test/common/tracing/http_tracer_impl_test.cc @@ -234,7 +234,7 @@ TEST(HttpTracerUtilityTest, IsTracing) { } } -TEST(HttpTracerUtilityTest, OriginalAndLongPath) { +TEST(HttpConnManFinalizerImpl, OriginalAndLongPath) { const std::string path(300, 'a'); const std::string expected_path(128, 'a'); std::unique_ptr> span(new NiceMock()); @@ -253,10 +253,31 @@ TEST(HttpTracerUtilityTest, OriginalAndLongPath) { EXPECT_CALL(*span, setTag(_, _)).Times(testing::AnyNumber()); EXPECT_CALL(*span, setTag("request_line", "GET " + expected_path + " HTTP/2")); NiceMock config; - HttpTracerUtility::finalizeSpan(*span, request_headers, request_info, config); + + HttpConnManFinalizerImpl finalizer(&request_headers, request_info, config); + finalizer.finalize(*span); } -TEST(HttpTracerUtilityTest, SpanOptionalHeaders) { +TEST(HttpConnManFinalizerImpl, NullRequestHeaders) { + std::unique_ptr> span(new NiceMock()); + NiceMock request_info; + + EXPECT_CALL(request_info, bytesReceived()).WillOnce(Return(10)); + EXPECT_CALL(request_info, bytesSent()).WillOnce(Return(11)); + Optional response_code; + EXPECT_CALL(request_info, responseCode()).WillRepeatedly(ReturnRef(response_code)); + + EXPECT_CALL(*span, setTag("response_code", "0")); + EXPECT_CALL(*span, setTag("response_size", "11")); + EXPECT_CALL(*span, setTag("response_flags", "-")); + EXPECT_CALL(*span, setTag("request_size", "10")); + NiceMock config; + + HttpConnManFinalizerImpl finalizer(nullptr, request_info, config); + finalizer.finalize(*span); +} + +TEST(HttpConnManFinalizerImpl, SpanOptionalHeaders) { std::unique_ptr> span(new NiceMock()); Http::TestHeaderMapImpl request_headers{ @@ -284,12 +305,13 @@ TEST(HttpTracerUtilityTest, SpanOptionalHeaders) { EXPECT_CALL(*span, setTag("response_size", "100")); EXPECT_CALL(*span, setTag("response_flags", "-")); - EXPECT_CALL(*span, finishSpan()); NiceMock config; - HttpTracerUtility::finalizeSpan(*span, request_headers, request_info, config); + + HttpConnManFinalizerImpl finalizer(&request_headers, request_info, config); + finalizer.finalize(*span); } -TEST(HttpTracerUtilityTest, SpanPopulatedFailureResponse) { +TEST(HttpConnManFinalizerImpl, SpanPopulatedFailureResponse) { std::unique_ptr> span(new NiceMock()); Http::TestHeaderMapImpl request_headers{ {"x-request-id", "id"}, {":path", "/test"}, {":method", "GET"}}; @@ -337,8 +359,8 @@ TEST(HttpTracerUtilityTest, SpanPopulatedFailureResponse) { EXPECT_CALL(*span, setTag("response_size", "100")); EXPECT_CALL(*span, setTag("response_flags", "UT")); - EXPECT_CALL(*span, finishSpan()); - HttpTracerUtility::finalizeSpan(*span, request_headers, request_info, config); + HttpConnManFinalizerImpl finalizer(&request_headers, request_info, config); + finalizer.finalize(*span); } TEST(HttpTracerUtilityTest, operationTypeToString) { @@ -352,7 +374,8 @@ TEST(HttpNullTracerTest, BasicFunctionality) { Http::AccessLog::MockRequestInfo request_info; Http::TestHeaderMapImpl request_headers; - EXPECT_EQ(nullptr, null_tracer.startSpan(config, request_headers, request_info)); + SpanPtr span_ptr = null_tracer.startSpan(config, request_headers, request_info); + EXPECT_TRUE(dynamic_cast(span_ptr.get()) != nullptr); } class HttpTracerImplTest : public Test { diff --git a/test/common/tracing/lightstep_tracer_impl_test.cc b/test/common/tracing/lightstep_tracer_impl_test.cc index 84db09ef30b5..2779c5ca7982 100644 --- a/test/common/tracing/lightstep_tracer_impl_test.cc +++ b/test/common/tracing/lightstep_tracer_impl_test.cc @@ -166,10 +166,15 @@ TEST_F(LightStepDriverTest, FlushSeveralSpans) { .WillOnce(Return(5000U)); SpanPtr first_span = driver_->startSpan(request_headers_, operation_name_, start_time_); - first_span->finishSpan(); + Tracing::MockFinalizer finalizer; + + EXPECT_CALL(finalizer, finalize(_)); + first_span->finishSpan(finalizer); SpanPtr second_span = driver_->startSpan(request_headers_, operation_name_, start_time_); - second_span->finishSpan(); + + EXPECT_CALL(finalizer, finalize(_)); + second_span->finishSpan(finalizer); Http::MessagePtr msg(new Http::ResponseMessageImpl( Http::HeaderMapPtr{new Http::TestHeaderMapImpl{{":status", "200"}}})); @@ -203,7 +208,10 @@ TEST_F(LightStepDriverTest, FlushSpansTimer) { .WillOnce(Return(5)); SpanPtr span = driver_->startSpan(request_headers_, operation_name_, start_time_); - span->finishSpan(); + Tracing::MockFinalizer finalizer; + + EXPECT_CALL(finalizer, finalize(_)); + span->finishSpan(finalizer); // Timer should be re-enabled. EXPECT_CALL(*timer_, enableTimer(std::chrono::milliseconds(1000))); @@ -244,7 +252,10 @@ TEST_F(LightStepDriverTest, FlushOneSpanGrpcFailure) { .WillOnce(Return(5000U)); SpanPtr span = driver_->startSpan(request_headers_, operation_name_, start_time_); - span->finishSpan(); + Tracing::MockFinalizer finalizer; + + EXPECT_CALL(finalizer, finalize(_)); + span->finishSpan(finalizer); Http::MessagePtr msg(new Http::ResponseMessageImpl( Http::HeaderMapPtr{new Http::TestHeaderMapImpl{{":status", "200"}}})); diff --git a/test/common/tracing/zipkin/zipkin_tracer_impl_test.cc b/test/common/tracing/zipkin/zipkin_tracer_impl_test.cc index 14be7cb7659b..a0969db26c1d 100644 --- a/test/common/tracing/zipkin/zipkin_tracer_impl_test.cc +++ b/test/common/tracing/zipkin/zipkin_tracer_impl_test.cc @@ -153,10 +153,15 @@ TEST_F(ZipkinDriverTest, FlushSeveralSpans) { .WillOnce(Return(5000U)); Tracing::SpanPtr first_span = driver_->startSpan(request_headers_, operation_name_, start_time_); - first_span->finishSpan(); + Tracing::MockFinalizer finalizer; + + EXPECT_CALL(finalizer, finalize(_)); + first_span->finishSpan(finalizer); Tracing::SpanPtr second_span = driver_->startSpan(request_headers_, operation_name_, start_time_); - second_span->finishSpan(); + + EXPECT_CALL(finalizer, finalize(_)); + second_span->finishSpan(finalizer); Http::MessagePtr msg(new Http::ResponseMessageImpl( Http::HeaderMapPtr{new Http::TestHeaderMapImpl{{":status", "202"}}})); @@ -198,7 +203,10 @@ TEST_F(ZipkinDriverTest, FlushOneSpanReportFailure) { .WillOnce(Return(5000U)); Tracing::SpanPtr span = driver_->startSpan(request_headers_, operation_name_, start_time_); - span->finishSpan(); + Tracing::MockFinalizer finalizer; + + EXPECT_CALL(finalizer, finalize(_)); + span->finishSpan(finalizer); Http::MessagePtr msg(new Http::ResponseMessageImpl( Http::HeaderMapPtr{new Http::TestHeaderMapImpl{{":status", "404"}}})); @@ -222,7 +230,10 @@ TEST_F(ZipkinDriverTest, FlushSpansTimer) { .WillOnce(Return(5)); Tracing::SpanPtr span = driver_->startSpan(request_headers_, operation_name_, start_time_); - span->finishSpan(); + Tracing::MockFinalizer finalizer; + + EXPECT_CALL(finalizer, finalize(_)); + span->finishSpan(finalizer); // Timer should be re-enabled. EXPECT_CALL(*timer_, enableTimer(std::chrono::milliseconds(5000))); diff --git a/test/mocks/tracing/mocks.cc b/test/mocks/tracing/mocks.cc index 9bd988b30ae1..dc0fb763d0b0 100644 --- a/test/mocks/tracing/mocks.cc +++ b/test/mocks/tracing/mocks.cc @@ -12,6 +12,9 @@ namespace Tracing { MockSpan::MockSpan() {} MockSpan::~MockSpan() {} +MockFinalizer::MockFinalizer() {} +MockFinalizer::~MockFinalizer() {} + MockConfig::MockConfig() { ON_CALL(*this, operationName()).WillByDefault(Return(operation_name_)); ON_CALL(*this, requestHeadersForTags()).WillByDefault(ReturnRef(headers_)); diff --git a/test/mocks/tracing/mocks.h b/test/mocks/tracing/mocks.h index 82e36825e82f..d939d0fc336e 100644 --- a/test/mocks/tracing/mocks.h +++ b/test/mocks/tracing/mocks.h @@ -33,7 +33,7 @@ class MockSpan : public Span { ~MockSpan(); MOCK_METHOD2(setTag, void(const std::string& name, const std::string& value)); - MOCK_METHOD0(finishSpan, void()); + MOCK_METHOD1(finishSpan, void(SpanFinalizer& finalizer)); MOCK_METHOD1(injectContext, void(Http::HeaderMap& request_headers)); SpanPtr spawnChild(const std::string& name, SystemTime start_time) override { @@ -43,6 +43,14 @@ class MockSpan : public Span { MOCK_METHOD2(spawnChild_, Span*(const std::string& name, SystemTime start_time)); }; +class MockFinalizer : public SpanFinalizer { +public: + MockFinalizer(); + ~MockFinalizer(); + + MOCK_METHOD1(finalize, void(Span& span)); +}; + class MockHttpTracer : public HttpTracer { public: MockHttpTracer();