From e051acf0eab08a19b6c5260ca0d06f337068ea7e Mon Sep 17 00:00:00 2001 From: Shriram Rajagopalan Date: Wed, 3 May 2017 23:20:18 -0400 Subject: [PATCH 1/7] Expose span context to filters --- include/envoy/http/BUILD | 1 + include/envoy/http/filter.h | 7 +++++++ source/common/http/async_client_impl.h | 10 ++++++++++ source/common/http/conn_manager_impl.cc | 4 ++++ source/common/http/conn_manager_impl.h | 1 + test/mocks/http/BUILD | 2 ++ test/mocks/http/mocks.h | 4 ++++ 7 files changed, 29 insertions(+) diff --git a/include/envoy/http/BUILD b/include/envoy/http/BUILD index df656c13ec1fc..7f9d7e4216044 100644 --- a/include/envoy/http/BUILD +++ b/include/envoy/http/BUILD @@ -64,6 +64,7 @@ envoy_cc_library( "//include/envoy/event:dispatcher_interface", "//include/envoy/router:router_interface", "//include/envoy/ssl:connection_interface", + "//include/envoy/tracing:http_tracer_interface", ], ) diff --git a/include/envoy/http/filter.h b/include/envoy/http/filter.h index 038f28336ac81..98f5e3af80cb0 100644 --- a/include/envoy/http/filter.h +++ b/include/envoy/http/filter.h @@ -11,6 +11,7 @@ #include "envoy/http/header_map.h" #include "envoy/router/router.h" #include "envoy/ssl/connection.h" +#include "envoy/tracing/http_tracer.h" namespace Http { @@ -117,6 +118,12 @@ class StreamFilterCallbacks { */ virtual AccessLog::RequestInfo& requestInfo() PURE; + /** + * @return span context used for tracing purposes. Individual filters may add or modify + * information in the span context. + */ + virtual Tracing::Span& activeSpan() PURE; + /** * @return the trusted downstream address for the connection. */ diff --git a/source/common/http/async_client_impl.h b/source/common/http/async_client_impl.h index b3aaadb28d692..1bc62da747b0b 100644 --- a/source/common/http/async_client_impl.h +++ b/source/common/http/async_client_impl.h @@ -18,6 +18,7 @@ #include "envoy/router/router_ratelimit.h" #include "envoy/router/shadow_writer.h" #include "envoy/ssl/connection.h" +#include "envoy/tracing/http_tracer.h" #include "common/common/empty_string.h" #include "common/common/linked_object.h" @@ -168,6 +169,13 @@ class AsyncStreamImpl : public AsyncClient::Stream, RouteEntryImpl route_entry_; }; + struct NullSpan: public Tracing::Span { + + // Tracing::Span + void setTag(const std::string&, const std::string&) override { NOT_IMPLEMENTED; } + void finishSpan() override { NOT_IMPLEMENTED; } + }; + void cleanup(); void closeLocal(bool end_stream); void closeRemote(bool end_stream); @@ -184,6 +192,7 @@ class AsyncStreamImpl : public AsyncClient::Stream, Router::RouteConstSharedPtr route() override { return route_; } uint64_t streamId() override { return stream_id_; } AccessLog::RequestInfo& requestInfo() override { return request_info_; } + Tracing::Span& activeSpan() override { return active_span_; } const std::string& downstreamAddress() override { return EMPTY_STRING; } void continueDecoding() override { NOT_IMPLEMENTED; } Buffer::InstancePtr& decodingBuffer() override { @@ -198,6 +207,7 @@ class AsyncStreamImpl : public AsyncClient::Stream, Router::ProdFilter router_; std::function reset_callback_; AccessLog::RequestInfoImpl request_info_; + NullSpan active_span_; std::shared_ptr route_; bool local_closed_{}; bool remote_closed_{}; diff --git a/source/common/http/conn_manager_impl.cc b/source/common/http/conn_manager_impl.cc index f4d08c8e8d94d..71cef4839fd2a 100644 --- a/source/common/http/conn_manager_impl.cc +++ b/source/common/http/conn_manager_impl.cc @@ -878,6 +878,10 @@ AccessLog::RequestInfo& ConnectionManagerImpl::ActiveStreamFilterBase::requestIn return parent_.request_info_; } +Tracing::Span& ConnectionManagerImpl::ActiveStreamFilterBase::activeSpan() { + return *parent_.active_span_; +} + Router::RouteConstSharedPtr ConnectionManagerImpl::ActiveStreamFilterBase::route() { if (!parent_.cached_route_.valid()) { parent_.cached_route_.value( diff --git a/source/common/http/conn_manager_impl.h b/source/common/http/conn_manager_impl.h index 99ff729529a48..a5ce265f4fc74 100644 --- a/source/common/http/conn_manager_impl.h +++ b/source/common/http/conn_manager_impl.h @@ -282,6 +282,7 @@ class ConnectionManagerImpl : Logger::Loggable, Router::RouteConstSharedPtr route() override; uint64_t streamId() override; AccessLog::RequestInfo& requestInfo() override; + Tracing::Span& activeSpan() override; const std::string& downstreamAddress() override; ActiveStream& parent_; diff --git a/test/mocks/http/BUILD b/test/mocks/http/BUILD index 77944ae0e65a7..9d233b6fc3569 100644 --- a/test/mocks/http/BUILD +++ b/test/mocks/http/BUILD @@ -21,10 +21,12 @@ envoy_cc_mock( "//include/envoy/http:conn_pool_interface", "//include/envoy/http:filter_interface", "//include/envoy/ssl:connection_interface", + "//include/envoy/tracing:http_tracer_interface", "//source/common/http:conn_manager_lib", "//source/common/http:header_map_lib", "//test/mocks/event:event_mocks", "//test/mocks/router:router_mocks", + "//test/mocks/tracing:tracing_mocks", "//test/mocks/upstream:host_mocks", ], ) diff --git a/test/mocks/http/mocks.h b/test/mocks/http/mocks.h index 1a7d8992dab95..4b749e6d0db63 100644 --- a/test/mocks/http/mocks.h +++ b/test/mocks/http/mocks.h @@ -20,6 +20,7 @@ #include "test/mocks/common.h" #include "test/mocks/event/mocks.h" #include "test/mocks/router/mocks.h" +#include "test/mocks/tracing/mocks.h" #include "test/mocks/upstream/host.h" #include "test/test_common/printers.h" @@ -210,6 +211,7 @@ class MockStreamFilterCallbacksBase { std::function reset_callback_; Event::MockDispatcher dispatcher_; testing::NiceMock request_info_; + std::shared_ptr active_span_; std::shared_ptr route_; std::string downstream_address_; }; @@ -229,6 +231,7 @@ class MockStreamDecoderFilterCallbacks : public StreamDecoderFilterCallbacks, MOCK_METHOD0(route, Router::RouteConstSharedPtr()); MOCK_METHOD0(streamId, uint64_t()); MOCK_METHOD0(requestInfo, Http::AccessLog::RequestInfo&()); + MOCK_METHOD0(activeSpan, Tracing::Span&()); MOCK_METHOD0(downstreamAddress, const std::string&()); // Http::StreamDecoderFilterCallbacks @@ -261,6 +264,7 @@ class MockStreamEncoderFilterCallbacks : public StreamEncoderFilterCallbacks, MOCK_METHOD0(route, Router::RouteConstSharedPtr()); MOCK_METHOD0(streamId, uint64_t()); MOCK_METHOD0(requestInfo, Http::AccessLog::RequestInfo&()); + MOCK_METHOD0(activeSpan, Tracing::Span&()); MOCK_METHOD0(downstreamAddress, const std::string&()); // Http::StreamEncoderFilterCallbacks From a5ad968ed1886688f9ebabdc2c8323d67f9a86c8 Mon Sep 17 00:00:00 2001 From: Shriram Rajagopalan Date: Wed, 3 May 2017 23:41:39 -0400 Subject: [PATCH 2/7] clang format fix --- source/common/http/async_client_impl.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/source/common/http/async_client_impl.h b/source/common/http/async_client_impl.h index 1bc62da747b0b..9b9b81d6b1d9e 100644 --- a/source/common/http/async_client_impl.h +++ b/source/common/http/async_client_impl.h @@ -169,7 +169,7 @@ class AsyncStreamImpl : public AsyncClient::Stream, RouteEntryImpl route_entry_; }; - struct NullSpan: public Tracing::Span { + struct NullSpan : public Tracing::Span { // Tracing::Span void setTag(const std::string&, const std::string&) override { NOT_IMPLEMENTED; } From 2d87e05fdf48175af99bf7fd97f0187f9387808b Mon Sep 17 00:00:00 2001 From: Shriram Rajagopalan Date: Thu, 4 May 2017 19:23:27 -0400 Subject: [PATCH 3/7] move NullSpan into Tracing namespace --- source/common/http/BUILD | 1 + source/common/http/async_client_impl.h | 10 ++-------- source/common/tracing/http_tracer_impl.h | 7 +++++++ 3 files changed, 10 insertions(+), 8 deletions(-) diff --git a/source/common/http/BUILD b/source/common/http/BUILD index 6b9f1a32e0463..9817993c4e586 100644 --- a/source/common/http/BUILD +++ b/source/common/http/BUILD @@ -26,6 +26,7 @@ envoy_cc_library( "//source/common/common:linked_object", "//source/common/http/access_log:request_info_lib", "//source/common/router:router_lib", + "//source/common/tracing:http_tracer_lib", ], ) diff --git a/source/common/http/async_client_impl.h b/source/common/http/async_client_impl.h index 9b9b81d6b1d9e..3ea1f14af8be7 100644 --- a/source/common/http/async_client_impl.h +++ b/source/common/http/async_client_impl.h @@ -25,6 +25,7 @@ #include "common/http/access_log/request_info_impl.h" #include "common/http/message_impl.h" #include "common/router/router.h" +#include "common/tracing/http_tracer_impl.h" namespace Http { @@ -169,13 +170,6 @@ class AsyncStreamImpl : public AsyncClient::Stream, RouteEntryImpl route_entry_; }; - struct NullSpan : public Tracing::Span { - - // Tracing::Span - void setTag(const std::string&, const std::string&) override { NOT_IMPLEMENTED; } - void finishSpan() override { NOT_IMPLEMENTED; } - }; - void cleanup(); void closeLocal(bool end_stream); void closeRemote(bool end_stream); @@ -207,7 +201,7 @@ class AsyncStreamImpl : public AsyncClient::Stream, Router::ProdFilter router_; std::function reset_callback_; AccessLog::RequestInfoImpl request_info_; - NullSpan active_span_; + Tracing::NullSpan active_span_; std::shared_ptr route_; bool local_closed_{}; bool remote_closed_{}; diff --git a/source/common/tracing/http_tracer_impl.h b/source/common/tracing/http_tracer_impl.h index 5b5e71620aac5..c2a3ba4c909b1 100644 --- a/source/common/tracing/http_tracer_impl.h +++ b/source/common/tracing/http_tracer_impl.h @@ -82,4 +82,11 @@ 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 {} +}; + } // Tracing From 4c545ae6eb7967e3bd62a80d7fa5808bd51e06d5 Mon Sep 17 00:00:00 2001 From: Shriram Rajagopalan Date: Fri, 5 May 2017 00:32:33 -0400 Subject: [PATCH 4/7] test activeSpan interface --- test/common/http/BUILD | 1 + test/common/http/conn_manager_impl_test.cc | 4 ++++ 2 files changed, 5 insertions(+) diff --git a/test/common/http/BUILD b/test/common/http/BUILD index dd7ab12537341..c0f85c63bc34e 100644 --- a/test/common/http/BUILD +++ b/test/common/http/BUILD @@ -82,6 +82,7 @@ envoy_cc_test( "//include/envoy/buffer:buffer_interface", "//include/envoy/event:dispatcher_interface", "//include/envoy/http:access_log_interface", + "//include/envoy/tracing:http_tracer_interface", "//source/common/buffer:buffer_lib", "//source/common/event:dispatcher_lib", "//source/common/http:conn_manager_lib", diff --git a/test/common/http/conn_manager_impl_test.cc b/test/common/http/conn_manager_impl_test.cc index 587e627c866ae..6e32f91d2d064 100644 --- a/test/common/http/conn_manager_impl_test.cc +++ b/test/common/http/conn_manager_impl_test.cc @@ -7,6 +7,7 @@ #include "envoy/buffer/buffer.h" #include "envoy/event/dispatcher.h" #include "envoy/http/access_log.h" +#include "envoy/tracing/http_tracer.h" #include "common/buffer/buffer_impl.h" #include "common/http/access_log/access_log_formatter.h" @@ -244,6 +245,9 @@ TEST_F(HttpConnectionManagerImplTest, StartAndFinishSpanNormalFlow) { EXPECT_CALL(filter_factory_, createFilterChain(_)) .WillRepeatedly(Invoke([&](Http::FilterChainFactoryCallbacks& callbacks) -> void { callbacks.addStreamDecoderFilter(filter); })); + // Verify if the activeSpan interface returns reference to the current span. + EXPECT_CALL(*span, setTag("service-cluster", "scoobydoo")); + filter->callbacks_->activeSpan().setTag("service-cluster", "scoobydoo"); Http::StreamDecoder* decoder = nullptr; NiceMock encoder; From 22fe3bc2189e3f7896ee805e66e84ea4c38979d0 Mon Sep 17 00:00:00 2001 From: Shriram Rajagopalan Date: Fri, 5 May 2017 15:35:02 -0400 Subject: [PATCH 5/7] bug fix --- test/common/http/conn_manager_impl_test.cc | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/test/common/http/conn_manager_impl_test.cc b/test/common/http/conn_manager_impl_test.cc index 6e32f91d2d064..4a8d96f89dabb 100644 --- a/test/common/http/conn_manager_impl_test.cc +++ b/test/common/http/conn_manager_impl_test.cc @@ -236,6 +236,8 @@ TEST_F(HttpConnectionManagerImplTest, StartAndFinishSpanNormalFlow) { EXPECT_CALL(*span, setTag(_, _)).Times(testing::AnyNumber()); // Verify tag is set based on the request headers. EXPECT_CALL(*span, setTag(":method", "GET")); + // Verify if the activeSpan interface returns reference to the current span. + EXPECT_CALL(*span, setTag("service-cluster", "scoobydoo")); EXPECT_CALL(runtime_.snapshot_, featureEnabled("tracing.global_enabled", 100, _)) .WillOnce(Return(true)); @@ -245,9 +247,6 @@ TEST_F(HttpConnectionManagerImplTest, StartAndFinishSpanNormalFlow) { EXPECT_CALL(filter_factory_, createFilterChain(_)) .WillRepeatedly(Invoke([&](Http::FilterChainFactoryCallbacks& callbacks) -> void { callbacks.addStreamDecoderFilter(filter); })); - // Verify if the activeSpan interface returns reference to the current span. - EXPECT_CALL(*span, setTag("service-cluster", "scoobydoo")); - filter->callbacks_->activeSpan().setTag("service-cluster", "scoobydoo"); Http::StreamDecoder* decoder = nullptr; NiceMock encoder; @@ -264,6 +263,7 @@ TEST_F(HttpConnectionManagerImplTest, StartAndFinishSpanNormalFlow) { Http::HeaderMapPtr response_headers{new TestHeaderMapImpl{{":status", "200"}}}; filter->callbacks_->encodeHeaders(std::move(response_headers), true); + filter->callbacks_->activeSpan().setTag("service-cluster", "scoobydoo"); data.drain(4); })); From eb261d6353ab9433cbe7a736583bc95d37917ee5 Mon Sep 17 00:00:00 2001 From: Shriram Rajagopalan Date: Fri, 5 May 2017 16:22:08 -0400 Subject: [PATCH 6/7] format 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 4a8d96f89dabb..6d93530f3224d 100644 --- a/test/common/http/conn_manager_impl_test.cc +++ b/test/common/http/conn_manager_impl_test.cc @@ -263,7 +263,7 @@ TEST_F(HttpConnectionManagerImplTest, StartAndFinishSpanNormalFlow) { Http::HeaderMapPtr response_headers{new TestHeaderMapImpl{{":status", "200"}}}; filter->callbacks_->encodeHeaders(std::move(response_headers), true); - filter->callbacks_->activeSpan().setTag("service-cluster", "scoobydoo"); + filter->callbacks_->activeSpan().setTag("service-cluster", "scoobydoo"); data.drain(4); })); From 3b8dda1855fc4e55763dee8e174828a8d135093e Mon Sep 17 00:00:00 2001 From: Shriram Rajagopalan Date: Sat, 6 May 2017 01:22:38 -0400 Subject: [PATCH 7/7] nits --- include/envoy/http/filter.h | 2 +- test/mocks/http/mocks.h | 1 - 2 files changed, 1 insertion(+), 2 deletions(-) diff --git a/include/envoy/http/filter.h b/include/envoy/http/filter.h index 98f5e3af80cb0..11190bfc45c7d 100644 --- a/include/envoy/http/filter.h +++ b/include/envoy/http/filter.h @@ -120,7 +120,7 @@ class StreamFilterCallbacks { /** * @return span context used for tracing purposes. Individual filters may add or modify - * information in the span context. + * information in the span context. */ virtual Tracing::Span& activeSpan() PURE; diff --git a/test/mocks/http/mocks.h b/test/mocks/http/mocks.h index 4b749e6d0db63..ad4baabb7bd2b 100644 --- a/test/mocks/http/mocks.h +++ b/test/mocks/http/mocks.h @@ -211,7 +211,6 @@ class MockStreamFilterCallbacksBase { std::function reset_callback_; Event::MockDispatcher dispatcher_; testing::NiceMock request_info_; - std::shared_ptr active_span_; std::shared_ptr route_; std::string downstream_address_; };