stats: use counterFromStatName for ratelimit filters

source/extensions/filters/common/ratelimit/BUILD

@@ -39,6 +39,15 @@ envoy_cc_library(
         "//include/envoy/ratelimit:ratelimit_interface",
         "//include/envoy/singleton:manager_interface",
         "//include/envoy/tracing:http_tracer_interface",
+        "//source/common/stats:symbol_table_lib",
         "@envoy_api//envoy/config/ratelimit/v2:rls_cc",
     ],
 )
+
+envoy_cc_library(
+    name = "stat_names_lib",
+    hdrs = ["stat_names.h"],
+    deps = [
+        "//source/common/stats:symbol_table_lib",
+    ],
+)

source/extensions/filters/common/ratelimit/stat_names.h

@@ -0,0 +1,30 @@
+#pragma once
+
+#include "common/stats/symbol_table_impl.h"
+
+namespace Envoy {
+namespace Extensions {
+namespace Filters {
+namespace Common {
+namespace RateLimit {
+
+// Captures a set of stat-names needed for recording during rate-limit
+// filters. These should generally be initialized once per process, and
+// not per-request, to avoid lock contention.
+struct StatNames {
+  explicit StatNames(Stats::SymbolTable& symbol_table)
+      : pool_(symbol_table), ok_(pool_.add("ratelimit.ok")), error_(pool_.add("ratelimit.error")),
+        failure_mode_allowed_(pool_.add("ratelimit.failure_mode_allowed")),
+        over_limit_(pool_.add("ratelimit.over_limit")) {}
+  Stats::StatNamePool pool_;
+  Stats::StatName ok_;
+  Stats::StatName error_;
+  Stats::StatName failure_mode_allowed_;
+  Stats::StatName over_limit_;
+};
+
+} // namespace RateLimit
+} // namespace Common
+} // namespace Filters
+} // namespace Extensions
+} // namespace Envoy

source/extensions/filters/http/ratelimit/BUILD

@@ -24,6 +24,7 @@ envoy_cc_library(
         "//source/common/http:codes_lib",
         "//source/common/router:config_lib",
         "//source/extensions/filters/common/ratelimit:ratelimit_client_interface",
+        "//source/extensions/filters/common/ratelimit:stat_names_lib",
         "@envoy_api//envoy/config/filter/http/rate_limit/v2:rate_limit_cc",
     ],
 )

source/extensions/filters/http/ratelimit/ratelimit.cc

@@ -130,16 +130,17 @@ void Filter::complete(Filters::Common::RateLimit::LimitStatus status,
   state_ = State::Complete;
   headers_to_add_ = std::move(headers);
   Stats::StatName empty_stat_name;
+  Filters::Common::RateLimit::StatNames& stat_names = config_->statNames();
 
   switch (status) {
   case Filters::Common::RateLimit::LimitStatus::OK:
-    cluster_->statsScope().counter("ratelimit.ok").inc();
+    cluster_->statsScope().counterFromStatName(stat_names.ok_).inc();
     break;
   case Filters::Common::RateLimit::LimitStatus::Error:
-    cluster_->statsScope().counter("ratelimit.error").inc();
+    cluster_->statsScope().counterFromStatName(stat_names.error_).inc();
     break;
   case Filters::Common::RateLimit::LimitStatus::OverLimit:
-    cluster_->statsScope().counter("ratelimit.over_limit").inc();
+    cluster_->statsScope().counterFromStatName(stat_names.over_limit_).inc();
     Http::CodeStats::ResponseStatInfo info{config_->scope(),
                                            cluster_->statsScope(),
                                            empty_stat_name,
@@ -165,7 +166,7 @@ void Filter::complete(Filters::Common::RateLimit::LimitStatus status,
     callbacks_->streamInfo().setResponseFlag(StreamInfo::ResponseFlag::RateLimited);
   } else if (status == Filters::Common::RateLimit::LimitStatus::Error) {
     if (config_->failureModeAllow()) {
-      cluster_->statsScope().counter("ratelimit.failure_mode_allowed").inc();
+      cluster_->statsScope().counterFromStatName(stat_names.failure_mode_allowed_).inc();
       if (!initiating_call_) {
         callbacks_->continueDecoding();
       }

source/extensions/filters/http/ratelimit/ratelimit.h

@@ -18,6 +18,7 @@
 #include "common/http/header_map_impl.h"
 
 #include "extensions/filters/common/ratelimit/ratelimit.h"
+#include "extensions/filters/common/ratelimit/stat_names.h"
 
 namespace Envoy {
 namespace Extensions {
@@ -46,7 +47,7 @@ class FilterConfig {
             config.rate_limited_as_resource_exhausted()
                 ? absl::make_optional(Grpc::Status::GrpcStatus::ResourceExhausted)
                 : absl::nullopt),
-        http_context_(http_context) {}
+        http_context_(http_context), stat_names_(scope.symbolTable()) {}
   const std::string& domain() const { return domain_; }
   const LocalInfo::LocalInfo& localInfo() const { return local_info_; }
   uint64_t stage() const { return stage_; }
@@ -58,6 +59,7 @@ class FilterConfig {
     return rate_limited_grpc_status_;
   }
   Http::Context& httpContext() { return http_context_; }
+  Filters::Common::RateLimit::StatNames& statNames() { return stat_names_; }
 
 private:
   static FilterRequestType stringToType(const std::string& request_type) {
@@ -80,6 +82,7 @@ class FilterConfig {
   const bool failure_mode_deny_;
   const absl::optional<Grpc::Status::GrpcStatus> rate_limited_grpc_status_;
   Http::Context& http_context_;
+  Filters::Common::RateLimit::StatNames stat_names_;
 };
 
 using FilterConfigSharedPtr = std::shared_ptr<FilterConfig>;

source/extensions/filters/network/thrift_proxy/filters/ratelimit/BUILD

@@ -18,6 +18,7 @@ envoy_cc_library(
         "//source/common/tracing:http_tracer_lib",
         "//source/extensions/filters/common/ratelimit:ratelimit_client_interface",
         "//source/extensions/filters/common/ratelimit:ratelimit_lib",
+        "//source/extensions/filters/common/ratelimit:stat_names_lib",
         "//source/extensions/filters/network/thrift_proxy:app_exception_lib",
         "//source/extensions/filters/network/thrift_proxy/filters:filter_interface",
         "//source/extensions/filters/network/thrift_proxy/router:router_ratelimit_interface",

source/extensions/filters/network/thrift_proxy/filters/ratelimit/ratelimit.cc

@@ -65,13 +65,14 @@ void Filter::complete(Filters::Common::RateLimit::LimitStatus status,
   UNREFERENCED_PARAMETER(headers);
 
   state_ = State::Complete;
+  Filters::Common::RateLimit::StatNames& stat_names = config_->statNames();
 
   switch (status) {
   case Filters::Common::RateLimit::LimitStatus::OK:
-    cluster_->statsScope().counter("ratelimit.ok").inc();
+    cluster_->statsScope().counterFromStatName(stat_names.ok_).inc();
     break;
   case Filters::Common::RateLimit::LimitStatus::Error:
-    cluster_->statsScope().counter("ratelimit.error").inc();
+    cluster_->statsScope().counterFromStatName(stat_names.error_).inc();
     if (!config_->failureModeAllow()) {
       state_ = State::Responded;
       callbacks_->sendLocalReply(
@@ -80,10 +81,10 @@ void Filter::complete(Filters::Common::RateLimit::LimitStatus status,
       callbacks_->streamInfo().setResponseFlag(StreamInfo::ResponseFlag::RateLimitServiceError);
       return;
     }
-    cluster_->statsScope().counter("ratelimit.failure_mode_allowed").inc();
+    cluster_->statsScope().counterFromStatName(stat_names.failure_mode_allowed_).inc();
     break;
   case Filters::Common::RateLimit::LimitStatus::OverLimit:
-    cluster_->statsScope().counter("ratelimit.over_limit").inc();
+    cluster_->statsScope().counterFromStatName(stat_names.over_limit_).inc();
     if (config_->runtime().snapshot().featureEnabled("ratelimit.thrift_filter_enforcing", 100)) {
       state_ = State::Responded;
       callbacks_->sendLocalReply(

source/extensions/filters/network/thrift_proxy/filters/ratelimit/ratelimit.h

@@ -9,7 +9,10 @@
 #include "envoy/stats/scope.h"
 #include "envoy/stats/stats_macros.h"
 
+#include "common/stats/symbol_table_impl.h"
+
 #include "extensions/filters/common/ratelimit/ratelimit.h"
+#include "extensions/filters/common/ratelimit/stat_names.h"
 #include "extensions/filters/network/thrift_proxy/filters/filter.h"
 
 namespace Envoy {
@@ -28,16 +31,17 @@ class Config {
          const LocalInfo::LocalInfo& local_info, Stats::Scope& scope, Runtime::Loader& runtime,
          Upstream::ClusterManager& cm)
       : domain_(config.domain()), stage_(config.stage()), local_info_(local_info), scope_(scope),
-        runtime_(runtime), cm_(cm), failure_mode_deny_(config.failure_mode_deny()) {}
+        runtime_(runtime), cm_(cm), failure_mode_deny_(config.failure_mode_deny()),
+        stat_names_(scope_.symbolTable()) {}
 
   const std::string& domain() const { return domain_; }
   const LocalInfo::LocalInfo& localInfo() const { return local_info_; }
   uint32_t stage() const { return stage_; }
   Stats::Scope& scope() { return scope_; }
   Runtime::Loader& runtime() { return runtime_; }
   Upstream::ClusterManager& cm() { return cm_; }
-
   bool failureModeAllow() const { return !failure_mode_deny_; };
+  Filters::Common::RateLimit::StatNames& statNames() { return stat_names_; }
 
 private:
   const std::string domain_;
@@ -47,6 +51,7 @@ class Config {
   Runtime::Loader& runtime_;
   Upstream::ClusterManager& cm_;
   const bool failure_mode_deny_;
+  Filters::Common::RateLimit::StatNames stat_names_;
 };
 
 using ConfigSharedPtr = std::shared_ptr<Config>;

test/extensions/filters/http/ratelimit/ratelimit_test.cc

@@ -76,16 +76,16 @@ class HttpRateLimitFilterTest : public testing::Test {
   domain: foo
   )EOF";
 
-  FilterConfigSharedPtr config_;
   Filters::Common::RateLimit::MockClient* client_;
-  std::unique_ptr<Filter> filter_;
   NiceMock<Http::MockStreamDecoderFilterCallbacks> filter_callbacks_;
   Filters::Common::RateLimit::RequestCallbacks* request_callbacks_{};
   Http::TestHeaderMapImpl request_headers_;
   Http::TestHeaderMapImpl response_headers_;
   Buffer::OwnedImpl data_;
   Buffer::OwnedImpl response_data_;
   NiceMock<Stats::MockIsolatedStatsStore> stats_store_;
+  FilterConfigSharedPtr config_;
+  std::unique_ptr<Filter> filter_;
   NiceMock<Runtime::MockLoader> runtime_;
   NiceMock<Router::MockRateLimitPolicyEntry> route_rate_limit_;
   NiceMock<Router::MockRateLimitPolicyEntry> vh_rate_limit_;

test/extensions/filters/network/thrift_proxy/filters/ratelimit/ratelimit_test.cc

@@ -75,6 +75,7 @@ class ThriftRateLimitFilterTest : public testing::Test {
   domain: foo
   )EOF";
 
+  Stats::IsolatedStoreImpl stats_store_;
   ConfigSharedPtr config_;
   Filters::Common::RateLimit::MockClient* client_;
   std::unique_ptr<Filter> filter_;
@@ -84,7 +85,6 @@ class ThriftRateLimitFilterTest : public testing::Test {
   Http::TestHeaderMapImpl response_headers_;
   Buffer::OwnedImpl data_;
   Buffer::OwnedImpl response_data_;
-  Stats::IsolatedStoreImpl stats_store_;
   NiceMock<Runtime::MockLoader> runtime_;
   NiceMock<Upstream::MockClusterManager> cm_;
   NiceMock<ThriftProxy::Router::MockRateLimitPolicyEntry> route_rate_limit_;

tools/check_format.py

@@ -42,6 +42,24 @@
                        "./test/test_common/utility.cc", "./test/test_common/utility.h",
                        "./test/integration/integration.h")
 
+# Files matching these directories can use stats by string for now. These should
+# be eliminated but for now we don't want to grow this work. The goal for this
+# whitelist is to eliminate it by making code transformations similar to
+# https://github.com/envoyproxy/envoy/pull/7573 and others.
+#
+# TODO(#4196): Eliminate this list completely and then merge #4980.
+STAT_FROM_STRING_WHITELIST = ("./source/common/memory/heap_shrinker.cc",
+                              "./source/extensions/filters/http/dynamo/dynamo_filter.cc",
+                              "./source/extensions/filters/http/ext_authz/ext_authz.cc",
+                              "./source/extensions/filters/http/fault/fault_filter.cc",
+                              "./source/extensions/filters/http/ip_tagging/ip_tagging_filter.cc",
+                              "./source/extensions/filters/network/mongo_proxy/proxy.cc",
+                              "./source/extensions/filters/network/zookeeper_proxy/filter.cc",
+                              "./source/extensions/stat_sinks/common/statsd/statsd.cc",
+                              "./source/extensions/transport_sockets/tls/context_impl.cc",
+                              "./source/server/guarddog_impl.cc",
+                              "./source/server/overload_manager_impl.cc")
+
 # Files in these paths can use MessageLite::SerializeAsString
 SERIALIZE_AS_STRING_WHITELIST = ("./test/common/protobuf/utility_test.cc",
                                  "./test/common/grpc/codec_test.cc")
@@ -316,6 +334,10 @@ def whitelistedForJsonStringToMessage(file_path):
   return file_path in JSON_STRING_TO_MESSAGE_WHITELIST
 
 
+def whitelistedForStatFromString(file_path):
+  return file_path in STAT_FROM_STRING_WHITELIST
+
+
 def findSubstringAndReturnError(pattern, file_path, error_message):
   with open(file_path) as f:
     text = f.read()
@@ -456,11 +478,24 @@ def hasCondVarWaitFor(line):
   return True
 
 
+# Determines whether the filename is either in the specified subdirectory, or
+# at the top level. We consider files in the top level for the benefit of
+# the check_format testcases in tools/testdata/check_format.
+def isInSubdir(filename, *subdirs):
+  # Skip this check for check_format's unit-tests.
+  if filename.count("/") <= 1:
+    return True
+  for subdir in subdirs:
+    if filename.startswith('./' + subdir + '/'):
+      return True
+  return False
+
+
 def checkSourceLine(line, file_path, reportError):
   # Check fixable errors. These may have been fixed already.
   if line.find(".  ") != -1:
     reportError("over-enthusiastic spaces")
-  if ('source' in file_path or 'include' in file_path) and X_ENVOY_USED_DIRECTLY_REGEX.match(line):
+  if isInSubdir(file_path, 'source', 'include') and X_ENVOY_USED_DIRECTLY_REGEX.match(line):
     reportError(
         "Please do not use the raw literal x-envoy in source code.  See Envoy::Http::PrefixValue.")
   if hasInvalidAngleBracketDirectory(line):
@@ -546,6 +581,11 @@ def checkSourceLine(line, file_path, reportError):
     # behavior.
     reportError("Don't use Protobuf::util::JsonStringToMessage, use TestUtility::loadFromJson.")
 
+  if isInSubdir(file_path, 'source') and file_path.endswith('.cc') and \
+     not whitelistedForStatFromString(file_path) and \
+     ('.counter(' in line or '.gauge(' in line or '.histogram(' in line):
+    reportError("Don't lookup stats by name at runtime; used StatName saved during construction")
+
 
 def checkBuildLine(line, file_path, reportError):
   if "@bazel_tools" in line and not (isSkylarkFile(file_path) or file_path.startswith("./bazel/")):

tools/check_format_test_helper.py

@@ -102,26 +102,26 @@ def emitStdoutAsError(stdout):
   logging.error("\n".join(stdout))
 
 
-def expectError(status, stdout, expected_substring):
+def expectError(filename, status, stdout, expected_substring):
   if status == 0:
-    logging.error("Expected failure `%s`, but succeeded" % expected_substring)
+    logging.error("%s: Expected failure `%s`, but succeeded" % (filename, expected_substring))
     return 1
   for line in stdout:
     if expected_substring in line:
       return 0
-  logging.error("Could not find '%s' in:\n" % expected_substring)
+  logging.error("%s: Could not find '%s' in:\n" % (filename, expected_substring))
   emitStdoutAsError(stdout)
   return 1
 
 
 def fixFileExpectingFailure(filename, expected_substring):
   command, infile, outfile, status, stdout = fixFileHelper(filename)
-  return expectError(status, stdout, expected_substring)
+  return expectError(filename, status, stdout, expected_substring)
 
 
 def checkFileExpectingError(filename, expected_substring):
   command, status, stdout = runCheckFormat("check", getInputFile(filename))
-  return expectError(status, stdout, expected_substring)
+  return expectError(filename, status, stdout, expected_substring)
 
 
 def checkAndFixError(filename, expected_substring):
@@ -208,6 +208,15 @@ def checkFileExpectingOK(filename):
       "version_history.rst",
       "Version history line malformed. Does not match VERSION_HISTORY_NEW_LINE_REGEX in "
       "check_format.py")
+  errors += checkUnfixableError(
+      "counter_from_string.cc",
+      "Don't lookup stats by name at runtime; used StatName saved during construction")
+  errors += checkUnfixableError(
+      "gauge_from_string.cc",
+      "Don't lookup stats by name at runtime; used StatName saved during construction")
+  errors += checkUnfixableError(
+      "histogram_from_string.cc",
+      "Don't lookup stats by name at runtime; used StatName saved during construction")
 
   errors += fixFileExpectingFailure(
       "api/missing_package.proto",

tools/format_python_tools.sh

@@ -9,8 +9,8 @@ cd "$SCRIPTPATH"
 
 source_venv "$VENV_DIR"
 echo "Installing requirements..."
-pip3 install --upgrade pip
-pip3 install -r requirements.txt
+pip3 -q install --upgrade pip
+pip3 -q install -r requirements.txt
 
 echo "Running Python format check..."
 python3 format_python_tools.py $1

tools/testdata/check_format/counter_from_string.cc

@@ -0,0 +1,7 @@
+namespace Envoy {
+
+void init(Stats::Scope& scope) {
+  scope.counter("hello");
+}
+
+} // namespace Envoy

tools/testdata/check_format/gauge_from_string.cc

@@ -0,0 +1,7 @@
+namespace Envoy {
+
+void init(Stats::Scope& scope) {
+  scope.gauge("hello");
+}
+
+} // namespace Envoy

tools/testdata/check_format/histogram_from_string.cc

@@ -0,0 +1,7 @@
+namespace Envoy {
+
+void init(Stats::Scope& scope) {
+  scope.histogram("hello");
+}
+
+} // namespace Envoy