stats: More StatName conversions (#7810)

* Convert a few more counter() references to use the StatName interface.

Signed-off-by: Joshua Marantz <jmarantz@google.com>

include/envoy/stats/symbol_table.h

@@ -19,6 +19,7 @@ namespace Stats {
  * declaration for StatName is in source/common/stats/symbol_table_impl.h
  */
 class StatName;
+using StatNameVec = std::vector<StatName>;
 
 class StatNameList;
 
@@ -105,7 +106,7 @@ class SymbolTable {
    * @param stat_names the names to join.
    * @return Storage allocated for the joined name.
    */
-  virtual StoragePtr join(const std::vector<StatName>& stat_names) const PURE;
+  virtual StoragePtr join(const StatNameVec& stat_names) const PURE;
 
   /**
    * Populates a StatNameList from a list of encodings. This is not done at

source/common/http/codes.cc

@@ -47,8 +47,7 @@ CodeStatsImpl::CodeStatsImpl(Stats::SymbolTable& symbol_table)
   upstreamRqStatName(Code::ServiceUnavailable);
 }
 
-void CodeStatsImpl::incCounter(Stats::Scope& scope,
-                               const std::vector<Stats::StatName>& names) const {
+void CodeStatsImpl::incCounter(Stats::Scope& scope, const Stats::StatNameVec& names) const {
   const Stats::SymbolTable::StoragePtr stat_name_storage = symbol_table_.join(names);
   scope.counterFromStatName(Stats::StatName(stat_name_storage.get())).inc();
 }
@@ -58,7 +57,7 @@ void CodeStatsImpl::incCounter(Stats::Scope& scope, Stats::StatName a, Stats::St
   scope.counterFromStatName(Stats::StatName(stat_name_storage.get())).inc();
 }
 
-void CodeStatsImpl::recordHistogram(Stats::Scope& scope, const std::vector<Stats::StatName>& names,
+void CodeStatsImpl::recordHistogram(Stats::Scope& scope, const Stats::StatNameVec& names,
                                     uint64_t count) const {
   const Stats::SymbolTable::StoragePtr stat_name_storage = symbol_table_.join(names);
   scope.histogramFromStatName(Stats::StatName(stat_name_storage.get())).recordValue(count);

source/common/http/codes.h

@@ -54,10 +54,9 @@ class CodeStatsImpl : public CodeStats {
 
   void writeCategory(const ResponseStatInfo& info, Stats::StatName rq_group,
                      Stats::StatName rq_code, Stats::StatName category) const;
-  void incCounter(Stats::Scope& scope, const std::vector<Stats::StatName>& names) const;
+  void incCounter(Stats::Scope& scope, const Stats::StatNameVec& names) const;
   void incCounter(Stats::Scope& scope, Stats::StatName a, Stats::StatName b) const;
-  void recordHistogram(Stats::Scope& scope, const std::vector<Stats::StatName>& names,
-                       uint64_t count) const;
+  void recordHistogram(Stats::Scope& scope, const Stats::StatNameVec& names, uint64_t count) const;
 
   static absl::string_view stripTrailingDot(absl::string_view prefix);
   Stats::StatName upstreamRqGroup(Code response_code) const;

source/common/stats/symbol_table_impl.cc

@@ -361,7 +361,7 @@ void StatNameStorageSet::free(SymbolTable& symbol_table) {
   }
 }
 
-SymbolTable::StoragePtr SymbolTableImpl::join(const std::vector<StatName>& stat_names) const {
+SymbolTable::StoragePtr SymbolTableImpl::join(const StatNameVec& stat_names) const {
   uint64_t num_bytes = 0;
   for (StatName stat_name : stat_names) {
     num_bytes += stat_name.dataSize();
@@ -429,5 +429,35 @@ void StatNameList::clear(SymbolTable& symbol_table) {
   storage_.reset();
 }
 
+StatNameSet::StatNameSet(SymbolTable& symbol_table) : pool_(symbol_table) {
+  builtin_stat_names_[""] = StatName();
+}
+
+void StatNameSet::rememberBuiltin(absl::string_view str) {
+  StatName stat_name;
+  {
+    absl::MutexLock lock(&mutex_);
+    stat_name = pool_.add(str);
+  }
+  builtin_stat_names_[str] = stat_name;
+}
+
+Stats::StatName StatNameSet::getStatName(absl::string_view token) {
+  // If token was recorded as a built-in during initialization, we can
+  // service this request lock-free.
+  const auto iter = builtin_stat_names_.find(token);
+  if (iter != builtin_stat_names_.end()) {
+    return iter->second;
+  }
+
+  // Other tokens require holding a lock for our local cache.
+  absl::MutexLock lock(&mutex_);
+  Stats::StatName& stat_name = dynamic_stat_names_[token];
+  if (stat_name.empty()) { // Note that builtin_stat_names_ already has one for "".
+    stat_name = pool_.add(token);
+  }
+  return stat_name;
+}
+
 } // namespace Stats
 } // namespace Envoy

source/common/stats/symbol_table_impl.h

@@ -132,7 +132,7 @@ class SymbolTableImpl : public SymbolTable {
   bool lessThan(const StatName& a, const StatName& b) const override;
   void free(const StatName& stat_name) override;
   void incRefCount(const StatName& stat_name) override;
-  StoragePtr join(const std::vector<StatName>& stat_names) const override;
+  StoragePtr join(const StatNameVec& stat_names) const override;
   void populateList(const absl::string_view* names, uint32_t num_names,
                     StatNameList& list) override;
   StoragePtr encode(absl::string_view name) override;
@@ -630,5 +630,51 @@ class StatNameStorageSet {
   HashSet hash_set_;
 };
 
+// Captures StatNames for lookup by string, keeping two maps: a map of
+// 'built-ins' that is expected to be populated during initialization, and a map
+// of dynamically discovered names. The latter map is protected by a mutex, and
+// can be mutated at runtime.
+//
+// Ideally, builtins should be added during process initialization, in the
+// outermost relevant context. And as the builtins map is not mutex protected,
+// builtins must *not* be added in the request-path.
+class StatNameSet {
+public:
+  explicit StatNameSet(SymbolTable& symbol_table);
+
+  /**
+   * Adds a string to the builtin map, which is not mutex protected. This map is
+   * always consulted first as a hit there means no lock is required.
+   *
+   * Builtins can only be added immediately after construction, as the builtins
+   * map is not mutex-protected.
+   */
+  void rememberBuiltin(absl::string_view str);
+
+  /**
+   * Finds a StatName by name. If 'str' has been remembered as a built-in, then
+   * no lock is required. Otherwise we first consult dynamic_stat_names_ under a
+   * lock that's private to the StatNameSet. If that's empty, we need to create
+   * the StatName in the pool, which requires taking a global lock.
+   *
+   * TODO(jmarantz): Potential perf issue here with contention, both on this
+   * set's mutex and also the SymbolTable mutex which must be taken during
+   * StatNamePool::add().
+   */
+  StatName getStatName(absl::string_view str);
+
+  /**
+   * Adds a StatName using the pool, but without remembering it in any maps.
+   */
+  StatName add(absl::string_view str) { return pool_.add(str); }
+
+private:
+  Stats::StatNamePool pool_;
+  absl::Mutex mutex_;
+  using StringStatNameMap = absl::flat_hash_map<std::string, Stats::StatName>;
+  StringStatNameMap builtin_stat_names_;
+  StringStatNameMap dynamic_stat_names_ GUARDED_BY(mutex_);
+};
+
 } // namespace Stats
 } // namespace Envoy

source/extensions/filters/http/dynamo/config.cc

@@ -16,7 +16,7 @@ Http::FilterFactoryCb
 DynamoFilterConfig::createFilter(const std::string& stat_prefix,
                                  Server::Configuration::FactoryContext& context) {
   auto stats = std::make_shared<DynamoStats>(context.scope(), stat_prefix);
-  return [&context, stat_prefix, stats](Http::FilterChainFactoryCallbacks& callbacks) -> void {
+  return [&context, stats](Http::FilterChainFactoryCallbacks& callbacks) -> void {
     callbacks.addStreamFilter(std::make_shared<Dynamo::DynamoFilter>(
         context.runtime(), stats, context.dispatcher().timeSource()));
   };

source/extensions/filters/http/dynamo/dynamo_stats.cc

@@ -1,6 +1,5 @@
 #include "extensions/filters/http/dynamo/dynamo_stats.h"
 
-#include <iostream>
 #include <memory>
 #include <string>
 
@@ -16,44 +15,44 @@ namespace HttpFilters {
 namespace Dynamo {
 
 DynamoStats::DynamoStats(Stats::Scope& scope, const std::string& prefix)
-    : scope_(scope), pool_(scope.symbolTable()), prefix_(pool_.add(prefix + "dynamodb")),
-      batch_failure_unprocessed_keys_(pool_.add("BatchFailureUnprocessedKeys")),
-      capacity_(pool_.add("capacity")), empty_response_body_(pool_.add("empty_response_body")),
-      error_(pool_.add("error")), invalid_req_body_(pool_.add("invalid_req_body")),
-      invalid_resp_body_(pool_.add("invalid_resp_body")),
-      multiple_tables_(pool_.add("multiple_tables")), no_table_(pool_.add("no_table")),
-      operation_missing_(pool_.add("operation_missing")), table_(pool_.add("table")),
-      table_missing_(pool_.add("table_missing")), upstream_rq_time_(pool_.add("upstream_rq_time")),
-      upstream_rq_total_(pool_.add("upstream_rq_total")) {
-  upstream_rq_total_groups_[0] = pool_.add("upstream_rq_total_unknown");
-  upstream_rq_time_groups_[0] = pool_.add("upstream_rq_time_unknown");
+    : scope_(scope), stat_name_set_(scope.symbolTable()),
+      prefix_(stat_name_set_.add(prefix + "dynamodb")),
+      batch_failure_unprocessed_keys_(stat_name_set_.add("BatchFailureUnprocessedKeys")),
+      capacity_(stat_name_set_.add("capacity")),
+      empty_response_body_(stat_name_set_.add("empty_response_body")),
+      error_(stat_name_set_.add("error")),
+      invalid_req_body_(stat_name_set_.add("invalid_req_body")),
+      invalid_resp_body_(stat_name_set_.add("invalid_resp_body")),
+      multiple_tables_(stat_name_set_.add("multiple_tables")),
+      no_table_(stat_name_set_.add("no_table")),
+      operation_missing_(stat_name_set_.add("operation_missing")),
+      table_(stat_name_set_.add("table")), table_missing_(stat_name_set_.add("table_missing")),
+      upstream_rq_time_(stat_name_set_.add("upstream_rq_time")),
+      upstream_rq_total_(stat_name_set_.add("upstream_rq_total")) {
+  upstream_rq_total_groups_[0] = stat_name_set_.add("upstream_rq_total_unknown");
+  upstream_rq_time_groups_[0] = stat_name_set_.add("upstream_rq_time_unknown");
   for (size_t i = 1; i < DynamoStats::NumGroupEntries; ++i) {
-    upstream_rq_total_groups_[i] = pool_.add(fmt::format("upstream_rq_total_{}xx", i));
-    upstream_rq_time_groups_[i] = pool_.add(fmt::format("upstream_rq_time_{}xx", i));
+    upstream_rq_total_groups_[i] = stat_name_set_.add(fmt::format("upstream_rq_total_{}xx", i));
+    upstream_rq_time_groups_[i] = stat_name_set_.add(fmt::format("upstream_rq_time_{}xx", i));
   }
-  RequestParser::forEachStatString([this](const std::string& str) {
-    // Thread annotation does not realize this function is only called from the
-    // constructor, so we need to lock the mutex. It's easier to just do that
-    // and it's no real penalty.
-    absl::MutexLock lock(&mutex_);
-    builtin_stat_names_[str] = pool_.add(str);
-  });
-  builtin_stat_names_[""] = Stats::StatName();
+  RequestParser::forEachStatString(
+      [this](const std::string& str) { stat_name_set_.rememberBuiltin(str); });
 }
 
-Stats::SymbolTable::StoragePtr DynamoStats::addPrefix(const std::vector<Stats::StatName>& names) {
-  std::vector<Stats::StatName> names_with_prefix{prefix_};
-  names_with_prefix.reserve(names.end() - names.begin());
+Stats::SymbolTable::StoragePtr DynamoStats::addPrefix(const Stats::StatNameVec& names) {
+  Stats::StatNameVec names_with_prefix;
+  names_with_prefix.reserve(1 + names.size());
+  names_with_prefix.push_back(prefix_);
   names_with_prefix.insert(names_with_prefix.end(), names.begin(), names.end());
   return scope_.symbolTable().join(names_with_prefix);
 }
 
-Stats::Counter& DynamoStats::counter(const std::vector<Stats::StatName>& names) {
+Stats::Counter& DynamoStats::counter(const Stats::StatNameVec& names) {
   const Stats::SymbolTable::StoragePtr stat_name_storage = addPrefix(names);
   return scope_.counterFromStatName(Stats::StatName(stat_name_storage.get()));
 }
 
-Stats::Histogram& DynamoStats::histogram(const std::vector<Stats::StatName>& names) {
+Stats::Histogram& DynamoStats::histogram(const Stats::StatNameVec& names) {
   const Stats::SymbolTable::StoragePtr stat_name_storage = addPrefix(names);
   return scope_.histogramFromStatName(Stats::StatName(stat_name_storage.get()));
 }
@@ -77,32 +76,6 @@ size_t DynamoStats::groupIndex(uint64_t status) {
   return index;
 }
 
-Stats::StatName DynamoStats::getStatName(const std::string& token) {
-  // The Dynamo system has a few well-known tokens that we have stored during
-  // construction, and we can access StatNames for those without a lock. Note
-  // that we only mutate builtin_stat_names_ during construction.
-  auto iter = builtin_stat_names_.find(token);
-  if (iter != builtin_stat_names_.end()) {
-    return iter->second;
-  }
-
-  // However, some of the tokens come from json data received during requests,
-  // so we need to store these in a mutex-protected map. Once we hold the mutex,
-  // we can check dynamic_stat_names_. If it's missing we'll have to add it
-  // to the symbol table, whose mutex is more likely to be contended, and then
-  // store it in dynamic_stat_names.
-  //
-  // TODO(jmarantz): Potential perf issue here with contention, both on this
-  // mutex and also the SymbolTable mutex which must be taken during
-  // StatNamePool::add().
-  absl::MutexLock lock(&mutex_);
-  Stats::StatName& stat_name = dynamic_stat_names_[token];
-  if (stat_name.empty()) { // Note that builtin_stat_names_ already has one for "".
-    stat_name = pool_.add(token);
-  }
-  return stat_name;
-}
-
 } // namespace Dynamo
 } // namespace HttpFilters
 } // namespace Extensions

source/extensions/filters/http/dynamo/dynamo_stats.h

@@ -16,8 +16,8 @@ class DynamoStats {
 public:
   DynamoStats(Stats::Scope& scope, const std::string& prefix);
 
-  Stats::Counter& counter(const std::vector<Stats::StatName>& names);
-  Stats::Histogram& histogram(const std::vector<Stats::StatName>& names);
+  Stats::Counter& counter(const Stats::StatNameVec& names);
+  Stats::Histogram& histogram(const Stats::StatNameVec& names);
 
   /**
    * Creates the partition id stats string. The stats format is
@@ -31,18 +31,20 @@ class DynamoStats {
 
   static size_t groupIndex(uint64_t status);
 
-  Stats::StatName getStatName(const std::string& str);
+  /**
+   * Finds or creates a StatName by string, taking a global lock if needed.
+   *
+   * TODO(jmarantz): Potential perf issue here with mutex contention for names
+   * that have not been remembered as builtins in the constructor.
+   */
+  Stats::StatName getStatName(const std::string& str) { return stat_name_set_.getStatName(str); }
 
 private:
-  Stats::SymbolTable::StoragePtr addPrefix(const std::vector<Stats::StatName>& names);
+  Stats::SymbolTable::StoragePtr addPrefix(const Stats::StatNameVec& names);
 
   Stats::Scope& scope_;
-  Stats::StatNamePool pool_ GUARDED_BY(mutex_);
+  Stats::StatNameSet stat_name_set_;
   const Stats::StatName prefix_;
-  absl::Mutex mutex_;
-  using StringStatNameMap = absl::flat_hash_map<std::string, Stats::StatName>;
-  StringStatNameMap builtin_stat_names_;
-  StringStatNameMap dynamic_stat_names_ GUARDED_BY(mutex_);
 
 public:
   const Stats::StatName batch_failure_unprocessed_keys_;

source/extensions/filters/http/ext_authz/ext_authz.cc

@@ -169,15 +169,15 @@ void Filter::onComplete(Filters::Common::ExtAuthz::ResponsePtr&& response) {
         Http::HeaderMapImpl::appendToHeader(header_to_modify->value(), header.second);
       }
     }
-    cluster_->statsScope().counter("ext_authz.ok").inc();
+    config_->incCounter(cluster_->statsScope(), config_->ext_authz_ok_);
     continueDecoding();
     break;
   }
 
   case CheckStatus::Denied: {
     ENVOY_STREAM_LOG(trace, "ext_authz filter rejected the request. Response status code: '{}",
                      *callbacks_, enumToInt(response->status_code));
-    cluster_->statsScope().counter("ext_authz.denied").inc();
+    config_->incCounter(cluster_->statsScope(), config_->ext_authz_denied_);
     Http::CodeStats::ResponseStatInfo info{config_->scope(),
                                            cluster_->statsScope(),
                                            empty_stat_name,
@@ -207,10 +207,10 @@ void Filter::onComplete(Filters::Common::ExtAuthz::ResponsePtr&& response) {
   }
 
   case CheckStatus::Error: {
-    cluster_->statsScope().counter("ext_authz.error").inc();
+    config_->incCounter(cluster_->statsScope(), config_->ext_authz_error_);
     if (config_->failureModeAllow()) {
       ENVOY_STREAM_LOG(trace, "ext_authz filter allowed the request with error", *callbacks_);
-      cluster_->statsScope().counter("ext_authz.failure_mode_allowed").inc();
+      config_->incCounter(cluster_->statsScope(), config_->ext_authz_failure_mode_allowed_);
       continueDecoding();
     } else {
       ENVOY_STREAM_LOG(

source/extensions/filters/http/ext_authz/ext_authz.h

@@ -46,7 +46,10 @@ class FilterConfig {
         clear_route_cache_(config.clear_route_cache()),
         max_request_bytes_(config.with_request_body().max_request_bytes()),
         status_on_error_(toErrorCode(config.status_on_error().code())), local_info_(local_info),
-        scope_(scope), runtime_(runtime), http_context_(http_context) {}
+        scope_(scope), runtime_(runtime), http_context_(http_context), pool_(scope.symbolTable()),
+        ext_authz_ok_(pool_.add("ext_authz.ok")), ext_authz_denied_(pool_.add("ext_authz.denied")),
+        ext_authz_error_(pool_.add("ext_authz.error")),
+        ext_authz_failure_mode_allowed_(pool_.add("ext_authz.failure_mode_allowed")) {}
 
   bool allowPartialMessage() const { return allow_partial_message_; }
 
@@ -68,6 +71,10 @@ class FilterConfig {
 
   Http::Context& httpContext() { return http_context_; }
 
+  void incCounter(Stats::Scope& scope, Stats::StatName name) {
+    scope.counterFromStatName(name).inc();
+  }
+
 private:
   static Http::Code toErrorCode(uint64_t status) {
     const auto code = static_cast<Http::Code>(status);
@@ -86,6 +93,13 @@ class FilterConfig {
   Stats::Scope& scope_;
   Runtime::Loader& runtime_;
   Http::Context& http_context_;
+  Stats::StatNamePool pool_;
+
+public:
+  const Stats::StatName ext_authz_ok_;
+  const Stats::StatName ext_authz_denied_;
+  const Stats::StatName ext_authz_error_;
+  const Stats::StatName ext_authz_failure_mode_allowed_;
 };
 
 using FilterConfigSharedPtr = std::shared_ptr<FilterConfig>;

source/extensions/filters/network/zookeeper_proxy/BUILD

@@ -31,6 +31,7 @@ envoy_cc_library(
         "//source/common/buffer:buffer_lib",
         "//source/common/common:enum_to_int",
         "//source/common/network:filter_lib",
+        "//source/common/stats:symbol_table_lib",
         "//source/extensions/filters/network:well_known_names",
     ],
 )