perf: various changes (#118)

1) Don't use fmt::format() in HTTP/1.1 codec hot path 2) Allow x-request-id generation to be disabled 3) Don't do tracing mutation if tracing is not enabled 3) Allow router dynamic stats to be disabled 4) Don't generate useless random numbers for null runtime
envoyproxy · Oct 5, 2016 · 6591604 · 6591604
1 parent e478d67
commit 6591604
Show file tree

Hide file tree

Showing 20 changed files with 84 additions and 26 deletions.
diff --git a/ci/do_ci.sh b/ci/do_ci.sh
@@ -25,7 +25,7 @@ else
   TEST_TARGET="envoy.check"
 fi
 
-mkdir build
+mkdir -p build
 cd build
 
 cmake \

diff --git a/docs/configuration/http_conn_man/http_conn_man.rst b/docs/configuration/http_conn_man/http_conn_man.rst
@@ -23,7 +23,8 @@ HTTP connection manager
       "idle_timeout_s": "...",
       "drain_timeout_ms": "...",
       "access_log": [],
-      "use_remote_address": "..."
+      "use_remote_address": "...",
+      "generate_request_id": "..."
     }
   }
 
@@ -130,6 +131,12 @@ use_remote_address
   :ref:`config_http_conn_man_headers_x-envoy-internal`, and
   :ref:`config_http_conn_man_headers_x-envoy-external-address` for more information.
 
+generate_request_id
+  *(optional, boolean)* Whether the connection manager will generate the
+  :ref:`config_http_conn_man_headers_x-request-id` header if it does not exist. This defaults to
+  *true*. Generating a random UUID4 is expensive so in high throughput scenarios where this
+  feature is not desired it can be disabled.
+
 .. toctree::
   :hidden:
 

diff --git a/docs/configuration/http_filters/router_filter.rst b/docs/configuration/http_filters/router_filter.rst
@@ -13,9 +13,16 @@ redirection, the filter also handles retry, statistics, etc.
   {
     "type": "decoder",
     "name": "router",
-    "config": {}
+    "config": {
+      "dynamic_stats": "..."
+    }
   }
 
+dynamic_stats
+  *(optional, boolean)* Whether the router generates :ref:`dynamic cluster statistics
+  <config_cluster_manager_cluster_stats_dynamic_http>`. Defaults to *true*. Can be disabled in high
+  performance scenarios.
+
 .. _config_http_filters_router_headers:
 
 HTTP headers

diff --git a/source/common/http/async_client_impl.cc b/source/common/http/async_client_impl.cc
@@ -14,7 +14,7 @@ AsyncClientImpl::AsyncClientImpl(const Upstream::Cluster& cluster, Stats::Store&
                                  Router::ShadowWriterPtr&& shadow_writer,
                                  const std::string& local_address)
     : cluster_(cluster), config_("http.async-client.", local_zone_name, stats_store, cm, runtime,
-                                 random, std::move(shadow_writer)),
+                                 random, std::move(shadow_writer), true),
       dispatcher_(dispatcher), local_address_(local_address) {}
 
 AsyncClientImpl::~AsyncClientImpl() { ASSERT(active_requests_.empty()); }

diff --git a/source/common/http/conn_manager_impl.h b/source/common/http/conn_manager_impl.h
@@ -113,6 +113,12 @@ class ConnectionManagerConfig {
    */
   virtual FilterChainFactory& filterFactory() PURE;
 
+  /**
+   * @return whether the connection manager will generate a fresh x-request-id if the request does
+   *         not have one.
+   */
+  virtual bool generateRequestId() PURE;
+
   /**
    * @return optional idle timeout for incoming connection manager connections.
    */

diff --git a/source/common/http/conn_manager_utility.cc b/source/common/http/conn_manager_utility.cc
@@ -88,7 +88,8 @@ void ConnectionManagerUtility::mutateRequestHeaders(Http::HeaderMap& request_hea
   }
 
   // Generate x-request-id for all edge requests, or if there is none.
-  if (edge_request || request_headers.get(Headers::get().RequestId).empty()) {
+  if (config.generateRequestId() &&
+      (edge_request || request_headers.get(Headers::get().RequestId).empty())) {
     std::string uuid = "";
 
     try {
@@ -103,7 +104,9 @@ void ConnectionManagerUtility::mutateRequestHeaders(Http::HeaderMap& request_hea
     }
   }
 
-  Tracing::HttpTracerUtility::mutateHeaders(request_headers, runtime);
+  if (config.isTracing()) {
+    Tracing::HttpTracerUtility::mutateHeaders(request_headers, runtime);
+  }
 }
 
 void ConnectionManagerUtility::mutateResponseHeaders(Http::HeaderMap& response_headers,

diff --git a/source/common/http/http1/codec_impl.cc b/source/common/http/http1/codec_impl.cc
@@ -18,7 +18,10 @@ const std::string StreamEncoderImpl::CRLF = "\r\n";
 const std::string StreamEncoderImpl::LAST_CHUNK = "0\r\n\r\n";
 
 void StreamEncoderImpl::encodeHeader(const std::string& key, const std::string& value) {
-  output_buffer_.add(fmt::format("{}: {}\r\n", key, value));
+  output_buffer_.add(key);
+  output_buffer_.add(": ");
+  output_buffer_.add(value);
+  output_buffer_.add(CRLF);
 }
 
 void StreamEncoderImpl::encodeHeaders(const HeaderMap& headers, bool end_stream) {
@@ -125,8 +128,11 @@ void ResponseStreamEncoderImpl::encodeHeaders(const HeaderMap& headers, bool end
   started_response_ = true;
   uint64_t numeric_status = Utility::getResponseStatus(headers);
 
-  output_buffer_.add(fmt::format("HTTP/1.1 {} {}\r\n", numeric_status,
-                                 CodeUtility::toString(static_cast<Code>(numeric_status))));
+  output_buffer_.add("HTTP/1.1 ");
+  output_buffer_.add(std::to_string(numeric_status));
+  output_buffer_.add(" ");
+  output_buffer_.add(CodeUtility::toString(static_cast<Code>(numeric_status)));
+  output_buffer_.add(CRLF);
   StreamEncoderImpl::encodeHeaders(headers, end_stream);
 }
 
@@ -141,7 +147,10 @@ void RequestStreamEncoderImpl::encodeHeaders(const HeaderMap& headers, bool end_
     head_request_ = true;
   }
 
-  output_buffer_.add(fmt::format("{} {} HTTP/1.1\r\n", method, path));
+  output_buffer_.add(method);
+  output_buffer_.add(" ");
+  output_buffer_.add(path);
+  output_buffer_.add(" HTTP/1.1\r\n");
   StreamEncoderImpl::encodeHeaders(headers, end_stream);
 }
 
@@ -265,8 +274,14 @@ void ConnectionImpl::onHeaderValue(const char* data, size_t length) {
 int ConnectionImpl::onHeadersCompleteBase() {
   conn_log_trace("headers complete", connection_);
   completeLastHeader();
-  current_header_map_->addViaMoveValue(
-      Headers::get().Version, fmt::format("HTTP/{}.{}", parser_.http_major, parser_.http_minor));
+  if (parser_.http_major == 1 && parser_.http_minor == 1) {
+    current_header_map_->addViaMoveValue(Headers::get().Version, "HTTP/1.1");
+  } else {
+    // This is not necessarily true, but it's good enough since higher layers only care if this is
+    // HTTP/1.1 or not.
+    current_header_map_->addViaMoveValue(Headers::get().Version, "HTTP/1.0");
+  }
+
   int rc = onHeadersComplete(std::move(current_header_map_));
   current_header_map_.reset();
   header_parsing_state_ = HeaderParsingState::Done;

diff --git a/source/common/http/http1/codec_impl.h b/source/common/http/http1/codec_impl.h
@@ -40,6 +40,9 @@ class StreamEncoderImpl : public StreamEncoder, public Stream, Logger::Loggable<
 protected:
   StreamEncoderImpl(ConnectionImpl& connection) : connection_(connection) {}
 
+  static const std::string CRLF;
+  static const std::string LAST_CHUNK;
+
   ConnectionImpl& connection_;
   Buffer::OwnedImpl output_buffer_;
 
@@ -61,9 +64,6 @@ class StreamEncoderImpl : public StreamEncoder, public Stream, Logger::Loggable<
    */
   void flushOutput();
 
-  static const std::string CRLF;
-  static const std::string LAST_CHUNK;
-
   std::list<StreamCallbacks*> callbacks_{};
   bool chunk_encoding_{true};
 };

diff --git a/source/common/router/router.cc b/source/common/router/router.cc
@@ -94,9 +94,10 @@ const std::string& Filter::upstreamZone() {
 }
 
 void Filter::chargeUpstreamCode(const Http::HeaderMap& response_headers) {
-  bool is_canary = (response_headers.get(Http::Headers::get().EnvoyUpstreamCanary) == "true") ||
-                   (upstream_host_ ? upstream_host_->canary() : false);
-  if (!callbacks_->requestInfo().healthCheck()) {
+  if (config_.emit_dynamic_stats_ && !callbacks_->requestInfo().healthCheck()) {
+    bool is_canary = (response_headers.get(Http::Headers::get().EnvoyUpstreamCanary) == "true") ||
+                     (upstream_host_ ? upstream_host_->canary() : false);
+
     Http::CodeUtility::ResponseStatInfo info{
         config_.stats_store_, stat_prefix_, response_headers,
         downstream_headers_->get(Http::Headers::get().EnvoyInternalRequest) == "true",
@@ -429,7 +430,7 @@ void Filter::onUpstreamComplete() {
     upstream_request_->upstream_encoder_->resetStream();
   }
 
-  if (!callbacks_->requestInfo().healthCheck()) {
+  if (config_.emit_dynamic_stats_ && !callbacks_->requestInfo().healthCheck()) {
     Http::CodeUtility::ResponseTimingInfo info{
         config_.stats_store_, stat_prefix_,
         upstream_request_->upstream_encoder_->requestCompleteTime(),

diff --git a/source/common/router/router.h b/source/common/router/router.h
@@ -67,10 +67,11 @@ class FilterConfig {
 public:
   FilterConfig(const std::string& stat_prefix, const std::string& service_zone, Stats::Store& stats,
                Upstream::ClusterManager& cm, Runtime::Loader& runtime,
-               Runtime::RandomGenerator& random, ShadowWriterPtr&& shadow_writer)
+               Runtime::RandomGenerator& random, ShadowWriterPtr&& shadow_writer,
+               bool emit_dynamic_stats)
       : stats_store_(stats), service_zone_(service_zone), cm_(cm), runtime_(runtime),
         random_(random), stats_{ALL_ROUTER_STATS(POOL_COUNTER_PREFIX(stats, stat_prefix))},
-        shadow_writer_(std::move(shadow_writer)) {}
+        emit_dynamic_stats_(emit_dynamic_stats), shadow_writer_(std::move(shadow_writer)) {}
 
   ShadowWriter& shadowWriter() { return *shadow_writer_; }
 
@@ -80,6 +81,7 @@ class FilterConfig {
   Runtime::Loader& runtime_;
   Runtime::RandomGenerator& random_;
   FilterStats stats_;
+  const bool emit_dynamic_stats_;
 
 private:
   ShadowWriterPtr shadow_writer_;

diff --git a/source/common/runtime/runtime_impl.h b/source/common/runtime/runtime_impl.h
@@ -159,7 +159,13 @@ class NullLoaderImpl : public Loader {
     }
 
     bool featureEnabled(const std::string& key, uint64_t default_value) const override {
-      return featureEnabled(key, default_value, generator_.random());
+      if (default_value == 0) {
+        return false;
+      } else if (default_value == 100) {
+        return true;
+      } else {
+        return featureEnabled(key, default_value, generator_.random());
+      }
     }
 
     bool featureEnabled(const std::string& key, uint64_t default_value,

diff --git a/source/server/config/http/router.cc b/source/server/config/http/router.cc
@@ -11,7 +11,8 @@ namespace Configuration {
 class FilterConfig : public HttpFilterConfigFactory {
 public:
   HttpFilterFactoryCb tryCreateFilterFactory(HttpFilterType type, const std::string& name,
-                                             const Json::Object&, const std::string& stat_prefix,
+                                             const Json::Object& json_config,
+                                             const std::string& stat_prefix,
                                              Instance& server) override {
     if (type != HttpFilterType::Decoder || name != "router") {
       return nullptr;
@@ -20,7 +21,8 @@ class FilterConfig : public HttpFilterConfigFactory {
     Router::FilterConfigPtr config(new Router::FilterConfig(
         stat_prefix, server.options().serviceZone(), server.stats(), server.clusterManager(),
         server.runtime(), server.random(),
-        Router::ShadowWriterPtr{new Router::ShadowWriterImpl(server.clusterManager())}));
+        Router::ShadowWriterPtr{new Router::ShadowWriterImpl(server.clusterManager())},
+        json_config.getBoolean("dynamic_stats", true)));
 
     return [config](Http::FilterChainFactoryCallbacks& callbacks) -> void {
       callbacks.addStreamDecoderFilter(

diff --git a/source/server/config/network/http_connection_manager.cc b/source/server/config/network/http_connection_manager.cc
@@ -75,7 +75,8 @@ HttpConnectionManagerConfig::HttpConnectionManagerConfig(const Json::Object& con
       codec_options_(Http::Utility::parseCodecOptions(config)),
       route_config_(new Router::ConfigImpl(config.getObject("route_config"), server.runtime(),
                                            server.clusterManager())),
-      drain_timeout_(config.getInteger("drain_timeout_ms", 5000)) {
+      drain_timeout_(config.getInteger("drain_timeout_ms", 5000)),
+      generate_request_id_(config.getBoolean("generate_request_id", true)) {
 
   if (config.hasObject("use_remote_address")) {
     use_remote_address_ = config.getBoolean("use_remote_address");

diff --git a/source/server/config/network/http_connection_manager.h b/source/server/config/network/http_connection_manager.h
@@ -64,6 +64,7 @@ class HttpConnectionManagerConfig : Logger::Loggable<Logger::Id::config>,
                                         Http::ServerConnectionCallbacks& callbacks) override;
   std::chrono::milliseconds drainTimeout() override { return drain_timeout_; }
   FilterChainFactory& filterFactory() override { return *this; }
+  bool generateRequestId() override { return generate_request_id_; }
   const Optional<std::chrono::milliseconds>& idleTimeout() override { return idle_timeout_; }
   const Router::Config& routeConfig() override { return *route_config_; }
   const std::string& serverName() override { return server_name_; }
@@ -103,6 +104,7 @@ class HttpConnectionManagerConfig : Logger::Loggable<Logger::Id::config>,
   Optional<std::chrono::milliseconds> idle_timeout_;
   Router::ConfigPtr route_config_;
   std::chrono::milliseconds drain_timeout_;
+  bool generate_request_id_;
 };
 
 /**

diff --git a/source/server/http/admin.h b/source/server/http/admin.h
@@ -44,6 +44,7 @@ class AdminImpl : public Admin,
                                         Http::ServerConnectionCallbacks& callbacks) override;
   std::chrono::milliseconds drainTimeout() override { return std::chrono::milliseconds(100); }
   Http::FilterChainFactory& filterFactory() override { return *this; }
+  bool generateRequestId() override { return false; }
   const Optional<std::chrono::milliseconds>& idleTimeout() override { return idle_timeout_; }
   const Router::Config& routeConfig() override { return *route_config_; }
   const std::string& serverName() override {

diff --git a/test/common/http/conn_manager_impl_test.cc b/test/common/http/conn_manager_impl_test.cc
@@ -69,6 +69,7 @@ class HttpConnectionManagerImplTest : public Test, public ConnectionManagerConfi
   }
   std::chrono::milliseconds drainTimeout() override { return std::chrono::milliseconds(100); }
   FilterChainFactory& filterFactory() override { return filter_factory_; }
+  bool generateRequestId() override { return true; }
   const Optional<std::chrono::milliseconds>& idleTimeout() override { return idle_timeout_; }
   const Router::Config& routeConfig() override { return route_config_; }
   const std::string& serverName() override { return server_name_; }

diff --git a/test/common/http/conn_manager_utility_test.cc b/test/common/http/conn_manager_utility_test.cc
@@ -91,6 +91,7 @@ TEST_F(ConnectionManagerUtilityTest, InternalServiceForceTrace) {
   const std::string uuid = "f4dca0a9-12c7-4307-8002-969403baf480";
 
   ON_CALL(config_, useRemoteAddress()).WillByDefault(Return(false));
+  ON_CALL(config_, isTracing()).WillByDefault(Return(true));
 
   {
     // Internal request, make traceable
@@ -127,6 +128,7 @@ TEST_F(ConnectionManagerUtilityTest, EdgeRequestRegenerateRequestIdAndWipeDownst
   ON_CALL(connection_, remoteAddress()).WillByDefault(ReturnRef(external_remote_address));
   ON_CALL(runtime_.snapshot_, featureEnabled("tracing.global_enabled", 100, _))
       .WillByDefault(Return(true));
+  ON_CALL(config_, isTracing()).WillByDefault(Return(true));
 
   {
     HeaderMapImpl headers{{"x-envoy-downstream-service-cluster", "foo"},

diff --git a/test/common/router/router_test.cc b/test/common/router/router_test.cc
@@ -38,7 +38,7 @@ class RouterTest : public testing::Test {
   RouterTest()
       : shadow_writer_(new MockShadowWriter()),
         config_("test.", "from_az", stats_store_, cm_, runtime_, random_,
-                ShadowWriterPtr{shadow_writer_}),
+                ShadowWriterPtr{shadow_writer_}, true),
         router_(config_) {
     router_.setDecoderFilterCallbacks(callbacks_);
     ON_CALL(*cm_.conn_pool_.host_, url()).WillByDefault(ReturnRef(host_url_));

diff --git a/test/mocks/http/mocks.cc b/test/mocks/http/mocks.cc
@@ -13,6 +13,7 @@ namespace Http {
 
 MockConnectionManagerConfig::MockConnectionManagerConfig() {
   ON_CALL(*this, routeConfig()).WillByDefault(ReturnRef(route_config_));
+  ON_CALL(*this, generateRequestId()).WillByDefault(Return(true));
 }
 
 MockConnectionManagerConfig::~MockConnectionManagerConfig() {}

diff --git a/test/mocks/http/mocks.h b/test/mocks/http/mocks.h
@@ -65,6 +65,7 @@ class MockConnectionManagerConfig : public ConnectionManagerConfig {
                                                ServerConnectionCallbacks&));
   MOCK_METHOD0(drainTimeout, std::chrono::milliseconds());
   MOCK_METHOD0(filterFactory, FilterChainFactory&());
+  MOCK_METHOD0(generateRequestId, bool());
   MOCK_METHOD0(idleTimeout, const Optional<std::chrono::milliseconds>&());
   MOCK_METHOD0(routeConfig, Router::Config&());
   MOCK_METHOD0(serverName, const std::string&());