stats: Remove remaining calls to Stats::Scope::counter() et al in sou…

…rce/..., found by adding more patterns to the check-format script (envoyproxy#10189)

Signed-off-by: Joshua Marantz <[email protected]>

include/envoy/grpc/context.h

@@ -8,6 +8,8 @@
 namespace Envoy {
 namespace Grpc {
 
+struct StatNames;
+
 /**
  * Captures grpc-related structures with cardinality of one per server.
  */
@@ -78,6 +80,11 @@ class Context {
    */
   virtual void chargeResponseMessageStat(const Upstream::ClusterInfo& cluster,
                                          const RequestNames& request_names, uint64_t amount) PURE;
+
+  /**
+   * @return a struct containing StatNames for gRPC stat tokens.
+   */
+  virtual StatNames& statNames() PURE;
 };
 
 using ContextPtr = std::unique_ptr<Context>;

source/common/grpc/BUILD

@@ -107,6 +107,7 @@ envoy_cc_library(
     hdrs = ["context_impl.h"],
     external_deps = ["abseil_optional"],
     deps = [
+        ":stat_names_lib",
         "//include/envoy/grpc:context_interface",
         "//include/envoy/http:header_map_interface",
         "//include/envoy/stats:stats_interface",
@@ -138,6 +139,16 @@ envoy_cc_library(
     ],
 )
 
+envoy_cc_library(
+    name = "stat_names_lib",
+    srcs = ["stat_names.cc"],
+    hdrs = ["stat_names.h"],
+    deps = [
+        "//include/envoy/grpc:status",
+        "//source/common/stats:symbol_table_lib",
+    ],
+)
+
 envoy_cc_library(
     name = "google_async_client_lib",
     srcs = ["google_async_client_impl.cc"],
@@ -151,6 +162,7 @@ envoy_cc_library(
         ":google_grpc_context_lib",
         ":google_grpc_creds_lib",
         ":google_grpc_utils_lib",
+        ":stat_names_lib",
         ":typed_async_client_lib",
         "//include/envoy/api:api_interface",
         "//include/envoy/grpc:google_grpc_creds_interface",

source/common/grpc/async_client_manager_impl.cc

@@ -33,8 +33,8 @@ AsyncClientFactoryImpl::AsyncClientFactoryImpl(Upstream::ClusterManager& cm,
 
 AsyncClientManagerImpl::AsyncClientManagerImpl(Upstream::ClusterManager& cm,
                                                ThreadLocal::Instance& tls, TimeSource& time_source,
-                                               Api::Api& api)
-    : cm_(cm), tls_(tls), time_source_(time_source), api_(api) {
+                                               Api::Api& api, const StatNames& stat_names)
+    : cm_(cm), tls_(tls), time_source_(time_source), api_(api), stat_names_(stat_names) {
 #ifdef ENVOY_GOOGLE_GRPC
   google_tls_slot_ = tls.allocateSlot();
   google_tls_slot_->set(
@@ -50,17 +50,18 @@ RawAsyncClientPtr AsyncClientFactoryImpl::create() {
 
 GoogleAsyncClientFactoryImpl::GoogleAsyncClientFactoryImpl(
     ThreadLocal::Instance& tls, ThreadLocal::Slot* google_tls_slot, Stats::Scope& scope,
-    const envoy::config::core::v3::GrpcService& config, Api::Api& api)
+    const envoy::config::core::v3::GrpcService& config, Api::Api& api, const StatNames& stat_names)
     : tls_(tls), google_tls_slot_(google_tls_slot),
       scope_(scope.createScope(fmt::format("grpc.{}.", config.google_grpc().stat_prefix()))),
-      config_(config), api_(api) {
+      config_(config), api_(api), stat_names_(stat_names) {
 
 #ifndef ENVOY_GOOGLE_GRPC
   UNREFERENCED_PARAMETER(tls_);
   UNREFERENCED_PARAMETER(google_tls_slot_);
   UNREFERENCED_PARAMETER(scope_);
   UNREFERENCED_PARAMETER(config_);
   UNREFERENCED_PARAMETER(api_);
+  UNREFERENCED_PARAMETER(stat_names_);
   throw EnvoyException("Google C++ gRPC client is not linked");
 #else
   ASSERT(google_tls_slot_ != nullptr);
@@ -72,7 +73,7 @@ RawAsyncClientPtr GoogleAsyncClientFactoryImpl::create() {
   GoogleGenericStubFactory stub_factory;
   return std::make_unique<GoogleAsyncClientImpl>(
       tls_.dispatcher(), google_tls_slot_->getTyped<GoogleAsyncClientThreadLocal>(), stub_factory,
-      scope_, config_, api_);
+      scope_, config_, api_, stat_names_);
 #else
   return nullptr;
 #endif
@@ -86,7 +87,7 @@ AsyncClientManagerImpl::factoryForGrpcService(const envoy::config::core::v3::Grp
     return std::make_unique<AsyncClientFactoryImpl>(cm_, config, skip_cluster_check, time_source_);
   case envoy::config::core::v3::GrpcService::TargetSpecifierCase::kGoogleGrpc:
     return std::make_unique<GoogleAsyncClientFactoryImpl>(tls_, google_tls_slot_.get(), scope,
-                                                          config, api_);
+                                                          config, api_, stat_names_);
   default:
     NOT_REACHED_GCOVR_EXCL_LINE;
   }

source/common/grpc/async_client_manager_impl.h

@@ -8,6 +8,8 @@
 #include "envoy/thread_local/thread_local.h"
 #include "envoy/upstream/cluster_manager.h"
 
+#include "common/grpc/stat_names.h"
+
 namespace Envoy {
 namespace Grpc {
 
@@ -29,7 +31,8 @@ class GoogleAsyncClientFactoryImpl : public AsyncClientFactory {
 public:
   GoogleAsyncClientFactoryImpl(ThreadLocal::Instance& tls, ThreadLocal::Slot* google_tls_slot,
                                Stats::Scope& scope,
-                               const envoy::config::core::v3::GrpcService& config, Api::Api& api);
+                               const envoy::config::core::v3::GrpcService& config, Api::Api& api,
+                               const StatNames& stat_names);
 
   RawAsyncClientPtr create() override;
 
@@ -39,12 +42,13 @@ class GoogleAsyncClientFactoryImpl : public AsyncClientFactory {
   Stats::ScopeSharedPtr scope_;
   const envoy::config::core::v3::GrpcService config_;
   Api::Api& api_;
+  const StatNames& stat_names_;
 };
 
 class AsyncClientManagerImpl : public AsyncClientManager {
 public:
   AsyncClientManagerImpl(Upstream::ClusterManager& cm, ThreadLocal::Instance& tls,
-                         TimeSource& time_source, Api::Api& api);
+                         TimeSource& time_source, Api::Api& api, const StatNames& stat_names);
 
   // Grpc::AsyncClientManager
   AsyncClientFactoryPtr factoryForGrpcService(const envoy::config::core::v3::GrpcService& config,
@@ -57,6 +61,7 @@ class AsyncClientManagerImpl : public AsyncClientManager {
   ThreadLocal::SlotPtr google_tls_slot_;
   TimeSource& time_source_;
   Api::Api& api_;
+  const StatNames& stat_names_;
 };
 
 } // namespace Grpc

source/common/grpc/context_impl.cc

@@ -12,7 +12,8 @@ ContextImpl::ContextImpl(Stats::SymbolTable& symbol_table)
       success_(stat_name_pool_.add("success")), failure_(stat_name_pool_.add("failure")),
       total_(stat_name_pool_.add("total")), zero_(stat_name_pool_.add("0")),
       request_message_count_(stat_name_pool_.add("request_message_count")),
-      response_message_count_(stat_name_pool_.add("response_message_count")) {}
+      response_message_count_(stat_name_pool_.add("response_message_count")),
+      stat_names_(symbol_table) {}
 
 // Makes a stat name from a string, if we don't already have one for it.
 // This always takes a lock on mutex_, and if we haven't seen the name
@@ -39,18 +40,15 @@ void ContextImpl::chargeStat(const Upstream::ClusterInfo& cluster, Protocol prot
   }
 
   absl::string_view status_str = grpc_status->value().getStringView();
-  const bool success = (status_str == "0");
-
-  // TODO(jmarantz): Perhaps the universe of likely grpc status codes is
-  // sufficiently bounded that we should precompute status StatNames for popular
-  // ones beyond "0".
-  const Stats::StatName status_stat_name = success ? zero_ : makeDynamicStatName(status_str);
+  auto iter = stat_names_.status_names_.find(status_str);
+  const Stats::StatName status_stat_name =
+      (iter != stat_names_.status_names_.end()) ? iter->second : makeDynamicStatName(status_str);
   const Stats::SymbolTable::StoragePtr stat_name_storage =
       symbol_table_.join({protocolStatName(protocol), request_names.service_, request_names.method_,
                           status_stat_name});
 
   cluster.statsScope().counterFromStatName(Stats::StatName(stat_name_storage.get())).inc();
-  chargeStat(cluster, protocol, request_names, success);
+  chargeStat(cluster, protocol, request_names, (status_str == "0"));
 }
 
 void ContextImpl::chargeStat(const Upstream::ClusterInfo& cluster, Protocol protocol,

source/common/grpc/context_impl.h

@@ -7,6 +7,7 @@
 #include "envoy/http/header_map.h"
 
 #include "common/common/hash.h"
+#include "common/grpc/stat_names.h"
 #include "common/stats/symbol_table_impl.h"
 
 #include "absl/types/optional.h"
@@ -49,6 +50,8 @@ class ContextImpl : public Context {
     return protocol == Context::Protocol::Grpc ? grpc_ : grpc_web_;
   }
 
+  StatNames& statNames() override { return stat_names_; }
+
 private:
   // Makes a stat name from a string, if we don't already have one for it.
   // This always takes a lock on mutex_, and if we haven't seen the name
@@ -70,6 +73,8 @@ class ContextImpl : public Context {
   const Stats::StatName zero_;
   const Stats::StatName request_message_count_;
   const Stats::StatName response_message_count_;
+
+  StatNames stat_names_;
 };
 
 } // namespace Grpc

source/common/grpc/google_async_client_impl.cc

@@ -73,7 +73,7 @@ GoogleAsyncClientImpl::GoogleAsyncClientImpl(Event::Dispatcher& dispatcher,
                                              GoogleStubFactory& stub_factory,
                                              Stats::ScopeSharedPtr scope,
                                              const envoy::config::core::v3::GrpcService& config,
-                                             Api::Api& api)
+                                             Api::Api& api, const StatNames& stat_names)
     : dispatcher_(dispatcher), tls_(tls), stat_prefix_(config.google_grpc().stat_prefix()),
       initial_metadata_(config.initial_metadata()), scope_(scope) {
   // We rebuild the channel each time we construct the channel. It appears that the gRPC library is
@@ -83,9 +83,11 @@ GoogleAsyncClientImpl::GoogleAsyncClientImpl(Event::Dispatcher& dispatcher,
   std::shared_ptr<grpc::Channel> channel = GoogleGrpcUtils::createChannel(config, api);
   stub_ = stub_factory.createStub(channel);
   // Initialize client stats.
-  stats_.streams_total_ = &scope_->counter("streams_total");
+  // TODO(jmarantz): Capture these names in async_client_manager_impl.cc and
+  // pass in a struct of StatName objects so we don't have to take locks here.
+  stats_.streams_total_ = &scope_->counterFromStatName(stat_names.streams_total_);
   for (uint32_t i = 0; i <= Status::WellKnownGrpcStatus::MaximumKnown; ++i) {
-    stats_.streams_closed_[i] = &scope_->counter(absl::StrCat("streams_closed_", i));
+    stats_.streams_closed_[i] = &scope_->counterFromStatName(stat_names.streams_closed_[i]);
   }
 }
 

source/common/grpc/google_async_client_impl.h

@@ -16,6 +16,7 @@
 #include "common/common/thread.h"
 #include "common/common/thread_annotations.h"
 #include "common/grpc/google_grpc_context.h"
+#include "common/grpc/stat_names.h"
 #include "common/grpc/typed_async_client.h"
 #include "common/tracing/http_tracer_impl.h"
 
@@ -169,7 +170,8 @@ class GoogleAsyncClientImpl final : public RawAsyncClient, Logger::Loggable<Logg
 public:
   GoogleAsyncClientImpl(Event::Dispatcher& dispatcher, GoogleAsyncClientThreadLocal& tls,
                         GoogleStubFactory& stub_factory, Stats::ScopeSharedPtr scope,
-                        const envoy::config::core::v3::GrpcService& config, Api::Api& api);
+                        const envoy::config::core::v3::GrpcService& config, Api::Api& api,
+                        const StatNames& stat_names);
   ~GoogleAsyncClientImpl() override;
 
   // Grpc::AsyncClient

source/common/grpc/stat_names.cc

@@ -0,0 +1,16 @@
+#include "common/grpc/stat_names.h"
+
+namespace Envoy {
+namespace Grpc {
+
+StatNames::StatNames(Stats::SymbolTable& symbol_table)
+    : pool_(symbol_table), streams_total_(pool_.add("streams_total")) {
+  for (uint32_t i = 0; i <= Status::WellKnownGrpcStatus::MaximumKnown; ++i) {
+    std::string status_str = absl::StrCat(i);
+    streams_closed_[i] = pool_.add(absl::StrCat("streams_closed_", status_str));
+    status_names_[status_str] = pool_.add(status_str);
+  }
+}
+
+} // namespace Grpc
+} // namespace Envoy

source/common/grpc/stat_names.h

@@ -0,0 +1,27 @@
+#pragma once
+
+#include "envoy/grpc/status.h"
+
+#include "common/stats/symbol_table_impl.h"
+
+#include "absl/container/flat_hash_map.h"
+
+namespace Envoy {
+namespace Grpc {
+
+/**
+ * Captures symbolized representation for tokens used in grpc stats. These are
+ * broken out so they can be allocated early and used across all gRPC-related
+ * filters.
+ */
+struct StatNames {
+  explicit StatNames(Stats::SymbolTable& symbol_table);
+
+  Stats::StatNamePool pool_;
+  Stats::StatName streams_total_;
+  std::array<Stats::StatName, Status::WellKnownGrpcStatus::MaximumKnown + 1> streams_closed_;
+  absl::flat_hash_map<std::string, Stats::StatName> status_names_;
+};
+
+} // namespace Grpc
+} // namespace Envoy

source/common/http/BUILD

@@ -317,6 +317,7 @@ envoy_cc_library(
         "//include/envoy/stats:stats_interface",
         "//include/envoy/stats:stats_macros",
         "//include/envoy/stats:timespan_interface",
+        "//source/common/stats:symbol_table_lib",
     ],
 )
 

source/common/http/user_agent.cc

@@ -9,6 +9,7 @@
 #include "envoy/stats/timespan.h"
 
 #include "common/http/headers.h"
+#include "common/stats/symbol_table_impl.h"
 
 namespace Envoy {
 namespace Http {
@@ -21,8 +22,12 @@ void UserAgent::completeConnectionLength(Stats::Timespan& span) {
     return;
   }
 
-  // TODO(jmarantz): use stat-names here.
-  scope_->histogram(prefix_ + "downstream_cx_length_ms", Stats::Histogram::Unit::Milliseconds)
+  // TODO(jmarantz): use stat-names properly here. This usage takes the symbol
+  // table lock on every request if real symbol tables are enabled, and we need
+  // to pre-allocate the strings, including the ones that go into the prefix
+  // calculation below, so they can be joined without taking a lock.
+  Stats::StatNameManagedStorage storage(prefix_ + "downstream_cx_length_ms", scope_->symbolTable());
+  scope_->histogramFromStatName(storage.statName(), Stats::Histogram::Unit::Milliseconds)
       .recordValue(span.elapsed().count());
 }
 

source/common/upstream/cluster_manager_impl.cc

@@ -208,7 +208,7 @@ ClusterManagerImpl::ClusterManagerImpl(
     Runtime::RandomGenerator& random, const LocalInfo::LocalInfo& local_info,
     AccessLog::AccessLogManager& log_manager, Event::Dispatcher& main_thread_dispatcher,
     Server::Admin& admin, ProtobufMessage::ValidationContext& validation_context, Api::Api& api,
-    Http::Context& http_context)
+    Http::Context& http_context, Grpc::Context& grpc_context)
     : factory_(factory), runtime_(runtime), stats_(stats), tls_(tls.allocateSlot()),
       random_(random), bind_config_(bootstrap.cluster_manager().upstream_bind_config()),
       local_info_(local_info), cm_stats_(generateStats(stats)),
@@ -219,8 +219,8 @@ ClusterManagerImpl::ClusterManagerImpl(
       http_context_(http_context),
       subscription_factory_(local_info, main_thread_dispatcher, *this, random,
                             validation_context.dynamicValidationVisitor(), api) {
-  async_client_manager_ =
-      std::make_unique<Grpc::AsyncClientManagerImpl>(*this, tls, time_source_, api);
+  async_client_manager_ = std::make_unique<Grpc::AsyncClientManagerImpl>(
+      *this, tls, time_source_, api, grpc_context.statNames());
   const auto& cm_config = bootstrap.cluster_manager();
   if (cm_config.has_outlier_detection()) {
     const std::string event_log_file_path = cm_config.outlier_detection().event_log_path();
@@ -1321,7 +1321,7 @@ ClusterManagerPtr ProdClusterManagerFactory::clusterManagerFromProto(
     const envoy::config::bootstrap::v3::Bootstrap& bootstrap) {
   return ClusterManagerPtr{new ClusterManagerImpl(
       bootstrap, *this, stats_, tls_, runtime_, random_, local_info_, log_manager_,
-      main_thread_dispatcher_, admin_, validation_context_, api_, http_context_)};
+      main_thread_dispatcher_, admin_, validation_context_, api_, http_context_, grpc_context_)};
 }
 
 Http::ConnectionPool::InstancePtr ProdClusterManagerFactory::allocateConnPool(

source/common/upstream/cluster_manager_impl.h

@@ -39,19 +39,17 @@ namespace Upstream {
  */
 class ProdClusterManagerFactory : public ClusterManagerFactory {
 public:
-  ProdClusterManagerFactory(Server::Admin& admin, Runtime::Loader& runtime, Stats::Store& stats,
-                            ThreadLocal::Instance& tls, Runtime::RandomGenerator& random,
-                            Network::DnsResolverSharedPtr dns_resolver,
-                            Ssl::ContextManager& ssl_context_manager,
-                            Event::Dispatcher& main_thread_dispatcher,
-                            const LocalInfo::LocalInfo& local_info,
-                            Secret::SecretManager& secret_manager,
-                            ProtobufMessage::ValidationContext& validation_context, Api::Api& api,
-                            Http::Context& http_context, AccessLog::AccessLogManager& log_manager,
-                            Singleton::Manager& singleton_manager)
+  ProdClusterManagerFactory(
+      Server::Admin& admin, Runtime::Loader& runtime, Stats::Store& stats,
+      ThreadLocal::Instance& tls, Runtime::RandomGenerator& random,
+      Network::DnsResolverSharedPtr dns_resolver, Ssl::ContextManager& ssl_context_manager,
+      Event::Dispatcher& main_thread_dispatcher, const LocalInfo::LocalInfo& local_info,
+      Secret::SecretManager& secret_manager, ProtobufMessage::ValidationContext& validation_context,
+      Api::Api& api, Http::Context& http_context, Grpc::Context& grpc_context,
+      AccessLog::AccessLogManager& log_manager, Singleton::Manager& singleton_manager)
       : main_thread_dispatcher_(main_thread_dispatcher), validation_context_(validation_context),
-        api_(api), http_context_(http_context), admin_(admin), runtime_(runtime), stats_(stats),
-        tls_(tls), random_(random), dns_resolver_(dns_resolver),
+        api_(api), http_context_(http_context), grpc_context_(grpc_context), admin_(admin),
+        runtime_(runtime), stats_(stats), tls_(tls), random_(random), dns_resolver_(dns_resolver),
         ssl_context_manager_(ssl_context_manager), local_info_(local_info),
         secret_manager_(secret_manager), log_manager_(log_manager),
         singleton_manager_(singleton_manager) {}
@@ -80,6 +78,7 @@ class ProdClusterManagerFactory : public ClusterManagerFactory {
   ProtobufMessage::ValidationContext& validation_context_;
   Api::Api& api_;
   Http::Context& http_context_;
+  Grpc::Context& grpc_context_;
   Server::Admin& admin_;
   Runtime::Loader& runtime_;
   Stats::Store& stats_;
@@ -186,7 +185,7 @@ class ClusterManagerImpl : public ClusterManager, Logger::Loggable<Logger::Id::u
                      AccessLog::AccessLogManager& log_manager,
                      Event::Dispatcher& main_thread_dispatcher, Server::Admin& admin,
                      ProtobufMessage::ValidationContext& validation_context, Api::Api& api,
-                     Http::Context& http_context);
+                     Http::Context& http_context, Grpc::Context& grpc_context);
 
   std::size_t warmingClusterCount() const { return warming_clusters_.size(); }