thrift proxy: add stats to the router filter

docs/root/intro/version_history.rst

@@ -22,6 +22,7 @@ Version history
 * tcp_proxy: added :ref:`ClusterWeight.metadata_match<envoy_api_field_config.filter.network.tcp_proxy.v2.TcpProxy.WeightedCluster.ClusterWeight.metadata_match>`
 * tcp_proxy: added :ref:`hash_policy<envoy_api_field_config.filter.network.tcp_proxy.v2.TcpProxy.hash_policy>`
 * thrift_proxy: added support for cluster header based routing.
+* thrift_proxy: added stats to the router filter.
 * tls: remove TLS 1.0 and 1.1 from client defaults
 * router: exposed DOWNSTREAM_REMOTE_ADDRESS as custom HTTP request/response headers.
 

source/extensions/filters/network/thrift_proxy/router/config.cc

@@ -14,10 +14,10 @@ ThriftFilters::FilterFactoryCb RouterFilterConfig::createFilterFactoryFromProtoT
     const envoy::config::filter::thrift::router::v2alpha1::Router& proto_config,
     const std::string& stat_prefix, Server::Configuration::FactoryContext& context) {
   UNREFERENCED_PARAMETER(proto_config);
-  UNREFERENCED_PARAMETER(stat_prefix);
 
-  return [&context](ThriftFilters::FilterChainFactoryCallbacks& callbacks) -> void {
-    callbacks.addDecoderFilter(std::make_shared<Router>(context.clusterManager()));
+  return [&context, stat_prefix](ThriftFilters::FilterChainFactoryCallbacks& callbacks) -> void {
+    callbacks.addDecoderFilter(
+        std::make_shared<Router>(context.clusterManager(), stat_prefix, context.scope()));
   };
 }
 

source/extensions/filters/network/thrift_proxy/router/router_impl.cc

@@ -209,13 +209,10 @@ FilterStatus Router::transportEnd() {
 }
 
 FilterStatus Router::messageBegin(MessageMetadataSharedPtr metadata) {
-  // TODO(zuercher): route stats (e.g., no_route, no_cluster, upstream_rq_maintenance_mode, no
-  // healthy upstream)
-
   route_ = callbacks_->route();
   if (!route_) {
-    ENVOY_STREAM_LOG(debug, "no cluster match for method '{}'", *callbacks_,
-                     metadata->methodName());
+    ENVOY_STREAM_LOG(debug, "no route match for method '{}'", *callbacks_, metadata->methodName());
+    stats_.route_missing_.inc();
     callbacks_->sendLocalReply(
         AppException(AppExceptionType::UnknownMethod,
                      fmt::format("no route for method '{}'", metadata->methodName())),
@@ -229,6 +226,7 @@ FilterStatus Router::messageBegin(MessageMetadataSharedPtr metadata) {
   Upstream::ThreadLocalCluster* cluster = cluster_manager_.get(cluster_name);
   if (!cluster) {
     ENVOY_STREAM_LOG(debug, "unknown cluster '{}'", *callbacks_, cluster_name);
+    stats_.unknown_cluster_.inc();
     callbacks_->sendLocalReply(AppException(AppExceptionType::InternalError,
                                             fmt::format("unknown cluster '{}'", cluster_name)),
                                true);
@@ -240,6 +238,7 @@ FilterStatus Router::messageBegin(MessageMetadataSharedPtr metadata) {
                    metadata->methodName());
 
   if (cluster_->maintenanceMode()) {
+    stats_.upstream_rq_maintenance_mode_.inc();
     callbacks_->sendLocalReply(
         AppException(AppExceptionType::InternalError,
                      fmt::format("maintenance mode for cluster '{}'", cluster_name)),
@@ -263,6 +262,7 @@ FilterStatus Router::messageBegin(MessageMetadataSharedPtr metadata) {
   Tcp::ConnectionPool::Instance* conn_pool = cluster_manager_.tcpConnPoolForCluster(
       cluster_name, Upstream::ResourcePriority::Default, this);
   if (!conn_pool) {
+    stats_.no_healthy_upstream_.inc();
     callbacks_->sendLocalReply(
         AppException(AppExceptionType::InternalError,
                      fmt::format("no healthy upstream for '{}'", cluster_name)),

source/extensions/filters/network/thrift_proxy/router/router_impl.h

@@ -6,6 +6,8 @@
 
 #include "envoy/config/filter/network/thrift_proxy/v2alpha1/thrift_proxy.pb.h"
 #include "envoy/router/router.h"
+#include "envoy/stats/scope.h"
+#include "envoy/stats/stats_macros.h"
 #include "envoy/tcp/conn_pool.h"
 #include "envoy/upstream/load_balancer.h"
 
@@ -162,13 +164,25 @@ class RouteMatcher {
   std::vector<RouteEntryImplBaseConstSharedPtr> routes_;
 };
 
+#define ALL_THRIFT_ROUTER_STATS(COUNTER, GAUGE, HISTOGRAM)                                         \
+  COUNTER(route_missing)                                                                           \
+  COUNTER(unknown_cluster)                                                                         \
+  COUNTER(upstream_rq_maintenance_mode)                                                            \
+  COUNTER(no_healthy_upstream)
+
+struct RouterStats {
+  ALL_THRIFT_ROUTER_STATS(GENERATE_COUNTER_STRUCT, GENERATE_GAUGE_STRUCT, GENERATE_HISTOGRAM_STRUCT)
+};
+
 class Router : public Tcp::ConnectionPool::UpstreamCallbacks,
                public Upstream::LoadBalancerContextBase,
                public ProtocolConverter,
                public ThriftFilters::DecoderFilter,
                Logger::Loggable<Logger::Id::thrift> {
 public:
-  Router(Upstream::ClusterManager& cluster_manager) : cluster_manager_(cluster_manager) {}
+  Router(Upstream::ClusterManager& cluster_manager, const std::string& stat_prefix,
+         Stats::Scope& scope)
+      : cluster_manager_(cluster_manager), stats_(generateStats(stat_prefix, scope)) {}
 
   ~Router() override = default;
 
@@ -239,8 +253,14 @@ class Router : public Tcp::ConnectionPool::UpstreamCallbacks,
 
   void convertMessageBegin(MessageMetadataSharedPtr metadata);
   void cleanup();
+  RouterStats generateStats(const std::string& prefix, Stats::Scope& scope) {
+    return RouterStats{ALL_THRIFT_ROUTER_STATS(POOL_COUNTER_PREFIX(scope, prefix),
+                                               POOL_GAUGE_PREFIX(scope, prefix),
+                                               POOL_HISTOGRAM_PREFIX(scope, prefix))};
+  }
 
   Upstream::ClusterManager& cluster_manager_;
+  RouterStats stats_;
 
   ThriftFilters::DecoderFilterCallbacks* callbacks_{};
   RouteConstSharedPtr route_{};

source/extensions/filters/network/thrift_proxy/stats.h

@@ -32,7 +32,7 @@ namespace ThriftProxy {
   HISTOGRAM(request_time_ms, Milliseconds)
 
 /**
- * Struct definition for all mongo proxy stats. @see stats_macros.h
+ * Struct definition for all thrift proxy stats. @see stats_macros.h
  */
 struct ThriftFilterStats {
   ALL_THRIFT_FILTER_STATS(GENERATE_COUNTER_STRUCT, GENERATE_GAUGE_STRUCT, GENERATE_HISTOGRAM_STRUCT)

test/extensions/filters/network/thrift_proxy/router_test.cc

@@ -85,7 +85,7 @@ class ThriftRouterTestBase {
     route_ = new NiceMock<MockRoute>();
     route_ptr_.reset(route_);
 
-    router_ = std::make_unique<Router>(context_.clusterManager());
+    router_ = std::make_unique<Router>(context_.clusterManager(), "test", context_.scope());
 
     EXPECT_EQ(nullptr, router_->downstreamConnection());
 
@@ -437,6 +437,7 @@ TEST_F(ThriftRouterTest, NoRoute) {
         EXPECT_TRUE(end_stream);
       }));
   EXPECT_EQ(FilterStatus::StopIteration, router_->messageBegin(metadata_));
+  EXPECT_EQ(1U, context_.scope().counter("test.route_missing").value());
 }
 
 TEST_F(ThriftRouterTest, NoCluster) {
@@ -455,6 +456,7 @@ TEST_F(ThriftRouterTest, NoCluster) {
         EXPECT_TRUE(end_stream);
       }));
   EXPECT_EQ(FilterStatus::StopIteration, router_->messageBegin(metadata_));
+  EXPECT_EQ(1U, context_.scope().counter("test.unknown_cluster").value());
 }
 
 TEST_F(ThriftRouterTest, ClusterMaintenanceMode) {
@@ -475,6 +477,7 @@ TEST_F(ThriftRouterTest, ClusterMaintenanceMode) {
         EXPECT_TRUE(end_stream);
       }));
   EXPECT_EQ(FilterStatus::StopIteration, router_->messageBegin(metadata_));
+  EXPECT_EQ(1U, context_.scope().counter("test.upstream_rq_maintenance_mode").value());
 }
 
 TEST_F(ThriftRouterTest, NoHealthyHosts) {
@@ -496,6 +499,7 @@ TEST_F(ThriftRouterTest, NoHealthyHosts) {
       }));
 
   EXPECT_EQ(FilterStatus::StopIteration, router_->messageBegin(metadata_));
+  EXPECT_EQ(1U, context_.scope().counter("test.no_healthy_upstream").value());
 }
 
 TEST_F(ThriftRouterTest, TruncatedResponse) {