From 8e59030cd57c45567b950e538f517c0b6e0c1cd6 Mon Sep 17 00:00:00 2001 From: Mike Schore Date: Mon, 22 May 2017 17:17:29 -0700 Subject: [PATCH 01/12] tracing: adopt NullSpan and add finalizer --- include/envoy/tracing/http_tracer.h | 22 +++++++++--- source/common/http/conn_manager_impl.cc | 11 +++--- source/common/http/conn_manager_impl.h | 2 +- source/common/tracing/http_tracer_impl.cc | 42 +++++++++++------------ source/common/tracing/http_tracer_impl.h | 21 ++++++++++-- 5 files changed, 63 insertions(+), 35 deletions(-) diff --git a/include/envoy/tracing/http_tracer.h b/include/envoy/tracing/http_tracer.h index b55c8c4c492c..be507188c577 100644 --- a/include/envoy/tracing/http_tracer.h +++ b/include/envoy/tracing/http_tracer.h @@ -31,13 +31,15 @@ class Config { virtual const std::vector& requestHeadersForTags() const PURE; }; -/* - * Basic abstraction for span. - */ class Span; typedef std::unique_ptr SpanPtr; +class SpanFinalizer; + +/* + * Basic abstraction for span. + */ class Span { public: virtual ~Span() {} @@ -52,8 +54,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 may be provided to perform 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 @@ -70,6 +73,17 @@ class Span { virtual SpanPtr spawnChild(const std::string& name, SystemTime start_time) PURE; }; +/* + * Interface to perform context-specific tasks when completing a Span. + */ +class SpanFinalizer { + /** + * Finalize the Span. + * @param span the Span to be finalized + */ + virtual void finalize(Span& span) PURE; +} + /** * Tracing driver is responsible for span creation. */ diff --git a/source/common/http/conn_manager_impl.cc b/source/common/http/conn_manager_impl.cc index fce76e724983..46e7f8d3c570 100644 --- a/source/common/http/conn_manager_impl.cc +++ b/source/common/http/conn_manager_impl.cc @@ -335,13 +335,10 @@ 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 { + active_span_->finishSpan(DefaultIngressFinalizer(*request_headers_, request_info_, *this)); } 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 bab674c88067..6721990765aa 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 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..4aefbe2f8e12 100644 --- a/source/common/tracing/http_tracer_impl.cc +++ b/source/common/tracing/http_tracer_impl.cc @@ -116,44 +116,44 @@ 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) { +DefaultIngressFinalizer::DefaultIngressFinalizer(const Http::HeaderMap& request_headers, + const Http::AccessLog::RequestInfo& request_info, + const Config& tracing_config) + : request_headers_(request_headers), request_info_(request_info), + tracing_config_(tracing_config) {} + +void DefaultIngressFinalizer::finalizeSpan(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(), "-")); + 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("request_size", std::to_string(request_info.bytesReceived())); + 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()) { - active_span.setTag("guid:x-client-trace-id", - std::string(request_headers.ClientTraceId()->value().c_str())); + 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()); + span.setTag(header.get(), entry->value().c_str()); } } // 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)); + 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)); if (request_info.responseCode().valid() && Http::CodeUtility::is5xx(request_info.responseCode().value())) { - active_span.setTag("error", "true"); + span.setTag("error", "true"); } - - active_span.finishSpan(); } 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..9287502d27da 100644 --- a/source/common/tracing/http_tracer_impl.h +++ b/source/common/tracing/http_tracer_impl.h @@ -27,6 +27,23 @@ struct Decision { bool is_tracing; }; +/** + * Finalizer for Spans covering standard request ingress. + */ +class DefaultIngressFinalizer : SpanFinalizer { +public: + DefaultIngressFinalizer(const Http::HeaderMap& request_headers, + const Http::AccessLog::RequestInfo& request_info, + const Config& tracing_config); + + finalizeSpan(Span& span); + +private: + Http::HeaderMap& request_headers_; + Http::AccessLog::RequestInfo& request_info; + +} + class HttpTracerUtility { public: /** @@ -66,7 +83,7 @@ class HttpNullTracer : public HttpTracer { public: // Tracing::HttpTracer SpanPtr startSpan(const Config&, Http::HeaderMap&, const Http::AccessLog::RequestInfo&) override { - return nullptr; + return SpanPtr{new NullSpan()}; } }; @@ -87,7 +104,7 @@ class NullSpan : public Tracing::Span { public: // Tracing::Span void setTag(const std::string&, const std::string&) override {} - void finishSpan() override {} + void finishSpan(SpanFinalizer&) override {} void injectContext(Http::HeaderMap&) override {} SpanPtr spawnChild(const std::string&, SystemTime) override { return SpanPtr{new NullSpan()}; } }; From 3e83c33de166fbd45695cfa8a3c9307b93635222 Mon Sep 17 00:00:00 2001 From: Mike Schore Date: Thu, 25 May 2017 10:29:28 -0700 Subject: [PATCH 02/12] checkpoint: compiles --- include/envoy/tracing/http_tracer.h | 5 +- source/common/http/conn_manager_impl.cc | 3 +- source/common/http/conn_manager_impl.h | 2 +- source/common/tracing/http_tracer_impl.cc | 38 ++++++------- source/common/tracing/http_tracer_impl.h | 57 ++++++++++--------- .../common/tracing/lightstep_tracer_impl.cc | 5 +- source/common/tracing/lightstep_tracer_impl.h | 2 +- .../tracing/zipkin/zipkin_tracer_impl.cc | 5 +- .../tracing/zipkin/zipkin_tracer_impl.h | 4 +- test/mocks/tracing/mocks.h | 2 +- 10 files changed, 70 insertions(+), 53 deletions(-) diff --git a/include/envoy/tracing/http_tracer.h b/include/envoy/tracing/http_tracer.h index be507188c577..92722547dc71 100644 --- a/include/envoy/tracing/http_tracer.h +++ b/include/envoy/tracing/http_tracer.h @@ -77,12 +77,15 @@ class Span { * 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; -} +}; /** * Tracing driver is responsible for span creation. diff --git a/source/common/http/conn_manager_impl.cc b/source/common/http/conn_manager_impl.cc index 46e7f8d3c570..d6e5b1d49c9d 100644 --- a/source/common/http/conn_manager_impl.cc +++ b/source/common/http/conn_manager_impl.cc @@ -338,7 +338,8 @@ ConnectionManagerImpl::ActiveStream::~ActiveStream() { if (request_info_.healthCheck()) { connection_manager_.config_.tracingStats().health_check_.inc(); } else { - active_span_->finishSpan(DefaultIngressFinalizer(*request_headers_, request_info_, *this)); + Tracing::DefaultIngressFinalizer&& finalizer{*request_headers_, 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 6721990765aa..2139f284d954 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_ = {new NullSpan()}; + 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 4aefbe2f8e12..cb4d5469ef52 100644 --- a/source/common/tracing/http_tracer_impl.cc +++ b/source/common/tracing/http_tracer_impl.cc @@ -116,42 +116,42 @@ Decision HttpTracerUtility::isTracing(const Http::AccessLog::RequestInfo& reques NOT_REACHED; } -DefaultIngressFinalizer::DefaultIngressFinalizer(const Http::HeaderMap& request_headers, - const Http::AccessLog::RequestInfo& request_info, - const Config& tracing_config) +DefaultIngressFinalizer::DefaultIngressFinalizer(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 DefaultIngressFinalizer::finalizeSpan(Span& span) { +void DefaultIngressFinalizer::finalize(Span& span) { // Pre response data. - 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("request_size", std::to_string(request_info.bytesReceived())); - span.setTag("host_header", valueOrDefault(request_headers.Host(), "-")); + 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("request_size", std::to_string(request_info_.bytesReceived())); + span.setTag("host_header", valueOrDefault(request_headers_.Host(), "-")); span.setTag("downstream_cluster", - valueOrDefault(request_headers.EnvoyDownstreamServiceCluster(), "-")); - span.setTag("user_agent", valueOrDefault(request_headers.UserAgent(), "-")); + valueOrDefault(request_headers_.EnvoyDownstreamServiceCluster(), "-")); + span.setTag("user_agent", valueOrDefault(request_headers_.UserAgent(), "-")); - if (request_headers.ClientTraceId()) { + if (request_headers_.ClientTraceId()) { span.setTag("guid:x-client-trace-id", - std::string(request_headers.ClientTraceId()->value().c_str())); + 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); + 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()); } } // Post response data. - 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)); + 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_)); - if (request_info.responseCode().valid() && - Http::CodeUtility::is5xx(request_info.responseCode().value())) { + if (request_info_.responseCode().valid() && + Http::CodeUtility::is5xx(request_info_.responseCode().value())) { span.setTag("error", "true"); } } diff --git a/source/common/tracing/http_tracer_impl.h b/source/common/tracing/http_tracer_impl.h index 9287502d27da..44f67af36c18 100644 --- a/source/common/tracing/http_tracer_impl.h +++ b/source/common/tracing/http_tracer_impl.h @@ -27,23 +27,6 @@ struct Decision { bool is_tracing; }; -/** - * Finalizer for Spans covering standard request ingress. - */ -class DefaultIngressFinalizer : SpanFinalizer { -public: - DefaultIngressFinalizer(const Http::HeaderMap& request_headers, - const Http::AccessLog::RequestInfo& request_info, - const Config& tracing_config); - - finalizeSpan(Span& span); - -private: - Http::HeaderMap& request_headers_; - Http::AccessLog::RequestInfo& request_info; - -} - class HttpTracerUtility { public: /** @@ -79,6 +62,37 @@ 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 DefaultIngressFinalizer : public SpanFinalizer { +public: + DefaultIngressFinalizer(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 @@ -100,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(SpanFinalizer&) 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/mocks/tracing/mocks.h b/test/mocks/tracing/mocks.h index 82e36825e82f..62fb223a52a0 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 { From 4ea0ef7a3b57aeba90be1561bf97edfac1f2fc33 Mon Sep 17 00:00:00 2001 From: Mike Schore Date: Tue, 30 May 2017 10:03:43 -0700 Subject: [PATCH 03/12] checkpoint: tests pass --- test/common/http/conn_manager_impl_test.cc | 6 ++-- test/common/tracing/http_tracer_impl_test.cc | 30 +++++++++++++------ .../tracing/lightstep_tracer_impl_test.cc | 16 +++++++--- .../tracing/zipkin/zipkin_tracer_impl_test.cc | 16 +++++++--- test/mocks/tracing/mocks.h | 8 +++++ 5 files changed, 57 insertions(+), 19 deletions(-) diff --git a/test/common/http/conn_manager_impl_test.cc b/test/common/http/conn_manager_impl_test.cc index e32be7959cb7..d33241bbdad2 100644 --- a/test/common/http/conn_manager_impl_test.cc +++ b/test/common/http/conn_manager_impl_test.cc @@ -290,7 +290,9 @@ TEST_F(HttpConnectionManagerImplTest, StartAndFinishSpanNormalFlow) { return span; })); - EXPECT_CALL(*span, finishSpan()); + EXPECT_CALL(*span, finishSpan(_)) + .WillOnce( + Invoke([&](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")); @@ -419,7 +421,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..42e95dbd0dc5 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(DefaultIngressFinalizer, OriginalAndLongPath) { const std::string path(300, 'a'); const std::string expected_path(128, 'a'); std::unique_ptr> span(new NiceMock()); @@ -253,10 +253,14 @@ 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); + + DefaultIngressFinalizer&& finalizer{request_headers, request_info, config}; + finalizer.finalize(*span); + // span->finishSpan(finalizer); + // HttpTracerUtility::finalizeSpan(*span, request_headers, request_info, config); } -TEST(HttpTracerUtilityTest, SpanOptionalHeaders) { +TEST(DefaultIngressFinalizer, SpanOptionalHeaders) { std::unique_ptr> span(new NiceMock()); Http::TestHeaderMapImpl request_headers{ @@ -284,12 +288,16 @@ TEST(HttpTracerUtilityTest, SpanOptionalHeaders) { EXPECT_CALL(*span, setTag("response_size", "100")); EXPECT_CALL(*span, setTag("response_flags", "-")); - EXPECT_CALL(*span, finishSpan()); + // EXPECT_CALL(*span, finishSpan(_)); NiceMock config; - HttpTracerUtility::finalizeSpan(*span, request_headers, request_info, config); + + DefaultIngressFinalizer&& finalizer{request_headers, request_info, config}; + finalizer.finalize(*span); + // span->finishSpan(finalizer); + // HttpTracerUtility::finalizeSpan(*span, request_headers, request_info, config); } -TEST(HttpTracerUtilityTest, SpanPopulatedFailureResponse) { +TEST(DefaultIngressFinalizer, SpanPopulatedFailureResponse) { std::unique_ptr> span(new NiceMock()); Http::TestHeaderMapImpl request_headers{ {"x-request-id", "id"}, {":path", "/test"}, {":method", "GET"}}; @@ -337,8 +345,11 @@ 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); + DefaultIngressFinalizer&& finalizer{request_headers, request_info, config}; + finalizer.finalize(*span); + // span->finishSpan(finalizer); + // EXPECT_CALL(*span, finishSpan(_)); + // HttpTracerUtility::finalizeSpan(*span, request_headers, request_info, config); } TEST(HttpTracerUtilityTest, operationTypeToString) { @@ -352,7 +363,8 @@ TEST(HttpNullTracerTest, BasicFunctionality) { Http::AccessLog::MockRequestInfo request_info; Http::TestHeaderMapImpl request_headers; - EXPECT_EQ(nullptr, null_tracer.startSpan(config, request_headers, request_info)); + EXPECT_TRUE(null_tracer.startSpan(config, request_headers, request_info) != nullptr); + // EXPECT_THAT(null_tracer.startSpan(config, request_headers, request_info), A()); } 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..c5159b72a2f6 100644 --- a/test/common/tracing/lightstep_tracer_impl_test.cc +++ b/test/common/tracing/lightstep_tracer_impl_test.cc @@ -166,10 +166,16 @@ TEST_F(LightStepDriverTest, FlushSeveralSpans) { .WillOnce(Return(5000U)); SpanPtr first_span = driver_->startSpan(request_headers_, operation_name_, start_time_); - first_span->finishSpan(); + // MockFinalizer&& finalizer{}; + NullFinalizer&& finalizer{}; + + // EXPECT_CALL(finalizer, finalize(*first_span)); + first_span->finishSpan(finalizer); SpanPtr second_span = driver_->startSpan(request_headers_, operation_name_, start_time_); - second_span->finishSpan(); + + // EXPECT_CALL(finalizer, finalize(*second_span)); + second_span->finishSpan(finalizer); Http::MessagePtr msg(new Http::ResponseMessageImpl( Http::HeaderMapPtr{new Http::TestHeaderMapImpl{{":status", "200"}}})); @@ -203,7 +209,8 @@ TEST_F(LightStepDriverTest, FlushSpansTimer) { .WillOnce(Return(5)); SpanPtr span = driver_->startSpan(request_headers_, operation_name_, start_time_); - span->finishSpan(); + NullFinalizer&& finalizer{}; + span->finishSpan(finalizer); // Timer should be re-enabled. EXPECT_CALL(*timer_, enableTimer(std::chrono::milliseconds(1000))); @@ -244,7 +251,8 @@ TEST_F(LightStepDriverTest, FlushOneSpanGrpcFailure) { .WillOnce(Return(5000U)); SpanPtr span = driver_->startSpan(request_headers_, operation_name_, start_time_); - span->finishSpan(); + NullFinalizer&& finalizer{}; + 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..9a0d3d4ac08a 100644 --- a/test/common/tracing/zipkin/zipkin_tracer_impl_test.cc +++ b/test/common/tracing/zipkin/zipkin_tracer_impl_test.cc @@ -153,10 +153,16 @@ 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{}; + Tracing::NullFinalizer&& finalizer{}; + + // EXPECT_CALL(finalizer, finalize(*first_span)); + 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)); + second_span->finishSpan(finalizer); Http::MessagePtr msg(new Http::ResponseMessageImpl( Http::HeaderMapPtr{new Http::TestHeaderMapImpl{{":status", "202"}}})); @@ -198,7 +204,8 @@ TEST_F(ZipkinDriverTest, FlushOneSpanReportFailure) { .WillOnce(Return(5000U)); Tracing::SpanPtr span = driver_->startSpan(request_headers_, operation_name_, start_time_); - span->finishSpan(); + Tracing::NullFinalizer&& finalizer{}; + span->finishSpan(finalizer); Http::MessagePtr msg(new Http::ResponseMessageImpl( Http::HeaderMapPtr{new Http::TestHeaderMapImpl{{":status", "404"}}})); @@ -222,7 +229,8 @@ TEST_F(ZipkinDriverTest, FlushSpansTimer) { .WillOnce(Return(5)); Tracing::SpanPtr span = driver_->startSpan(request_headers_, operation_name_, start_time_); - span->finishSpan(); + Tracing::NullFinalizer&& finalizer{}; + span->finishSpan(finalizer); // Timer should be re-enabled. EXPECT_CALL(*timer_, enableTimer(std::chrono::milliseconds(5000))); diff --git a/test/mocks/tracing/mocks.h b/test/mocks/tracing/mocks.h index 62fb223a52a0..d939d0fc336e 100644 --- a/test/mocks/tracing/mocks.h +++ b/test/mocks/tracing/mocks.h @@ -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(); From 53ec11b32b9c0ad1d46558cb24bb4cd36a4cb895 Mon Sep 17 00:00:00 2001 From: Mike Schore Date: Tue, 30 May 2017 12:03:52 -0700 Subject: [PATCH 04/12] minor fix --- source/common/http/conn_manager_impl.cc | 2 +- test/common/tracing/http_tracer_impl_test.cc | 6 +++--- test/common/tracing/lightstep_tracer_impl_test.cc | 8 ++++---- test/common/tracing/zipkin/zipkin_tracer_impl_test.cc | 8 ++++---- 4 files changed, 12 insertions(+), 12 deletions(-) diff --git a/source/common/http/conn_manager_impl.cc b/source/common/http/conn_manager_impl.cc index d6e5b1d49c9d..7ee12ee10335 100644 --- a/source/common/http/conn_manager_impl.cc +++ b/source/common/http/conn_manager_impl.cc @@ -338,7 +338,7 @@ ConnectionManagerImpl::ActiveStream::~ActiveStream() { if (request_info_.healthCheck()) { connection_manager_.config_.tracingStats().health_check_.inc(); } else { - Tracing::DefaultIngressFinalizer&& finalizer{*request_headers_, request_info_, *this}; + Tracing::DefaultIngressFinalizer finalizer{*request_headers_, request_info_, *this}; active_span_->finishSpan(finalizer); } diff --git a/test/common/tracing/http_tracer_impl_test.cc b/test/common/tracing/http_tracer_impl_test.cc index 42e95dbd0dc5..598dde241f2f 100644 --- a/test/common/tracing/http_tracer_impl_test.cc +++ b/test/common/tracing/http_tracer_impl_test.cc @@ -254,7 +254,7 @@ TEST(DefaultIngressFinalizer, OriginalAndLongPath) { EXPECT_CALL(*span, setTag("request_line", "GET " + expected_path + " HTTP/2")); NiceMock config; - DefaultIngressFinalizer&& finalizer{request_headers, request_info, config}; + DefaultIngressFinalizer finalizer{request_headers, request_info, config}; finalizer.finalize(*span); // span->finishSpan(finalizer); // HttpTracerUtility::finalizeSpan(*span, request_headers, request_info, config); @@ -291,7 +291,7 @@ TEST(DefaultIngressFinalizer, SpanOptionalHeaders) { // EXPECT_CALL(*span, finishSpan(_)); NiceMock config; - DefaultIngressFinalizer&& finalizer{request_headers, request_info, config}; + DefaultIngressFinalizer finalizer{request_headers, request_info, config}; finalizer.finalize(*span); // span->finishSpan(finalizer); // HttpTracerUtility::finalizeSpan(*span, request_headers, request_info, config); @@ -345,7 +345,7 @@ TEST(DefaultIngressFinalizer, SpanPopulatedFailureResponse) { EXPECT_CALL(*span, setTag("response_size", "100")); EXPECT_CALL(*span, setTag("response_flags", "UT")); - DefaultIngressFinalizer&& finalizer{request_headers, request_info, config}; + DefaultIngressFinalizer finalizer{request_headers, request_info, config}; finalizer.finalize(*span); // span->finishSpan(finalizer); // EXPECT_CALL(*span, finishSpan(_)); diff --git a/test/common/tracing/lightstep_tracer_impl_test.cc b/test/common/tracing/lightstep_tracer_impl_test.cc index c5159b72a2f6..02607a50bf08 100644 --- a/test/common/tracing/lightstep_tracer_impl_test.cc +++ b/test/common/tracing/lightstep_tracer_impl_test.cc @@ -166,8 +166,8 @@ TEST_F(LightStepDriverTest, FlushSeveralSpans) { .WillOnce(Return(5000U)); SpanPtr first_span = driver_->startSpan(request_headers_, operation_name_, start_time_); - // MockFinalizer&& finalizer{}; - NullFinalizer&& finalizer{}; + // MockFinalizer finalizer{}; + NullFinalizer finalizer{}; // EXPECT_CALL(finalizer, finalize(*first_span)); first_span->finishSpan(finalizer); @@ -209,7 +209,7 @@ TEST_F(LightStepDriverTest, FlushSpansTimer) { .WillOnce(Return(5)); SpanPtr span = driver_->startSpan(request_headers_, operation_name_, start_time_); - NullFinalizer&& finalizer{}; + NullFinalizer finalizer{}; span->finishSpan(finalizer); // Timer should be re-enabled. @@ -251,7 +251,7 @@ TEST_F(LightStepDriverTest, FlushOneSpanGrpcFailure) { .WillOnce(Return(5000U)); SpanPtr span = driver_->startSpan(request_headers_, operation_name_, start_time_); - NullFinalizer&& finalizer{}; + NullFinalizer finalizer{}; span->finishSpan(finalizer); Http::MessagePtr msg(new Http::ResponseMessageImpl( diff --git a/test/common/tracing/zipkin/zipkin_tracer_impl_test.cc b/test/common/tracing/zipkin/zipkin_tracer_impl_test.cc index 9a0d3d4ac08a..cfde50b829b9 100644 --- a/test/common/tracing/zipkin/zipkin_tracer_impl_test.cc +++ b/test/common/tracing/zipkin/zipkin_tracer_impl_test.cc @@ -153,8 +153,8 @@ TEST_F(ZipkinDriverTest, FlushSeveralSpans) { .WillOnce(Return(5000U)); Tracing::SpanPtr first_span = driver_->startSpan(request_headers_, operation_name_, start_time_); - // Tracing::MockFinalizer&& finalizer{}; - Tracing::NullFinalizer&& finalizer{}; + // Tracing::MockFinalizer finalizer{}; + Tracing::NullFinalizer finalizer{}; // EXPECT_CALL(finalizer, finalize(*first_span)); first_span->finishSpan(finalizer); @@ -204,7 +204,7 @@ TEST_F(ZipkinDriverTest, FlushOneSpanReportFailure) { .WillOnce(Return(5000U)); Tracing::SpanPtr span = driver_->startSpan(request_headers_, operation_name_, start_time_); - Tracing::NullFinalizer&& finalizer{}; + Tracing::NullFinalizer finalizer{}; span->finishSpan(finalizer); Http::MessagePtr msg(new Http::ResponseMessageImpl( @@ -229,7 +229,7 @@ TEST_F(ZipkinDriverTest, FlushSpansTimer) { .WillOnce(Return(5)); Tracing::SpanPtr span = driver_->startSpan(request_headers_, operation_name_, start_time_); - Tracing::NullFinalizer&& finalizer{}; + Tracing::NullFinalizer finalizer{}; span->finishSpan(finalizer); // Timer should be re-enabled. From 1ffcde9d5df2abe8bf2726fcf4eb3dc0890d65b1 Mon Sep 17 00:00:00 2001 From: Mike Schore Date: Tue, 30 May 2017 15:22:13 -0700 Subject: [PATCH 05/12] updates for comments --- include/envoy/tracing/http_tracer.h | 30 +++++++++---------- source/common/http/conn_manager_impl.cc | 2 +- source/common/tracing/http_tracer_impl.cc | 8 ++--- source/common/tracing/http_tracer_impl.h | 6 ++-- test/common/tracing/http_tracer_impl_test.cc | 19 ++++-------- .../tracing/lightstep_tracer_impl_test.cc | 8 ++--- .../tracing/zipkin/zipkin_tracer_impl_test.cc | 8 ++--- 7 files changed, 36 insertions(+), 45 deletions(-) diff --git a/include/envoy/tracing/http_tracer.h b/include/envoy/tracing/http_tracer.h index 92722547dc71..109debdf5145 100644 --- a/include/envoy/tracing/http_tracer.h +++ b/include/envoy/tracing/http_tracer.h @@ -35,7 +35,19 @@ class Span; typedef std::unique_ptr SpanPtr; -class SpanFinalizer; +/* + * 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. @@ -54,7 +66,7 @@ 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 may be provided to perform context-specific work to complete the Span + * @param finalizer callback for context-specific work to complete the Span */ virtual void finishSpan(SpanFinalizer& finalizer) PURE; @@ -73,20 +85,6 @@ class Span { virtual SpanPtr spawnChild(const std::string& name, SystemTime start_time) PURE; }; -/* - * 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; -}; - /** * Tracing driver is responsible for span creation. */ diff --git a/source/common/http/conn_manager_impl.cc b/source/common/http/conn_manager_impl.cc index 7ee12ee10335..93bda507c179 100644 --- a/source/common/http/conn_manager_impl.cc +++ b/source/common/http/conn_manager_impl.cc @@ -338,7 +338,7 @@ ConnectionManagerImpl::ActiveStream::~ActiveStream() { if (request_info_.healthCheck()) { connection_manager_.config_.tracingStats().health_check_.inc(); } else { - Tracing::DefaultIngressFinalizer finalizer{*request_headers_, request_info_, *this}; + Tracing::HttpConnManFinalizerImpl finalizer{*request_headers_, request_info_, *this}; active_span_->finishSpan(finalizer); } diff --git a/source/common/tracing/http_tracer_impl.cc b/source/common/tracing/http_tracer_impl.cc index cb4d5469ef52..b050db77ae3d 100644 --- a/source/common/tracing/http_tracer_impl.cc +++ b/source/common/tracing/http_tracer_impl.cc @@ -116,13 +116,13 @@ Decision HttpTracerUtility::isTracing(const Http::AccessLog::RequestInfo& reques NOT_REACHED; } -DefaultIngressFinalizer::DefaultIngressFinalizer(Http::HeaderMap& request_headers, - Http::AccessLog::RequestInfo& request_info, - Config& tracing_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 DefaultIngressFinalizer::finalize(Span& span) { +void HttpConnManFinalizerImpl::finalize(Span& span) { // Pre response data. span.setTag("guid:x-request-id", std::string(request_headers_.RequestId()->value().c_str())); span.setTag("request_line", buildRequestLine(request_headers_, request_info_)); diff --git a/source/common/tracing/http_tracer_impl.h b/source/common/tracing/http_tracer_impl.h index 44f67af36c18..28584248fe3d 100644 --- a/source/common/tracing/http_tracer_impl.h +++ b/source/common/tracing/http_tracer_impl.h @@ -80,10 +80,10 @@ class NullFinalizer : public SpanFinalizer { /** * Finalizer for Spans covering standard request ingress. */ -class DefaultIngressFinalizer : public SpanFinalizer { +class HttpConnManFinalizerImpl : public SpanFinalizer { public: - DefaultIngressFinalizer(Http::HeaderMap& request_headers, - Http::AccessLog::RequestInfo& request_info, Config& tracing_config); + HttpConnManFinalizerImpl(Http::HeaderMap& request_headers, + Http::AccessLog::RequestInfo& request_info, Config& tracing_config); void finalize(Span& span) override; diff --git a/test/common/tracing/http_tracer_impl_test.cc b/test/common/tracing/http_tracer_impl_test.cc index 598dde241f2f..cf466b024d09 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(DefaultIngressFinalizer, OriginalAndLongPath) { +TEST(HttpConnManFinalizerImpl, OriginalAndLongPath) { const std::string path(300, 'a'); const std::string expected_path(128, 'a'); std::unique_ptr> span(new NiceMock()); @@ -254,13 +254,11 @@ TEST(DefaultIngressFinalizer, OriginalAndLongPath) { EXPECT_CALL(*span, setTag("request_line", "GET " + expected_path + " HTTP/2")); NiceMock config; - DefaultIngressFinalizer finalizer{request_headers, request_info, config}; + HttpConnManFinalizerImpl finalizer{request_headers, request_info, config}; finalizer.finalize(*span); - // span->finishSpan(finalizer); - // HttpTracerUtility::finalizeSpan(*span, request_headers, request_info, config); } -TEST(DefaultIngressFinalizer, SpanOptionalHeaders) { +TEST(HttpConnManFinalizerImpl, SpanOptionalHeaders) { std::unique_ptr> span(new NiceMock()); Http::TestHeaderMapImpl request_headers{ @@ -291,13 +289,11 @@ TEST(DefaultIngressFinalizer, SpanOptionalHeaders) { // EXPECT_CALL(*span, finishSpan(_)); NiceMock config; - DefaultIngressFinalizer finalizer{request_headers, request_info, config}; + HttpConnManFinalizerImpl finalizer{request_headers, request_info, config}; finalizer.finalize(*span); - // span->finishSpan(finalizer); - // HttpTracerUtility::finalizeSpan(*span, request_headers, request_info, config); } -TEST(DefaultIngressFinalizer, SpanPopulatedFailureResponse) { +TEST(HttpConnManFinalizerImpl, SpanPopulatedFailureResponse) { std::unique_ptr> span(new NiceMock()); Http::TestHeaderMapImpl request_headers{ {"x-request-id", "id"}, {":path", "/test"}, {":method", "GET"}}; @@ -345,11 +341,8 @@ TEST(DefaultIngressFinalizer, SpanPopulatedFailureResponse) { EXPECT_CALL(*span, setTag("response_size", "100")); EXPECT_CALL(*span, setTag("response_flags", "UT")); - DefaultIngressFinalizer finalizer{request_headers, request_info, config}; + HttpConnManFinalizerImpl finalizer{request_headers, request_info, config}; finalizer.finalize(*span); - // span->finishSpan(finalizer); - // EXPECT_CALL(*span, finishSpan(_)); - // HttpTracerUtility::finalizeSpan(*span, request_headers, request_info, config); } TEST(HttpTracerUtilityTest, operationTypeToString) { diff --git a/test/common/tracing/lightstep_tracer_impl_test.cc b/test/common/tracing/lightstep_tracer_impl_test.cc index 02607a50bf08..62754bbf11f9 100644 --- a/test/common/tracing/lightstep_tracer_impl_test.cc +++ b/test/common/tracing/lightstep_tracer_impl_test.cc @@ -166,8 +166,8 @@ TEST_F(LightStepDriverTest, FlushSeveralSpans) { .WillOnce(Return(5000U)); SpanPtr first_span = driver_->startSpan(request_headers_, operation_name_, start_time_); - // MockFinalizer finalizer{}; - NullFinalizer finalizer{}; + // MockFinalizer finalizer; + NullFinalizer finalizer; // EXPECT_CALL(finalizer, finalize(*first_span)); first_span->finishSpan(finalizer); @@ -209,7 +209,7 @@ TEST_F(LightStepDriverTest, FlushSpansTimer) { .WillOnce(Return(5)); SpanPtr span = driver_->startSpan(request_headers_, operation_name_, start_time_); - NullFinalizer finalizer{}; + NullFinalizer finalizer; span->finishSpan(finalizer); // Timer should be re-enabled. @@ -251,7 +251,7 @@ TEST_F(LightStepDriverTest, FlushOneSpanGrpcFailure) { .WillOnce(Return(5000U)); SpanPtr span = driver_->startSpan(request_headers_, operation_name_, start_time_); - NullFinalizer finalizer{}; + NullFinalizer finalizer; span->finishSpan(finalizer); Http::MessagePtr msg(new Http::ResponseMessageImpl( diff --git a/test/common/tracing/zipkin/zipkin_tracer_impl_test.cc b/test/common/tracing/zipkin/zipkin_tracer_impl_test.cc index cfde50b829b9..13821ff93556 100644 --- a/test/common/tracing/zipkin/zipkin_tracer_impl_test.cc +++ b/test/common/tracing/zipkin/zipkin_tracer_impl_test.cc @@ -153,8 +153,8 @@ TEST_F(ZipkinDriverTest, FlushSeveralSpans) { .WillOnce(Return(5000U)); Tracing::SpanPtr first_span = driver_->startSpan(request_headers_, operation_name_, start_time_); - // Tracing::MockFinalizer finalizer{}; - Tracing::NullFinalizer finalizer{}; + // Tracing::MockFinalizer finalizer; + Tracing::NullFinalizer finalizer; // EXPECT_CALL(finalizer, finalize(*first_span)); first_span->finishSpan(finalizer); @@ -204,7 +204,7 @@ TEST_F(ZipkinDriverTest, FlushOneSpanReportFailure) { .WillOnce(Return(5000U)); Tracing::SpanPtr span = driver_->startSpan(request_headers_, operation_name_, start_time_); - Tracing::NullFinalizer finalizer{}; + Tracing::NullFinalizer finalizer; span->finishSpan(finalizer); Http::MessagePtr msg(new Http::ResponseMessageImpl( @@ -229,7 +229,7 @@ TEST_F(ZipkinDriverTest, FlushSpansTimer) { .WillOnce(Return(5)); Tracing::SpanPtr span = driver_->startSpan(request_headers_, operation_name_, start_time_); - Tracing::NullFinalizer finalizer{}; + Tracing::NullFinalizer finalizer; span->finishSpan(finalizer); // Timer should be re-enabled. From 0956877d438526ab24756b8ea0b60999fbb62dc1 Mon Sep 17 00:00:00 2001 From: Mike Schore Date: Tue, 30 May 2017 17:24:07 -0700 Subject: [PATCH 06/12] improve test --- test/common/tracing/http_tracer_impl_test.cc | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/test/common/tracing/http_tracer_impl_test.cc b/test/common/tracing/http_tracer_impl_test.cc index cf466b024d09..69a22d3bfde7 100644 --- a/test/common/tracing/http_tracer_impl_test.cc +++ b/test/common/tracing/http_tracer_impl_test.cc @@ -356,8 +356,8 @@ TEST(HttpNullTracerTest, BasicFunctionality) { Http::AccessLog::MockRequestInfo request_info; Http::TestHeaderMapImpl request_headers; - EXPECT_TRUE(null_tracer.startSpan(config, request_headers, request_info) != nullptr); - // EXPECT_THAT(null_tracer.startSpan(config, request_headers, request_info), A()); + SpanPtr span_ptr = null_tracer.startSpan(config, request_headers, request_info); + EXPECT_TRUE(dynamic_cast(span_ptr.get()) != nullptr); } class HttpTracerImplTest : public Test { From 8fc3d01d2d24ed1c56732dc64db0bcd24bbeda16 Mon Sep 17 00:00:00 2001 From: Mike Schore Date: Tue, 30 May 2017 17:29:47 -0700 Subject: [PATCH 07/12] delete stray comment --- test/common/tracing/http_tracer_impl_test.cc | 1 - 1 file changed, 1 deletion(-) diff --git a/test/common/tracing/http_tracer_impl_test.cc b/test/common/tracing/http_tracer_impl_test.cc index 69a22d3bfde7..6d2a1daf1ed3 100644 --- a/test/common/tracing/http_tracer_impl_test.cc +++ b/test/common/tracing/http_tracer_impl_test.cc @@ -286,7 +286,6 @@ TEST(HttpConnManFinalizerImpl, SpanOptionalHeaders) { EXPECT_CALL(*span, setTag("response_size", "100")); EXPECT_CALL(*span, setTag("response_flags", "-")); - // EXPECT_CALL(*span, finishSpan(_)); NiceMock config; HttpConnManFinalizerImpl finalizer{request_headers, request_info, config}; From a67ef7cd429ed2a6d1aa3ea88a43cbe263f97a94 Mon Sep 17 00:00:00 2001 From: Mike Schore Date: Thu, 1 Jun 2017 10:49:16 -0700 Subject: [PATCH 08/12] update tests --- test/common/tracing/lightstep_tracer_impl_test.cc | 15 +++++++++------ .../tracing/zipkin/zipkin_tracer_impl_test.cc | 15 +++++++++------ test/mocks/tracing/mocks.cc | 3 +++ 3 files changed, 21 insertions(+), 12 deletions(-) diff --git a/test/common/tracing/lightstep_tracer_impl_test.cc b/test/common/tracing/lightstep_tracer_impl_test.cc index 62754bbf11f9..2779c5ca7982 100644 --- a/test/common/tracing/lightstep_tracer_impl_test.cc +++ b/test/common/tracing/lightstep_tracer_impl_test.cc @@ -166,15 +166,14 @@ TEST_F(LightStepDriverTest, FlushSeveralSpans) { .WillOnce(Return(5000U)); SpanPtr first_span = driver_->startSpan(request_headers_, operation_name_, start_time_); - // MockFinalizer finalizer; - NullFinalizer finalizer; + Tracing::MockFinalizer finalizer; - // EXPECT_CALL(finalizer, finalize(*first_span)); + EXPECT_CALL(finalizer, finalize(_)); first_span->finishSpan(finalizer); SpanPtr second_span = driver_->startSpan(request_headers_, operation_name_, start_time_); - // EXPECT_CALL(finalizer, finalize(*second_span)); + EXPECT_CALL(finalizer, finalize(_)); second_span->finishSpan(finalizer); Http::MessagePtr msg(new Http::ResponseMessageImpl( @@ -209,7 +208,9 @@ TEST_F(LightStepDriverTest, FlushSpansTimer) { .WillOnce(Return(5)); SpanPtr span = driver_->startSpan(request_headers_, operation_name_, start_time_); - NullFinalizer finalizer; + Tracing::MockFinalizer finalizer; + + EXPECT_CALL(finalizer, finalize(_)); span->finishSpan(finalizer); // Timer should be re-enabled. @@ -251,7 +252,9 @@ TEST_F(LightStepDriverTest, FlushOneSpanGrpcFailure) { .WillOnce(Return(5000U)); SpanPtr span = driver_->startSpan(request_headers_, operation_name_, start_time_); - NullFinalizer finalizer; + Tracing::MockFinalizer finalizer; + + EXPECT_CALL(finalizer, finalize(_)); span->finishSpan(finalizer); Http::MessagePtr msg(new Http::ResponseMessageImpl( diff --git a/test/common/tracing/zipkin/zipkin_tracer_impl_test.cc b/test/common/tracing/zipkin/zipkin_tracer_impl_test.cc index 13821ff93556..a0969db26c1d 100644 --- a/test/common/tracing/zipkin/zipkin_tracer_impl_test.cc +++ b/test/common/tracing/zipkin/zipkin_tracer_impl_test.cc @@ -153,15 +153,14 @@ TEST_F(ZipkinDriverTest, FlushSeveralSpans) { .WillOnce(Return(5000U)); Tracing::SpanPtr first_span = driver_->startSpan(request_headers_, operation_name_, start_time_); - // Tracing::MockFinalizer finalizer; - Tracing::NullFinalizer finalizer; + Tracing::MockFinalizer finalizer; - // EXPECT_CALL(finalizer, finalize(*first_span)); + EXPECT_CALL(finalizer, finalize(_)); first_span->finishSpan(finalizer); Tracing::SpanPtr second_span = driver_->startSpan(request_headers_, operation_name_, start_time_); - // EXPECT_CALL(finalizer, finalize(*second_span)); + EXPECT_CALL(finalizer, finalize(_)); second_span->finishSpan(finalizer); Http::MessagePtr msg(new Http::ResponseMessageImpl( @@ -204,7 +203,9 @@ TEST_F(ZipkinDriverTest, FlushOneSpanReportFailure) { .WillOnce(Return(5000U)); Tracing::SpanPtr span = driver_->startSpan(request_headers_, operation_name_, start_time_); - Tracing::NullFinalizer finalizer; + Tracing::MockFinalizer finalizer; + + EXPECT_CALL(finalizer, finalize(_)); span->finishSpan(finalizer); Http::MessagePtr msg(new Http::ResponseMessageImpl( @@ -229,7 +230,9 @@ TEST_F(ZipkinDriverTest, FlushSpansTimer) { .WillOnce(Return(5)); Tracing::SpanPtr span = driver_->startSpan(request_headers_, operation_name_, start_time_); - Tracing::NullFinalizer finalizer; + Tracing::MockFinalizer finalizer; + + EXPECT_CALL(finalizer, finalize(_)); span->finishSpan(finalizer); // Timer should be re-enabled. 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_)); From 469f3c3ee6be09f18629bdfe12c99dbdcbbf60f4 Mon Sep 17 00:00:00 2001 From: Mike Schore Date: Fri, 2 Jun 2017 13:36:36 -0700 Subject: [PATCH 09/12] asan and tsan failurse --- test/common/http/conn_manager_impl_test.cc | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/test/common/http/conn_manager_impl_test.cc b/test/common/http/conn_manager_impl_test.cc index d33241bbdad2..179ae473ca0b 100644 --- a/test/common/http/conn_manager_impl_test.cc +++ b/test/common/http/conn_manager_impl_test.cc @@ -332,6 +332,10 @@ TEST_F(HttpConnectionManagerImplTest, StartAndFinishSpanNormalFlow) { EXPECT_EQ(1UL, tracing_stats_.service_forced_.value()); EXPECT_EQ(0UL, tracing_stats_.random_sampling_.value()); + + // Uncomment either of these lines and the test fails + // EXPECT_TRUE(testing::Mock::VerifyAndClearExpectations(span)); + // delete span; } TEST_F(HttpConnectionManagerImplTest, TestAccessLog) { From a70d244328e37e62984ef8296b3bbc1563274518 Mon Sep 17 00:00:00 2001 From: Roman Dzhabarov Date: Thu, 8 Jun 2017 12:38:16 -0700 Subject: [PATCH 10/12] remove comment. --- test/common/http/conn_manager_impl_test.cc | 4 ---- 1 file changed, 4 deletions(-) diff --git a/test/common/http/conn_manager_impl_test.cc b/test/common/http/conn_manager_impl_test.cc index 179ae473ca0b..d33241bbdad2 100644 --- a/test/common/http/conn_manager_impl_test.cc +++ b/test/common/http/conn_manager_impl_test.cc @@ -332,10 +332,6 @@ TEST_F(HttpConnectionManagerImplTest, StartAndFinishSpanNormalFlow) { EXPECT_EQ(1UL, tracing_stats_.service_forced_.value()); EXPECT_EQ(0UL, tracing_stats_.random_sampling_.value()); - - // Uncomment either of these lines and the test fails - // EXPECT_TRUE(testing::Mock::VerifyAndClearExpectations(span)); - // delete span; } TEST_F(HttpConnectionManagerImplTest, TestAccessLog) { From 27b08757a5889cc970c2f67b73a1bd8c4f5f29c4 Mon Sep 17 00:00:00 2001 From: Matt Klein Date: Thu, 8 Jun 2017 17:13:18 -0700 Subject: [PATCH 11/12] fix --- test/common/http/conn_manager_impl_test.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/common/http/conn_manager_impl_test.cc b/test/common/http/conn_manager_impl_test.cc index f28f8c5c82b3..d7bee10f1a03 100644 --- a/test/common/http/conn_manager_impl_test.cc +++ b/test/common/http/conn_manager_impl_test.cc @@ -295,7 +295,7 @@ TEST_F(HttpConnectionManagerImplTest, StartAndFinishSpanNormalFlow) { })); EXPECT_CALL(*span, finishSpan(_)) .WillOnce( - Invoke([&](Tracing::SpanFinalizer& finalizer) -> void { finalizer.finalize(*span); })); + 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")); From 5b91a78d09862654e8c52fb04660b55fab25c3d4 Mon Sep 17 00:00:00 2001 From: Roman Dzhabarov Date: Fri, 9 Jun 2017 12:55:40 -0700 Subject: [PATCH 12/12] pass header map * --- source/common/http/conn_manager_impl.cc | 2 +- source/common/tracing/http_tracer_impl.cc | 38 ++++++++++---------- source/common/tracing/http_tracer_impl.h | 4 +-- test/common/tracing/http_tracer_impl_test.cc | 25 +++++++++++-- 4 files changed, 45 insertions(+), 24 deletions(-) diff --git a/source/common/http/conn_manager_impl.cc b/source/common/http/conn_manager_impl.cc index f358732f695a..d5c5f3aff66f 100644 --- a/source/common/http/conn_manager_impl.cc +++ b/source/common/http/conn_manager_impl.cc @@ -339,7 +339,7 @@ ConnectionManagerImpl::ActiveStream::~ActiveStream() { if (request_info_.healthCheck()) { connection_manager_.config_.tracingStats().health_check_.inc(); } else { - Tracing::HttpConnManFinalizerImpl finalizer{*request_headers_, request_info_, *this}; + Tracing::HttpConnManFinalizerImpl finalizer(request_headers_.get(), request_info_, *this); active_span_->finishSpan(finalizer); } diff --git a/source/common/tracing/http_tracer_impl.cc b/source/common/tracing/http_tracer_impl.cc index b050db77ae3d..59a47307ac7b 100644 --- a/source/common/tracing/http_tracer_impl.cc +++ b/source/common/tracing/http_tracer_impl.cc @@ -116,7 +116,7 @@ Decision HttpTracerUtility::isTracing(const Http::AccessLog::RequestInfo& reques NOT_REACHED; } -HttpConnManFinalizerImpl::HttpConnManFinalizerImpl(Http::HeaderMap& request_headers, +HttpConnManFinalizerImpl::HttpConnManFinalizerImpl(Http::HeaderMap* request_headers, Http::AccessLog::RequestInfo& request_info, Config& tracing_config) : request_headers_(request_headers), request_info_(request_info), @@ -124,26 +124,28 @@ HttpConnManFinalizerImpl::HttpConnManFinalizerImpl(Http::HeaderMap& request_head void HttpConnManFinalizerImpl::finalize(Span& span) { // Pre response data. - 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("request_size", std::to_string(request_info_.bytesReceived())); - 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())); - } + 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 : tracing_config_.requestHeadersForTags()) { - const Http::HeaderEntry* entry = request_headers_.get(header); - if (entry) { - 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. span.setTag("response_code", buildResponseCode(request_info_)); diff --git a/source/common/tracing/http_tracer_impl.h b/source/common/tracing/http_tracer_impl.h index 28584248fe3d..61ebcb09cf39 100644 --- a/source/common/tracing/http_tracer_impl.h +++ b/source/common/tracing/http_tracer_impl.h @@ -82,13 +82,13 @@ class NullFinalizer : public SpanFinalizer { */ class HttpConnManFinalizerImpl : public SpanFinalizer { public: - HttpConnManFinalizerImpl(Http::HeaderMap& request_headers, + HttpConnManFinalizerImpl(Http::HeaderMap* request_headers, Http::AccessLog::RequestInfo& request_info, Config& tracing_config); void finalize(Span& span) override; private: - Http::HeaderMap& request_headers_; + Http::HeaderMap* request_headers_; Http::AccessLog::RequestInfo& request_info_; Config& tracing_config_; }; diff --git a/test/common/tracing/http_tracer_impl_test.cc b/test/common/tracing/http_tracer_impl_test.cc index 6d2a1daf1ed3..6bfa704b0dea 100644 --- a/test/common/tracing/http_tracer_impl_test.cc +++ b/test/common/tracing/http_tracer_impl_test.cc @@ -254,7 +254,26 @@ TEST(HttpConnManFinalizerImpl, OriginalAndLongPath) { EXPECT_CALL(*span, setTag("request_line", "GET " + expected_path + " HTTP/2")); NiceMock config; - HttpConnManFinalizerImpl finalizer{request_headers, request_info, config}; + HttpConnManFinalizerImpl finalizer(&request_headers, request_info, config); + finalizer.finalize(*span); +} + +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); } @@ -288,7 +307,7 @@ TEST(HttpConnManFinalizerImpl, SpanOptionalHeaders) { NiceMock config; - HttpConnManFinalizerImpl finalizer{request_headers, request_info, config}; + HttpConnManFinalizerImpl finalizer(&request_headers, request_info, config); finalizer.finalize(*span); } @@ -340,7 +359,7 @@ TEST(HttpConnManFinalizerImpl, SpanPopulatedFailureResponse) { EXPECT_CALL(*span, setTag("response_size", "100")); EXPECT_CALL(*span, setTag("response_flags", "UT")); - HttpConnManFinalizerImpl finalizer{request_headers, request_info, config}; + HttpConnManFinalizerImpl finalizer(&request_headers, request_info, config); finalizer.finalize(*span); }