diff --git a/source/extensions/filters/common/ratelimit/BUILD b/source/extensions/filters/common/ratelimit/BUILD index 6b0c197a810d8..9d39e1980093e 100644 --- a/source/extensions/filters/common/ratelimit/BUILD +++ b/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", + ], +) diff --git a/source/extensions/filters/common/ratelimit/stat_names.h b/source/extensions/filters/common/ratelimit/stat_names.h new file mode 100644 index 0000000000000..33dd13a188b34 --- /dev/null +++ b/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 diff --git a/source/extensions/filters/http/ratelimit/BUILD b/source/extensions/filters/http/ratelimit/BUILD index 45e8abef7c789..22b8e4bba0b6c 100644 --- a/source/extensions/filters/http/ratelimit/BUILD +++ b/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", ], ) diff --git a/source/extensions/filters/http/ratelimit/ratelimit.cc b/source/extensions/filters/http/ratelimit/ratelimit.cc index 2a5761c6b6122..6bd65be54811d 100644 --- a/source/extensions/filters/http/ratelimit/ratelimit.cc +++ b/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(); } diff --git a/source/extensions/filters/http/ratelimit/ratelimit.h b/source/extensions/filters/http/ratelimit/ratelimit.h index 1103f9e9763b3..dfda9883ee2d3 100644 --- a/source/extensions/filters/http/ratelimit/ratelimit.h +++ b/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 rate_limited_grpc_status_; Http::Context& http_context_; + Filters::Common::RateLimit::StatNames stat_names_; }; using FilterConfigSharedPtr = std::shared_ptr; diff --git a/source/extensions/filters/network/thrift_proxy/filters/ratelimit/BUILD b/source/extensions/filters/network/thrift_proxy/filters/ratelimit/BUILD index 8796cbcc7400d..1d6128fda8fba 100644 --- a/source/extensions/filters/network/thrift_proxy/filters/ratelimit/BUILD +++ b/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", diff --git a/source/extensions/filters/network/thrift_proxy/filters/ratelimit/ratelimit.cc b/source/extensions/filters/network/thrift_proxy/filters/ratelimit/ratelimit.cc index 6131bb77b8b9e..d4e5dc70f5d07 100644 --- a/source/extensions/filters/network/thrift_proxy/filters/ratelimit/ratelimit.cc +++ b/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( diff --git a/source/extensions/filters/network/thrift_proxy/filters/ratelimit/ratelimit.h b/source/extensions/filters/network/thrift_proxy/filters/ratelimit/ratelimit.h index eb9e60dbb5f0d..2b68090bc8c97 100644 --- a/source/extensions/filters/network/thrift_proxy/filters/ratelimit/ratelimit.h +++ b/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,7 +31,8 @@ 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_; } @@ -36,8 +40,8 @@ class Config { 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; diff --git a/test/extensions/filters/http/ratelimit/ratelimit_test.cc b/test/extensions/filters/http/ratelimit/ratelimit_test.cc index c956709a2a547..4e030d64b8e7a 100644 --- a/test/extensions/filters/http/ratelimit/ratelimit_test.cc +++ b/test/extensions/filters/http/ratelimit/ratelimit_test.cc @@ -76,9 +76,7 @@ class HttpRateLimitFilterTest : public testing::Test { domain: foo )EOF"; - FilterConfigSharedPtr config_; Filters::Common::RateLimit::MockClient* client_; - std::unique_ptr filter_; NiceMock filter_callbacks_; Filters::Common::RateLimit::RequestCallbacks* request_callbacks_{}; Http::TestHeaderMapImpl request_headers_; @@ -86,6 +84,8 @@ class HttpRateLimitFilterTest : public testing::Test { Buffer::OwnedImpl data_; Buffer::OwnedImpl response_data_; NiceMock stats_store_; + FilterConfigSharedPtr config_; + std::unique_ptr filter_; NiceMock runtime_; NiceMock route_rate_limit_; NiceMock vh_rate_limit_; diff --git a/test/extensions/filters/network/thrift_proxy/filters/ratelimit/ratelimit_test.cc b/test/extensions/filters/network/thrift_proxy/filters/ratelimit/ratelimit_test.cc index f8f3d4dd812b9..01877c365c4e2 100644 --- a/test/extensions/filters/network/thrift_proxy/filters/ratelimit/ratelimit_test.cc +++ b/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_; @@ -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_; NiceMock cm_; NiceMock route_rate_limit_; diff --git a/tools/check_format.py b/tools/check_format.py index 4e9db37bf5059..8a9bece5c4c01 100755 --- a/tools/check_format.py +++ b/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/")): diff --git a/tools/check_format_test_helper.py b/tools/check_format_test_helper.py index 646512239866c..2641112ea7224 100755 --- a/tools/check_format_test_helper.py +++ b/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", diff --git a/tools/format_python_tools.sh b/tools/format_python_tools.sh index 91b34feb21227..6749ade470712 100755 --- a/tools/format_python_tools.sh +++ b/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 diff --git a/tools/testdata/check_format/counter_from_string.cc b/tools/testdata/check_format/counter_from_string.cc new file mode 100644 index 0000000000000..8c89250fefe97 --- /dev/null +++ b/tools/testdata/check_format/counter_from_string.cc @@ -0,0 +1,7 @@ +namespace Envoy { + +void init(Stats::Scope& scope) { + scope.counter("hello"); +} + +} // namespace Envoy diff --git a/tools/testdata/check_format/gauge_from_string.cc b/tools/testdata/check_format/gauge_from_string.cc new file mode 100644 index 0000000000000..06dbd01d2ea37 --- /dev/null +++ b/tools/testdata/check_format/gauge_from_string.cc @@ -0,0 +1,7 @@ +namespace Envoy { + +void init(Stats::Scope& scope) { + scope.gauge("hello"); +} + +} // namespace Envoy diff --git a/tools/testdata/check_format/histogram_from_string.cc b/tools/testdata/check_format/histogram_from_string.cc new file mode 100644 index 0000000000000..3c16b433a1a9e --- /dev/null +++ b/tools/testdata/check_format/histogram_from_string.cc @@ -0,0 +1,7 @@ +namespace Envoy { + +void init(Stats::Scope& scope) { + scope.histogram("hello"); +} + +} // namespace Envoy