Http Conn Man: Per Listener downstream_rq_*xx stats (#1783)

Http Conn Man: Add HTTP status code stats to the listener stats scope. Signed-off-by: Constance Caramanolis <ccaramanolis@lyft.com>
envoyproxy · Oct 3, 2017 · 58c3224 · 58c3224
1 parent 208c099
commit 58c3224
Show file tree

Hide file tree

Showing 12 changed files with 80 additions and 6 deletions.
diff --git a/docs/configuration/http_conn_man/stats.rst b/docs/configuration/http_conn_man/stats.rst
@@ -68,3 +68,19 @@ the following statistics:
    downstream_cx_total, Counter, Total connections
    downstream_cx_destroy_remote_active_rq, Counter, Total connections destroyed remotely with 1+ active requests
    downstream_rq_total, Counter, Total requests
+
+
+Per listener statistics
+-----------------------
+
+Additional per listener statistics are rooted at *listener.<address>.http.<stat_prefix>.* with the
+following statistics:
+
+ .. csv-table::
+    :header: Name, Type, Description
+    :widths: 1, 1, 2
+
+    downstream_rq_2xx, Counter, Total 2xx responses
+    downstream_rq_3xx, Counter, Total 3xx responses
+    downstream_rq_4xx, Counter, Total 4xx responses
+    downstream_rq_5xx, Counter, Total 5xx responses
diff --git a/include/envoy/server/filter_config.h b/include/envoy/server/filter_config.h
@@ -116,6 +116,11 @@ class FactoryContext {
    * @return Server::Admin& the server's global admin HTTP endpoint.
    */
   virtual Server::Admin& admin() PURE;
+
+  /**
+   * @return Stats::Scope& the listener's stats scope.
+   */
+  virtual Stats::Scope& listenerScope() PURE;
 };
 
 /**

diff --git a/source/common/http/conn_manager_impl.cc b/source/common/http/conn_manager_impl.cc
@@ -48,6 +48,11 @@ ConnectionManagerTracingStats ConnectionManagerImpl::generateTracingStats(const
   return {CONN_MAN_TRACING_STATS(POOL_COUNTER_PREFIX(scope, prefix + "tracing."))};
 }
 
+ConnectionManagerListenerStats
+ConnectionManagerImpl::generateListenerStats(const std::string& prefix, Stats::Scope& scope) {
+  return {CONN_MAN_LISTENER_STATS(POOL_COUNTER_PREFIX(scope, prefix))};
+}
+
 ConnectionManagerImpl::ConnectionManagerImpl(ConnectionManagerConfig& config,
                                              const Network::DrainDecision& drain_close,
                                              Runtime::RandomGenerator& random_generator,
@@ -57,7 +62,8 @@ ConnectionManagerImpl::ConnectionManagerImpl(ConnectionManagerConfig& config,
     : config_(config), stats_(config_.stats()),
       conn_length_(stats_.named_.downstream_cx_length_ms_.allocateSpan()),
       drain_close_(drain_close), random_generator_(random_generator), tracer_(tracer),
-      runtime_(runtime), local_info_(local_info), cluster_manager_(cluster_manager) {}
+      runtime_(runtime), local_info_(local_info), cluster_manager_(cluster_manager),
+      listener_stats_(config_.listenerStats()) {}
 
 void ConnectionManagerImpl::initializeReadFilterCallbacks(Network::ReadFilterCallbacks& callbacks) {
   read_callbacks_ = &callbacks;
@@ -401,12 +407,16 @@ void ConnectionManagerImpl::ActiveStream::chargeStats(HeaderMap& headers) {
 
   if (CodeUtility::is2xx(response_code)) {
     connection_manager_.stats_.named_.downstream_rq_2xx_.inc();
+    connection_manager_.listener_stats_.downstream_rq_2xx_.inc();
   } else if (CodeUtility::is3xx(response_code)) {
     connection_manager_.stats_.named_.downstream_rq_3xx_.inc();
+    connection_manager_.listener_stats_.downstream_rq_3xx_.inc();
   } else if (CodeUtility::is4xx(response_code)) {
     connection_manager_.stats_.named_.downstream_rq_4xx_.inc();
+    connection_manager_.listener_stats_.downstream_rq_4xx_.inc();
   } else if (CodeUtility::is5xx(response_code)) {
     connection_manager_.stats_.named_.downstream_rq_5xx_.inc();
+    connection_manager_.listener_stats_.downstream_rq_5xx_.inc();
   }
 }
 

diff --git a/source/common/http/conn_manager_impl.h b/source/common/http/conn_manager_impl.h
@@ -75,7 +75,7 @@ namespace Http {
   COUNTER(downstream_rq_non_relative_path)                                                         \
   COUNTER(downstream_rq_ws_on_non_ws_route)                                                        \
   COUNTER(downstream_rq_non_ws_on_ws_route)                                                        \
-  COUNTER(downstream_rq_too_large)                                                              \
+  COUNTER(downstream_rq_too_large)                                                                 \
   COUNTER(downstream_rq_2xx)                                                                       \
   COUNTER(downstream_rq_3xx)                                                                       \
   COUNTER(downstream_rq_4xx)                                                                       \
@@ -128,6 +128,24 @@ struct TracingConnectionManagerConfig {
 
 typedef std::unique_ptr<TracingConnectionManagerConfig> TracingConnectionManagerConfigPtr;
 
+/**
+ * Connection manager per listener stats. @see stats_macros.h
+ */
+// clang-format off
+#define CONN_MAN_LISTENER_STATS(COUNTER)                                                           \
+  COUNTER(downstream_rq_2xx)                                                                       \
+  COUNTER(downstream_rq_3xx)                                                                       \
+  COUNTER(downstream_rq_4xx)                                                                       \
+  COUNTER(downstream_rq_5xx)
+// clang-format on
+
+/**
+ * Wrapper struct for connection manager listener stats. @see stats_macros.h
+ */
+struct ConnectionManagerListenerStats {
+  CONN_MAN_LISTENER_STATS(GENERATE_COUNTER_STRUCT)
+};
+
 /**
  * Configuration for how to forward client certs.
  */
@@ -253,6 +271,11 @@ class ConnectionManagerConfig {
    * @return tracing config.
    */
   virtual const TracingConnectionManagerConfig* tracingConfig() PURE;
+
+  /**
+   * @return ConnectionManagerListenerStats& the stats to write to.
+   */
+  virtual ConnectionManagerListenerStats& listenerStats() PURE;
 };
 
 /**
@@ -276,6 +299,8 @@ class ConnectionManagerImpl : Logger::Loggable<Logger::Id::http>,
                                                             Stats::Scope& scope);
   static void chargeTracingStats(const Tracing::Reason& tracing_reason,
                                  ConnectionManagerTracingStats& tracing_stats);
+  static ConnectionManagerListenerStats generateListenerStats(const std::string& prefix,
+                                                              Stats::Scope& scope);
 
   // Network::ReadFilter
   Network::FilterStatus onData(Buffer::Instance& data) override;
@@ -603,6 +628,7 @@ class ConnectionManagerImpl : Logger::Loggable<Logger::Id::http>,
   Upstream::ClusterManager& cluster_manager_;
   WebSocket::WsHandlerImplPtr ws_connection_{};
   Network::ReadFilterCallbacks* read_callbacks_{};
+  ConnectionManagerListenerStats& listener_stats_;
 };
 
 } // Http

diff --git a/source/server/config/network/http_connection_manager.cc b/source/server/config/network/http_connection_manager.cc
@@ -124,8 +124,9 @@ HttpConnectionManagerConfig::HttpConnectionManagerConfig(
       http1_settings_(Http::Utility::parseHttp1Settings(config.http_protocol_options())),
       drain_timeout_(PROTOBUF_GET_MS_OR_DEFAULT(config, drain_timeout, 5000)),
       generate_request_id_(PROTOBUF_GET_WRAPPED_OR_DEFAULT(config, generate_request_id, true)),
-
-      date_provider_(date_provider) {
+      date_provider_(date_provider),
+      listener_stats_(Http::ConnectionManagerImpl::generateListenerStats(
+          stats_prefix_, context_.listenerScope())) {
 
   route_config_provider_ = Router::RouteConfigProviderUtil::create(
       config, context_.runtime(), context_.clusterManager(), context_.scope(), stats_prefix_,

diff --git a/source/server/config/network/http_connection_manager.h b/source/server/config/network/http_connection_manager.h
@@ -92,6 +92,7 @@ class HttpConnectionManagerConfig : Logger::Loggable<Logger::Id::config>,
   }
   const Network::Address::Instance& localAddress() override;
   const Optional<std::string>& userAgent() override { return user_agent_; }
+  Http::ConnectionManagerListenerStats& listenerStats() override { return listener_stats_; }
 
   static const std::string DEFAULT_SERVER_STRING;
 
@@ -119,6 +120,7 @@ class HttpConnectionManagerConfig : Logger::Loggable<Logger::Id::config>,
   std::chrono::milliseconds drain_timeout_;
   bool generate_request_id_;
   Http::DateProvider& date_provider_;
+  Http::ConnectionManagerListenerStats listener_stats_;
 };
 
 } // namespace Configuration

diff --git a/source/server/http/admin.cc b/source/server/http/admin.cc
@@ -362,7 +362,9 @@ AdminImpl::AdminImpl(const std::string& access_log_path, const std::string& prof
            MAKE_ADMIN_HANDLER(handlerServerInfo), false},
           {"/stats", "print server stats", MAKE_ADMIN_HANDLER(handlerStats), false},
           {"/listeners", "print listener addresses", MAKE_ADMIN_HANDLER(handlerListenerInfo),
-           false}} {
+           false}},
+      listener_stats_(
+          Http::ConnectionManagerImpl::generateListenerStats("http.admin.", server_.stats())) {
 
   if (!address_out_path.empty()) {
     std::ofstream address_out_file(address_out_path);

diff --git a/source/server/http/admin.h b/source/server/http/admin.h
@@ -78,6 +78,7 @@ class AdminImpl : public Admin,
   const Network::Address::Instance& localAddress() override;
   const Optional<std::string>& userAgent() override { return user_agent_; }
   const Http::TracingConnectionManagerConfig* tracingConfig() override { return nullptr; }
+  Http::ConnectionManagerListenerStats& listenerStats() override { return listener_stats_; }
 
 private:
   /**
@@ -143,6 +144,7 @@ class AdminImpl : public Admin,
   Optional<std::string> user_agent_;
   Http::SlowDateProviderImpl date_provider_;
   std::vector<Http::ClientCertDetailsType> set_current_client_cert_details_;
+  Http::ConnectionManagerListenerStats listener_stats_;
 };
 
 /**

diff --git a/test/common/http/conn_manager_impl_test.cc b/test/common/http/conn_manager_impl_test.cc
@@ -74,7 +74,8 @@ class HttpConnectionManagerImplTest : public Test, public ConnectionManagerConfi
                                         POOL_TIMER(fake_stats_))},
                "",
                fake_stats_},
-        tracing_stats_{CONN_MAN_TRACING_STATS(POOL_COUNTER(fake_stats_))} {
+        tracing_stats_{CONN_MAN_TRACING_STATS(POOL_COUNTER(fake_stats_))},
+        listener_stats_{CONN_MAN_LISTENER_STATS(POOL_COUNTER(fake_listener_stats_))} {
     tracing_config_.reset(new TracingConnectionManagerConfig(
         {Tracing::OperationName::Ingress, {LowerCaseString(":method")}}));
 
@@ -234,6 +235,7 @@ class HttpConnectionManagerImplTest : public Test, public ConnectionManagerConfi
   const Network::Address::Instance& localAddress() override { return local_address_; }
   const Optional<std::string>& userAgent() override { return user_agent_; }
   const TracingConnectionManagerConfig* tracingConfig() override { return tracing_config_.get(); }
+  ConnectionManagerListenerStats& listenerStats() override { return listener_stats_; }
 
   NiceMock<Tracing::MockHttpTracer> tracer_;
   NiceMock<Runtime::MockLoader> runtime_;
@@ -267,6 +269,8 @@ class HttpConnectionManagerImplTest : public Test, public ConnectionManagerConfi
   NiceMock<Upstream::MockClusterManager> cluster_manager_;
   uint32_t initial_buffer_limit_{};
   bool streaming_filter_{false};
+  Stats::IsolatedStoreImpl fake_listener_stats_;
+  ConnectionManagerListenerStats listener_stats_;
 
   // TODO(mattklein123): Not all tests have been converted over to better setup. Convert the rest.
   MockStreamEncoder response_encoder_;
@@ -333,6 +337,7 @@ TEST_F(HttpConnectionManagerImplTest, HeaderOnlyRequestAndResponse) {
   conn_manager_->onData(fake_input);
 
   EXPECT_EQ(1U, stats_.named_.downstream_rq_2xx_.value());
+  EXPECT_EQ(1U, listener_stats_.downstream_rq_2xx_.value());
 }
 
 TEST_F(HttpConnectionManagerImplTest, InvalidPathWithDualFilter) {
@@ -763,6 +768,7 @@ TEST_F(HttpConnectionManagerImplTest, DrainClose) {
 
   EXPECT_EQ(1U, stats_.named_.downstream_cx_drain_close_.value());
   EXPECT_EQ(1U, stats_.named_.downstream_rq_3xx_.value());
+  EXPECT_EQ(1U, listener_stats_.downstream_rq_3xx_.value());
 }
 
 TEST_F(HttpConnectionManagerImplTest, ResponseBeforeRequestComplete) {

diff --git a/test/mocks/http/mocks.h b/test/mocks/http/mocks.h
@@ -103,6 +103,7 @@ class MockConnectionManagerConfig : public ConnectionManagerConfig {
   MOCK_METHOD0(localAddress, const Network::Address::Instance&());
   MOCK_METHOD0(userAgent, const Optional<std::string>&());
   MOCK_METHOD0(tracingConfig, const Http::TracingConnectionManagerConfig*());
+  MOCK_METHOD0(listenerStats, ConnectionManagerListenerStats&());
 };
 
 class MockConnectionCallbacks : public virtual ConnectionCallbacks {

diff --git a/test/mocks/server/mocks.cc b/test/mocks/server/mocks.cc
@@ -131,6 +131,7 @@ MockFactoryContext::MockFactoryContext() : singleton_manager_(new Singleton::Man
   ON_CALL(*this, singletonManager()).WillByDefault(ReturnRef(*singleton_manager_));
   ON_CALL(*this, threadLocal()).WillByDefault(ReturnRef(thread_local_));
   ON_CALL(*this, admin()).WillByDefault(ReturnRef(admin_));
+  ON_CALL(*this, listenerScope()).WillByDefault(ReturnRef(listener_scope_));
 }
 
 MockFactoryContext::~MockFactoryContext() {}

diff --git a/test/mocks/server/mocks.h b/test/mocks/server/mocks.h
@@ -338,6 +338,7 @@ class MockFactoryContext : public FactoryContext {
   MOCK_METHOD0(singletonManager, Singleton::Manager&());
   MOCK_METHOD0(threadLocal, ThreadLocal::Instance&());
   MOCK_METHOD0(admin, Server::Admin&());
+  MOCK_METHOD0(listenerScope, Stats::Scope&());
 
   testing::NiceMock<AccessLog::MockAccessLogManager> access_log_manager_;
   testing::NiceMock<Upstream::MockClusterManager> cluster_manager_;
@@ -352,6 +353,7 @@ class MockFactoryContext : public FactoryContext {
   testing::NiceMock<ThreadLocal::MockInstance> thread_local_;
   Singleton::ManagerPtr singleton_manager_;
   testing::NiceMock<MockAdmin> admin_;
+  Stats::IsolatedStoreImpl listener_scope_;
 };
 
 } // namespace Configuration