From e444f36e54dc57d44b4224fb7c0ab01cfbb9237c Mon Sep 17 00:00:00 2001 From: Raul Gutierrez Segales Date: Tue, 3 Dec 2019 12:33:14 -0500 Subject: [PATCH 1/2] thrift proxy: add stats to the router filter This adds stats for missing route, missing cluster, no healthy hosts, etc. Should be useful to protect operators against bad config pushes, plus gives more visibility. Signed-off-by: Raul Gutierrez Segales --- docs/root/intro/version_history.rst | 1 + .../network/thrift_proxy/router/config.cc | 6 ++--- .../thrift_proxy/router/router_impl.cc | 10 ++++----- .../network/thrift_proxy/router/router_impl.h | 22 ++++++++++++++++++- .../filters/network/thrift_proxy/stats.h | 2 +- .../network/thrift_proxy/router_test.cc | 6 ++++- 6 files changed, 36 insertions(+), 11 deletions(-) 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) { From b44c7236e3b88d6bdb9bcf3d13ee7730578ad5fb Mon Sep 17 00:00:00 2001 From: Raul Gutierrez Segales Date: Tue, 3 Dec 2019 13:27:41 -0500 Subject: [PATCH 2/2] Kick CI Signed-off-by: Raul Gutierrez Segales