From 277e7175091cdf9d6c8732bc40381ff4e0fee48c Mon Sep 17 00:00:00 2001 From: Douglas Reid Date: Thu, 5 Sep 2019 11:26:43 -0700 Subject: [PATCH] http conn man: add tracing config for path length in tag (#8095) This PR adds a configuration option for controlling the length of the request path that is included in the HttpUrl span tag. Currently, this length is hard-coded to 256. With this PR, that length will be configurable (defaulting to the old value). Risk Level: Low Testing: Unit Docs Changes: Inline with the API proto. Documented new field. Release Notes: Added in the PR. Related issue: istio/istio#14563 Signed-off-by: Douglas Reid --- .../v2/http_connection_manager.proto | 5 +++++ docs/root/intro/version_history.rst | 1 + include/envoy/tracing/http_tracer.h | 7 +++++++ source/common/http/conn_manager_config.h | 1 + source/common/http/conn_manager_impl.cc | 4 ++++ source/common/http/conn_manager_impl.h | 1 + source/common/tracing/http_tracer_impl.cc | 8 +++++--- source/common/tracing/http_tracer_impl.h | 1 + .../network/http_connection_manager/config.cc | 6 +++++- test/common/http/conn_manager_impl_test.cc | 12 ++++++++---- test/common/http/conn_manager_utility_test.cc | 3 ++- test/common/tracing/http_tracer_impl_test.cc | 1 + .../network/http_connection_manager/config_test.cc | 3 +++ test/mocks/tracing/mocks.cc | 1 + test/mocks/tracing/mocks.h | 1 + 15 files changed, 46 insertions(+), 9 deletions(-) diff --git a/api/envoy/config/filter/network/http_connection_manager/v2/http_connection_manager.proto b/api/envoy/config/filter/network/http_connection_manager/v2/http_connection_manager.proto index 735cde619558..86076823cc7e 100644 --- a/api/envoy/config/filter/network/http_connection_manager/v2/http_connection_manager.proto +++ b/api/envoy/config/filter/network/http_connection_manager/v2/http_connection_manager.proto @@ -130,6 +130,11 @@ message HttpConnectionManager { // Whether to annotate spans with additional data. If true, spans will include logs for stream // events. bool verbose = 6; + + // Maximum length of the request path to extract and include in the HttpUrl tag. Used to + // truncate lengthy request paths to meet the needs of a tracing backend. + // Default: 256 + google.protobuf.UInt32Value max_path_tag_length = 7; } // Presence of the object defines whether the connection manager diff --git a/docs/root/intro/version_history.rst b/docs/root/intro/version_history.rst index b10f9ce98b29..eebe4f9dd960 100644 --- a/docs/root/intro/version_history.rst +++ b/docs/root/intro/version_history.rst @@ -52,6 +52,7 @@ Version history * tracing: added support to the Zipkin reporter for sending list of spans as Zipkin JSON v2 and protobuf message over HTTP. certificate validation context. * tracing: added tags for gRPC response status and meesage. +* tracing: added :ref:`max_path_tag_length ` to support customizing the length of the request path included in the extracted `http.url ` tag. * upstream: added :ref:`an option ` that allows draining HTTP, TCP connection pools on cluster membership change. * upstream: added network filter chains to upstream connections, see :ref:`filters`. * upstream: use p2c to select hosts for least-requests load balancers if all host weights are the same, even in cases where weights are not equal to 1. diff --git a/include/envoy/tracing/http_tracer.h b/include/envoy/tracing/http_tracer.h index eb6f4960b62a..ec53d00e4bcc 100644 --- a/include/envoy/tracing/http_tracer.h +++ b/include/envoy/tracing/http_tracer.h @@ -11,6 +11,8 @@ namespace Envoy { namespace Tracing { +constexpr uint32_t DefaultMaxPathTagLength = 256; + enum class OperationName { Ingress, Egress }; /** @@ -58,6 +60,11 @@ class Config { * @return true if spans should be annotated with more detailed information. */ virtual bool verbose() const PURE; + + /** + * @return the maximum length allowed for paths in the extracted HttpUrl tag. + */ + virtual uint32_t maxPathTagLength() const PURE; }; class Span; diff --git a/source/common/http/conn_manager_config.h b/source/common/http/conn_manager_config.h index b32d8f520e40..ce4682de2a15 100644 --- a/source/common/http/conn_manager_config.h +++ b/source/common/http/conn_manager_config.h @@ -108,6 +108,7 @@ struct TracingConnectionManagerConfig { envoy::type::FractionalPercent random_sampling_; envoy::type::FractionalPercent overall_sampling_; bool verbose_; + uint32_t max_path_tag_length_; }; using TracingConnectionManagerConfigPtr = std::unique_ptr; diff --git a/source/common/http/conn_manager_impl.cc b/source/common/http/conn_manager_impl.cc index 102373f9cadf..e8bde0714898 100644 --- a/source/common/http/conn_manager_impl.cc +++ b/source/common/http/conn_manager_impl.cc @@ -1757,6 +1757,10 @@ bool ConnectionManagerImpl::ActiveStream::verbose() const { return connection_manager_.config_.tracingConfig()->verbose_; } +uint32_t ConnectionManagerImpl::ActiveStream::maxPathTagLength() const { + return connection_manager_.config_.tracingConfig()->max_path_tag_length_; +} + void ConnectionManagerImpl::ActiveStream::callHighWatermarkCallbacks() { ++high_watermark_count_; for (auto watermark_callbacks : watermark_callbacks_) { diff --git a/source/common/http/conn_manager_impl.h b/source/common/http/conn_manager_impl.h index 661871ea991a..f6dd0ceb662c 100644 --- a/source/common/http/conn_manager_impl.h +++ b/source/common/http/conn_manager_impl.h @@ -479,6 +479,7 @@ class ConnectionManagerImpl : Logger::Loggable, Tracing::OperationName operationName() const override; const std::vector& requestHeadersForTags() const override; bool verbose() const override; + uint32_t maxPathTagLength() const override; // ScopeTrackedObject void dumpState(std::ostream& os, int indent_level = 0) const override { diff --git a/source/common/tracing/http_tracer_impl.cc b/source/common/tracing/http_tracer_impl.cc index 004e41c84a86..35bee4ff7de1 100644 --- a/source/common/tracing/http_tracer_impl.cc +++ b/source/common/tracing/http_tracer_impl.cc @@ -27,11 +27,12 @@ static std::string valueOrDefault(const Http::HeaderEntry* header, const char* d return header ? std::string(header->value().getStringView()) : default_value; } -static std::string buildUrl(const Http::HeaderMap& request_headers) { +static std::string buildUrl(const Http::HeaderMap& request_headers, + const uint32_t max_path_length) { std::string path(request_headers.EnvoyOriginalPath() ? request_headers.EnvoyOriginalPath()->value().getStringView() : request_headers.Path()->value().getStringView()); - static const size_t max_path_length = 256; + if (path.length() > max_path_length) { path = path.substr(0, max_path_length); } @@ -148,7 +149,8 @@ void HttpTracerUtility::finalizeSpan(Span& span, const Http::HeaderMap* request_ span.setTag(Tracing::Tags::get().GuidXRequestId, std::string(request_headers->RequestId()->value().getStringView())); } - span.setTag(Tracing::Tags::get().HttpUrl, buildUrl(*request_headers)); + span.setTag(Tracing::Tags::get().HttpUrl, + buildUrl(*request_headers, tracing_config.maxPathTagLength())); span.setTag(Tracing::Tags::get().HttpMethod, std::string(request_headers->Method()->value().getStringView())); span.setTag(Tracing::Tags::get().DownstreamCluster, diff --git a/source/common/tracing/http_tracer_impl.h b/source/common/tracing/http_tracer_impl.h index ec421736eab4..e7560acac0ea 100644 --- a/source/common/tracing/http_tracer_impl.h +++ b/source/common/tracing/http_tracer_impl.h @@ -118,6 +118,7 @@ class EgressConfigImpl : public Config { return request_headers_for_tags_; } bool verbose() const override { return false; } + uint32_t maxPathTagLength() const override { return Tracing::DefaultMaxPathTagLength; } private: const std::vector request_headers_for_tags_{}; diff --git a/source/extensions/filters/network/http_connection_manager/config.cc b/source/extensions/filters/network/http_connection_manager/config.cc index a229ecc08008..532f0e086e5d 100644 --- a/source/extensions/filters/network/http_connection_manager/config.cc +++ b/source/extensions/filters/network/http_connection_manager/config.cc @@ -8,6 +8,7 @@ #include "envoy/config/filter/network/http_connection_manager/v2/http_connection_manager.pb.validate.h" #include "envoy/filesystem/filesystem.h" #include "envoy/server/admin.h" +#include "envoy/tracing/http_tracer.h" #include "common/access_log/access_log_impl.h" #include "common/common/fmt.h" @@ -288,10 +289,13 @@ HttpConnectionManagerConfig::HttpConnectionManagerConfig( overall_sampling.set_numerator( tracing_config.has_overall_sampling() ? tracing_config.overall_sampling().value() : 100); + const uint32_t max_path_tag_length = PROTOBUF_GET_WRAPPED_OR_DEFAULT( + tracing_config, max_path_tag_length, Tracing::DefaultMaxPathTagLength); + tracing_config_ = std::make_unique(Http::TracingConnectionManagerConfig{ tracing_operation_name, request_headers_for_tags, client_sampling, random_sampling, - overall_sampling, tracing_config.verbose()}); + overall_sampling, tracing_config.verbose(), max_path_tag_length}); } for (const auto& access_log : config.access_log()) { diff --git a/test/common/http/conn_manager_impl_test.cc b/test/common/http/conn_manager_impl_test.cc index ebf2c71d59ad..179367e62589 100644 --- a/test/common/http/conn_manager_impl_test.cc +++ b/test/common/http/conn_manager_impl_test.cc @@ -128,7 +128,8 @@ class HttpConnectionManagerImplTest : public testing::Test, public ConnectionMan percent1, percent2, percent1, - false}); + false, + 256}); } } @@ -984,7 +985,8 @@ TEST_F(HttpConnectionManagerImplTest, StartAndFinishSpanNormalFlowEgressDecorato percent1, percent2, percent1, - false}); + false, + 256}); auto* span = new NiceMock(); EXPECT_CALL(tracer_, startSpan_(_, _, _, _)) @@ -1062,7 +1064,8 @@ TEST_F(HttpConnectionManagerImplTest, StartAndFinishSpanNormalFlowEgressDecorato percent1, percent2, percent1, - false}); + false, + 256}); auto* span = new NiceMock(); EXPECT_CALL(tracer_, startSpan_(_, _, _, _)) @@ -1135,7 +1138,8 @@ TEST_F(HttpConnectionManagerImplTest, percent1, percent2, percent1, - false}); + false, + 256}); EXPECT_CALL(runtime_.snapshot_, featureEnabled("tracing.global_enabled", An(), _)) diff --git a/test/common/http/conn_manager_utility_test.cc b/test/common/http/conn_manager_utility_test.cc index b0c8e005124f..a9694d8ec785 100644 --- a/test/common/http/conn_manager_utility_test.cc +++ b/test/common/http/conn_manager_utility_test.cc @@ -102,7 +102,8 @@ class ConnectionManagerUtilityTest : public testing::Test { envoy::type::FractionalPercent percent2; percent2.set_numerator(10000); percent2.set_denominator(envoy::type::FractionalPercent::TEN_THOUSAND); - tracing_config_ = {Tracing::OperationName::Ingress, {}, percent1, percent2, percent1, false}; + tracing_config_ = { + Tracing::OperationName::Ingress, {}, percent1, percent2, percent1, false, 256}; ON_CALL(config_, tracingConfig()).WillByDefault(Return(&tracing_config_)); ON_CALL(config_, via()).WillByDefault(ReturnRef(via_)); diff --git a/test/common/tracing/http_tracer_impl_test.cc b/test/common/tracing/http_tracer_impl_test.cc index 4e1b991507f3..c78fa72e4106 100644 --- a/test/common/tracing/http_tracer_impl_test.cc +++ b/test/common/tracing/http_tracer_impl_test.cc @@ -336,6 +336,7 @@ TEST(HttpConnManFinalizerImpl, SpanPopulatedFailureResponse) { EXPECT_CALL(span, setTag(Eq("cc"), Eq("c"))); EXPECT_CALL(config, requestHeadersForTags()); EXPECT_CALL(config, verbose).WillOnce(Return(false)); + EXPECT_CALL(config, maxPathTagLength).WillOnce(Return(256)); absl::optional response_code(503); EXPECT_CALL(stream_info, responseCode()).WillRepeatedly(ReturnPointee(&response_code)); diff --git a/test/extensions/filters/network/http_connection_manager/config_test.cc b/test/extensions/filters/network/http_connection_manager/config_test.cc index c039d7a89c3f..b16fbfc97ebb 100644 --- a/test/extensions/filters/network/http_connection_manager/config_test.cc +++ b/test/extensions/filters/network/http_connection_manager/config_test.cc @@ -224,6 +224,7 @@ stat_prefix: router operation_name: ingress request_headers_for_tags: - foo + max_path_tag_length: 128 http_filters: - name: envoy.router config: {} @@ -235,6 +236,7 @@ stat_prefix: router EXPECT_THAT(std::vector({Http::LowerCaseString("foo")}), ContainerEq(config.tracingConfig()->request_headers_for_tags_)); + EXPECT_EQ(128, config.tracingConfig()->max_path_tag_length_); EXPECT_EQ(*context_.local_info_.address_, config.localAddress()); EXPECT_EQ("foo", config.serverName()); EXPECT_EQ(HttpConnectionManagerConfig::HttpConnectionManagerProto::OVERWRITE, @@ -316,6 +318,7 @@ TEST_F(HttpConnectionManagerConfigTest, SamplingDefault) { scoped_routes_config_provider_manager_); EXPECT_EQ(100, config.tracingConfig()->client_sampling_.numerator()); + EXPECT_EQ(Tracing::DefaultMaxPathTagLength, config.tracingConfig()->max_path_tag_length_); EXPECT_EQ(envoy::type::FractionalPercent::HUNDRED, config.tracingConfig()->client_sampling_.denominator()); EXPECT_EQ(10000, config.tracingConfig()->random_sampling_.numerator()); diff --git a/test/mocks/tracing/mocks.cc b/test/mocks/tracing/mocks.cc index db9be3902320..3a688957f0af 100644 --- a/test/mocks/tracing/mocks.cc +++ b/test/mocks/tracing/mocks.cc @@ -16,6 +16,7 @@ MockConfig::MockConfig() { ON_CALL(*this, operationName()).WillByDefault(Return(operation_name_)); ON_CALL(*this, requestHeadersForTags()).WillByDefault(ReturnRef(headers_)); ON_CALL(*this, verbose()).WillByDefault(Return(verbose_)); + ON_CALL(*this, maxPathTagLength()).WillByDefault(Return(uint32_t(256))); } MockConfig::~MockConfig() = default; diff --git a/test/mocks/tracing/mocks.h b/test/mocks/tracing/mocks.h index 22a694edfd9a..1cd5b3a0c9c0 100644 --- a/test/mocks/tracing/mocks.h +++ b/test/mocks/tracing/mocks.h @@ -18,6 +18,7 @@ class MockConfig : public Config { MOCK_CONST_METHOD0(operationName, OperationName()); MOCK_CONST_METHOD0(requestHeadersForTags, const std::vector&()); MOCK_CONST_METHOD0(verbose, bool()); + MOCK_CONST_METHOD0(maxPathTagLength, uint32_t()); OperationName operation_name_{OperationName::Ingress}; std::vector headers_;