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: do not allocate stat prefix and maintenance mode strings #189

Merged
merged 1 commit into from
Nov 4, 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
13 changes: 13 additions & 0 deletions include/envoy/upstream/upstream.h
Original file line number Diff line number Diff line change
Expand Up @@ -257,6 +257,14 @@ class Cluster : public virtual HostSet {
*/
virtual LoadBalancerType lbType() const PURE;

/**
* @return Whether the cluster is currently in maintenance mode and should not be routed to.
* Different filters may handle this situation in different ways. The implementation
* of this routine is typically based on randomness and may not return the same answer
* on each call.
*/
virtual bool maintenanceMode() const PURE;

/**
* @return uint64_t the maximum number of outbound requests that a connection pool will make on
* each upstream connection. This can be used to increase spread if the backends cannot
Expand All @@ -280,6 +288,11 @@ class Cluster : public virtual HostSet {
*/
virtual void shutdown() PURE;

/**
* @return the stat prefix to use for cluster specific stats.
*/
virtual const std::string& statPrefix() const PURE;

/**
* @return ClusterStats& strongly named stats for this cluster.
*/
Expand Down
17 changes: 8 additions & 9 deletions source/common/router/router.cc
Original file line number Diff line number Diff line change
Expand Up @@ -104,7 +104,7 @@ void Filter::chargeUpstreamCode(const Http::HeaderMap& response_headers) {
}

Http::CodeUtility::ResponseStatInfo info{
config_.stats_store_, stat_prefix_, response_headers,
config_.stats_store_, cluster_->statPrefix(), response_headers,
downstream_headers_->get(Http::Headers::get().EnvoyInternalRequest) == "true",
route_->virtualHostName(), request_vcluster_ ? request_vcluster_->name() : "",
config_.service_zone_, upstreamZone(), is_canary};
Expand Down Expand Up @@ -159,12 +159,11 @@ Http::FilterHeadersStatus Filter::decodeHeaders(Http::HeaderMap& headers, bool e

// Set up stat prefixes, etc.
request_vcluster_ = route_->virtualCluster(headers);
stat_prefix_ = fmt::format("cluster.{}.", route_->clusterName());
stream_log_debug("cluster '{}' match for URL '{}'", *callbacks_, route_->clusterName(),
headers.get(Http::Headers::get().Path));

const Upstream::Cluster& cluster = *config_.cm_.get(route_->clusterName());
const std::string& cluster_alt_name = cluster.altStatName();
cluster_ = config_.cm_.get(route_->clusterName());
const std::string& cluster_alt_name = cluster_->altStatName();
if (!cluster_alt_name.empty()) {
alt_stat_prefixes_.push_back(fmt::format("cluster.{}.", cluster_alt_name));
}
Expand All @@ -177,8 +176,7 @@ Http::FilterHeadersStatus Filter::decodeHeaders(Http::HeaderMap& headers, bool e
headers.remove(Http::Headers::get().EnvoyUpstreamAltStatName);

// See if we are supposed to immediately kill some percentage of this cluster's traffic.
if (config_.runtime_.snapshot().featureEnabled(
fmt::format("upstream.maintenance_mode.{}", route_->clusterName()), 0)) {
if (cluster_->maintenanceMode()) {
callbacks_->requestInfo().onFailedResponse(Http::AccessLog::FailureReason::UpstreamOverflow);
chargeUpstreamCode(Http::Code::ServiceUnavailable);
Http::Utility::sendLocalReply(*callbacks_, Http::Code::ServiceUnavailable, "maintenance mode");
Expand All @@ -195,7 +193,7 @@ Http::FilterHeadersStatus Filter::decodeHeaders(Http::HeaderMap& headers, bool e

timeout_ = FilterUtility::finalTimeout(*route_, headers);
route_->finalizeRequestHeaders(headers);
retry_state_ = createRetryState(route_->retryPolicy(), headers, cluster, config_.runtime_,
retry_state_ = createRetryState(route_->retryPolicy(), headers, *cluster_, config_.runtime_,
config_.random_, callbacks_->dispatcher(), finalPriority());
do_shadowing_ =
FilterUtility::shouldShadow(route_->shadowPolicy(), config_.runtime_, callbacks_->streamId());
Expand Down Expand Up @@ -393,7 +391,7 @@ void Filter::onUpstreamHeaders(Http::HeaderMapPtr&& headers, bool end_stream) {
[this]() -> void { doRetry(); }) &&
setupRetry(end_stream)) {
Http::CodeUtility::chargeBasicResponseStat(
config_.stats_store_, stat_prefix_ + "retry.",
config_.stats_store_, cluster_->statPrefix() + "retry.",
static_cast<Http::Code>(Http::Utility::getResponseStatus(*headers)));
return;
} else {
Expand Down Expand Up @@ -452,7 +450,8 @@ void Filter::onUpstreamComplete() {
upstream_host_->outlierDetector().putResponseTime(response_time);

Http::CodeUtility::ResponseTimingInfo info{
config_.stats_store_, stat_prefix_, response_time, upstream_request_->upstream_canary_,
config_.stats_store_, cluster_->statPrefix(), response_time,
upstream_request_->upstream_canary_,
downstream_headers_->get(Http::Headers::get().EnvoyInternalRequest) == "true",
route_->virtualHostName(), request_vcluster_ ? request_vcluster_->name() : "",
config_.service_zone_, upstreamZone()};
Expand Down
2 changes: 1 addition & 1 deletion source/common/router/router.h
Original file line number Diff line number Diff line change
Expand Up @@ -171,7 +171,7 @@ class Filter : Logger::Loggable<Logger::Id::router>, public Http::StreamDecoderF
FilterConfig& config_;
Http::StreamDecoderFilterCallbacks* callbacks_{};
const RouteEntry* route_;
std::string stat_prefix_;
const Upstream::Cluster* cluster_;
std::list<std::string> alt_stat_prefixes_;
const VirtualCluster* request_vcluster_;
bool downstream_response_started_{};
Expand Down
4 changes: 2 additions & 2 deletions source/common/ssl/context_impl.cc
Original file line number Diff line number Diff line change
Expand Up @@ -49,8 +49,8 @@ DH* get_dh2048() {
const unsigned char ContextImpl::SERVER_SESSION_ID_CONTEXT = 1;

ContextImpl::ContextImpl(const std::string& name, Stats::Store& store, ContextConfig& config)
: ctx_(SSL_CTX_new(SSLv23_method())), store_(store),
stats_prefix_(fmt::format("{}.ssl.", name)), stats_(generateStats(stats_prefix_, store)) {
: ctx_(SSL_CTX_new(SSLv23_method())), store_(store), stats_prefix_(fmt::format("{}ssl.", name)),
stats_(generateStats(stats_prefix_, store)) {
RELEASE_ASSERT(ctx_);
// the list of ciphers that will be supported
if (!config.cipherSuites().empty()) {
Expand Down
19 changes: 11 additions & 8 deletions source/common/upstream/upstream_impl.cc
Original file line number Diff line number Diff line change
Expand Up @@ -39,8 +39,7 @@ void HostSetImpl::addMemberUpdateCb(MemberUpdateCb callback) const {
callbacks_.emplace_back(callback);
}

ClusterStats ClusterImplBase::generateStats(const std::string& name, Stats::Store& stats) {
std::string prefix(fmt::format("cluster.{}.", name));
ClusterStats ClusterImplBase::generateStats(const std::string& prefix, Stats::Store& stats) {
return {ALL_CLUSTER_STATS(POOL_COUNTER_PREFIX(stats, prefix), POOL_GAUGE_PREFIX(stats, prefix),
POOL_TIMER_PREFIX(stats, prefix))};
}
Expand All @@ -56,13 +55,14 @@ const ConstHostListsPtr ClusterImplBase::empty_host_lists_{new std::vector<std::

ClusterImplBase::ClusterImplBase(const Json::Object& config, Runtime::Loader& runtime,
Stats::Store& stats, Ssl::ContextManager& ssl_context_manager)
: name_(config.getString("name")),
: runtime_(runtime), name_(config.getString("name")),
max_requests_per_connection_(config.getInteger("max_requests_per_connection", 0)),
connect_timeout_(std::chrono::milliseconds(config.getInteger("connect_timeout_ms"))),
stats_(generateStats(name_, stats)), alt_stat_name_(config.getString("alt_stat_name", "")),
features_(parseFeatures(config)),
stat_prefix_(fmt::format("cluster.{}.", name_)), stats_(generateStats(stat_prefix_, stats)),
alt_stat_name_(config.getString("alt_stat_name", "")), features_(parseFeatures(config)),
http_codec_options_(Http::Utility::parseCodecOptions(config)),
resource_managers_(config, runtime, name_) {
resource_managers_(config, runtime, name_),
maintenance_mode_runtime_key_(fmt::format("upstream.maintenance_mode.{}", name_)) {

std::string string_lb_type = config.getString("lb_type");
if (string_lb_type == "round_robin") {
Expand All @@ -78,8 +78,7 @@ ClusterImplBase::ClusterImplBase(const Json::Object& config, Runtime::Loader& ru
ssl_ctx_ = nullptr;
if (config.hasObject("ssl_context")) {
Ssl::ContextConfigImpl context_config(config.getObject("ssl_context"));
ssl_ctx_ = &ssl_context_manager.createSslClientContext(fmt::format("cluster.{}", name_), stats,
context_config);
ssl_ctx_ = &ssl_context_manager.createSslClientContext(stat_prefix_, stats, context_config);
}
}

Expand Down Expand Up @@ -111,6 +110,10 @@ ClusterImplBase::createHealthyHostLists(const std::vector<std::vector<HostPtr>>&
return healthy_list;
}

bool ClusterImplBase::maintenanceMode() const {
return runtime_.snapshot().featureEnabled(maintenance_mode_runtime_key_, 0);
}

uint64_t ClusterImplBase::parseFeatures(const Json::Object& config) {
uint64_t features = 0;
for (const std::string& feature : StringUtil::split(config.getString("features", ""), ',')) {
Expand Down
17 changes: 11 additions & 6 deletions source/common/upstream/upstream_impl.h
Original file line number Diff line number Diff line change
Expand Up @@ -159,7 +159,7 @@ class ClusterImplBase : public Cluster,
protected Logger::Loggable<Logger::Id::upstream> {

public:
static ClusterStats generateStats(const std::string& name, Stats::Store& stats);
static ClusterStats generateStats(const std::string& prefix, Stats::Store& stats);

/**
* Optionally set the health checker for the primary cluster. This is done after cluster
Expand All @@ -181,9 +181,11 @@ class ClusterImplBase : public Cluster,
uint64_t httpCodecOptions() const override { return http_codec_options_; }
Ssl::ClientContext* sslContext() const override { return ssl_ctx_; }
LoadBalancerType lbType() const override { return lb_type_; }
bool maintenanceMode() const override;
uint64_t maxRequestsPerConnection() const override { return max_requests_per_connection_; }
const std::string& name() const override { return name_; }
ResourceManager& resourceManager(ResourcePriority priority) const override;
const std::string& statPrefix() const override { return stat_prefix_; }
ClusterStats& stats() const override { return stats_; }

protected:
Expand All @@ -197,15 +199,17 @@ class ClusterImplBase : public Cluster,

static const ConstHostListsPtr empty_host_lists_;

Runtime::Loader& runtime_;
Ssl::ClientContext* ssl_ctx_;
std::string name_;
const std::string name_;
LoadBalancerType lb_type_;
uint64_t max_requests_per_connection_;
std::chrono::milliseconds connect_timeout_;
const uint64_t max_requests_per_connection_;
const std::chrono::milliseconds connect_timeout_;
const std::string stat_prefix_;
mutable ClusterStats stats_;
HealthCheckerPtr health_checker_;
std::string alt_stat_name_;
uint64_t features_;
const std::string alt_stat_name_;
const uint64_t features_;
OutlierDetectorPtr outlier_detector_;

private:
Expand All @@ -224,6 +228,7 @@ class ClusterImplBase : public Cluster,

const uint64_t http_codec_options_;
mutable ResourceManagers resource_managers_;
const std::string maintenance_mode_runtime_key_;
};

typedef std::shared_ptr<ClusterImplBase> ClusterImplBasePtr;
Expand Down
2 changes: 1 addition & 1 deletion source/server/configuration_impl.cc
Original file line number Diff line number Diff line change
Expand Up @@ -123,7 +123,7 @@ MainImpl::ListenerConfig::ListenerConfig(MainImpl& parent, Json::Object& json)
if (json.hasObject("ssl_context")) {
Ssl::ContextConfigImpl context_config(json.getObject("ssl_context"));
ssl_context_ = &parent_.server_.sslContextManager().createSslServerContext(
fmt::format("listener.{}", port_), parent_.server_.stats(), context_config);
fmt::format("listener.{}.", port_), parent_.server_.stats(), context_config);
}

if (json.hasObject("use_proxy_proto")) {
Expand Down
3 changes: 1 addition & 2 deletions test/common/router/router_test.cc
Original file line number Diff line number Diff line change
Expand Up @@ -142,8 +142,7 @@ TEST_F(RouterTest, NoHost) {
}

TEST_F(RouterTest, MaintenanceMode) {
EXPECT_CALL(runtime_.snapshot_, featureEnabled("upstream.maintenance_mode.fake_cluster", 0))
.WillOnce(Return(true));
EXPECT_CALL(cm_.cluster_, maintenanceMode()).WillOnce(Return(true));

Http::HeaderMapImpl response_headers{
{":status", "503"}, {"content-length", "16"}, {"content-type", "text/plain"}};
Expand Down
5 changes: 5 additions & 0 deletions test/common/upstream/upstream_impl_test.cc
Original file line number Diff line number Diff line change
Expand Up @@ -93,6 +93,11 @@ TEST(StrictDnsClusterImplTest, Basic) {
EXPECT_EQ(4U, cluster.resourceManager(ResourcePriority::High).retries().max());
EXPECT_EQ(3U, cluster.maxRequestsPerConnection());
EXPECT_EQ(Http::CodecOptions::NoCompression, cluster.httpCodecOptions());
EXPECT_EQ("cluster.name.", cluster.statPrefix());

EXPECT_CALL(runtime.snapshot_, featureEnabled("upstream.maintenance_mode.name", 0));
EXPECT_FALSE(cluster.maintenanceMode());

ReadyWatcher membership_updated;
cluster.addMemberUpdateCb([&](const std::vector<HostPtr>&, const std::vector<HostPtr>&)
-> void { membership_updated.ready(); });
Expand Down
3 changes: 2 additions & 1 deletion test/mocks/upstream/mocks.cc
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,7 @@ MockHost::MockHost() {}
MockHost::~MockHost() {}

MockCluster::MockCluster()
: stats_(ClusterImplBase::generateStats(name_, stats_store_)),
: stats_(ClusterImplBase::generateStats(stat_prefix_, stats_store_)),
resource_manager_(new Upstream::ResourceManagerImpl(runtime_, "fake_key", 1, 1024, 1024, 1)) {
ON_CALL(*this, connectTimeout()).WillByDefault(Return(std::chrono::milliseconds(1)));
ON_CALL(*this, hosts()).WillByDefault(ReturnRef(hosts_));
Expand All @@ -47,6 +47,7 @@ MockCluster::MockCluster()
.WillByDefault(Invoke([this](MemberUpdateCb cb) -> void { callbacks_.push_back(cb); }));
ON_CALL(*this, maxRequestsPerConnection())
.WillByDefault(ReturnPointee(&max_requests_per_connection_));
ON_CALL(*this, statPrefix()).WillByDefault(ReturnRef(stat_prefix_));
ON_CALL(*this, stats()).WillByDefault(ReturnRef(stats_));
ON_CALL(*this, resourceManager(_))
.WillByDefault(Invoke([this](ResourcePriority)
Expand Down
7 changes: 5 additions & 2 deletions test/mocks/upstream/mocks.h
Original file line number Diff line number Diff line change
Expand Up @@ -40,21 +40,24 @@ class MockCluster : public Cluster {
MOCK_METHOD1(setInitializedCb, void(std::function<void()>));
MOCK_CONST_METHOD0(sslContext, Ssl::ClientContext*());
MOCK_CONST_METHOD0(lbType, LoadBalancerType());
MOCK_CONST_METHOD0(maintenanceMode, bool());
MOCK_CONST_METHOD0(maxRequestsPerConnection, uint64_t());
MOCK_CONST_METHOD0(name, const std::string&());
MOCK_CONST_METHOD1(resourceManager, ResourceManager&(ResourcePriority priority));
MOCK_METHOD0(shutdown, void());
MOCK_CONST_METHOD0(statPrefix, const std::string&());
MOCK_CONST_METHOD0(stats, ClusterStats&());

std::vector<HostPtr> hosts_;
std::vector<HostPtr> healthy_hosts_;
std::vector<std::vector<HostPtr>> hosts_per_zone_;
std::vector<std::vector<HostPtr>> healthy_hosts_per_zone_;
std::string name_{"fake_cluster"};
std::string alt_stat_name_{"fake_alt_cluster"};
const std::string name_{"fake_cluster"};
const std::string alt_stat_name_{"fake_alt_cluster"};
std::list<MemberUpdateCb> callbacks_;
uint64_t max_requests_per_connection_{};
Stats::IsolatedStoreImpl stats_store_;
const std::string stat_prefix_{"cluster.fake_cluster."};
ClusterStats stats_;
std::unique_ptr<Upstream::ResourceManager> resource_manager_;
NiceMock<Runtime::MockLoader> runtime_;
Expand Down