diff --git a/docs/configuration/http_conn_man/tracing.rst b/docs/configuration/http_conn_man/tracing.rst index 5aa03a6509607..c263aeeff3a97 100644 --- a/docs/configuration/http_conn_man/tracing.rst +++ b/docs/configuration/http_conn_man/tracing.rst @@ -7,7 +7,8 @@ Tracing { "tracing": { - "operation_name": "..." + "operation_name": "...", + "request_headers_for_tags": [] } } @@ -15,4 +16,8 @@ operation_name *(required, string)* Span name will be derived from operation_name. "ingress" and "egress" are the only supported values. +request_headers_for_tags + *(optional, array)* A list of header names used to create tags for the active span. + The header name is used to populate the tag name, and the header value is used to populate the tag value. + The tag is created if the specified header name is present in the request's headers. diff --git a/include/envoy/tracing/http_tracer.h b/include/envoy/tracing/http_tracer.h index 743e3109f551a..ef2ed8ee9a6b7 100644 --- a/include/envoy/tracing/http_tracer.h +++ b/include/envoy/tracing/http_tracer.h @@ -15,7 +15,15 @@ class Config { public: virtual ~Config() {} + /** + * @return operation name for tracing, e.g., ingress. + */ virtual OperationName operationName() const PURE; + + /** + * @return list of headers to populate tags on the active span. + */ + virtual const std::vector& requestHeadersForTags() const PURE; }; /* diff --git a/source/common/http/conn_manager_impl.cc b/source/common/http/conn_manager_impl.cc index 0e68d8f78c630..b90acc3332ea2 100644 --- a/source/common/http/conn_manager_impl.cc +++ b/source/common/http/conn_manager_impl.cc @@ -315,7 +315,8 @@ ConnectionManagerImpl::ActiveStream::~ActiveStream() { if (request_info_.healthCheck()) { connection_manager_.config_.tracingStats().health_check_.inc(); } else { - Tracing::HttpTracerUtility::finalizeSpan(*active_span_, *request_headers_, request_info_); + Tracing::HttpTracerUtility::finalizeSpan(*active_span_, *request_headers_, request_info_, + *this); } } } @@ -446,7 +447,7 @@ void ConnectionManagerImpl::ActiveStream::decodeHeaders(HeaderMapPtr&& headers, connection_manager_.runtime_); // Check if tracing is enabled at all. - if (connection_manager_.config_.tracingConfig().valid()) { + if (connection_manager_.config_.tracingConfig()) { Tracing::Decision tracing_decision = Tracing::HttpTracerUtility::isTracing(request_info_, *request_headers_); ConnectionManagerImpl::chargeTracingStats(tracing_decision.reason, @@ -745,7 +746,12 @@ void ConnectionManagerImpl::ActiveStream::onResetStream(StreamResetReason) { } Tracing::OperationName ConnectionManagerImpl::ActiveStream::operationName() const { - return connection_manager_.config_.tracingConfig().value().operation_name_; + return connection_manager_.config_.tracingConfig()->operation_name_; +} + +const std::vector& +ConnectionManagerImpl::ActiveStream::requestHeadersForTags() const { + return connection_manager_.config_.tracingConfig()->request_headers_for_tags_; } void ConnectionManagerImpl::ActiveStreamFilterBase::addResetStreamCallback( diff --git a/source/common/http/conn_manager_impl.h b/source/common/http/conn_manager_impl.h index 244b1418bf9b8..bf8d014621ab4 100644 --- a/source/common/http/conn_manager_impl.h +++ b/source/common/http/conn_manager_impl.h @@ -104,8 +104,11 @@ struct ConnectionManagerTracingStats { */ struct TracingConnectionManagerConfig { Tracing::OperationName operation_name_; + std::vector request_headers_for_tags_; }; +typedef std::unique_ptr TracingConnectionManagerConfigPtr; + /** * Abstract configuration for the connection manager. */ @@ -202,7 +205,7 @@ class ConnectionManagerConfig { /** * @return tracing config. */ - virtual const Optional& tracingConfig() PURE; + virtual const TracingConnectionManagerConfig* tracingConfig() PURE; }; /** @@ -384,6 +387,7 @@ class ConnectionManagerImpl : Logger::Loggable, // Tracing::TracingConfig virtual Tracing::OperationName operationName() const override; + virtual const std::vector& requestHeadersForTags() const override; // All state for the stream. Put here for readability. We could move this to a bit field // eventually if we want. diff --git a/source/common/http/conn_manager_utility.cc b/source/common/http/conn_manager_utility.cc index 81e34b6db6e17..fda7dcfbae018 100644 --- a/source/common/http/conn_manager_utility.cc +++ b/source/common/http/conn_manager_utility.cc @@ -117,7 +117,7 @@ void ConnectionManagerUtility::mutateRequestHeaders(Http::HeaderMap& request_hea } } - if (config.tracingConfig().valid()) { + if (config.tracingConfig()) { Tracing::HttpTracerUtility::mutateHeaders(request_headers, runtime); } } diff --git a/source/common/json/config_schemas.cc b/source/common/json/config_schemas.cc index 6788ea6c75c63..6acb1a201473c 100644 --- a/source/common/json/config_schemas.cc +++ b/source/common/json/config_schemas.cc @@ -214,6 +214,11 @@ const std::string Json::Schema::HTTP_CONN_NETWORK_FILTER_SCHEMA(R"EOF( "operation_name" : { "type" : "string", "enum": ["ingress", "egress"] + }, + "request_headers_for_tags": { + "type" : "array", + "uniqueItems": true, + "items" : {"type" : "string"} } }, "required" : ["operation_name"], diff --git a/source/common/tracing/http_tracer_impl.cc b/source/common/tracing/http_tracer_impl.cc index 247598ab36240..82f94acba9190 100644 --- a/source/common/tracing/http_tracer_impl.cc +++ b/source/common/tracing/http_tracer_impl.cc @@ -111,11 +111,12 @@ Decision HttpTracerUtility::isTracing(const Http::AccessLog::RequestInfo& reques return {Reason::NotTraceableRequestId, false}; } - throw std::invalid_argument("Unknown trace_status"); + NOT_REACHED; } void HttpTracerUtility::finalizeSpan(Span& active_span, const Http::HeaderMap& request_headers, - const Http::AccessLog::RequestInfo& request_info) { + const Http::AccessLog::RequestInfo& request_info, + const Config& config) { // Pre response data. active_span.setTag("guid:x-request-id", std::string(request_headers.RequestId()->value().c_str())); @@ -131,6 +132,14 @@ void HttpTracerUtility::finalizeSpan(Span& active_span, const Http::HeaderMap& r 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()); + } + } + // Post response data. active_span.setTag("response_code", buildResponseCode(request_info)); active_span.setTag("response_size", std::to_string(request_info.bytesSent())); diff --git a/source/common/tracing/http_tracer_impl.h b/source/common/tracing/http_tracer_impl.h index 879b333a06b00..0bb1980861c75 100644 --- a/source/common/tracing/http_tracer_impl.h +++ b/source/common/tracing/http_tracer_impl.h @@ -63,7 +63,8 @@ class HttpTracerUtility { * 2) Finish active span. */ static void finalizeSpan(Span& active_span, const Http::HeaderMap& request_headers, - const Http::AccessLog::RequestInfo& request_info); + const Http::AccessLog::RequestInfo& request_info, + const Config& tracing_config); static const std::string INGRESS_OPERATION; static const std::string EGRESS_OPERATION; diff --git a/source/server/config/network/http_connection_manager.cc b/source/server/config/network/http_connection_manager.cc index 7e85b15593f07..ad7154354ba44 100644 --- a/source/server/config/network/http_connection_manager.cc +++ b/source/server/config/network/http_connection_manager.cc @@ -86,13 +86,27 @@ HttpConnectionManagerConfig::HttpConnectionManagerConfig(const Json::Object& con } if (config.hasObject("tracing")) { - const std::string operation_name = config.getObject("tracing")->getString("operation_name"); + Json::ObjectPtr tracing_config = config.getObject("tracing"); + + const std::string operation_name = tracing_config->getString("operation_name"); + Tracing::OperationName tracing_operation_name; + std::vector request_headers_for_tags; + if (operation_name == "ingress") { - tracing_config_.value({Tracing::OperationName::Ingress}); + tracing_operation_name = Tracing::OperationName::Ingress; } else { ASSERT(operation_name == "egress"); - tracing_config_.value({Tracing::OperationName::Egress}); + tracing_operation_name = Tracing::OperationName::Egress; + } + + if (tracing_config->hasObject("request_headers_for_tags")) { + for (const std::string& header : tracing_config->getStringArray("request_headers_for_tags")) { + request_headers_for_tags.push_back(Http::LowerCaseString(header)); + } } + + tracing_config_.reset(new Http::TracingConnectionManagerConfig( + {tracing_operation_name, request_headers_for_tags})); } if (config.hasObject("idle_timeout_s")) { diff --git a/source/server/config/network/http_connection_manager.h b/source/server/config/network/http_connection_manager.h index bf29eed3b5e47..62fc63dc44962 100644 --- a/source/server/config/network/http_connection_manager.h +++ b/source/server/config/network/http_connection_manager.h @@ -88,8 +88,8 @@ class HttpConnectionManagerConfig : Logger::Loggable, Http::ConnectionManagerStats& stats() override { return stats_; } Http::ConnectionManagerTracingStats& tracingStats() override { return tracing_stats_; } bool useRemoteAddress() override { return use_remote_address_; } - const Optional& tracingConfig() override { - return tracing_config_; + const Http::TracingConnectionManagerConfig* tracingConfig() override { + return tracing_config_.get(); } const Network::Address::Instance& localAddress() override; const Optional& userAgent() override { return user_agent_; } @@ -120,7 +120,7 @@ class HttpConnectionManagerConfig : Logger::Loggable, CodecType codec_type_; const uint64_t codec_options_; std::string server_name_; - Optional tracing_config_; + Http::TracingConnectionManagerConfigPtr tracing_config_; Optional user_agent_; Optional idle_timeout_; Router::RouteConfigProviderPtr route_config_provider_; diff --git a/source/server/http/admin.h b/source/server/http/admin.h index 964c1665c6db1..ca068536703b6 100644 --- a/source/server/http/admin.h +++ b/source/server/http/admin.h @@ -63,9 +63,7 @@ class AdminImpl : public Admin, bool useRemoteAddress() override { return true; } const Network::Address::Instance& localAddress() override; const Optional& userAgent() override { return user_agent_; } - const Optional& tracingConfig() override { - return tracing_config_; - } + const Http::TracingConnectionManagerConfig* tracingConfig() override { return nullptr; } private: /** @@ -126,7 +124,6 @@ class AdminImpl : public Admin, std::list handlers_; Optional idle_timeout_; Optional user_agent_; - Optional tracing_config_; Http::SlowDateProviderImpl date_provider_; }; diff --git a/test/common/http/conn_manager_impl_test.cc b/test/common/http/conn_manager_impl_test.cc index 29deb89730f9c..fae1b0838817c 100644 --- a/test/common/http/conn_manager_impl_test.cc +++ b/test/common/http/conn_manager_impl_test.cc @@ -54,7 +54,8 @@ class HttpConnectionManagerImplTest : public Test, public ConnectionManagerConfi "", fake_stats_}, tracing_stats_{CONN_MAN_TRACING_STATS(POOL_COUNTER(fake_stats_))} { - tracing_config_.value({Tracing::OperationName::Ingress}); + tracing_config_.reset(new Http::TracingConnectionManagerConfig( + {Tracing::OperationName::Ingress, {Http::LowerCaseString(":method")}})); } ~HttpConnectionManagerImplTest() { @@ -94,8 +95,8 @@ class HttpConnectionManagerImplTest : public Test, public ConnectionManagerConfi bool useRemoteAddress() override { return use_remote_address_; } const Network::Address::Instance& localAddress() override { return local_address_; } const Optional& userAgent() override { return user_agent_; } - const Optional& tracingConfig() override { - return tracing_config_; + const Http::TracingConnectionManagerConfig* tracingConfig() override { + return tracing_config_.get(); } NiceMock tracer_; @@ -121,7 +122,7 @@ class HttpConnectionManagerImplTest : public Test, public ConnectionManagerConfi NiceMock random_; std::unique_ptr ssl_connection_; RouteConfigProvider route_config_provider_; - Optional tracing_config_; + Http::TracingConnectionManagerConfigPtr tracing_config_; Http::SlowDateProviderImpl date_provider_; }; @@ -221,6 +222,9 @@ TEST_F(HttpConnectionManagerImplTest, StartAndFinishSpanNormalFlow) { return span; })); EXPECT_CALL(*span, finishSpan()); + EXPECT_CALL(*span, setTag(_, _)).Times(testing::AnyNumber()); + // Verify tag is set based on the request headers. + EXPECT_CALL(*span, setTag(":method", "GET")); EXPECT_CALL(runtime_.snapshot_, featureEnabled("tracing.global_enabled", 100, _)) .WillOnce(Return(true)); @@ -305,7 +309,7 @@ TEST_F(HttpConnectionManagerImplTest, DoNotStartSpanIfTracingIsNotEnabled) { setup(false, ""); // Disable tracing. - tracing_config_ = Optional(); + tracing_config_.reset(); EXPECT_CALL(tracer_, startSpan_(_, _, _)).Times(0); ON_CALL(runtime_.snapshot_, featureEnabled("tracing.global_enabled", 100, _)) diff --git a/test/common/http/conn_manager_utility_test.cc b/test/common/http/conn_manager_utility_test.cc index 058c549caa3ae..adc4aa015b5fa 100644 --- a/test/common/http/conn_manager_utility_test.cc +++ b/test/common/http/conn_manager_utility_test.cc @@ -24,8 +24,8 @@ class ConnectionManagerUtilityTest : public testing::Test { ConnectionManagerUtilityTest() { ON_CALL(config_, userAgent()).WillByDefault(ReturnRef(user_agent_)); - tracing_config_.value({Tracing::OperationName::Ingress}); - ON_CALL(config_, tracingConfig()).WillByDefault(ReturnRef(tracing_config_)); + tracing_config_ = {Tracing::OperationName::Ingress, {}}; + ON_CALL(config_, tracingConfig()).WillByDefault(Return(&tracing_config_)); } NiceMock connection_; @@ -34,7 +34,7 @@ class ConnectionManagerUtilityTest : public testing::Test { NiceMock route_config_; Optional user_agent_; NiceMock runtime_; - Optional tracing_config_; + Http::TracingConnectionManagerConfig tracing_config_; }; TEST_F(ConnectionManagerUtilityTest, generateStreamId) { diff --git a/test/common/tracing/http_tracer_impl_test.cc b/test/common/tracing/http_tracer_impl_test.cc index a531389c8d579..66094db919f9b 100644 --- a/test/common/tracing/http_tracer_impl_test.cc +++ b/test/common/tracing/http_tracer_impl_test.cc @@ -242,7 +242,8 @@ TEST(HttpTracerUtilityTest, OriginalAndLongPath) { EXPECT_CALL(*span, setTag(_, _)).Times(testing::AnyNumber()); EXPECT_CALL(*span, setTag("request_line", "GET " + expected_path + " HTTP/2")); - HttpTracerUtility::finalizeSpan(*span, request_headers, request_info); + NiceMock config; + HttpTracerUtility::finalizeSpan(*span, request_headers, request_info, config); } TEST(HttpTracerUtilityTest, SpanOptionalHeaders) { @@ -274,7 +275,8 @@ TEST(HttpTracerUtilityTest, SpanOptionalHeaders) { EXPECT_CALL(*span, setTag("response_flags", "-")); EXPECT_CALL(*span, finishSpan()); - HttpTracerUtility::finalizeSpan(*span, request_headers, request_info); + NiceMock config; + HttpTracerUtility::finalizeSpan(*span, request_headers, request_info, config); } TEST(HttpTracerUtilityTest, SpanPopulatedFailureResponse) { @@ -302,6 +304,18 @@ TEST(HttpTracerUtilityTest, SpanPopulatedFailureResponse) { EXPECT_CALL(*span, setTag("request_size", "10")); EXPECT_CALL(*span, setTag("guid:x-client-trace-id", "client_trace_id")); + // Check that span has tags from custom headers. + request_headers.addViaCopy(Http::LowerCaseString("aa"), "a"); + request_headers.addViaCopy(Http::LowerCaseString("bb"), "b"); + request_headers.addViaCopy(Http::LowerCaseString("cc"), "c"); + MockConfig config; + config.headers_.push_back(Http::LowerCaseString("aa")); + config.headers_.push_back(Http::LowerCaseString("cc")); + config.headers_.push_back(Http::LowerCaseString("ee")); + EXPECT_CALL(*span, setTag("aa", "a")); + EXPECT_CALL(*span, setTag("cc", "c")); + EXPECT_CALL(config, requestHeadersForTags()); + Optional response_code(503); EXPECT_CALL(request_info, responseCode()).WillRepeatedly(ReturnRef(response_code)); EXPECT_CALL(request_info, bytesSent()).WillOnce(Return(100)); @@ -314,7 +328,7 @@ TEST(HttpTracerUtilityTest, SpanPopulatedFailureResponse) { EXPECT_CALL(*span, setTag("response_flags", "UT")); EXPECT_CALL(*span, finishSpan()); - HttpTracerUtility::finalizeSpan(*span, request_headers, request_info); + HttpTracerUtility::finalizeSpan(*span, request_headers, request_info, config); } TEST(HttpTracerUtilityTest, operationTypeToString) { diff --git a/test/mocks/http/mocks.h b/test/mocks/http/mocks.h index f512268b1888d..8bed8375ddb5b 100644 --- a/test/mocks/http/mocks.h +++ b/test/mocks/http/mocks.h @@ -81,7 +81,7 @@ class MockConnectionManagerConfig : public ConnectionManagerConfig { MOCK_METHOD0(useRemoteAddress, bool()); MOCK_METHOD0(localAddress, const Network::Address::Instance&()); MOCK_METHOD0(userAgent, const Optional&()); - MOCK_METHOD0(tracingConfig, const Optional&()); + MOCK_METHOD0(tracingConfig, const Http::TracingConnectionManagerConfig*()); }; class MockConnectionCallbacks : public virtual ConnectionCallbacks { diff --git a/test/mocks/tracing/mocks.cc b/test/mocks/tracing/mocks.cc index 91be279d5805f..a83c394cb4c99 100644 --- a/test/mocks/tracing/mocks.cc +++ b/test/mocks/tracing/mocks.cc @@ -1,13 +1,17 @@ #include "mocks.h" using testing::Return; +using testing::ReturnRef; namespace Tracing { MockSpan::MockSpan() {} MockSpan::~MockSpan() {} -MockConfig::MockConfig() { ON_CALL(*this, operationName()).WillByDefault(Return(operation_name_)); } +MockConfig::MockConfig() { + ON_CALL(*this, operationName()).WillByDefault(Return(operation_name_)); + ON_CALL(*this, requestHeadersForTags()).WillByDefault(ReturnRef(headers_)); +} MockConfig::~MockConfig() {} MockHttpTracer::MockHttpTracer() {} diff --git a/test/mocks/tracing/mocks.h b/test/mocks/tracing/mocks.h index 719f4e95598e3..489a80604244b 100644 --- a/test/mocks/tracing/mocks.h +++ b/test/mocks/tracing/mocks.h @@ -15,8 +15,10 @@ class MockConfig : public Config { ~MockConfig(); MOCK_CONST_METHOD0(operationName, OperationName()); + MOCK_CONST_METHOD0(requestHeadersForTags, const std::vector&()); OperationName operation_name_{OperationName::Ingress}; + std::vector headers_; }; class MockSpan : public Span {