stats: Clean up all calls to Scope::counter() et al in production cod…

…e. (#7842)

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

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

source/common/stats/symbol_table_impl.h

@@ -652,10 +652,13 @@ class StatNameSet {
   void rememberBuiltin(absl::string_view str);
 
   /**
-   * Finds a StatName by name. If 'token' 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.
+   * 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_
+   * 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, and
+   * then remember the new StatName in the dynamic_stat_names_. This allows
+   * subsequent lookups of the same string to take only the set's lock, and not
+   * the whole symbol-table 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

source/extensions/filters/http/fault/fault_filter.cc

@@ -78,7 +78,17 @@ FaultFilterConfig::FaultFilterConfig(const envoy::config::filter::http::fault::v
                                      Runtime::Loader& runtime, const std::string& stats_prefix,
                                      Stats::Scope& scope, TimeSource& time_source)
     : settings_(fault), runtime_(runtime), stats_(generateStats(stats_prefix, scope)),
-      stats_prefix_(stats_prefix), scope_(scope), time_source_(time_source) {}
+      scope_(scope), time_source_(time_source), stat_name_set_(scope.symbolTable()),
+      aborts_injected_(stat_name_set_.add("aborts_injected")),
+      delays_injected_(stat_name_set_.add("delays_injected")),
+      stats_prefix_(stat_name_set_.add(absl::StrCat(stats_prefix, "fault"))) {}
+
+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});
+  scope_.counterFromStatName(Stats::StatName(storage.get())).inc();
+}
 
 FaultFilter::FaultFilter(FaultFilterConfigSharedPtr config) : config_(config) {}
 
@@ -279,10 +289,7 @@ uint64_t FaultFilter::abortHttpStatus() {
 void FaultFilter::recordDelaysInjectedStats() {
   // Downstream specific stats.
   if (!downstream_cluster_.empty()) {
-    const std::string stats_counter =
-        fmt::format("{}fault.{}.delays_injected", config_->statsPrefix(), downstream_cluster_);
-
-    config_->scope().counter(stats_counter).inc();
+    config_->incDelays(downstream_cluster_);
   }
 
   // General stats. All injected faults are considered a single aggregate active fault.
@@ -293,10 +300,7 @@ void FaultFilter::recordDelaysInjectedStats() {
 void FaultFilter::recordAbortsInjectedStats() {
   // Downstream specific stats.
   if (!downstream_cluster_.empty()) {
-    const std::string stats_counter =
-        fmt::format("{}fault.{}.aborts_injected", config_->statsPrefix(), downstream_cluster_);
-
-    config_->scope().counter(stats_counter).inc();
+    config_->incAborts(downstream_cluster_);
   }
 
   // General stats. All injected faults are considered a single aggregate active fault.

source/extensions/filters/http/fault/fault_filter.h

@@ -16,6 +16,7 @@
 #include "common/buffer/watermark_buffer.h"
 #include "common/common/token_bucket_impl.h"
 #include "common/http/header_utility.h"
+#include "common/stats/symbol_table_impl.h"
 
 #include "extensions/filters/common/fault/fault_config.h"
 
@@ -111,20 +112,31 @@ class FaultFilterConfig {
 
   Runtime::Loader& runtime() { return runtime_; }
   FaultFilterStats& stats() { return stats_; }
-  const std::string& statsPrefix() { return stats_prefix_; }
   Stats::Scope& scope() { return scope_; }
   const FaultSettings* settings() { return &settings_; }
   TimeSource& timeSource() { return time_source_; }
 
+  void incDelays(absl::string_view downstream_cluster) {
+    incCounter(downstream_cluster, delays_injected_);
+  }
+
+  void incAborts(absl::string_view downstream_cluster) {
+    incCounter(downstream_cluster, aborts_injected_);
+  }
+
 private:
   static FaultFilterStats generateStats(const std::string& prefix, Stats::Scope& scope);
+  void incCounter(absl::string_view downstream_cluster, Stats::StatName stat_name);
 
   const FaultSettings settings_;
   Runtime::Loader& runtime_;
   FaultFilterStats stats_;
-  const std::string stats_prefix_;
   Stats::Scope& scope_;
   TimeSource& time_source_;
+  Stats::StatNameSet stat_name_set_;
+  const Stats::StatName aborts_injected_;
+  const Stats::StatName delays_injected_;
+  const Stats::StatName stats_prefix_; // Includes ".fault".
 };
 
 using FaultFilterConfigSharedPtr = std::shared_ptr<FaultFilterConfig>;

source/extensions/filters/http/ip_tagging/BUILD

@@ -22,6 +22,7 @@ envoy_cc_library(
         "//source/common/http:header_map_lib",
         "//source/common/http:headers_lib",
         "//source/common/network:lc_trie_lib",
+        "//source/common/stats:symbol_table_lib",
         "@envoy_api//envoy/config/filter/http/ip_tagging/v2:ip_tagging_cc",
     ],
 )

source/extensions/filters/http/ip_tagging/ip_tagging_filter.cc

@@ -10,6 +10,52 @@ namespace Extensions {
 namespace HttpFilters {
 namespace IpTagging {
 
+IpTaggingFilterConfig::IpTaggingFilterConfig(
+    const envoy::config::filter::http::ip_tagging::v2::IPTagging& config,
+    const std::string& stat_prefix, Stats::Scope& scope, Runtime::Loader& runtime)
+    : request_type_(requestTypeEnum(config.request_type())), scope_(scope), runtime_(runtime),
+      stat_name_set_(scope.symbolTable()),
+      stats_prefix_(stat_name_set_.add(stat_prefix + "ip_tagging")),
+      hit_(stat_name_set_.add("hit")), no_hit_(stat_name_set_.add("no_hit")),
+      total_(stat_name_set_.add("total")) {
+
+  // Once loading IP tags from a file system is supported, the restriction on the size
+  // of the set should be removed and observability into what tags are loaded needs
+  // to be implemented.
+  // TODO(ccaraman): Remove size check once file system support is implemented.
+  // Work is tracked by issue https://github.com/envoyproxy/envoy/issues/2695.
+  if (config.ip_tags().empty()) {
+    throw EnvoyException("HTTP IP Tagging Filter requires ip_tags to be specified.");
+  }
+
+  std::vector<std::pair<std::string, std::vector<Network::Address::CidrRange>>> tag_data;
+  tag_data.reserve(config.ip_tags().size());
+  for (const auto& ip_tag : config.ip_tags()) {
+    std::vector<Network::Address::CidrRange> cidr_set;
+    cidr_set.reserve(ip_tag.ip_list().size());
+    for (const envoy::api::v2::core::CidrRange& entry : ip_tag.ip_list()) {
+
+      // Currently, CidrRange::create doesn't guarantee that the CidrRanges are valid.
+      Network::Address::CidrRange cidr_entry = Network::Address::CidrRange::create(entry);
+      if (cidr_entry.isValid()) {
+        cidr_set.emplace_back(std::move(cidr_entry));
+      } else {
+        throw EnvoyException(
+            fmt::format("invalid ip/mask combo '{}/{}' (format is <ip>/<# mask bits>)",
+                        entry.address_prefix(), entry.prefix_len().value()));
+      }
+    }
+    tag_data.emplace_back(ip_tag.ip_tag_name(), cidr_set);
+  }
+  trie_ = std::make_unique<Network::LcTrie::LcTrie<std::string>>(tag_data);
+}
+
+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_.counterFromStatName(Stats::StatName(storage.get())).inc();
+}
+
 IpTaggingFilter::IpTaggingFilter(IpTaggingFilterConfigSharedPtr config) : config_(config) {}
 
 IpTaggingFilter::~IpTaggingFilter() = default;
@@ -42,12 +88,12 @@ Http::FilterHeadersStatus IpTaggingFilter::decodeHeaders(Http::HeaderMap& header
     // If there are use cases with a large set of tags, a way to opt into these stats
     // should be exposed and other observability options like logging tags need to be implemented.
     for (const std::string& tag : tags) {
-      config_->scope().counter(fmt::format("{}{}.hit", config_->statsPrefix(), tag)).inc();
+      config_->incHit(tag);
     }
   } else {
-    config_->scope().counter(fmt::format("{}no_hit", config_->statsPrefix())).inc();
+    config_->incNoHit();
   }
-  config_->scope().counter(fmt::format("{}total", config_->statsPrefix())).inc();
+  config_->incTotal();
   return Http::FilterHeadersStatus::Continue;
 }
 

source/extensions/filters/http/ip_tagging/ip_tagging_filter.h

@@ -14,6 +14,7 @@
 
 #include "common/network/cidr_range.h"
 #include "common/network/lc_trie.h"
+#include "common/stats/symbol_table_impl.h"
 
 namespace Envoy {
 namespace Extensions {
@@ -32,46 +33,16 @@ class IpTaggingFilterConfig {
 public:
   IpTaggingFilterConfig(const envoy::config::filter::http::ip_tagging::v2::IPTagging& config,
                         const std::string& stat_prefix, Stats::Scope& scope,
-                        Runtime::Loader& runtime)
-      : request_type_(requestTypeEnum(config.request_type())), scope_(scope), runtime_(runtime),
-        stats_prefix_(stat_prefix + "ip_tagging.") {
-
-    // Once loading IP tags from a file system is supported, the restriction on the size
-    // of the set should be removed and observability into what tags are loaded needs
-    // to be implemented.
-    // TODO(ccaraman): Remove size check once file system support is implemented.
-    // Work is tracked by issue https://github.com/envoyproxy/envoy/issues/2695.
-    if (config.ip_tags().empty()) {
-      throw EnvoyException("HTTP IP Tagging Filter requires ip_tags to be specified.");
-    }
-
-    std::vector<std::pair<std::string, std::vector<Network::Address::CidrRange>>> tag_data;
-    tag_data.reserve(config.ip_tags().size());
-    for (const auto& ip_tag : config.ip_tags()) {
-      std::vector<Network::Address::CidrRange> cidr_set;
-      cidr_set.reserve(ip_tag.ip_list().size());
-      for (const envoy::api::v2::core::CidrRange& entry : ip_tag.ip_list()) {
-
-        // Currently, CidrRange::create doesn't guarantee that the CidrRanges are valid.
-        Network::Address::CidrRange cidr_entry = Network::Address::CidrRange::create(entry);
-        if (cidr_entry.isValid()) {
-          cidr_set.emplace_back(std::move(cidr_entry));
-        } else {
-          throw EnvoyException(
-              fmt::format("invalid ip/mask combo '{}/{}' (format is <ip>/<# mask bits>)",
-                          entry.address_prefix(), entry.prefix_len().value()));
-        }
-      }
-      tag_data.emplace_back(ip_tag.ip_tag_name(), cidr_set);
-    }
-    trie_ = std::make_unique<Network::LcTrie::LcTrie<std::string>>(tag_data);
-  }
+                        Runtime::Loader& runtime);
 
   Runtime::Loader& runtime() { return runtime_; }
   Stats::Scope& scope() { return scope_; }
   FilterRequestType requestType() const { return request_type_; }
   const Network::LcTrie::LcTrie<std::string>& trie() const { return *trie_; }
-  const std::string& statsPrefix() const { return stats_prefix_; }
+
+  void incHit(absl::string_view tag) { incCounter(hit_, tag); }
+  void incNoHit() { incCounter(no_hit_); }
+  void incTotal() { incCounter(total_); }
 
 private:
   static FilterRequestType requestTypeEnum(
@@ -88,10 +59,16 @@ class IpTaggingFilterConfig {
     }
   }
 
+  void incCounter(Stats::StatName name1, absl::string_view tag = "");
+
   const FilterRequestType request_type_;
   Stats::Scope& scope_;
   Runtime::Loader& runtime_;
-  const std::string stats_prefix_;
+  Stats::StatNameSet stat_name_set_;
+  const Stats::StatName stats_prefix_;
+  const Stats::StatName hit_;
+  const Stats::StatName no_hit_;
+  const Stats::StatName total_;
   std::unique_ptr<Network::LcTrie::LcTrie<std::string>> trie_;
 };
 

source/extensions/filters/network/mongo_proxy/BUILD

@@ -58,6 +58,7 @@ envoy_cc_library(
     deps = [
         ":codec_interface",
         ":codec_lib",
+        ":mongo_stats_lib",
         ":utility_lib",
         "//include/envoy/access_log:access_log_interface",
         "//include/envoy/common:time_interface",
@@ -81,6 +82,16 @@ envoy_cc_library(
     ],
 )
 
+envoy_cc_library(
+    name = "mongo_stats_lib",
+    srcs = ["mongo_stats.cc"],
+    hdrs = ["mongo_stats.h"],
+    deps = [
+        "//include/envoy/stats:stats_interface",
+        "//source/common/stats:symbol_table_lib",
+    ],
+)
+
 envoy_cc_library(
     name = "utility_lib",
     srcs = ["utility.cc"],

source/extensions/filters/network/mongo_proxy/config.cc

@@ -32,12 +32,13 @@ Network::FilterFactoryCb MongoProxyFilterConfigFactory::createFilterFactoryFromP
     fault_config = std::make_shared<Filters::Common::Fault::FaultDelayConfig>(proto_config.delay());
   }
 
+  auto stats = std::make_shared<MongoStats>(context.scope(), stat_prefix);
   const bool emit_dynamic_metadata = proto_config.emit_dynamic_metadata();
-  return [stat_prefix, &context, access_log, fault_config,
-          emit_dynamic_metadata](Network::FilterManager& filter_manager) -> void {
+  return [stat_prefix, &context, access_log, fault_config, emit_dynamic_metadata,
+          stats](Network::FilterManager& filter_manager) -> void {
     filter_manager.addFilter(std::make_shared<ProdProxyFilter>(
         stat_prefix, context.scope(), context.runtime(), access_log, fault_config,
-        context.drainDecision(), context.dispatcher().timeSource(), emit_dynamic_metadata));
+        context.drainDecision(), context.dispatcher().timeSource(), emit_dynamic_metadata, stats));
   };
 }
 

source/extensions/filters/network/mongo_proxy/mongo_stats.cc

@@ -0,0 +1,47 @@
+#include "extensions/filters/network/mongo_proxy/mongo_stats.h"
+
+#include <memory>
+#include <string>
+#include <vector>
+
+#include "envoy/stats/scope.h"
+
+#include "common/stats/symbol_table_impl.h"
+
+namespace Envoy {
+namespace Extensions {
+namespace NetworkFilters {
+namespace MongoProxy {
+
+MongoStats::MongoStats(Stats::Scope& scope, const std::string& prefix)
+    : scope_(scope), stat_name_set_(scope.symbolTable()), prefix_(stat_name_set_.add(prefix)),
+      callsite_(stat_name_set_.add("callsite")), cmd_(stat_name_set_.add("cmd")),
+      collection_(stat_name_set_.add("collection")), multi_get_(stat_name_set_.add("multi_get")),
+      reply_num_docs_(stat_name_set_.add("reply_num_docs")),
+      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")) {}
+
+Stats::SymbolTable::StoragePtr MongoStats::addPrefix(const std::vector<Stats::StatName>& names) {
+  std::vector<Stats::StatName> 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);
+}
+
+void MongoStats::incCounter(const std::vector<Stats::StatName>& names) {
+  const Stats::SymbolTable::StoragePtr stat_name_storage = addPrefix(names);
+  scope_.counterFromStatName(Stats::StatName(stat_name_storage.get())).inc();
+}
+
+void MongoStats::recordHistogram(const std::vector<Stats::StatName>& names, uint64_t sample) {
+  const Stats::SymbolTable::StoragePtr stat_name_storage = addPrefix(names);
+  scope_.histogramFromStatName(Stats::StatName(stat_name_storage.get())).recordValue(sample);
+}
+
+} // namespace MongoProxy
+} // namespace NetworkFilters
+} // namespace Extensions
+} // namespace Envoy