From 74a291917336760649f8f90cf23f8eff913ecc7c Mon Sep 17 00:00:00 2001 From: Jose Nino Date: Mon, 3 Oct 2016 12:36:19 -0700 Subject: [PATCH 1/5] Add upstream_cx_overflow stat --- include/envoy/upstream/upstream.h | 1 + source/common/filter/tcp_proxy.cc | 1 + source/common/http/http1/conn_pool.cc | 7 ++++++- 3 files changed, 8 insertions(+), 1 deletion(-) diff --git a/include/envoy/upstream/upstream.h b/include/envoy/upstream/upstream.h index e706a1d16d6de..29d3b7a0cb814 100644 --- a/include/envoy/upstream/upstream.h +++ b/include/envoy/upstream/upstream.h @@ -140,6 +140,7 @@ class HostSet { COUNTER(upstream_cx_http2_total) \ COUNTER(upstream_cx_connect_fail) \ COUNTER(upstream_cx_connect_timeout) \ + COUNTER(upstream_cx_overflow) \ TIMER (upstream_cx_connect_ms) \ TIMER (upstream_cx_length_ms) \ COUNTER(upstream_cx_destroy) \ diff --git a/source/common/filter/tcp_proxy.cc b/source/common/filter/tcp_proxy.cc index 12be486010d39..4d8524ba95a08 100644 --- a/source/common/filter/tcp_proxy.cc +++ b/source/common/filter/tcp_proxy.cc @@ -51,6 +51,7 @@ void TcpProxy::initializeUpstreamConnection() { ->resourceManager(Upstream::ResourcePriority::Default); if (!upstream_cluster_resource_manager.connections().canCreate()) { + cluster_manager_.get(config_->clusterName())->stats().upstream_cx_overflow_.inc(); read_callbacks_->connection().close(Network::ConnectionCloseType::NoFlush); return; } diff --git a/source/common/http/http1/conn_pool.cc b/source/common/http/http1/conn_pool.cc index 5f5f3f49b1a21..97b038f24e13d 100644 --- a/source/common/http/http1/conn_pool.cc +++ b/source/common/http/http1/conn_pool.cc @@ -72,9 +72,14 @@ ConnectionPool::Cancellable* ConnPoolImpl::newStream(StreamDecoder& response_dec } if (host_->cluster().resourceManager(priority_).pendingRequests().canCreate()) { + bool connections_canCreate = host_->cluster().resourceManager(priority_).connections().canCreate(); + if (!connections_canCreate) { + host_->cluster().stats().upstream_cx_overflow_.inc(); + } + // If we have no connections at all, make one no matter what so we don't starve. if ((ready_clients_.size() == 0 && busy_clients_.size() == 0) || - host_->cluster().resourceManager(priority_).connections().canCreate()) { + connections_canCreate) { createNewConnection(); } From 10f0a30e9b403701f9a88d094d859edf07c8354f Mon Sep 17 00:00:00 2001 From: Jose Nino Date: Mon, 3 Oct 2016 12:39:05 -0700 Subject: [PATCH 2/5] Fix format --- source/common/http/http1/conn_pool.cc | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/source/common/http/http1/conn_pool.cc b/source/common/http/http1/conn_pool.cc index 97b038f24e13d..af6fa96f899ba 100644 --- a/source/common/http/http1/conn_pool.cc +++ b/source/common/http/http1/conn_pool.cc @@ -72,14 +72,14 @@ ConnectionPool::Cancellable* ConnPoolImpl::newStream(StreamDecoder& response_dec } if (host_->cluster().resourceManager(priority_).pendingRequests().canCreate()) { - bool connections_canCreate = host_->cluster().resourceManager(priority_).connections().canCreate(); + bool connections_canCreate = + host_->cluster().resourceManager(priority_).connections().canCreate(); if (!connections_canCreate) { host_->cluster().stats().upstream_cx_overflow_.inc(); } // If we have no connections at all, make one no matter what so we don't starve. - if ((ready_clients_.size() == 0 && busy_clients_.size() == 0) || - connections_canCreate) { + if ((ready_clients_.size() == 0 && busy_clients_.size() == 0) || connections_canCreate) { createNewConnection(); } From ee605eb26bc46f4f9874903e37a124699926c446 Mon Sep 17 00:00:00 2001 From: Jose Nino Date: Mon, 3 Oct 2016 13:34:16 -0700 Subject: [PATCH 3/5] Fix comments, add test expectations, and document the stat. --- docs/configuration/cluster_manager/cluster_stats.rst | 1 + include/envoy/upstream/upstream.h | 2 +- source/common/http/http1/conn_pool.cc | 6 +++--- test/common/filter/tcp_proxy_test.cc | 4 ++++ test/common/http/http1/conn_pool_test.cc | 1 + 5 files changed, 10 insertions(+), 4 deletions(-) diff --git a/docs/configuration/cluster_manager/cluster_stats.rst b/docs/configuration/cluster_manager/cluster_stats.rst index 4a6e69645a06b..2eda182c4ba11 100644 --- a/docs/configuration/cluster_manager/cluster_stats.rst +++ b/docs/configuration/cluster_manager/cluster_stats.rst @@ -15,6 +15,7 @@ Every cluster has a statistics tree rooted at *cluster..* with the followi upstream_cx_http2_total, Counter, Total HTTP/2 connections upstream_cx_connect_fail, Counter, Total connection failures upstream_cx_connect_timeout, Counter, Total connection timeouts + upstream_cx_overflow, Counter, Total times that the cluster's connection circuit breaker overflowed. upstream_cx_connect_ms, Timer, Connection establishment milliseconds upstream_cx_length_ms, Timer, Connection length milliseconds upstream_cx_destroy, Counter, Total destroyed connections diff --git a/include/envoy/upstream/upstream.h b/include/envoy/upstream/upstream.h index 29d3b7a0cb814..50192c8d2e06e 100644 --- a/include/envoy/upstream/upstream.h +++ b/include/envoy/upstream/upstream.h @@ -140,7 +140,7 @@ class HostSet { COUNTER(upstream_cx_http2_total) \ COUNTER(upstream_cx_connect_fail) \ COUNTER(upstream_cx_connect_timeout) \ - COUNTER(upstream_cx_overflow) \ + COUNTER(upstream_cx_overflow) \ TIMER (upstream_cx_connect_ms) \ TIMER (upstream_cx_length_ms) \ COUNTER(upstream_cx_destroy) \ diff --git a/source/common/http/http1/conn_pool.cc b/source/common/http/http1/conn_pool.cc index af6fa96f899ba..0db93d5355b47 100644 --- a/source/common/http/http1/conn_pool.cc +++ b/source/common/http/http1/conn_pool.cc @@ -72,14 +72,14 @@ ConnectionPool::Cancellable* ConnPoolImpl::newStream(StreamDecoder& response_dec } if (host_->cluster().resourceManager(priority_).pendingRequests().canCreate()) { - bool connections_canCreate = + bool connections_can_create = host_->cluster().resourceManager(priority_).connections().canCreate(); - if (!connections_canCreate) { + if (!connections_can_create) { host_->cluster().stats().upstream_cx_overflow_.inc(); } // If we have no connections at all, make one no matter what so we don't starve. - if ((ready_clients_.size() == 0 && busy_clients_.size() == 0) || connections_canCreate) { + if ((ready_clients_.size() == 0 && busy_clients_.size() == 0) || connections_can_create) { createNewConnection(); } diff --git a/test/common/filter/tcp_proxy_test.cc b/test/common/filter/tcp_proxy_test.cc index 2d87a2eff2d22..01789890c2a2b 100644 --- a/test/common/filter/tcp_proxy_test.cc +++ b/test/common/filter/tcp_proxy_test.cc @@ -170,6 +170,10 @@ TEST_F(TcpProxyTest, UpstreamConnectionLimit) { // The downstream connection closes if the proxy can't make an upstream connection. EXPECT_CALL(filter_callbacks_.connection_, close(Network::ConnectionCloseType::NoFlush)); ASSERT_EQ(Network::FilterStatus::StopIteration, filter_->onData(buffer)); + EXPECT_EQ( + 1U, + cluster_manager_.cluster_.stats_store_.counter("cluster.fake_cluster.upstream_cx_overflow") + .value()); } } // Filter diff --git a/test/common/http/http1/conn_pool_test.cc b/test/common/http/http1/conn_pool_test.cc index 73f5d36bb140c..183bd2e0d017d 100644 --- a/test/common/http/http1/conn_pool_test.cc +++ b/test/common/http/http1/conn_pool_test.cc @@ -349,6 +349,7 @@ TEST_F(Http1ConnPoolImplTest, MaxConnections) { NiceMock outer_decoder2; ConnPoolCallbacks callbacks2; handle = conn_pool_.newStream(outer_decoder2, callbacks2); + EXPECT_EQ(1U, cluster_.stats_.upstream_cx_overflow_.value()); EXPECT_NE(nullptr, handle); From 1e302c10aa70fda5245f43688e61811e10de3767 Mon Sep 17 00:00:00 2001 From: Jose Nino Date: Mon, 3 Oct 2016 13:55:29 -0700 Subject: [PATCH 4/5] Delete period --- docs/configuration/cluster_manager/cluster_stats.rst | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/configuration/cluster_manager/cluster_stats.rst b/docs/configuration/cluster_manager/cluster_stats.rst index 2eda182c4ba11..737746ffabc22 100644 --- a/docs/configuration/cluster_manager/cluster_stats.rst +++ b/docs/configuration/cluster_manager/cluster_stats.rst @@ -15,7 +15,7 @@ Every cluster has a statistics tree rooted at *cluster..* with the followi upstream_cx_http2_total, Counter, Total HTTP/2 connections upstream_cx_connect_fail, Counter, Total connection failures upstream_cx_connect_timeout, Counter, Total connection timeouts - upstream_cx_overflow, Counter, Total times that the cluster's connection circuit breaker overflowed. + upstream_cx_overflow, Counter, Total times that the cluster's connection circuit breaker overflowed upstream_cx_connect_ms, Timer, Connection establishment milliseconds upstream_cx_length_ms, Timer, Connection length milliseconds upstream_cx_destroy, Counter, Total destroyed connections From 65eeb7cce5028fe0ff0cf82ef096e609fd488760 Mon Sep 17 00:00:00 2001 From: Jose Nino Date: Mon, 3 Oct 2016 14:09:00 -0700 Subject: [PATCH 5/5] nit --- source/common/http/http1/conn_pool.cc | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/source/common/http/http1/conn_pool.cc b/source/common/http/http1/conn_pool.cc index 0db93d5355b47..03f55766fb76d 100644 --- a/source/common/http/http1/conn_pool.cc +++ b/source/common/http/http1/conn_pool.cc @@ -72,14 +72,14 @@ ConnectionPool::Cancellable* ConnPoolImpl::newStream(StreamDecoder& response_dec } if (host_->cluster().resourceManager(priority_).pendingRequests().canCreate()) { - bool connections_can_create = + bool can_create_connection = host_->cluster().resourceManager(priority_).connections().canCreate(); - if (!connections_can_create) { + if (!can_create_connection) { host_->cluster().stats().upstream_cx_overflow_.inc(); } // If we have no connections at all, make one no matter what so we don't starve. - if ((ready_clients_.size() == 0 && busy_clients_.size() == 0) || connections_can_create) { + if ((ready_clients_.size() == 0 && busy_clients_.size() == 0) || can_create_connection) { createNewConnection(); }