stats: In StatNameSet, differentiate between dynamic and builtin name…

… lookup, which should have a fallback and avoid locks (envoyproxy#8243) * Split StatNameSet API into explicit Dynamic and Builtin requests, with a required fallback stat-name for Builtins rather than a potential lock. Signed-off-by: Joshua Marantz <[email protected]>
danzh2010 · Sep 19, 2019 · 3c4e0a8 · 3c4e0a8
1 parent a94da21
commit 3c4e0a8
Show file tree

Hide file tree

Showing 18 changed files with 204 additions and 213 deletions.
diff --git a/source/common/stats/symbol_table_impl.cc b/source/common/stats/symbol_table_impl.cc
@@ -442,19 +442,26 @@ void StatNameSet::rememberBuiltin(absl::string_view str) {
   builtin_stat_names_[str] = stat_name;
 }
 
-Stats::StatName StatNameSet::getStatName(absl::string_view token) {
+StatName StatNameSet::getBuiltin(absl::string_view token, StatName fallback) {
   // 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;
   }
+  return fallback;
+}
 
-  // 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);
+StatName StatNameSet::getDynamic(absl::string_view token) {
+  Stats::StatName stat_name = getBuiltin(token, StatName());
+  if (stat_name.empty()) {
+    // Other tokens require holding a lock for our local cache.
+    absl::MutexLock lock(&mutex_);
+    Stats::StatName& stat_name_ref = dynamic_stat_names_[token];
+    if (stat_name_ref.empty()) { // Note that builtin_stat_names_ already has one for "".
+      stat_name_ref = pool_.add(token);
+    }
+    stat_name = stat_name_ref;
   }
   return stat_name;
 }

diff --git a/source/common/stats/symbol_table_impl.h b/source/common/stats/symbol_table_impl.h
@@ -658,6 +658,18 @@ class StatNameSet {
    */
   void rememberBuiltin(absl::string_view str);
 
+  /**
+   * Remembers every string in a container as builtins.
+   */
+  template <class StringContainer> void rememberBuiltins(const StringContainer& container) {
+    for (const auto& str : container) {
+      rememberBuiltin(str);
+    }
+  }
+  void rememberBuiltins(const std::vector<const char*>& container) {
+    rememberBuiltins<std::vector<const char*>>(container);
+  }
+
   /**
    * Finds a StatName by name. If 'token' has been remembered as a built-in,
    * then no lock is required. Otherwise we must consult dynamic_stat_names_
@@ -671,15 +683,19 @@ class StatNameSet {
    * set's mutex and also the SymbolTable mutex which must be taken during
    * StatNamePool::add().
    */
-  StatName getStatName(absl::string_view token);
+  StatName getDynamic(absl::string_view token);
+  StatName getBuiltin(absl::string_view token, StatName fallback);
 
   /**
    * Adds a StatName using the pool, but without remembering it in any maps.
    */
-  StatName add(absl::string_view str) { return pool_.add(str); }
+  StatName add(absl::string_view str) {
+    absl::MutexLock lock(&mutex_);
+    return pool_.add(str);
+  }
 
 private:
-  Stats::StatNamePool pool_;
+  Stats::StatNamePool pool_ GUARDED_BY(mutex_);
   absl::Mutex mutex_;
   using StringStatNameMap = absl::flat_hash_map<std::string, Stats::StatName>;
   StringStatNameMap builtin_stat_names_;

diff --git a/source/extensions/filters/http/dynamo/dynamo_filter.cc b/source/extensions/filters/http/dynamo/dynamo_filter.cc
@@ -176,11 +176,11 @@ void DynamoFilter::chargeStatsPerEntity(const std::string& entity, const std::st
       time_source_.monotonicTime() - start_decode_);
 
   size_t group_index = DynamoStats::groupIndex(status);
-  const Stats::StatName entity_type_name = stats_->getStatName(entity_type);
-  const Stats::StatName entity_name = stats_->getStatName(entity);
-  const Stats::StatName total_name =
-      stats_->getStatName(absl::StrCat("upstream_rq_total_", status));
-  const Stats::StatName time_name = stats_->getStatName(absl::StrCat("upstream_rq_time_", status));
+  const Stats::StatName entity_type_name =
+      stats_->getBuiltin(entity_type, stats_->unknown_entity_type_);
+  const Stats::StatName entity_name = stats_->getDynamic(entity);
+  const Stats::StatName total_name = stats_->getDynamic(absl::StrCat("upstream_rq_total_", status));
+  const Stats::StatName time_name = stats_->getDynamic(absl::StrCat("upstream_rq_time_", status));
 
   stats_->counter({entity_type_name, entity_name, stats_->upstream_rq_total_}).inc();
   const Stats::StatName total_group = stats_->upstream_rq_total_groups_[group_index];
@@ -200,7 +200,7 @@ void DynamoFilter::chargeUnProcessedKeysStats(const Json::Object& json_body) {
   std::vector<std::string> unprocessed_tables = RequestParser::parseBatchUnProcessedKeys(json_body);
   for (const std::string& unprocessed_table : unprocessed_tables) {
     stats_
-        ->counter({stats_->error_, stats_->getStatName(unprocessed_table),
+        ->counter({stats_->error_, stats_->getDynamic(unprocessed_table),
                    stats_->batch_failure_unprocessed_keys_})
         .inc();
   }
@@ -211,11 +211,11 @@ void DynamoFilter::chargeFailureSpecificStats(const Json::Object& json_body) {
 
   if (!error_type.empty()) {
     if (table_descriptor_.table_name.empty()) {
-      stats_->counter({stats_->error_, stats_->no_table_, stats_->getStatName(error_type)}).inc();
+      stats_->counter({stats_->error_, stats_->no_table_, stats_->getDynamic(error_type)}).inc();
     } else {
       stats_
-          ->counter({stats_->error_, stats_->getStatName(table_descriptor_.table_name),
-                     stats_->getStatName(error_type)})
+          ->counter({stats_->error_, stats_->getDynamic(table_descriptor_.table_name),
+                     stats_->getDynamic(error_type)})
           .inc();
     }
   } else {

diff --git a/source/extensions/filters/http/dynamo/dynamo_stats.cc b/source/extensions/filters/http/dynamo/dynamo_stats.cc
@@ -28,7 +28,9 @@ DynamoStats::DynamoStats(Stats::Scope& scope, const std::string& prefix)
       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_(stat_name_set_.add("upstream_rq_total")),
+      unknown_entity_type_(stat_name_set_.add("unknown_entity_type")),
+      unknown_operation_(stat_name_set_.add("unknown_operation")) {
   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) {
@@ -37,6 +39,11 @@ DynamoStats::DynamoStats(Stats::Scope& scope, const std::string& prefix)
   }
   RequestParser::forEachStatString(
       [this](const std::string& str) { stat_name_set_.rememberBuiltin(str); });
+  for (uint32_t status_code : {200, 400, 403, 502}) {
+    stat_name_set_.rememberBuiltin(absl::StrCat("upstream_rq_time_", status_code));
+    stat_name_set_.rememberBuiltin(absl::StrCat("upstream_rq_total_", status_code));
+  }
+  stat_name_set_.rememberBuiltins({"operation", "table"});
 }
 
 Stats::SymbolTable::StoragePtr DynamoStats::addPrefix(const Stats::StatNameVec& names) {
@@ -62,9 +69,9 @@ Stats::Counter& DynamoStats::buildPartitionStatCounter(const std::string& table_
                                                        const std::string& partition_id) {
   // Use the last 7 characters of the partition id.
   absl::string_view id_last_7 = absl::string_view(partition_id).substr(partition_id.size() - 7);
-  const Stats::SymbolTable::StoragePtr stat_name_storage =
-      addPrefix({table_, getStatName(table_name), capacity_, getStatName(operation),
-                 getStatName(absl::StrCat("__partition_id=", id_last_7))});
+  const Stats::SymbolTable::StoragePtr stat_name_storage = addPrefix(
+      {table_, getDynamic(table_name), capacity_, getBuiltin(operation, unknown_operation_),
+       getDynamic(absl::StrCat("__partition_id=", id_last_7))});
   return scope_.counterFromStatName(Stats::StatName(stat_name_storage.get()));
 }
 

diff --git a/source/extensions/filters/http/dynamo/dynamo_stats.h b/source/extensions/filters/http/dynamo/dynamo_stats.h
@@ -37,7 +37,10 @@ class DynamoStats {
    * 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); }
+  Stats::StatName getDynamic(const std::string& str) { return stat_name_set_.getDynamic(str); }
+  Stats::StatName getBuiltin(const std::string& str, Stats::StatName fallback) {
+    return stat_name_set_.getBuiltin(str, fallback);
+  }
 
 private:
   Stats::SymbolTable::StoragePtr addPrefix(const Stats::StatNameVec& names);
@@ -61,6 +64,8 @@ class DynamoStats {
   const Stats::StatName upstream_rq_time_;
   const Stats::StatName upstream_rq_total_;
   const Stats::StatName upstream_rq_unknown_;
+  const Stats::StatName unknown_entity_type_;
+  const Stats::StatName unknown_operation_;
 
   // Keep group codes for HTTP status codes through the 500s.
   static constexpr size_t NumGroupEntries = 6;

diff --git a/source/extensions/filters/http/fault/fault_filter.cc b/source/extensions/filters/http/fault/fault_filter.cc
@@ -86,7 +86,7 @@ FaultFilterConfig::FaultFilterConfig(const envoy::config::filter::http::fault::v
 void FaultFilterConfig::incCounter(absl::string_view downstream_cluster,
                                    Stats::StatName stat_name) {
   Stats::SymbolTable::StoragePtr storage = scope_.symbolTable().join(
-      {stats_prefix_, stat_name_set_.getStatName(downstream_cluster), stat_name});
+      {stats_prefix_, stat_name_set_.getDynamic(downstream_cluster), stat_name});
   scope_.counterFromStatName(Stats::StatName(storage.get())).inc();
 }
 

diff --git a/source/extensions/filters/http/ip_tagging/ip_tagging_filter.cc b/source/extensions/filters/http/ip_tagging/ip_tagging_filter.cc
@@ -52,7 +52,7 @@ IpTaggingFilterConfig::IpTaggingFilterConfig(
 
 void IpTaggingFilterConfig::incCounter(Stats::StatName name, absl::string_view tag) {
   Stats::SymbolTable::StoragePtr storage =
-      scope_.symbolTable().join({stats_prefix_, stat_name_set_.getStatName(tag), name});
+      scope_.symbolTable().join({stats_prefix_, stat_name_set_.getDynamic(tag), name});
   scope_.counterFromStatName(Stats::StatName(storage.get())).inc();
 }
 

diff --git a/source/extensions/filters/network/common/redis/redis_command_stats.cc b/source/extensions/filters/network/common/redis/redis_command_stats.cc
@@ -9,34 +9,25 @@ namespace Common {
 namespace Redis {
 
 RedisCommandStats::RedisCommandStats(Stats::SymbolTable& symbol_table, const std::string& prefix)
-    : symbol_table_(symbol_table), stat_name_pool_(symbol_table_),
-      prefix_(stat_name_pool_.add(prefix)),
-      upstream_rq_time_(stat_name_pool_.add("upstream_rq_time")),
-      latency_(stat_name_pool_.add("latency")), total_(stat_name_pool_.add("total")),
-      success_(stat_name_pool_.add("success")), error_(stat_name_pool_.add("error")),
-      unused_metric_(stat_name_pool_.add("unused")), null_metric_(stat_name_pool_.add("null")),
-      unknown_metric_(stat_name_pool_.add("unknown")) {
+    : symbol_table_(symbol_table), stat_name_set_(symbol_table_),
+      prefix_(stat_name_set_.add(prefix)),
+      upstream_rq_time_(stat_name_set_.add("upstream_rq_time")),
+      latency_(stat_name_set_.add("latency")), total_(stat_name_set_.add("total")),
+      success_(stat_name_set_.add("success")), error_(stat_name_set_.add("error")),
+      unused_metric_(stat_name_set_.add("unused")), null_metric_(stat_name_set_.add("null")),
+      unknown_metric_(stat_name_set_.add("unknown")) {
   // Note: Even if this is disabled, we track the upstream_rq_time.
   // Create StatName for each Redis command. Note that we don't include Auth or Ping.
-  for (const std::string& command :
-       Extensions::NetworkFilters::Common::Redis::SupportedCommands::simpleCommands()) {
-    addCommandToPool(command);
-  }
-  for (const std::string& command :
-       Extensions::NetworkFilters::Common::Redis::SupportedCommands::evalCommands()) {
-    addCommandToPool(command);
-  }
-  for (const std::string& command : Extensions::NetworkFilters::Common::Redis::SupportedCommands::
-           hashMultipleSumResultCommands()) {
-    addCommandToPool(command);
-  }
-  addCommandToPool(Extensions::NetworkFilters::Common::Redis::SupportedCommands::mget());
-  addCommandToPool(Extensions::NetworkFilters::Common::Redis::SupportedCommands::mset());
-}
-
-void RedisCommandStats::addCommandToPool(const std::string& command_string) {
-  Stats::StatName command = stat_name_pool_.add(command_string);
-  stat_name_map_[command_string] = command;
+  stat_name_set_.rememberBuiltins(
+      Extensions::NetworkFilters::Common::Redis::SupportedCommands::simpleCommands());
+  stat_name_set_.rememberBuiltins(
+      Extensions::NetworkFilters::Common::Redis::SupportedCommands::evalCommands());
+  stat_name_set_.rememberBuiltins(Extensions::NetworkFilters::Common::Redis::SupportedCommands::
+                                      hashMultipleSumResultCommands());
+  stat_name_set_.rememberBuiltin(
+      Extensions::NetworkFilters::Common::Redis::SupportedCommands::mget());
+  stat_name_set_.rememberBuiltin(
+      Extensions::NetworkFilters::Common::Redis::SupportedCommands::mset());
 }
 
 Stats::Counter& RedisCommandStats::counter(Stats::Scope& scope,
@@ -76,17 +67,9 @@ Stats::StatName RedisCommandStats::getCommandFromRequest(const RespValue& reques
   case RespType::Null:
     return null_metric_;
   default:
-    // Once we have a RespType::String we lowercase it and then look it up in our stat_name_map.
-    // If it does not exist, we return our unknown stat name.
     std::string to_lower_command(request.asString());
     to_lower_table_.toLowerCase(to_lower_command);
-
-    auto iter = stat_name_map_.find(to_lower_command);
-    if (iter != stat_name_map_.end()) {
-      return iter->second;
-    } else {
-      return unknown_metric_;
-    }
+    return stat_name_set_.getBuiltin(to_lower_command, unknown_metric_);
   }
 }
 
@@ -107,4 +90,4 @@ void RedisCommandStats::updateStats(Stats::Scope& scope, Stats::StatName command
 } // namespace Common
 } // namespace NetworkFilters
 } // namespace Extensions
-} // namespace Envoy
+} // namespace Envoy
diff --git a/source/extensions/filters/network/common/redis/redis_command_stats.h b/source/extensions/filters/network/common/redis/redis_command_stats.h
@@ -41,11 +41,8 @@ class RedisCommandStats {
   Stats::StatName getUnusedStatName() { return unused_metric_; }
 
 private:
-  void addCommandToPool(const std::string& command_string);
-
   Stats::SymbolTable& symbol_table_;
-  Stats::StatNamePool stat_name_pool_;
-  StringMap<Stats::StatName> stat_name_map_;
+  Stats::StatNameSet stat_name_set_;
   const Stats::StatName prefix_;
   const Stats::StatName upstream_rq_time_;
   const Stats::StatName latency_;

diff --git a/source/extensions/filters/network/mongo_proxy/mongo_stats.cc b/source/extensions/filters/network/mongo_proxy/mongo_stats.cc
@@ -21,7 +21,13 @@ MongoStats::MongoStats(Stats::Scope& scope, const std::string& prefix)
       reply_size_(stat_name_set_.add("reply_size")),
       reply_time_ms_(stat_name_set_.add("reply_time_ms")), time_ms_(stat_name_set_.add("time_ms")),
       query_(stat_name_set_.add("query")), scatter_get_(stat_name_set_.add("scatter_get")),
-      total_(stat_name_set_.add("total")) {}
+      total_(stat_name_set_.add("total")), unknown_command_(stat_name_set_.add("unknown_command")) {
+
+  // TODO(jmarantz): is this the right set of mongo commands to use as builtins?
+  // Should we also have builtins for callsites or collections, or do those need
+  // to be dynamic?
+  stat_name_set_.rememberBuiltins({"insert", "query", "update", "delete"});
+}
 
 Stats::SymbolTable::StoragePtr MongoStats::addPrefix(const std::vector<Stats::StatName>& names) {
   std::vector<Stats::StatName> names_with_prefix;

diff --git a/source/extensions/filters/network/mongo_proxy/mongo_stats.h b/source/extensions/filters/network/mongo_proxy/mongo_stats.h
@@ -26,7 +26,11 @@ class MongoStats {
    * 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); }
+  Stats::StatName getBuiltin(const std::string& str, Stats::StatName fallback) {
+    return stat_name_set_.getBuiltin(str, fallback);
+  }
+
+  Stats::StatName getDynamic(const std::string& str) { return stat_name_set_.getDynamic(str); }
 
 private:
   Stats::SymbolTable::StoragePtr addPrefix(const std::vector<Stats::StatName>& names);
@@ -47,6 +51,7 @@ class MongoStats {
   const Stats::StatName query_;
   const Stats::StatName scatter_get_;
   const Stats::StatName total_;
+  const Stats::StatName unknown_command_;
 };
 using MongoStatsSharedPtr = std::shared_ptr<MongoStats>;
 

diff --git a/source/extensions/filters/network/mongo_proxy/proxy.cc b/source/extensions/filters/network/mongo_proxy/proxy.cc
@@ -146,21 +146,22 @@ void ProxyFilter::decodeQuery(QueryMessagePtr&& message) {
   if (!active_query->query_info_.command().empty()) {
     // First field key is the operation.
     mongo_stats_->incCounter({mongo_stats_->cmd_,
-                              mongo_stats_->getStatName(active_query->query_info_.command()),
+                              mongo_stats_->getBuiltin(active_query->query_info_.command(),
+                                                       mongo_stats_->unknown_command_),
                               mongo_stats_->total_});
   } else {
     // Normal query, get stats on a per collection basis first.
     QueryMessageInfo::QueryType query_type = active_query->query_info_.type();
     Stats::StatNameVec names;
     names.reserve(6); // 2 entries are added by chargeQueryStats().
     names.push_back(mongo_stats_->collection_);
-    names.push_back(mongo_stats_->getStatName(active_query->query_info_.collection()));
+    names.push_back(mongo_stats_->getDynamic(active_query->query_info_.collection()));
     chargeQueryStats(names, query_type);
 
     // Callsite stats if we have it.
     if (!active_query->query_info_.callsite().empty()) {
       names.push_back(mongo_stats_->callsite_);
-      names.push_back(mongo_stats_->getStatName(active_query->query_info_.callsite()));
+      names.push_back(mongo_stats_->getDynamic(active_query->query_info_.callsite()));
       chargeQueryStats(names, query_type);
     }
 
@@ -223,12 +224,13 @@ void ProxyFilter::decodeReply(ReplyMessagePtr&& message) {
 
     if (!active_query.query_info_.command().empty()) {
       Stats::StatNameVec names{mongo_stats_->cmd_,
-                               mongo_stats_->getStatName(active_query.query_info_.command())};
+                               mongo_stats_->getBuiltin(active_query.query_info_.command(),
+                                                        mongo_stats_->unknown_command_)};
       chargeReplyStats(active_query, names, *message);
     } else {
       // Collection stats first.
       Stats::StatNameVec names{mongo_stats_->collection_,
-                               mongo_stats_->getStatName(active_query.query_info_.collection()),
+                               mongo_stats_->getDynamic(active_query.query_info_.collection()),
                                mongo_stats_->query_};
       chargeReplyStats(active_query, names, *message);
 
@@ -238,7 +240,7 @@ void ProxyFilter::decodeReply(ReplyMessagePtr&& message) {
         // to mutate the array to {"collection", collection, "callsite", callsite, "query"}.
         ASSERT(names.size() == 3);
         names.back() = mongo_stats_->callsite_; // Replaces "query".
-        names.push_back(mongo_stats_->getStatName(active_query.query_info_.callsite()));
+        names.push_back(mongo_stats_->getDynamic(active_query.query_info_.callsite()));
         names.push_back(mongo_stats_->query_);
         chargeReplyStats(active_query, names, *message);
       }