From fb83d882062c886f5437a6ebbbc7b5be25956858 Mon Sep 17 00:00:00 2001 From: Gary Brown Date: Wed, 18 Oct 2017 19:52:54 +0100 Subject: [PATCH] =?UTF-8?q?tracing:=20Enable=20decorated=20operation=20in?= =?UTF-8?q?=20outbound=20(egress)=20listener=20to=20=E2=80=A6=20(#1858)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit tracing: Enable decorated operation in outbound (egress) listener to be passed to inbound (ingress) listener to override server span operation. Signed-off-by: Gary Brown --- .../http_conn_man/route_config/route.rst | 4 +- .../http_filters/router_filter.rst | 8 +++ docs/intro/arch_overview/tracing.rst | 4 +- include/envoy/http/header_map.h | 1 + include/envoy/router/router.h | 6 ++ source/common/http/conn_manager_impl.cc | 57 +++++++++++---- source/common/http/conn_manager_impl.h | 2 + source/common/http/headers.h | 1 + source/common/router/config_impl.cc | 2 + source/common/router/config_impl.h | 3 + test/common/http/conn_manager_impl_test.cc | 71 +++++++++++++++++++ test/mocks/router/mocks.cc | 4 +- test/mocks/router/mocks.h | 2 +- 13 files changed, 149 insertions(+), 16 deletions(-) diff --git a/docs/configuration/http_conn_man/route_config/route.rst b/docs/configuration/http_conn_man/route_config/route.rst index 0628554e4208..af9ba854a175 100644 --- a/docs/configuration/http_conn_man/route_config/route.rst +++ b/docs/configuration/http_conn_man/route_config/route.rst @@ -450,7 +450,9 @@ Specifies the route's decorator. operation *(required, string)* The operation name associated with the request matched to this route. If tracing is - enabled, this information will be used as the span name reported for this request. + enabled, this information will be used as the span name reported for this request. NOTE: For ingress + (inbound) requests this value may be overridden by the + :ref:`x-envoy-decorator-operation ` request header. .. _config_http_conn_man_route_table_route_add_req_headers: diff --git a/docs/configuration/http_filters/router_filter.rst b/docs/configuration/http_filters/router_filter.rst index 9cdc294d9b11..f42e499e9c26 100644 --- a/docs/configuration/http_filters/router_filter.rst +++ b/docs/configuration/http_filters/router_filter.rst @@ -236,6 +236,14 @@ if a request was dropped due to either :ref:`maintenance mode ` or upstream :ref:`circuit breaking `. +.. _config_http_filters_router_x-envoy-decorator-operation: + +x-envoy-decorator-operation +^^^^^^^^^^^^^^^^^^^^^^^^^^^ + +If this header is present on ingress requests, its value will override any locally defined +operation (span) name on the server span generated by the tracing mechanism. + .. _config_http_filters_router_stats: Statistics diff --git a/docs/intro/arch_overview/tracing.rst b/docs/intro/arch_overview/tracing.rst index 8d69e0c06d46..1af7ec32aece 100644 --- a/docs/intro/arch_overview/tracing.rst +++ b/docs/intro/arch_overview/tracing.rst @@ -90,7 +90,9 @@ associated with it. Each span generated by Envoy contains the following data: * Tracing system-specific metadata. The span also includes a name (or operation) which by default is defined as the host of the invoked service. -However this can be customized using a :ref:`config_http_conn_man_route_table_decorator` on the route. +However this can be customized using a :ref:`config_http_conn_man_route_table_decorator` on the route. The +name used for the server (ingress) span can also be overridden using the +:ref:`config_http_filters_router_x-envoy-decorator-operation` header. Envoy automatically sends spans to tracing collectors. Depending on the tracing collector, multiple spans are stitched together using common information such as the globally unique diff --git a/include/envoy/http/header_map.h b/include/envoy/http/header_map.h index 1cf077713a74..53aba737dcc7 100644 --- a/include/envoy/http/header_map.h +++ b/include/envoy/http/header_map.h @@ -215,6 +215,7 @@ class HeaderEntry { HEADER_FUNC(ContentLength) \ HEADER_FUNC(ContentType) \ HEADER_FUNC(Date) \ + HEADER_FUNC(EnvoyDecoratorOperation) \ HEADER_FUNC(EnvoyDownstreamServiceCluster) \ HEADER_FUNC(EnvoyDownstreamServiceNode) \ HEADER_FUNC(EnvoyExpectedRequestTimeoutMs) \ diff --git a/include/envoy/router/router.h b/include/envoy/router/router.h index f23ad5ac0b86..f5480e014e5a 100644 --- a/include/envoy/router/router.h +++ b/include/envoy/router/router.h @@ -382,6 +382,12 @@ class Decorator { * @param Tracing::Span& the span. */ virtual void apply(Tracing::Span& span) const PURE; + + /** + * This method returns the operation name. + * @return the operation name + */ + virtual const std::string& getOperation() const PURE; }; typedef std::unique_ptr DecoratorConstPtr; diff --git a/source/common/http/conn_manager_impl.cc b/source/common/http/conn_manager_impl.cc index 744fa60ec87e..0f7ffb8f72db 100644 --- a/source/common/http/conn_manager_impl.cc +++ b/source/common/http/conn_manager_impl.cc @@ -540,18 +540,7 @@ void ConnectionManagerImpl::ActiveStream::decodeHeaders(HeaderMapPtr&& headers, // Check if tracing is enabled at all. if (connection_manager_.config_.tracingConfig()) { - Tracing::Decision tracing_decision = - Tracing::HttpTracerUtility::isTracing(request_info_, *request_headers_); - ConnectionManagerImpl::chargeTracingStats(tracing_decision.reason, - connection_manager_.config_.tracingStats()); - - if (tracing_decision.is_tracing) { - active_span_ = connection_manager_.tracer_.startSpan(*this, *request_headers_, request_info_); - if (cached_route_.value() && cached_route_.value()->decorator()) { - cached_route_.value()->decorator()->apply(*active_span_); - } - active_span_->injectContext(*request_headers_); - } + traceRequest(); } // Set the trusted address for the connection by taking the last address in XFF. @@ -559,6 +548,50 @@ void ConnectionManagerImpl::ActiveStream::decodeHeaders(HeaderMapPtr&& headers, decodeHeaders(nullptr, *request_headers_, end_stream); } +void ConnectionManagerImpl::ActiveStream::traceRequest() { + Tracing::Decision tracing_decision = + Tracing::HttpTracerUtility::isTracing(request_info_, *request_headers_); + ConnectionManagerImpl::chargeTracingStats(tracing_decision.reason, + connection_manager_.config_.tracingStats()); + + if (!tracing_decision.is_tracing) { + return; + } + + active_span_ = connection_manager_.tracer_.startSpan(*this, *request_headers_, request_info_); + + // If a decorator has been defined, apply it to the active span. + if (cached_route_.value() && cached_route_.value()->decorator()) { + cached_route_.value()->decorator()->apply(*active_span_); + + // For egress (outbound) requests, pass the decorator's operation name (if defined) + // as a request header to enable the receiving service to use it in its server span. + if (connection_manager_.config_.tracingConfig()->operation_name_ == + Tracing::OperationName::Egress && + !cached_route_.value()->decorator()->getOperation().empty()) { + request_headers_->insertEnvoyDecoratorOperation().value( + cached_route_.value()->decorator()->getOperation()); + } + } + + const HeaderEntry* decorator_operation = request_headers_->EnvoyDecoratorOperation(); + + // For ingress (inbound) requests, if a decorator operation name has been provided, it + // should be used to override the active span's operation. + if (connection_manager_.config_.tracingConfig()->operation_name_ == + Tracing::OperationName::Ingress && + decorator_operation) { + if (!decorator_operation->value().empty()) { + active_span_->setOperation(decorator_operation->value().c_str()); + } + // Remove header so not propagated to service + request_headers_->removeEnvoyDecoratorOperation(); + } + + // Inject the active span's tracing context into the request headers. + active_span_->injectContext(*request_headers_); +} + void ConnectionManagerImpl::ActiveStream::decodeHeaders(ActiveStreamDecoderFilter* filter, HeaderMap& headers, bool end_stream) { std::list::iterator entry; diff --git a/source/common/http/conn_manager_impl.h b/source/common/http/conn_manager_impl.h index eb71664812da..cb75c6607b65 100644 --- a/source/common/http/conn_manager_impl.h +++ b/source/common/http/conn_manager_impl.h @@ -520,6 +520,8 @@ class ConnectionManagerImpl : Logger::Loggable, virtual Tracing::OperationName operationName() const override; virtual const std::vector& requestHeadersForTags() const override; + void traceRequest(); + // Pass on watermark callbacks to watermark subscribers. This boils down to passing watermark // events for this stream and the downstream connection to the router filter. void callHighWatermarkCallbacks(); diff --git a/source/common/http/headers.h b/source/common/http/headers.h index 96f1a5449318..905b39878b21 100644 --- a/source/common/http/headers.h +++ b/source/common/http/headers.h @@ -51,6 +51,7 @@ class HeaderValues { const LowerCaseString EnvoyExpectedRequestTimeoutMs{"x-envoy-expected-rq-timeout-ms"}; const LowerCaseString EnvoyUpstreamServiceTime{"x-envoy-upstream-service-time"}; const LowerCaseString EnvoyUpstreamHealthCheckedCluster{"x-envoy-upstream-healthchecked-cluster"}; + const LowerCaseString EnvoyDecoratorOperation{"x-envoy-decorator-operation"}; const LowerCaseString Expect{"expect"}; const LowerCaseString ForwardedClientCert{"x-forwarded-client-cert"}; const LowerCaseString ForwardedFor{"x-forwarded-for"}; diff --git a/source/common/router/config_impl.cc b/source/common/router/config_impl.cc index 1fa868f72481..faca251faf26 100644 --- a/source/common/router/config_impl.cc +++ b/source/common/router/config_impl.cc @@ -213,6 +213,8 @@ void DecoratorImpl::apply(Tracing::Span& span) const { } } +const std::string& DecoratorImpl::getOperation() const { return operation_; } + const uint64_t RouteEntryImplBase::WeightedClusterEntry::MAX_CLUSTER_WEIGHT = 100UL; RouteEntryImplBase::RouteEntryImplBase(const VirtualHostImpl& vhost, diff --git a/source/common/router/config_impl.h b/source/common/router/config_impl.h index c220fe71b434..82ef5f787b9d 100644 --- a/source/common/router/config_impl.h +++ b/source/common/router/config_impl.h @@ -274,6 +274,9 @@ class DecoratorImpl : public Decorator { // Decorator::apply void apply(Tracing::Span& span) const override; + // Decorator::getOperation + const std::string& getOperation() const override; + private: const std::string operation_; }; diff --git a/test/common/http/conn_manager_impl_test.cc b/test/common/http/conn_manager_impl_test.cc index b7e2048e1c43..56793a5e2d55 100644 --- a/test/common/http/conn_manager_impl_test.cc +++ b/test/common/http/conn_manager_impl_test.cc @@ -395,6 +395,70 @@ TEST_F(HttpConnectionManagerImplTest, StartAndFinishSpanNormalFlow) { 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)); + EXPECT_CALL(*span, setOperation("testOp")); + + std::shared_ptr filter(new NiceMock()); + + EXPECT_CALL(filter_factory_, createFilterChain(_)) + .WillRepeatedly(Invoke([&](FilterChainFactoryCallbacks& callbacks) -> void { + callbacks.addStreamDecoderFilter(filter); + })); + + // Treat request as internal, otherwise x-request-id header will be overwritten. + use_remote_address_ = false; + EXPECT_CALL(random_, uuid()).Times(0); + + StreamDecoder* decoder = nullptr; + NiceMock encoder; + EXPECT_CALL(*codec_, dispatch(_)).WillRepeatedly(Invoke([&](Buffer::Instance& data) -> void { + decoder = &conn_manager_->newStream(encoder); + + HeaderMapPtr headers{ + new TestHeaderMapImpl{{":method", "GET"}, + {":authority", "host"}, + {":path", "/"}, + {"x-request-id", "125a4afb-6f55-a4ba-ad80-413f09f48a28"}, + {"x-envoy-decorator-operation", "testOp"}}}; + decoder->decodeHeaders(std::move(headers), true); + + 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); + })); + + Buffer::OwnedImpl fake_input("1234"); + conn_manager_->onData(fake_input); + + EXPECT_EQ(1UL, tracing_stats_.service_forced_.value()); + EXPECT_EQ(0UL, tracing_stats_.random_sampling_.value()); +} + +TEST_F(HttpConnectionManagerImplTest, StartAndFinishSpanNormalFlowEgress) { + setup(false, ""); + tracing_config_.reset(new TracingConnectionManagerConfig( + {Tracing::OperationName::Egress, {LowerCaseString(":method")}})); + + NiceMock* span = new NiceMock(); + EXPECT_CALL(tracer_, startSpan_(_, _, _)) + .WillOnce(Invoke([&](const Tracing::Config& config, const HeaderMap&, + const AccessLog::RequestInfo&) -> Tracing::Span* { + EXPECT_EQ(Tracing::OperationName::Egress, config.operationName()); + + return span; + })); + route_config_provider_.route_config_->route_->decorator_.operation_ = "testOp"; + EXPECT_CALL(*route_config_provider_.route_config_->route_, decorator()).Times(4); + EXPECT_CALL(route_config_provider_.route_config_->route_->decorator_, apply(_)) + .WillOnce( + Invoke([&](const Tracing::Span& applyToSpan) -> void { EXPECT_EQ(span, &applyToSpan); })); + EXPECT_CALL(*span, finishSpan(_)) + .WillOnce( + Invoke([span](Tracing::SpanFinalizer& finalizer) -> void { finalizer.finalize(*span); })); + EXPECT_CALL(*span, setTag(_, _)).Times(testing::AnyNumber()); EXPECT_CALL(runtime_.snapshot_, featureEnabled("tracing.global_enabled", 100, _)) .WillOnce(Return(true)); @@ -428,6 +492,13 @@ TEST_F(HttpConnectionManagerImplTest, StartAndFinishSpanNormalFlow) { data.drain(4); })); + EXPECT_CALL(*filter, decodeHeaders(_, true)) + .WillOnce(Invoke([](HeaderMap& headers, bool) -> FilterHeadersStatus { + EXPECT_NE(nullptr, headers.EnvoyDecoratorOperation()); + EXPECT_STREQ("testOp", headers.EnvoyDecoratorOperation()->value().c_str()); + return FilterHeadersStatus::StopIteration; + })); + Buffer::OwnedImpl fake_input("1234"); conn_manager_->onData(fake_input); diff --git a/test/mocks/router/mocks.cc b/test/mocks/router/mocks.cc index 132de7f91026..9b92dffcac4f 100644 --- a/test/mocks/router/mocks.cc +++ b/test/mocks/router/mocks.cc @@ -78,7 +78,9 @@ MockConfig::MockConfig() : route_(new NiceMock()) { MockConfig::~MockConfig() {} -MockDecorator::MockDecorator() { ON_CALL(*this, operation()).WillByDefault(ReturnRef(operation_)); } +MockDecorator::MockDecorator() { + ON_CALL(*this, getOperation()).WillByDefault(ReturnRef(operation_)); +} MockDecorator::~MockDecorator() {} MockRoute::MockRoute() { diff --git a/test/mocks/router/mocks.h b/test/mocks/router/mocks.h index 117e2c67c3a5..5c098b980f6d 100644 --- a/test/mocks/router/mocks.h +++ b/test/mocks/router/mocks.h @@ -228,7 +228,7 @@ class MockDecorator : public Decorator { ~MockDecorator(); // Router::Decorator - MOCK_CONST_METHOD0(operation, const std::string&()); + MOCK_CONST_METHOD0(getOperation, const std::string&()); MOCK_CONST_METHOD1(apply, void(Tracing::Span& span)); std::string operation_{"fake_operation"};