From d14aa607214215ee16b3defa8ea3cbe6db56a9e1 Mon Sep 17 00:00:00 2001 From: AlanDiaz Date: Tue, 22 Aug 2023 20:09:04 -0400 Subject: [PATCH 1/9] Optimize AsyncClientManager by moving hashing to control plane Signed-off-by: AlanDiaz --- envoy/grpc/async_client_manager.h | 26 ++++++++++++ .../common/grpc/async_client_manager_impl.cc | 42 ++++++++++++++----- .../common/grpc/async_client_manager_impl.h | 17 ++++---- .../filters/http/ext_authz/config.cc | 27 ++++-------- .../filters/http/ext_authz/config_test.cc | 16 ++++--- test/mocks/grpc/mocks.h | 4 ++ 6 files changed, 89 insertions(+), 43 deletions(-) diff --git a/envoy/grpc/async_client_manager.h b/envoy/grpc/async_client_manager.h index 7cf027ee90533..d2b160d23504a 100644 --- a/envoy/grpc/async_client_manager.h +++ b/envoy/grpc/async_client_manager.h @@ -32,6 +32,28 @@ class AsyncClientFactory { using AsyncClientFactoryPtr = std::unique_ptr; +class GrpcServiceHashKeyWrapper { +public: + GrpcServiceHashKeyWrapper(const envoy::config::core::v3::GrpcService& config); + + template friend H AbslHashValue(H h, const GrpcServiceHashKeyWrapper& wrapper) { + return H::combine(std::move(h), wrapper.pre_computed_hash_); + } + + friend bool operator==(const GrpcServiceHashKeyWrapper& lhs, + const GrpcServiceHashKeyWrapper& rhs) { + return lhs.config_.GetTypeName() == rhs.config_.GetTypeName() && + lhs.serialized_config_ == rhs.serialized_config_; + } + + const envoy::config::core::v3::GrpcService& config() const { return config_; } + +private: + const envoy::config::core::v3::GrpcService config_; + const std::size_t pre_computed_hash_; + const std::string serialized_config_; +}; + // Singleton gRPC client manager. Grpc::AsyncClientManager can be used to create per-service // Grpc::AsyncClientFactory instances. All manufactured Grpc::AsyncClients must // be destroyed before the AsyncClientManager can be safely destructed. @@ -54,6 +76,10 @@ class AsyncClientManager { getOrCreateRawAsyncClient(const envoy::config::core::v3::GrpcService& grpc_service, Stats::Scope& scope, bool skip_cluster_check) PURE; + virtual RawAsyncClientSharedPtr + getOrCreateRawAsyncClientWithWrapper(const GrpcServiceHashKeyWrapper& grpc_service, + Stats::Scope& scope, bool skip_cluster_check) PURE; + /** * Create a Grpc::AsyncClients factory for a service. Validation of the service is performed and * will raise an exception on failure. diff --git a/source/common/grpc/async_client_manager_impl.cc b/source/common/grpc/async_client_manager_impl.cc index 949ae4bd75391..a5bbb71ce3477 100644 --- a/source/common/grpc/async_client_manager_impl.cc +++ b/source/common/grpc/async_client_manager_impl.cc @@ -5,6 +5,7 @@ #include "source/common/common/base64.h" #include "source/common/grpc/async_client_impl.h" +#include "source/common/protobuf/utility.h" #include "absl/strings/match.h" @@ -48,6 +49,11 @@ AsyncClientFactoryImpl::AsyncClientFactoryImpl(Upstream::ClusterManager& cm, cm_.checkActiveStaticCluster(config.envoy_grpc().cluster_name()); } +GrpcServiceHashKeyWrapper::GrpcServiceHashKeyWrapper( + const envoy::config::core::v3::GrpcService& config) + : config_(config), pre_computed_hash_(Envoy::MessageUtil::hash(config)), + serialized_config_(config.SerializeAsString()) {} + AsyncClientManagerImpl::AsyncClientManagerImpl(Upstream::ClusterManager& cm, ThreadLocal::Instance& tls, TimeSource& time_source, Api::Api& api, const StatNames& stat_names) @@ -136,12 +142,26 @@ AsyncClientManagerImpl::factoryForGrpcService(const envoy::config::core::v3::Grp RawAsyncClientSharedPtr AsyncClientManagerImpl::getOrCreateRawAsyncClient( const envoy::config::core::v3::GrpcService& config, Stats::Scope& scope, bool skip_cluster_check) { - RawAsyncClientSharedPtr client = raw_async_client_cache_->getCache(config); + const GrpcServiceHashKeyWrapper& key_wrapper = GrpcServiceHashKeyWrapper(config); + RawAsyncClientSharedPtr client = raw_async_client_cache_->getCache(key_wrapper); + if (client != nullptr) { + return client; + } + client = factoryForGrpcService(key_wrapper.config(), scope, skip_cluster_check) + ->createUncachedRawAsyncClient(); + raw_async_client_cache_->setCache(key_wrapper, client); + return client; +} + +RawAsyncClientSharedPtr AsyncClientManagerImpl::getOrCreateRawAsyncClientWithWrapper( + const GrpcServiceHashKeyWrapper& key_wrapper, Stats::Scope& scope, bool skip_cluster_check) { + RawAsyncClientSharedPtr client = raw_async_client_cache_->getCache(key_wrapper); if (client != nullptr) { return client; } - client = factoryForGrpcService(config, scope, skip_cluster_check)->createUncachedRawAsyncClient(); - raw_async_client_cache_->setCache(config, client); + client = factoryForGrpcService(key_wrapper.config(), scope, skip_cluster_check) + ->createUncachedRawAsyncClient(); + raw_async_client_cache_->setCache(key_wrapper, client); return client; } @@ -151,11 +171,11 @@ AsyncClientManagerImpl::RawAsyncClientCache::RawAsyncClientCache(Event::Dispatch } void AsyncClientManagerImpl::RawAsyncClientCache::setCache( - const envoy::config::core::v3::GrpcService& config, const RawAsyncClientSharedPtr& client) { - ASSERT(lru_map_.find(config) == lru_map_.end()); + const GrpcServiceHashKeyWrapper& key_wrapper, const RawAsyncClientSharedPtr& client) { + ASSERT(lru_map_wrapper_.find(key_wrapper) == lru_map_wrapper_.end()); // Create a new cache entry at the beginning of the list. - lru_list_.emplace_front(config, client, dispatcher_.timeSource().monotonicTime()); - lru_map_[config] = lru_list_.begin(); + lru_list_.emplace_front(key_wrapper.config(), client, dispatcher_.timeSource().monotonicTime()); + lru_map_wrapper_[key_wrapper] = lru_list_.begin(); // If inserting to an empty cache, enable eviction timer. if (lru_list_.size() == 1) { evictEntriesAndResetEvictionTimer(); @@ -163,9 +183,9 @@ void AsyncClientManagerImpl::RawAsyncClientCache::setCache( } RawAsyncClientSharedPtr AsyncClientManagerImpl::RawAsyncClientCache::getCache( - const envoy::config::core::v3::GrpcService& config) { - auto it = lru_map_.find(config); - if (it == lru_map_.end()) { + const GrpcServiceHashKeyWrapper& key_wrapper) { + auto it = lru_map_wrapper_.find(key_wrapper); + if (it == lru_map_wrapper_.end()) { return nullptr; } const auto cache_entry = it->second; @@ -189,7 +209,7 @@ void AsyncClientManagerImpl::RawAsyncClientCache::evictEntriesAndResetEvictionTi MonotonicTime next_expire = lru_list_.back().accessed_time_ + EntryTimeoutInterval; if (now >= next_expire) { // Erase the expired entry. - lru_map_.erase(lru_list_.back().config_); + lru_map_wrapper_.erase(lru_list_.back().key_wrapper_); lru_list_.pop_back(); } else { cache_eviction_timer_->enableTimer( diff --git a/source/common/grpc/async_client_manager_impl.h b/source/common/grpc/async_client_manager_impl.h index eee1400ff84cd..6cfd0329604b7 100644 --- a/source/common/grpc/async_client_manager_impl.h +++ b/source/common/grpc/async_client_manager_impl.h @@ -9,6 +9,7 @@ #include "envoy/upstream/cluster_manager.h" #include "source/common/grpc/stat_names.h" +#include "source/common/protobuf/utility.h" namespace Envoy { namespace Grpc { @@ -51,32 +52,34 @@ class AsyncClientManagerImpl : public AsyncClientManager { getOrCreateRawAsyncClient(const envoy::config::core::v3::GrpcService& config, Stats::Scope& scope, bool skip_cluster_check) override; + RawAsyncClientSharedPtr + getOrCreateRawAsyncClientWithWrapper(const GrpcServiceHashKeyWrapper& key_wrapper, + Stats::Scope& scope, bool skip_cluster_check) override; + AsyncClientFactoryPtr factoryForGrpcService(const envoy::config::core::v3::GrpcService& config, Stats::Scope& scope, bool skip_cluster_check) override; class RawAsyncClientCache : public ThreadLocal::ThreadLocalObject { public: explicit RawAsyncClientCache(Event::Dispatcher& dispatcher); - void setCache(const envoy::config::core::v3::GrpcService& config, + void setCache(const GrpcServiceHashKeyWrapper& key_wrapper, const RawAsyncClientSharedPtr& client); - RawAsyncClientSharedPtr getCache(const envoy::config::core::v3::GrpcService& config); + RawAsyncClientSharedPtr getCache(const GrpcServiceHashKeyWrapper& key_wrapper); private: void evictEntriesAndResetEvictionTimer(); struct CacheEntry { CacheEntry(const envoy::config::core::v3::GrpcService& config, RawAsyncClientSharedPtr const& client, MonotonicTime create_time) - : config_(config), client_(client), accessed_time_(create_time) {} - envoy::config::core::v3::GrpcService config_; + : key_wrapper_(config), client_(client), accessed_time_(create_time) {} + GrpcServiceHashKeyWrapper key_wrapper_; RawAsyncClientSharedPtr client_; MonotonicTime accessed_time_; }; using LruList = std::list; - absl::flat_hash_map - lru_map_; LruList lru_list_; + absl::flat_hash_map lru_map_wrapper_; Event::Dispatcher& dispatcher_; Envoy::Event::TimerPtr cache_eviction_timer_; static constexpr std::chrono::seconds EntryTimeoutInterval{50}; diff --git a/source/extensions/filters/http/ext_authz/config.cc b/source/extensions/filters/http/ext_authz/config.cc index 54ca6fc5db455..f1a7d2878fd39 100644 --- a/source/extensions/filters/http/ext_authz/config.cc +++ b/source/extensions/filters/http/ext_authz/config.cc @@ -6,6 +6,7 @@ #include "envoy/config/core/v3/grpc_service.pb.h" #include "envoy/extensions/filters/http/ext_authz/v3/ext_authz.pb.h" #include "envoy/extensions/filters/http/ext_authz/v3/ext_authz.pb.validate.h" +#include "envoy/grpc/async_client_manager.h" #include "envoy/registry/registry.h" #include "source/common/config/utility.h" @@ -42,34 +43,22 @@ Http::FilterFactoryCb ExtAuthzFilterConfig::createFilterFactoryFromProtoTyped( context.clusterManager(), client_config); callbacks.addStreamFilter(std::make_shared(filter_config, std::move(client))); }; - } else if (proto_config.grpc_service().has_google_grpc()) { - // Google gRPC client. + } else { + // gRPC client. const uint32_t timeout_ms = PROTOBUF_GET_MS_OR_DEFAULT(proto_config.grpc_service(), timeout, DefaultTimeout); Config::Utility::checkTransportVersion(proto_config); + Envoy::Grpc::GrpcServiceHashKeyWrapper wrapper = + Envoy::Grpc::GrpcServiceHashKeyWrapper(proto_config.grpc_service()); callback = [&context, filter_config, timeout_ms, - proto_config](Http::FilterChainFactoryCallbacks& callbacks) { + wrapper](Http::FilterChainFactoryCallbacks& callbacks) { auto client = std::make_unique( - context.clusterManager().grpcAsyncClientManager().getOrCreateRawAsyncClient( - proto_config.grpc_service(), context.scope(), true), + context.clusterManager().grpcAsyncClientManager().getOrCreateRawAsyncClientWithWrapper( + wrapper, context.scope(), true), std::chrono::milliseconds(timeout_ms)); callbacks.addStreamFilter(std::make_shared(filter_config, std::move(client))); }; - } else { - // Envoy gRPC client. - const uint32_t timeout_ms = - PROTOBUF_GET_MS_OR_DEFAULT(proto_config.grpc_service(), timeout, DefaultTimeout); - Config::Utility::checkTransportVersion(proto_config); - callback = [grpc_service = proto_config.grpc_service(), &context, filter_config, - timeout_ms](Http::FilterChainFactoryCallbacks& callbacks) { - Grpc::RawAsyncClientSharedPtr raw_client = - context.clusterManager().grpcAsyncClientManager().getOrCreateRawAsyncClient( - grpc_service, context.scope(), true); - auto client = std::make_unique( - raw_client, std::chrono::milliseconds(timeout_ms)); - callbacks.addStreamFilter(std::make_shared(filter_config, std::move(client))); - }; } return callback; diff --git a/test/extensions/filters/http/ext_authz/config_test.cc b/test/extensions/filters/http/ext_authz/config_test.cc index 6995304446fb2..cf588b176b6dd 100644 --- a/test/extensions/filters/http/ext_authz/config_test.cc +++ b/test/extensions/filters/http/ext_authz/config_test.cc @@ -63,11 +63,12 @@ class ExtAuthzFilterTest : public Event::TestUsingSimulatedTime, const envoy::extensions::filters::http::ext_authz::v3::ExtAuthz& ext_authz_config) { // Delegate call to mock async client manager to real async client manager. ON_CALL(context_, getServerFactoryContext()).WillByDefault(testing::ReturnRef(server_context_)); - ON_CALL(context_.cluster_manager_.async_client_manager_, getOrCreateRawAsyncClient(_, _, _)) - .WillByDefault(Invoke([&](const envoy::config::core::v3::GrpcService& config, + ON_CALL(context_.cluster_manager_.async_client_manager_, + getOrCreateRawAsyncClientWithWrapper(_, _, _)) + .WillByDefault(Invoke([&](const Envoy::Grpc::GrpcServiceHashKeyWrapper& config, Stats::Scope& scope, bool skip_cluster_check) { - return async_client_manager_->getOrCreateRawAsyncClient(config, scope, - skip_cluster_check); + return async_client_manager_->getOrCreateRawAsyncClientWithWrapper(config, scope, + skip_cluster_check); })); ExtAuthzFilterConfig factory; return factory.createFilterFactoryFromProto(ext_authz_config, "stats", context_); @@ -204,8 +205,11 @@ class ExtAuthzFilterGrpcTest : public ExtAuthzFilterTest { void expectGrpcClientSentRequest( const envoy::extensions::filters::http::ext_authz::v3::ExtAuthz& ext_authz_config, int requests_sent_per_thread) { - Grpc::RawAsyncClientSharedPtr async_client = async_client_manager_->getOrCreateRawAsyncClient( - ext_authz_config.grpc_service(), context_.scope(), false); + Envoy::Grpc::GrpcServiceHashKeyWrapper wrapper = + Envoy::Grpc::GrpcServiceHashKeyWrapper(ext_authz_config.grpc_service()); + Grpc::RawAsyncClientSharedPtr async_client = + async_client_manager_->getOrCreateRawAsyncClientWithWrapper(wrapper, context_.scope(), + false); Grpc::MockAsyncClient* mock_async_client = dynamic_cast(async_client.get()); EXPECT_NE(mock_async_client, nullptr); diff --git a/test/mocks/grpc/mocks.h b/test/mocks/grpc/mocks.h index 3cff1d2d75aa5..8f135ebcb298c 100644 --- a/test/mocks/grpc/mocks.h +++ b/test/mocks/grpc/mocks.h @@ -116,6 +116,10 @@ class MockAsyncClientManager : public AsyncClientManager { MOCK_METHOD(RawAsyncClientSharedPtr, getOrCreateRawAsyncClient, (const envoy::config::core::v3::GrpcService& grpc_service, Stats::Scope& scope, bool skip_cluster_check)); + + MOCK_METHOD(RawAsyncClientSharedPtr, getOrCreateRawAsyncClientWithWrapper, + (const GrpcServiceHashKeyWrapper& grpc_service, Stats::Scope& scope, + bool skip_cluster_check)); }; MATCHER_P(ProtoBufferEq, expected, "") { From 67d29f6415e76a57b6c65c40ed394731336f998f Mon Sep 17 00:00:00 2001 From: AlanDiaz Date: Wed, 23 Aug 2023 10:41:16 -0400 Subject: [PATCH 2/9] Change SerializeAsString in wrapper Signed-off-by: AlanDiaz --- envoy/grpc/async_client_manager.h | 4 ++-- source/common/grpc/async_client_manager_impl.cc | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/envoy/grpc/async_client_manager.h b/envoy/grpc/async_client_manager.h index d2b160d23504a..556e27dea4919 100644 --- a/envoy/grpc/async_client_manager.h +++ b/envoy/grpc/async_client_manager.h @@ -43,7 +43,7 @@ class GrpcServiceHashKeyWrapper { friend bool operator==(const GrpcServiceHashKeyWrapper& lhs, const GrpcServiceHashKeyWrapper& rhs) { return lhs.config_.GetTypeName() == rhs.config_.GetTypeName() && - lhs.serialized_config_ == rhs.serialized_config_; + lhs.config_as_string_ == rhs.config_as_string_; } const envoy::config::core::v3::GrpcService& config() const { return config_; } @@ -51,7 +51,7 @@ class GrpcServiceHashKeyWrapper { private: const envoy::config::core::v3::GrpcService config_; const std::size_t pre_computed_hash_; - const std::string serialized_config_; + const std::string config_as_string_; }; // Singleton gRPC client manager. Grpc::AsyncClientManager can be used to create per-service diff --git a/source/common/grpc/async_client_manager_impl.cc b/source/common/grpc/async_client_manager_impl.cc index a5bbb71ce3477..cb9f18aa38da4 100644 --- a/source/common/grpc/async_client_manager_impl.cc +++ b/source/common/grpc/async_client_manager_impl.cc @@ -52,7 +52,7 @@ AsyncClientFactoryImpl::AsyncClientFactoryImpl(Upstream::ClusterManager& cm, GrpcServiceHashKeyWrapper::GrpcServiceHashKeyWrapper( const envoy::config::core::v3::GrpcService& config) : config_(config), pre_computed_hash_(Envoy::MessageUtil::hash(config)), - serialized_config_(config.SerializeAsString()) {} + config_as_string_(Envoy::MessageUtil::getYamlStringFromMessage(config)) {} AsyncClientManagerImpl::AsyncClientManagerImpl(Upstream::ClusterManager& cm, ThreadLocal::Instance& tls, TimeSource& time_source, From 0cc13b88842075958c281bf5397a0ee2e7b95d97 Mon Sep 17 00:00:00 2001 From: AlanDiaz Date: Thu, 24 Aug 2023 11:41:39 -0400 Subject: [PATCH 3/9] Fix error in build and moved implementation for constructor. Signed-off-by: AlanDiaz --- envoy/grpc/async_client_manager.h | 7 +++---- source/common/grpc/async_client_manager_impl.cc | 5 ----- 2 files changed, 3 insertions(+), 9 deletions(-) diff --git a/envoy/grpc/async_client_manager.h b/envoy/grpc/async_client_manager.h index 556e27dea4919..ef058f85399b9 100644 --- a/envoy/grpc/async_client_manager.h +++ b/envoy/grpc/async_client_manager.h @@ -34,7 +34,8 @@ using AsyncClientFactoryPtr = std::unique_ptr; class GrpcServiceHashKeyWrapper { public: - GrpcServiceHashKeyWrapper(const envoy::config::core::v3::GrpcService& config); + GrpcServiceHashKeyWrapper(const envoy::config::core::v3::GrpcService& config) + : config_(config), pre_computed_hash_(Envoy::MessageUtil::hash(config)){}; template friend H AbslHashValue(H h, const GrpcServiceHashKeyWrapper& wrapper) { return H::combine(std::move(h), wrapper.pre_computed_hash_); @@ -42,8 +43,7 @@ class GrpcServiceHashKeyWrapper { friend bool operator==(const GrpcServiceHashKeyWrapper& lhs, const GrpcServiceHashKeyWrapper& rhs) { - return lhs.config_.GetTypeName() == rhs.config_.GetTypeName() && - lhs.config_as_string_ == rhs.config_as_string_; + return Protobuf::util::MessageDifferencer::Equivalent(lhs.config_, rhs.config_); } const envoy::config::core::v3::GrpcService& config() const { return config_; } @@ -51,7 +51,6 @@ class GrpcServiceHashKeyWrapper { private: const envoy::config::core::v3::GrpcService config_; const std::size_t pre_computed_hash_; - const std::string config_as_string_; }; // Singleton gRPC client manager. Grpc::AsyncClientManager can be used to create per-service diff --git a/source/common/grpc/async_client_manager_impl.cc b/source/common/grpc/async_client_manager_impl.cc index cb9f18aa38da4..fddd6308ca44d 100644 --- a/source/common/grpc/async_client_manager_impl.cc +++ b/source/common/grpc/async_client_manager_impl.cc @@ -49,11 +49,6 @@ AsyncClientFactoryImpl::AsyncClientFactoryImpl(Upstream::ClusterManager& cm, cm_.checkActiveStaticCluster(config.envoy_grpc().cluster_name()); } -GrpcServiceHashKeyWrapper::GrpcServiceHashKeyWrapper( - const envoy::config::core::v3::GrpcService& config) - : config_(config), pre_computed_hash_(Envoy::MessageUtil::hash(config)), - config_as_string_(Envoy::MessageUtil::getYamlStringFromMessage(config)) {} - AsyncClientManagerImpl::AsyncClientManagerImpl(Upstream::ClusterManager& cm, ThreadLocal::Instance& tls, TimeSource& time_source, Api::Api& api, const StatNames& stat_names) From 13e108168dbc841c41c14d68dadd0c8fef9ee474 Mon Sep 17 00:00:00 2001 From: AlanDiaz Date: Thu, 24 Aug 2023 15:39:13 -0400 Subject: [PATCH 4/9] Addressed review comments and did some minor refactoring Signed-off-by: AlanDiaz --- envoy/grpc/async_client_manager.h | 18 ++++++---- .../common/grpc/async_client_manager_impl.cc | 36 ++++++++++--------- .../common/grpc/async_client_manager_impl.h | 15 ++++---- .../filters/http/ext_authz/config.cc | 10 +++--- .../filters/http/ext_authz/config_test.cc | 21 +++++------ test/mocks/grpc/mocks.h | 4 +-- 6 files changed, 56 insertions(+), 48 deletions(-) diff --git a/envoy/grpc/async_client_manager.h b/envoy/grpc/async_client_manager.h index ef058f85399b9..56af9c4859991 100644 --- a/envoy/grpc/async_client_manager.h +++ b/envoy/grpc/async_client_manager.h @@ -32,18 +32,21 @@ class AsyncClientFactory { using AsyncClientFactoryPtr = std::unique_ptr; -class GrpcServiceHashKeyWrapper { +class GrpcServiceConfigWithHashKey { public: - GrpcServiceHashKeyWrapper(const envoy::config::core::v3::GrpcService& config) + GrpcServiceConfigWithHashKey(const envoy::config::core::v3::GrpcService& config) : config_(config), pre_computed_hash_(Envoy::MessageUtil::hash(config)){}; - template friend H AbslHashValue(H h, const GrpcServiceHashKeyWrapper& wrapper) { + template friend H AbslHashValue(H h, const GrpcServiceConfigWithHashKey& wrapper) { return H::combine(std::move(h), wrapper.pre_computed_hash_); } - friend bool operator==(const GrpcServiceHashKeyWrapper& lhs, - const GrpcServiceHashKeyWrapper& rhs) { - return Protobuf::util::MessageDifferencer::Equivalent(lhs.config_, rhs.config_); + friend bool operator==(const GrpcServiceConfigWithHashKey& lhs, + const GrpcServiceConfigWithHashKey& rhs) { + if (lhs.pre_computed_hash_ == rhs.pre_computed_hash_) { + return Protobuf::util::MessageDifferencer::Equivalent(lhs.config_, rhs.config_); + } + return false; } const envoy::config::core::v3::GrpcService& config() const { return config_; } @@ -75,8 +78,9 @@ class AsyncClientManager { getOrCreateRawAsyncClient(const envoy::config::core::v3::GrpcService& grpc_service, Stats::Scope& scope, bool skip_cluster_check) PURE; + // TODO(diazalan) deprecate old getOrCreateRawAsyncClient once all filters have been transitioned virtual RawAsyncClientSharedPtr - getOrCreateRawAsyncClientWithWrapper(const GrpcServiceHashKeyWrapper& grpc_service, + getOrCreateRawAsyncClientWithHashKey(const GrpcServiceConfigWithHashKey& grpc_service, Stats::Scope& scope, bool skip_cluster_check) PURE; /** diff --git a/source/common/grpc/async_client_manager_impl.cc b/source/common/grpc/async_client_manager_impl.cc index fddd6308ca44d..e741ec56383dc 100644 --- a/source/common/grpc/async_client_manager_impl.cc +++ b/source/common/grpc/async_client_manager_impl.cc @@ -137,26 +137,27 @@ AsyncClientManagerImpl::factoryForGrpcService(const envoy::config::core::v3::Grp RawAsyncClientSharedPtr AsyncClientManagerImpl::getOrCreateRawAsyncClient( const envoy::config::core::v3::GrpcService& config, Stats::Scope& scope, bool skip_cluster_check) { - const GrpcServiceHashKeyWrapper& key_wrapper = GrpcServiceHashKeyWrapper(config); - RawAsyncClientSharedPtr client = raw_async_client_cache_->getCache(key_wrapper); + const GrpcServiceConfigWithHashKey config_with_hash_key = GrpcServiceConfigWithHashKey(config); + RawAsyncClientSharedPtr client = raw_async_client_cache_->getCache(config_with_hash_key); if (client != nullptr) { return client; } - client = factoryForGrpcService(key_wrapper.config(), scope, skip_cluster_check) + client = factoryForGrpcService(config_with_hash_key.config(), scope, skip_cluster_check) ->createUncachedRawAsyncClient(); - raw_async_client_cache_->setCache(key_wrapper, client); + raw_async_client_cache_->setCache(config_with_hash_key, client); return client; } -RawAsyncClientSharedPtr AsyncClientManagerImpl::getOrCreateRawAsyncClientWithWrapper( - const GrpcServiceHashKeyWrapper& key_wrapper, Stats::Scope& scope, bool skip_cluster_check) { - RawAsyncClientSharedPtr client = raw_async_client_cache_->getCache(key_wrapper); +RawAsyncClientSharedPtr AsyncClientManagerImpl::getOrCreateRawAsyncClientWithHashKey( + const GrpcServiceConfigWithHashKey& config_with_hash_key, Stats::Scope& scope, + bool skip_cluster_check) { + RawAsyncClientSharedPtr client = raw_async_client_cache_->getCache(config_with_hash_key); if (client != nullptr) { return client; } - client = factoryForGrpcService(key_wrapper.config(), scope, skip_cluster_check) + client = factoryForGrpcService(config_with_hash_key.config(), scope, skip_cluster_check) ->createUncachedRawAsyncClient(); - raw_async_client_cache_->setCache(key_wrapper, client); + raw_async_client_cache_->setCache(config_with_hash_key, client); return client; } @@ -166,11 +167,12 @@ AsyncClientManagerImpl::RawAsyncClientCache::RawAsyncClientCache(Event::Dispatch } void AsyncClientManagerImpl::RawAsyncClientCache::setCache( - const GrpcServiceHashKeyWrapper& key_wrapper, const RawAsyncClientSharedPtr& client) { - ASSERT(lru_map_wrapper_.find(key_wrapper) == lru_map_wrapper_.end()); + const GrpcServiceConfigWithHashKey& config_with_hash_key, + const RawAsyncClientSharedPtr& client) { + ASSERT(lru_map_.find(config_with_hash_key) == lru_map_.end()); // Create a new cache entry at the beginning of the list. - lru_list_.emplace_front(key_wrapper.config(), client, dispatcher_.timeSource().monotonicTime()); - lru_map_wrapper_[key_wrapper] = lru_list_.begin(); + lru_list_.emplace_front(config_with_hash_key, client, dispatcher_.timeSource().monotonicTime()); + lru_map_[config_with_hash_key] = lru_list_.begin(); // If inserting to an empty cache, enable eviction timer. if (lru_list_.size() == 1) { evictEntriesAndResetEvictionTimer(); @@ -178,9 +180,9 @@ void AsyncClientManagerImpl::RawAsyncClientCache::setCache( } RawAsyncClientSharedPtr AsyncClientManagerImpl::RawAsyncClientCache::getCache( - const GrpcServiceHashKeyWrapper& key_wrapper) { - auto it = lru_map_wrapper_.find(key_wrapper); - if (it == lru_map_wrapper_.end()) { + const GrpcServiceConfigWithHashKey& config_with_hash_key) { + auto it = lru_map_.find(config_with_hash_key); + if (it == lru_map_.end()) { return nullptr; } const auto cache_entry = it->second; @@ -204,7 +206,7 @@ void AsyncClientManagerImpl::RawAsyncClientCache::evictEntriesAndResetEvictionTi MonotonicTime next_expire = lru_list_.back().accessed_time_ + EntryTimeoutInterval; if (now >= next_expire) { // Erase the expired entry. - lru_map_wrapper_.erase(lru_list_.back().key_wrapper_); + lru_map_.erase(lru_list_.back().config_with_hash_key_); lru_list_.pop_back(); } else { cache_eviction_timer_->enableTimer( diff --git a/source/common/grpc/async_client_manager_impl.h b/source/common/grpc/async_client_manager_impl.h index 6cfd0329604b7..eadf3009490cd 100644 --- a/source/common/grpc/async_client_manager_impl.h +++ b/source/common/grpc/async_client_manager_impl.h @@ -53,7 +53,7 @@ class AsyncClientManagerImpl : public AsyncClientManager { bool skip_cluster_check) override; RawAsyncClientSharedPtr - getOrCreateRawAsyncClientWithWrapper(const GrpcServiceHashKeyWrapper& key_wrapper, + getOrCreateRawAsyncClientWithHashKey(const GrpcServiceConfigWithHashKey& config_with_hash_key, Stats::Scope& scope, bool skip_cluster_check) override; AsyncClientFactoryPtr factoryForGrpcService(const envoy::config::core::v3::GrpcService& config, @@ -62,24 +62,25 @@ class AsyncClientManagerImpl : public AsyncClientManager { class RawAsyncClientCache : public ThreadLocal::ThreadLocalObject { public: explicit RawAsyncClientCache(Event::Dispatcher& dispatcher); - void setCache(const GrpcServiceHashKeyWrapper& key_wrapper, + void setCache(const GrpcServiceConfigWithHashKey& config_with_hash_key, const RawAsyncClientSharedPtr& client); - RawAsyncClientSharedPtr getCache(const GrpcServiceHashKeyWrapper& key_wrapper); + RawAsyncClientSharedPtr getCache(const GrpcServiceConfigWithHashKey& config_with_hash_key); private: void evictEntriesAndResetEvictionTimer(); struct CacheEntry { - CacheEntry(const envoy::config::core::v3::GrpcService& config, + CacheEntry(const GrpcServiceConfigWithHashKey& config_with_hash_key, RawAsyncClientSharedPtr const& client, MonotonicTime create_time) - : key_wrapper_(config), client_(client), accessed_time_(create_time) {} - GrpcServiceHashKeyWrapper key_wrapper_; + : config_with_hash_key_(config_with_hash_key), client_(client), + accessed_time_(create_time) {} + GrpcServiceConfigWithHashKey config_with_hash_key_; RawAsyncClientSharedPtr client_; MonotonicTime accessed_time_; }; using LruList = std::list; LruList lru_list_; - absl::flat_hash_map lru_map_wrapper_; + absl::flat_hash_map lru_map_; Event::Dispatcher& dispatcher_; Envoy::Event::TimerPtr cache_eviction_timer_; static constexpr std::chrono::seconds EntryTimeoutInterval{50}; diff --git a/source/extensions/filters/http/ext_authz/config.cc b/source/extensions/filters/http/ext_authz/config.cc index f1a7d2878fd39..1e15833e5fc14 100644 --- a/source/extensions/filters/http/ext_authz/config.cc +++ b/source/extensions/filters/http/ext_authz/config.cc @@ -49,13 +49,13 @@ Http::FilterFactoryCb ExtAuthzFilterConfig::createFilterFactoryFromProtoTyped( PROTOBUF_GET_MS_OR_DEFAULT(proto_config.grpc_service(), timeout, DefaultTimeout); Config::Utility::checkTransportVersion(proto_config); - Envoy::Grpc::GrpcServiceHashKeyWrapper wrapper = - Envoy::Grpc::GrpcServiceHashKeyWrapper(proto_config.grpc_service()); + Envoy::Grpc::GrpcServiceConfigWithHashKey config_with_hash_key = + Envoy::Grpc::GrpcServiceConfigWithHashKey(proto_config.grpc_service()); callback = [&context, filter_config, timeout_ms, - wrapper](Http::FilterChainFactoryCallbacks& callbacks) { + config_with_hash_key](Http::FilterChainFactoryCallbacks& callbacks) { auto client = std::make_unique( - context.clusterManager().grpcAsyncClientManager().getOrCreateRawAsyncClientWithWrapper( - wrapper, context.scope(), true), + context.clusterManager().grpcAsyncClientManager().getOrCreateRawAsyncClientWithHashKey( + config_with_hash_key, context.scope(), true), std::chrono::milliseconds(timeout_ms)); callbacks.addStreamFilter(std::make_shared(filter_config, std::move(client))); }; diff --git a/test/extensions/filters/http/ext_authz/config_test.cc b/test/extensions/filters/http/ext_authz/config_test.cc index cf588b176b6dd..2eddc7f01cec3 100644 --- a/test/extensions/filters/http/ext_authz/config_test.cc +++ b/test/extensions/filters/http/ext_authz/config_test.cc @@ -64,12 +64,13 @@ class ExtAuthzFilterTest : public Event::TestUsingSimulatedTime, // Delegate call to mock async client manager to real async client manager. ON_CALL(context_, getServerFactoryContext()).WillByDefault(testing::ReturnRef(server_context_)); ON_CALL(context_.cluster_manager_.async_client_manager_, - getOrCreateRawAsyncClientWithWrapper(_, _, _)) - .WillByDefault(Invoke([&](const Envoy::Grpc::GrpcServiceHashKeyWrapper& config, - Stats::Scope& scope, bool skip_cluster_check) { - return async_client_manager_->getOrCreateRawAsyncClientWithWrapper(config, scope, - skip_cluster_check); - })); + getOrCreateRawAsyncClientWithHashKey(_, _, _)) + .WillByDefault( + Invoke([&](const Envoy::Grpc::GrpcServiceConfigWithHashKey& config_with_hash_key, + Stats::Scope& scope, bool skip_cluster_check) { + return async_client_manager_->getOrCreateRawAsyncClientWithHashKey( + config_with_hash_key, scope, skip_cluster_check); + })); ExtAuthzFilterConfig factory; return factory.createFilterFactoryFromProto(ext_authz_config, "stats", context_); } @@ -205,11 +206,11 @@ class ExtAuthzFilterGrpcTest : public ExtAuthzFilterTest { void expectGrpcClientSentRequest( const envoy::extensions::filters::http::ext_authz::v3::ExtAuthz& ext_authz_config, int requests_sent_per_thread) { - Envoy::Grpc::GrpcServiceHashKeyWrapper wrapper = - Envoy::Grpc::GrpcServiceHashKeyWrapper(ext_authz_config.grpc_service()); + Envoy::Grpc::GrpcServiceConfigWithHashKey config_with_hash_key = + Envoy::Grpc::GrpcServiceConfigWithHashKey(ext_authz_config.grpc_service()); Grpc::RawAsyncClientSharedPtr async_client = - async_client_manager_->getOrCreateRawAsyncClientWithWrapper(wrapper, context_.scope(), - false); + async_client_manager_->getOrCreateRawAsyncClientWithHashKey(config_with_hash_key, + context_.scope(), false); Grpc::MockAsyncClient* mock_async_client = dynamic_cast(async_client.get()); EXPECT_NE(mock_async_client, nullptr); diff --git a/test/mocks/grpc/mocks.h b/test/mocks/grpc/mocks.h index 8f135ebcb298c..620e9f6ce646a 100644 --- a/test/mocks/grpc/mocks.h +++ b/test/mocks/grpc/mocks.h @@ -117,8 +117,8 @@ class MockAsyncClientManager : public AsyncClientManager { (const envoy::config::core::v3::GrpcService& grpc_service, Stats::Scope& scope, bool skip_cluster_check)); - MOCK_METHOD(RawAsyncClientSharedPtr, getOrCreateRawAsyncClientWithWrapper, - (const GrpcServiceHashKeyWrapper& grpc_service, Stats::Scope& scope, + MOCK_METHOD(RawAsyncClientSharedPtr, getOrCreateRawAsyncClientWithHashKey, + (const GrpcServiceConfigWithHashKey& config_with_hash_key, Stats::Scope& scope, bool skip_cluster_check)); }; From 81c7cc8c0568013d3778a113f89fdd28c86f37bc Mon Sep 17 00:00:00 2001 From: AlanDiaz Date: Thu, 24 Aug 2023 15:45:20 -0400 Subject: [PATCH 5/9] Add comment to new getOrCreateRawAsyncClient function Signed-off-by: AlanDiaz --- envoy/grpc/async_client_manager.h | 14 +++++++++++++- 1 file changed, 13 insertions(+), 1 deletion(-) diff --git a/envoy/grpc/async_client_manager.h b/envoy/grpc/async_client_manager.h index 56af9c4859991..2ed2bf2f6182b 100644 --- a/envoy/grpc/async_client_manager.h +++ b/envoy/grpc/async_client_manager.h @@ -63,6 +63,7 @@ class AsyncClientManager { public: virtual ~AsyncClientManager() = default; + // TODO(diazalan) deprecate old getOrCreateRawAsyncClient once all filters have been transitioned /** * Create a Grpc::RawAsyncClient. The async client is cached thread locally and shared across * different filter instances. @@ -78,7 +79,18 @@ class AsyncClientManager { getOrCreateRawAsyncClient(const envoy::config::core::v3::GrpcService& grpc_service, Stats::Scope& scope, bool skip_cluster_check) PURE; - // TODO(diazalan) deprecate old getOrCreateRawAsyncClient once all filters have been transitioned + /** + * Create a Grpc::RawAsyncClient. The async client is cached thread locally and shared across + * different filter instances. + * @param grpc_service Envoy::Grpc::GrpcServiceConfigWithHashKey which contains config and + * hashkey. + * @param scope stats scope. + * @param skip_cluster_check if set to true skips checks for cluster presence and being statically + * configured. + * @param cache_option always use cache or use cache when runtime is enabled. + * @return RawAsyncClientPtr a grpc async client. + * @throws EnvoyException when grpc_service validation fails. + */ virtual RawAsyncClientSharedPtr getOrCreateRawAsyncClientWithHashKey(const GrpcServiceConfigWithHashKey& grpc_service, Stats::Scope& scope, bool skip_cluster_check) PURE; From b208980e3fdf3e9fe825ee337997911a9fb5cd2c Mon Sep 17 00:00:00 2001 From: AlanDiaz Date: Thu, 31 Aug 2023 15:23:07 -0400 Subject: [PATCH 6/9] Addressed review comments Signed-off-by: AlanDiaz --- envoy/grpc/async_client_manager.h | 4 +- .../grpc/async_client_manager_impl_test.cc | 44 ++++++++++++++----- 2 files changed, 36 insertions(+), 12 deletions(-) diff --git a/envoy/grpc/async_client_manager.h b/envoy/grpc/async_client_manager.h index 2ed2bf2f6182b..629873e9f16d6 100644 --- a/envoy/grpc/async_client_manager.h +++ b/envoy/grpc/async_client_manager.h @@ -34,13 +34,15 @@ using AsyncClientFactoryPtr = std::unique_ptr; class GrpcServiceConfigWithHashKey { public: - GrpcServiceConfigWithHashKey(const envoy::config::core::v3::GrpcService& config) + explicit GrpcServiceConfigWithHashKey(const envoy::config::core::v3::GrpcService& config) : config_(config), pre_computed_hash_(Envoy::MessageUtil::hash(config)){}; template friend H AbslHashValue(H h, const GrpcServiceConfigWithHashKey& wrapper) { return H::combine(std::move(h), wrapper.pre_computed_hash_); } + std::size_t getPreComputedHash() const { return pre_computed_hash_; } + friend bool operator==(const GrpcServiceConfigWithHashKey& lhs, const GrpcServiceConfigWithHashKey& rhs) { if (lhs.pre_computed_hash_ == rhs.pre_computed_hash_) { diff --git a/test/common/grpc/async_client_manager_impl_test.cc b/test/common/grpc/async_client_manager_impl_test.cc index 121f8cbc533d5..90c2bf94075b6 100644 --- a/test/common/grpc/async_client_manager_impl_test.cc +++ b/test/common/grpc/async_client_manager_impl_test.cc @@ -52,16 +52,17 @@ class RawAsyncClientCacheTest : public testing::Test { TEST_F(RawAsyncClientCacheTest, CacheEviction) { envoy::config::core::v3::GrpcService foo_service; foo_service.mutable_envoy_grpc()->set_cluster_name("foo"); + GrpcServiceConfigWithHashKey config_with_hash_key(foo_service); RawAsyncClientSharedPtr foo_client = std::make_shared(); - client_cache_.setCache(foo_service, foo_client); + client_cache_.setCache(config_with_hash_key, foo_client); waitForSeconds(49); // Cache entry hasn't been evicted because it was created 49s ago. - EXPECT_EQ(client_cache_.getCache(foo_service).get(), foo_client.get()); + EXPECT_EQ(client_cache_.getCache(config_with_hash_key).get(), foo_client.get()); waitForSeconds(49); // Cache entry hasn't been evicted because it was accessed 49s ago. - EXPECT_EQ(client_cache_.getCache(foo_service).get(), foo_client.get()); + EXPECT_EQ(client_cache_.getCache(config_with_hash_key).get(), foo_client.get()); waitForSeconds(51); - EXPECT_EQ(client_cache_.getCache(foo_service).get(), nullptr); + EXPECT_EQ(client_cache_.getCache(config_with_hash_key).get(), nullptr); } TEST_F(RawAsyncClientCacheTest, MultipleCacheEntriesEviction) { @@ -69,23 +70,27 @@ TEST_F(RawAsyncClientCacheTest, MultipleCacheEntriesEviction) { RawAsyncClientSharedPtr foo_client = std::make_shared(); for (int i = 1; i <= 50; i++) { grpc_service.mutable_envoy_grpc()->set_cluster_name(std::to_string(i)); - client_cache_.setCache(grpc_service, foo_client); + GrpcServiceConfigWithHashKey config_with_hash_key(grpc_service); + client_cache_.setCache(config_with_hash_key, foo_client); } waitForSeconds(20); for (int i = 51; i <= 100; i++) { grpc_service.mutable_envoy_grpc()->set_cluster_name(std::to_string(i)); - client_cache_.setCache(grpc_service, foo_client); + GrpcServiceConfigWithHashKey config_with_hash_key(grpc_service); + client_cache_.setCache(config_with_hash_key, foo_client); } waitForSeconds(30); // Cache entries created 50s before have expired. for (int i = 1; i <= 50; i++) { grpc_service.mutable_envoy_grpc()->set_cluster_name(std::to_string(i)); - EXPECT_EQ(client_cache_.getCache(grpc_service).get(), nullptr); + GrpcServiceConfigWithHashKey config_with_hash_key(grpc_service); + EXPECT_EQ(client_cache_.getCache(config_with_hash_key).get(), nullptr); } // Cache entries 30s before haven't expired. for (int i = 51; i <= 100; i++) { grpc_service.mutable_envoy_grpc()->set_cluster_name(std::to_string(i)); - EXPECT_EQ(client_cache_.getCache(grpc_service).get(), foo_client.get()); + GrpcServiceConfigWithHashKey config_with_hash_key(grpc_service); + EXPECT_EQ(client_cache_.getCache(config_with_hash_key).get(), foo_client.get()); } } @@ -95,14 +100,15 @@ TEST_F(RawAsyncClientCacheTest, GetExpiredButNotEvictedCacheEntry) { envoy::config::core::v3::GrpcService foo_service; foo_service.mutable_envoy_grpc()->set_cluster_name("foo"); RawAsyncClientSharedPtr foo_client = std::make_shared(); - client_cache_.setCache(foo_service, foo_client); + GrpcServiceConfigWithHashKey config_with_hash_key(foo_service); + client_cache_.setCache(config_with_hash_key, foo_client); time_system_.advanceTimeAsyncImpl(std::chrono::seconds(50)); // Cache entry hasn't been evicted because it is accessed before timer fire. - EXPECT_EQ(client_cache_.getCache(foo_service).get(), foo_client.get()); + EXPECT_EQ(client_cache_.getCache(config_with_hash_key).get(), foo_client.get()); time_system_.advanceTimeAndRun(std::chrono::seconds(50), *dispatcher_, Event::Dispatcher::RunType::NonBlock); // Cache entry has been evicted because it is accessed after timer fire. - EXPECT_EQ(client_cache_.getCache(foo_service).get(), nullptr); + EXPECT_EQ(client_cache_.getCache(config_with_hash_key).get(), nullptr); } class AsyncClientManagerImplTest : public testing::Test { @@ -128,6 +134,22 @@ TEST_F(AsyncClientManagerImplTest, EnvoyGrpcOk) { async_client_manager_.factoryForGrpcService(grpc_service, scope_, false); } +TEST_F(AsyncClientManagerImplTest, GrpcServiceConfigWithHashKeyTest) { + envoy::config::core::v3::GrpcService grpc_service; + grpc_service.mutable_envoy_grpc()->set_cluster_name("foo"); + envoy::config::core::v3::GrpcService grpc_service_c; + grpc_service.mutable_envoy_grpc()->set_cluster_name("bar"); + + GrpcServiceConfigWithHashKey config_with_hash_key_a = GrpcServiceConfigWithHashKey(grpc_service); + GrpcServiceConfigWithHashKey config_with_hash_key_b = GrpcServiceConfigWithHashKey(grpc_service); + GrpcServiceConfigWithHashKey config_with_hash_key_c = GrpcServiceConfigWithHashKey(grpc_service_c); + EXPECT_TRUE(config_with_hash_key_a == config_with_hash_key_b); + EXPECT_FALSE(config_with_hash_key_a == config_with_hash_key_c); + + EXPECT_EQ(config_with_hash_key_a.getPreComputedHash(), config_with_hash_key_b.getPreComputedHash()); + EXPECT_NE(config_with_hash_key_a.getPreComputedHash(), config_with_hash_key_c.getPreComputedHash()); +} + TEST_F(AsyncClientManagerImplTest, RawAsyncClientCache) { envoy::config::core::v3::GrpcService grpc_service; grpc_service.mutable_envoy_grpc()->set_cluster_name("foo"); From 850a865f7acb2b9085583d8de4d17070506055a1 Mon Sep 17 00:00:00 2001 From: AlanDiaz Date: Thu, 31 Aug 2023 16:36:59 -0400 Subject: [PATCH 7/9] Format fix Signed-off-by: AlanDiaz --- test/common/grpc/async_client_manager_impl_test.cc | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/test/common/grpc/async_client_manager_impl_test.cc b/test/common/grpc/async_client_manager_impl_test.cc index 90c2bf94075b6..36dae3f1001e9 100644 --- a/test/common/grpc/async_client_manager_impl_test.cc +++ b/test/common/grpc/async_client_manager_impl_test.cc @@ -142,12 +142,15 @@ TEST_F(AsyncClientManagerImplTest, GrpcServiceConfigWithHashKeyTest) { GrpcServiceConfigWithHashKey config_with_hash_key_a = GrpcServiceConfigWithHashKey(grpc_service); GrpcServiceConfigWithHashKey config_with_hash_key_b = GrpcServiceConfigWithHashKey(grpc_service); - GrpcServiceConfigWithHashKey config_with_hash_key_c = GrpcServiceConfigWithHashKey(grpc_service_c); + GrpcServiceConfigWithHashKey config_with_hash_key_c = + GrpcServiceConfigWithHashKey(grpc_service_c); EXPECT_TRUE(config_with_hash_key_a == config_with_hash_key_b); EXPECT_FALSE(config_with_hash_key_a == config_with_hash_key_c); - EXPECT_EQ(config_with_hash_key_a.getPreComputedHash(), config_with_hash_key_b.getPreComputedHash()); - EXPECT_NE(config_with_hash_key_a.getPreComputedHash(), config_with_hash_key_c.getPreComputedHash()); + EXPECT_EQ(config_with_hash_key_a.getPreComputedHash(), + config_with_hash_key_b.getPreComputedHash()); + EXPECT_NE(config_with_hash_key_a.getPreComputedHash(), + config_with_hash_key_c.getPreComputedHash()); } TEST_F(AsyncClientManagerImplTest, RawAsyncClientCache) { From 8a9dc73c48540d94e10bc010a83696ef0603ceec Mon Sep 17 00:00:00 2001 From: AlanDiaz Date: Tue, 5 Sep 2023 11:41:48 -0400 Subject: [PATCH 8/9] Add benchmark test Signed-off-by: AlanDiaz --- test/common/grpc/BUILD | 27 +++++++ .../grpc/async_client_manager_benchmark.cc | 81 +++++++++++++++++++ 2 files changed, 108 insertions(+) create mode 100644 test/common/grpc/async_client_manager_benchmark.cc diff --git a/test/common/grpc/BUILD b/test/common/grpc/BUILD index 6cf74e7f7d7e7..e8a14be1d5d71 100644 --- a/test/common/grpc/BUILD +++ b/test/common/grpc/BUILD @@ -1,5 +1,7 @@ load( "//bazel:envoy_build_system.bzl", + "envoy_benchmark_test", + "envoy_cc_benchmark_binary", "envoy_cc_fuzz_test", "envoy_cc_test", "envoy_cc_test_library", @@ -213,3 +215,28 @@ envoy_cc_test( "//test/test_common:utility_lib", ], ) + +envoy_cc_benchmark_binary( + name = "async_client_manager_benchmark", + srcs = ["async_client_manager_benchmark.cc"], + external_deps = [ + "benchmark", + ], + deps = [ + "//source/common/api:api_lib", + "//source/common/grpc:async_client_manager_lib", + "//test/mocks/stats:stats_mocks", + "//test/mocks/thread_local:thread_local_mocks", + "//test/mocks/upstream:cluster_manager_mocks", + "//test/mocks/upstream:cluster_priority_set_mocks", + "//test/test_common:test_runtime_lib", + "//test/test_common:utility_lib", + "@envoy_api//envoy/config/core/v3:pkg_cc_proto", + ] +) + +envoy_benchmark_test( + name = "async_client_manager_benchmark_test", + timeout = "long", + benchmark_binary = "async_client_manager_benchmark", +) diff --git a/test/common/grpc/async_client_manager_benchmark.cc b/test/common/grpc/async_client_manager_benchmark.cc new file mode 100644 index 0000000000000..092df6130ce3d --- /dev/null +++ b/test/common/grpc/async_client_manager_benchmark.cc @@ -0,0 +1,81 @@ +#include + +#include "envoy/config/core/v3/grpc_service.pb.h" +#include "envoy/grpc/async_client.h" + +#include "source/common/api/api_impl.h" +#include "source/common/event/dispatcher_impl.h" +#include "source/common/grpc/async_client_manager_impl.h" + +#include "test/mocks/stats/mocks.h" +#include "test/mocks/thread_local/mocks.h" +#include "test/mocks/upstream/cluster_manager.h" +#include "test/mocks/upstream/cluster_priority_set.h" +#include "test/test_common/test_runtime.h" +#include "test/test_common/test_time.h" +#include "test/test_common/utility.h" + +#include "gmock/gmock.h" +#include "gtest/gtest.h" + +#include "test/benchmark/main.h" + +#include "benchmark/benchmark.h" + +namespace Envoy { +namespace Grpc { +namespace { + +class AsyncClientManagerImplTest { +public: + AsyncClientManagerImplTest() + : api_(Api::createApiForTest()), stat_names_(scope_.symbolTable()), + async_client_manager_(cm_, tls_, test_time_.timeSystem(), *api_, stat_names_) {} + + Upstream::MockClusterManager cm_; + NiceMock tls_; + Stats::MockStore store_; + Stats::MockScope& scope_{store_.mockScope()}; + DangerousDeprecatedTestTime test_time_; + Api::ApiPtr api_; + StatNames stat_names_; + AsyncClientManagerImpl async_client_manager_; +}; + +void testGetOrCreateAsyncClientWithConfig(::benchmark::State& state) { + AsyncClientManagerImplTest async_client_man_test; + + envoy::config::core::v3::GrpcService grpc_service; + grpc_service.mutable_envoy_grpc()->set_cluster_name("foo"); + + for(auto _ : state) { + for(int i = 0; i < 1000; i++){ + RawAsyncClientSharedPtr foo_client0 = + async_client_man_test.async_client_manager_.getOrCreateRawAsyncClient(grpc_service, async_client_man_test.scope_, true); + } + } +} + +void testGetOrCreateAsyncClientWithHashConfig(::benchmark::State& state) { + AsyncClientManagerImplTest async_client_man_test; + + envoy::config::core::v3::GrpcService grpc_service; + grpc_service.mutable_envoy_grpc()->set_cluster_name("foo"); + GrpcServiceConfigWithHashKey config_with_hash_key_a = GrpcServiceConfigWithHashKey(grpc_service); + + for(auto _ : state) { + for(int i = 0; i < 1000; i++){ + RawAsyncClientSharedPtr foo_client0 = + async_client_man_test.async_client_manager_.getOrCreateRawAsyncClientWithHashKey(config_with_hash_key_a, async_client_man_test.scope_, true); + } + } + +} + +BENCHMARK(testGetOrCreateAsyncClientWithConfig)->Unit(::benchmark::kMicrosecond); +BENCHMARK(testGetOrCreateAsyncClientWithHashConfig)->Unit(::benchmark::kMicrosecond); + + +} +} +} From 1e03215b70d9a21c50cbb235aa67a7ad1ff5b91c Mon Sep 17 00:00:00 2001 From: AlanDiaz Date: Tue, 5 Sep 2023 12:53:32 -0400 Subject: [PATCH 9/9] format fix Signed-off-by: AlanDiaz --- test/common/grpc/BUILD | 22 +++++++-------- .../grpc/async_client_manager_benchmark.cc | 28 +++++++++---------- 2 files changed, 24 insertions(+), 26 deletions(-) diff --git a/test/common/grpc/BUILD b/test/common/grpc/BUILD index e8a14be1d5d71..01458712de953 100644 --- a/test/common/grpc/BUILD +++ b/test/common/grpc/BUILD @@ -217,13 +217,13 @@ envoy_cc_test( ) envoy_cc_benchmark_binary( - name = "async_client_manager_benchmark", - srcs = ["async_client_manager_benchmark.cc"], - external_deps = [ - "benchmark", - ], - deps = [ - "//source/common/api:api_lib", + name = "async_client_manager_benchmark", + srcs = ["async_client_manager_benchmark.cc"], + external_deps = [ + "benchmark", + ], + deps = [ + "//source/common/api:api_lib", "//source/common/grpc:async_client_manager_lib", "//test/mocks/stats:stats_mocks", "//test/mocks/thread_local:thread_local_mocks", @@ -232,11 +232,11 @@ envoy_cc_benchmark_binary( "//test/test_common:test_runtime_lib", "//test/test_common:utility_lib", "@envoy_api//envoy/config/core/v3:pkg_cc_proto", - ] + ], ) envoy_benchmark_test( - name = "async_client_manager_benchmark_test", - timeout = "long", - benchmark_binary = "async_client_manager_benchmark", + name = "async_client_manager_benchmark_test", + timeout = "long", + benchmark_binary = "async_client_manager_benchmark", ) diff --git a/test/common/grpc/async_client_manager_benchmark.cc b/test/common/grpc/async_client_manager_benchmark.cc index 092df6130ce3d..f23a0aba54e50 100644 --- a/test/common/grpc/async_client_manager_benchmark.cc +++ b/test/common/grpc/async_client_manager_benchmark.cc @@ -7,6 +7,7 @@ #include "source/common/event/dispatcher_impl.h" #include "source/common/grpc/async_client_manager_impl.h" +#include "test/benchmark/main.h" #include "test/mocks/stats/mocks.h" #include "test/mocks/thread_local/mocks.h" #include "test/mocks/upstream/cluster_manager.h" @@ -15,13 +16,10 @@ #include "test/test_common/test_time.h" #include "test/test_common/utility.h" +#include "benchmark/benchmark.h" #include "gmock/gmock.h" #include "gtest/gtest.h" -#include "test/benchmark/main.h" - -#include "benchmark/benchmark.h" - namespace Envoy { namespace Grpc { namespace { @@ -48,10 +46,11 @@ void testGetOrCreateAsyncClientWithConfig(::benchmark::State& state) { envoy::config::core::v3::GrpcService grpc_service; grpc_service.mutable_envoy_grpc()->set_cluster_name("foo"); - for(auto _ : state) { - for(int i = 0; i < 1000; i++){ + for (auto _ : state) { + for (int i = 0; i < 1000; i++) { RawAsyncClientSharedPtr foo_client0 = - async_client_man_test.async_client_manager_.getOrCreateRawAsyncClient(grpc_service, async_client_man_test.scope_, true); + async_client_man_test.async_client_manager_.getOrCreateRawAsyncClient( + grpc_service, async_client_man_test.scope_, true); } } } @@ -63,19 +62,18 @@ void testGetOrCreateAsyncClientWithHashConfig(::benchmark::State& state) { grpc_service.mutable_envoy_grpc()->set_cluster_name("foo"); GrpcServiceConfigWithHashKey config_with_hash_key_a = GrpcServiceConfigWithHashKey(grpc_service); - for(auto _ : state) { - for(int i = 0; i < 1000; i++){ + for (auto _ : state) { + for (int i = 0; i < 1000; i++) { RawAsyncClientSharedPtr foo_client0 = - async_client_man_test.async_client_manager_.getOrCreateRawAsyncClientWithHashKey(config_with_hash_key_a, async_client_man_test.scope_, true); + async_client_man_test.async_client_manager_.getOrCreateRawAsyncClientWithHashKey( + config_with_hash_key_a, async_client_man_test.scope_, true); } } - } BENCHMARK(testGetOrCreateAsyncClientWithConfig)->Unit(::benchmark::kMicrosecond); BENCHMARK(testGetOrCreateAsyncClientWithHashConfig)->Unit(::benchmark::kMicrosecond); - -} -} -} +} // namespace +} // namespace Grpc +} // namespace Envoy