diff --git a/docs/root/intro/version_history.rst b/docs/root/intro/version_history.rst index e2da76d116970..25a3c522fe354 100644 --- a/docs/root/intro/version_history.rst +++ b/docs/root/intro/version_history.rst @@ -22,6 +22,7 @@ Version history * tcp_proxy: added :ref:`ClusterWeight.metadata_match` * tcp_proxy: added :ref:`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. diff --git a/source/extensions/filters/network/thrift_proxy/router/config.cc b/source/extensions/filters/network/thrift_proxy/router/config.cc index ffdf57e82bd6e..312f8143b445e 100644 --- a/source/extensions/filters/network/thrift_proxy/router/config.cc +++ b/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(context.clusterManager())); + return [&context, stat_prefix](ThriftFilters::FilterChainFactoryCallbacks& callbacks) -> void { + callbacks.addDecoderFilter( + std::make_shared(context.clusterManager(), stat_prefix, context.scope())); }; } diff --git a/source/extensions/filters/network/thrift_proxy/router/router_impl.cc b/source/extensions/filters/network/thrift_proxy/router/router_impl.cc index 449fddb85f63a..d7de50a5d6655 100644 --- a/source/extensions/filters/network/thrift_proxy/router/router_impl.cc +++ b/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)), diff --git a/source/extensions/filters/network/thrift_proxy/router/router_impl.h b/source/extensions/filters/network/thrift_proxy/router/router_impl.h index d61c7fe7b59ac..14f6e310a7336 100644 --- a/source/extensions/filters/network/thrift_proxy/router/router_impl.h +++ b/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 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 { 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_{}; diff --git a/source/extensions/filters/network/thrift_proxy/stats.h b/source/extensions/filters/network/thrift_proxy/stats.h index 583c02882f863..9166f37be6ca3 100644 --- a/source/extensions/filters/network/thrift_proxy/stats.h +++ b/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) diff --git a/test/extensions/filters/network/thrift_proxy/router_test.cc b/test/extensions/filters/network/thrift_proxy/router_test.cc index 5b29318b4997f..a3f28a5183629 100644 --- a/test/extensions/filters/network/thrift_proxy/router_test.cc +++ b/test/extensions/filters/network/thrift_proxy/router_test.cc @@ -85,7 +85,7 @@ class ThriftRouterTestBase { route_ = new NiceMock(); route_ptr_.reset(route_); - router_ = std::make_unique(context_.clusterManager()); + router_ = std::make_unique(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) {