Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

perf: various changes #118

Merged
merged 2 commits into from
Oct 5, 2016
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion ci/do_ci.sh
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@ else
TEST_TARGET="envoy.check"
fi

mkdir build
mkdir -p build
cd build

cmake \
Expand Down
9 changes: 8 additions & 1 deletion docs/configuration/http_conn_man/http_conn_man.rst
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,8 @@ HTTP connection manager
"idle_timeout_s": "...",
"drain_timeout_ms": "...",
"access_log": [],
"use_remote_address": "..."
"use_remote_address": "...",
"generate_request_id": "..."
}
}

Expand Down Expand Up @@ -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:

Expand Down
9 changes: 8 additions & 1 deletion docs/configuration/http_filters/router_filter.rst
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
2 changes: 1 addition & 1 deletion source/common/http/async_client_impl.cc
Original file line number Diff line number Diff line change
Expand Up @@ -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()); }
Expand Down
6 changes: 6 additions & 0 deletions source/common/http/conn_manager_impl.h
Original file line number Diff line number Diff line change
Expand Up @@ -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.
*/
Expand Down
7 changes: 5 additions & 2 deletions source/common/http/conn_manager_utility.cc
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand All @@ -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,
Expand Down
27 changes: 21 additions & 6 deletions source/common/http/http1/codec_impl.cc
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand Down Expand Up @@ -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);
}

Expand All @@ -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");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

use CRLF as well?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

another function call for no particular reason

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok, sound good

StreamEncoderImpl::encodeHeaders(headers, end_stream);
}

Expand Down Expand Up @@ -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;
Expand Down
6 changes: 3 additions & 3 deletions source/common/http/http1/codec_impl.h
Original file line number Diff line number Diff line change
Expand Up @@ -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_;

Expand All @@ -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};
};
Expand Down
9 changes: 5 additions & 4 deletions source/common/router/router.cc
Original file line number Diff line number Diff line change
Expand Up @@ -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",
Expand Down Expand Up @@ -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(),
Expand Down
6 changes: 4 additions & 2 deletions source/common/router/router.h
Original file line number Diff line number Diff line change
Expand Up @@ -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_; }

Expand All @@ -80,6 +81,7 @@ class FilterConfig {
Runtime::Loader& runtime_;
Runtime::RandomGenerator& random_;
FilterStats stats_;
const bool emit_dynamic_stats_;

private:
ShadowWriterPtr shadow_writer_;
Expand Down
8 changes: 7 additions & 1 deletion source/common/runtime/runtime_impl.h
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down
6 changes: 4 additions & 2 deletions source/server/config/http/router.cc
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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(
Expand Down
3 changes: 2 additions & 1 deletion source/server/config/network/http_connection_manager.cc
Original file line number Diff line number Diff line change
Expand Up @@ -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");
Expand Down
2 changes: 2 additions & 0 deletions source/server/config/network/http_connection_manager.h
Original file line number Diff line number Diff line change
Expand Up @@ -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_; }
Expand Down Expand Up @@ -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_;
};

/**
Expand Down
1 change: 1 addition & 0 deletions source/server/http/admin.h
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down
1 change: 1 addition & 0 deletions test/common/http/conn_manager_impl_test.cc
Original file line number Diff line number Diff line change
Expand Up @@ -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_; }
Expand Down
2 changes: 2 additions & 0 deletions test/common/http/conn_manager_utility_test.cc
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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"},
Expand Down
2 changes: 1 addition & 1 deletion test/common/router/router_test.cc
Original file line number Diff line number Diff line change
Expand Up @@ -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_));
Expand Down
1 change: 1 addition & 0 deletions test/mocks/http/mocks.cc
Original file line number Diff line number Diff line change
Expand Up @@ -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() {}
Expand Down
1 change: 1 addition & 0 deletions test/mocks/http/mocks.h
Original file line number Diff line number Diff line change
Expand Up @@ -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&());
Expand Down