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 <douglas-reid@users.noreply.github.com>
envoyproxy · Sep 5, 2019 · 277e717 · 277e717
1 parent d11c7e7
commit 277e717
Show file tree

Hide file tree

Showing 15 changed files with 46 additions and 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
@@ -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
@@ -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 <envoy_api_field_config.filter.network.http_connection_manager.v2.HttpConnectionManager.tracing>` to support customizing the length of the request path included in the extracted `http.url <https://github.com/opentracing/specification/blob/master/semantic_conventions.md#standard-span-tags-and-log-fields>` tag.
 * upstream: added :ref:`an option <envoy_api_field_Cluster.CommonLbConfig.close_connections_on_host_set_change>` that allows draining HTTP, TCP connection pools on cluster membership change.
 * upstream: added network filter chains to upstream connections, see :ref:`filters<envoy_api_field_Cluster.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
@@ -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
@@ -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<TracingConnectionManagerConfig>;

diff --git 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
@@ -479,6 +479,7 @@ class ConnectionManagerImpl : Logger::Loggable<Logger::Id::http>,
     Tracing::OperationName operationName() const override;
     const std::vector<Http::LowerCaseString>& 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
@@ -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
@@ -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<Http::LowerCaseString> 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
@@ -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>(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
@@ -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<Tracing::MockSpan>();
   EXPECT_CALL(tracer_, startSpan_(_, _, _, _))
@@ -1062,7 +1064,8 @@ TEST_F(HttpConnectionManagerImplTest, StartAndFinishSpanNormalFlowEgressDecorato
                                      percent1,
                                      percent2,
                                      percent1,
-                                     false});
+                                     false,
+                                     256});
 
   auto* span = new NiceMock<Tracing::MockSpan>();
   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<const envoy::type::FractionalPercent&>(), _))

diff --git 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
@@ -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<uint32_t> 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
@@ -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>({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
@@ -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
@@ -18,6 +18,7 @@ class MockConfig : public Config {
   MOCK_CONST_METHOD0(operationName, OperationName());
   MOCK_CONST_METHOD0(requestHeadersForTags, const std::vector<Http::LowerCaseString>&());
   MOCK_CONST_METHOD0(verbose, bool());
+  MOCK_CONST_METHOD0(maxPathTagLength, uint32_t());
 
   OperationName operation_name_{OperationName::Ingress};
   std::vector<Http::LowerCaseString> headers_;