From 6591604546dbc2a71084bbf436ffece99541027b Mon Sep 17 00:00:00 2001 From: Matt Klein Date: Wed, 5 Oct 2016 10:31:47 -0700 Subject: [PATCH] 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 --- ci/do_ci.sh | 2 +- .../http_conn_man/http_conn_man.rst | 9 ++++++- .../http_filters/router_filter.rst | 9 ++++++- source/common/http/async_client_impl.cc | 2 +- source/common/http/conn_manager_impl.h | 6 +++++ source/common/http/conn_manager_utility.cc | 7 +++-- source/common/http/http1/codec_impl.cc | 27 ++++++++++++++----- source/common/http/http1/codec_impl.h | 6 ++--- source/common/router/router.cc | 9 ++++--- source/common/router/router.h | 6 +++-- source/common/runtime/runtime_impl.h | 8 +++++- source/server/config/http/router.cc | 6 +++-- .../config/network/http_connection_manager.cc | 3 ++- .../config/network/http_connection_manager.h | 2 ++ source/server/http/admin.h | 1 + test/common/http/conn_manager_impl_test.cc | 1 + test/common/http/conn_manager_utility_test.cc | 2 ++ test/common/router/router_test.cc | 2 +- test/mocks/http/mocks.cc | 1 + test/mocks/http/mocks.h | 1 + 20 files changed, 84 insertions(+), 26 deletions(-) diff --git a/ci/do_ci.sh b/ci/do_ci.sh index 421b2496bf91..45c12c587d27 100755 --- 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 index 72a08094ee47..257978745106 100644 --- 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 index 64f6350edc73..a5d57dcf3fd8 100644 --- 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 + `. 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 index 896f1288d3a4..c5a716d68ecf 100644 --- 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 index f6db6c337303..b60936610d5e 100644 --- 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 index a51f75981c92..f37a7331f555 100644 --- 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 index 9eb98490cc4f..10dbc9c78966 100644 --- 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(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(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 index 3823818013ea..20140e4ace1e 100644 --- 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 callbacks_{}; bool chunk_encoding_{true}; }; diff --git a/source/common/router/router.cc b/source/common/router/router.cc index 168f1745054e..3bfe20043bb2 100644 --- 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 index d60223b3d5c0..c87be5e2627c 100644 --- 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 index d94a1429f822..fc40077bd7d4 100644 --- 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 index 5ab3002808ee..0f7f78776ce3 100644 --- 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 index c351066aaab2..9a68b1fc215f 100644 --- 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 index 5a7c780a0a5a..8fd05d652d6f 100644 --- 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, 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& 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, Optional 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 index 8ee0135d8d14..480768f46dda 100644 --- 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& 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 index 4ff3c32bbf59..ad7d91645dc4 100644 --- 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& 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 index d4cc221887f8..c3c6565f6af5 100644 --- 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 index 6403216fdcd9..9cfd176d3dee 100644 --- 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 index 61da0c61fe6b..8ccbbd16fd5b 100644 --- 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 index 6608b32beb9e..9257a968d924 100644 --- 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&()); MOCK_METHOD0(routeConfig, Router::Config&()); MOCK_METHOD0(serverName, const std::string&());