From 5e45d4874db85be56259df2daa8c69af78987fcf Mon Sep 17 00:00:00 2001 From: Joshua Marantz Date: Thu, 29 Aug 2019 07:26:01 +0900 Subject: [PATCH] stats: Clean up all calls to Scope::counter() et al in production code. (#7842) * Convert a few more counter() references to use the StatName interface. Signed-off-by: Joshua Marantz --- source/common/stats/symbol_table_impl.h | 11 ++- .../filters/http/fault/fault_filter.cc | 22 +++-- .../filters/http/fault/fault_filter.h | 16 +++- .../extensions/filters/http/ip_tagging/BUILD | 1 + .../http/ip_tagging/ip_tagging_filter.cc | 52 +++++++++- .../http/ip_tagging/ip_tagging_filter.h | 49 +++------- .../filters/network/mongo_proxy/BUILD | 11 +++ .../filters/network/mongo_proxy/config.cc | 7 +- .../network/mongo_proxy/mongo_stats.cc | 47 +++++++++ .../filters/network/mongo_proxy/mongo_stats.h | 56 +++++++++++ .../filters/network/mongo_proxy/proxy.cc | 96 ++++++++++++------- .../filters/network/mongo_proxy/proxy.h | 17 +++- .../stat_sinks/common/statsd/statsd.cc | 6 +- source/extensions/transport_sockets/tls/BUILD | 1 + .../transport_sockets/tls/context_impl.cc | 60 ++++++++++-- .../transport_sockets/tls/context_impl.h | 8 ++ source/server/BUILD | 2 + source/server/guarddog_impl.cc | 9 +- source/server/overload_manager_impl.cc | 24 +++-- .../filters/http/fault/fault_filter_test.cc | 2 +- .../http/ip_tagging/ip_tagging_filter_test.cc | 2 +- .../filters/network/mongo_proxy/proxy_test.cc | 10 +- .../transport_sockets/tls/ssl_socket_test.cc | 3 +- tools/check_format.py | 19 ---- tools/spelling_dictionary.txt | 2 + 25 files changed, 390 insertions(+), 143 deletions(-) create mode 100644 source/extensions/filters/network/mongo_proxy/mongo_stats.cc create mode 100644 source/extensions/filters/network/mongo_proxy/mongo_stats.h diff --git a/source/common/stats/symbol_table_impl.h b/source/common/stats/symbol_table_impl.h index 925ff26368a4..dbbb633dcbe8 100644 --- a/source/common/stats/symbol_table_impl.h +++ b/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 diff --git a/source/extensions/filters/http/fault/fault_filter.cc b/source/extensions/filters/http/fault/fault_filter.cc index 0649366a7e5c..0fb09f1c95f1 100644 --- a/source/extensions/filters/http/fault/fault_filter.cc +++ b/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. diff --git a/source/extensions/filters/http/fault/fault_filter.h b/source/extensions/filters/http/fault/fault_filter.h index be6ef435420b..b0d3a0f1bf30 100644 --- a/source/extensions/filters/http/fault/fault_filter.h +++ b/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; diff --git a/source/extensions/filters/http/ip_tagging/BUILD b/source/extensions/filters/http/ip_tagging/BUILD index 583893eadb23..bc88c1313356 100644 --- a/source/extensions/filters/http/ip_tagging/BUILD +++ b/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", ], ) diff --git a/source/extensions/filters/http/ip_tagging/ip_tagging_filter.cc b/source/extensions/filters/http/ip_tagging/ip_tagging_filter.cc index 1d82239c7dfd..d3b2549a87f7 100644 --- a/source/extensions/filters/http/ip_tagging/ip_tagging_filter.cc +++ b/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>> tag_data; + tag_data.reserve(config.ip_tags().size()); + for (const auto& ip_tag : config.ip_tags()) { + std::vector 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 /<# mask bits>)", + entry.address_prefix(), entry.prefix_len().value())); + } + } + tag_data.emplace_back(ip_tag.ip_tag_name(), cidr_set); + } + trie_ = std::make_unique>(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; } diff --git a/source/extensions/filters/http/ip_tagging/ip_tagging_filter.h b/source/extensions/filters/http/ip_tagging/ip_tagging_filter.h index b79103aab9bd..6cf2b19b0e74 100644 --- a/source/extensions/filters/http/ip_tagging/ip_tagging_filter.h +++ b/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>> tag_data; - tag_data.reserve(config.ip_tags().size()); - for (const auto& ip_tag : config.ip_tags()) { - std::vector 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 /<# mask bits>)", - entry.address_prefix(), entry.prefix_len().value())); - } - } - tag_data.emplace_back(ip_tag.ip_tag_name(), cidr_set); - } - trie_ = std::make_unique>(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& 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> trie_; }; diff --git a/source/extensions/filters/network/mongo_proxy/BUILD b/source/extensions/filters/network/mongo_proxy/BUILD index 36a94de85abc..f2be1d6a9774 100644 --- a/source/extensions/filters/network/mongo_proxy/BUILD +++ b/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"], diff --git a/source/extensions/filters/network/mongo_proxy/config.cc b/source/extensions/filters/network/mongo_proxy/config.cc index a8989947e75a..24539b6bd953 100644 --- a/source/extensions/filters/network/mongo_proxy/config.cc +++ b/source/extensions/filters/network/mongo_proxy/config.cc @@ -32,12 +32,13 @@ Network::FilterFactoryCb MongoProxyFilterConfigFactory::createFilterFactoryFromP fault_config = std::make_shared(proto_config.delay()); } + auto stats = std::make_shared(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( 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)); }; } diff --git a/source/extensions/filters/network/mongo_proxy/mongo_stats.cc b/source/extensions/filters/network/mongo_proxy/mongo_stats.cc new file mode 100644 index 000000000000..11dd1877cefc --- /dev/null +++ b/source/extensions/filters/network/mongo_proxy/mongo_stats.cc @@ -0,0 +1,47 @@ +#include "extensions/filters/network/mongo_proxy/mongo_stats.h" + +#include +#include +#include + +#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& names) { + std::vector 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& 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& 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 diff --git a/source/extensions/filters/network/mongo_proxy/mongo_stats.h b/source/extensions/filters/network/mongo_proxy/mongo_stats.h new file mode 100644 index 000000000000..d27a8478824a --- /dev/null +++ b/source/extensions/filters/network/mongo_proxy/mongo_stats.h @@ -0,0 +1,56 @@ +#pragma once + +#include +#include +#include + +#include "envoy/stats/scope.h" + +#include "common/stats/symbol_table_impl.h" + +namespace Envoy { +namespace Extensions { +namespace NetworkFilters { +namespace MongoProxy { + +class MongoStats { +public: + MongoStats(Stats::Scope& scope, const std::string& prefix); + + void incCounter(const std::vector& names); + void recordHistogram(const std::vector& names, uint64_t sample); + + /** + * 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& names); + + Stats::Scope& scope_; + Stats::StatNameSet stat_name_set_; + +public: + const Stats::StatName prefix_; + const Stats::StatName callsite_; + const Stats::StatName cmd_; + const Stats::StatName collection_; + const Stats::StatName multi_get_; + const Stats::StatName reply_num_docs_; + const Stats::StatName reply_size_; + const Stats::StatName reply_time_ms_; + const Stats::StatName time_ms_; + const Stats::StatName query_; + const Stats::StatName scatter_get_; + const Stats::StatName total_; +}; +using MongoStatsSharedPtr = std::shared_ptr; + +} // namespace MongoProxy +} // namespace NetworkFilters +} // namespace Extensions +} // namespace Envoy diff --git a/source/extensions/filters/network/mongo_proxy/proxy.cc b/source/extensions/filters/network/mongo_proxy/proxy.cc index be08a04fc638..bc35859113f8 100644 --- a/source/extensions/filters/network/mongo_proxy/proxy.cc +++ b/source/extensions/filters/network/mongo_proxy/proxy.cc @@ -58,11 +58,11 @@ ProxyFilter::ProxyFilter(const std::string& stat_prefix, Stats::Scope& scope, Runtime::Loader& runtime, AccessLogSharedPtr access_log, const Filters::Common::Fault::FaultDelayConfigSharedPtr& fault_config, const Network::DrainDecision& drain_decision, TimeSource& time_source, - bool emit_dynamic_metadata) - : stat_prefix_(stat_prefix), scope_(scope), stats_(generateStats(stat_prefix, scope)), - runtime_(runtime), drain_decision_(drain_decision), access_log_(access_log), - fault_config_(fault_config), time_source_(time_source), - emit_dynamic_metadata_(emit_dynamic_metadata) { + bool emit_dynamic_metadata, const MongoStatsSharedPtr& mongo_stats) + : stat_prefix_(stat_prefix), stats_(generateStats(stat_prefix, scope)), runtime_(runtime), + drain_decision_(drain_decision), access_log_(access_log), fault_config_(fault_config), + time_source_(time_source), emit_dynamic_metadata_(emit_dynamic_metadata), + mongo_stats_(mongo_stats) { if (!runtime_.snapshot().featureEnabled(MongoRuntimeConfig::get().ConnectionLoggingEnabled, 100)) { // If we are not logging at the connection level, just release the shared pointer so that we @@ -145,21 +145,23 @@ void ProxyFilter::decodeQuery(QueryMessagePtr&& message) { ActiveQueryPtr active_query(new ActiveQuery(*this, *message)); if (!active_query->query_info_.command().empty()) { // First field key is the operation. - scope_.counter(fmt::format("{}cmd.{}.total", stat_prefix_, active_query->query_info_.command())) - .inc(); + mongo_stats_->incCounter({mongo_stats_->cmd_, + mongo_stats_->getStatName(active_query->query_info_.command()), + mongo_stats_->total_}); } else { // Normal query, get stats on a per collection basis first. - std::string collection_stat_prefix = - fmt::format("{}collection.{}", stat_prefix_, active_query->query_info_.collection()); QueryMessageInfo::QueryType query_type = active_query->query_info_.type(); - chargeQueryStats(collection_stat_prefix, query_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())); + chargeQueryStats(names, query_type); // Callsite stats if we have it. if (!active_query->query_info_.callsite().empty()) { - std::string callsite_stat_prefix = - fmt::format("{}collection.{}.callsite.{}", stat_prefix_, - active_query->query_info_.collection(), active_query->query_info_.callsite()); - chargeQueryStats(callsite_stat_prefix, query_type); + names.push_back(mongo_stats_->callsite_); + names.push_back(mongo_stats_->getStatName(active_query->query_info_.callsite())); + chargeQueryStats(names, query_type); } // Global stats. @@ -176,14 +178,26 @@ void ProxyFilter::decodeQuery(QueryMessagePtr&& message) { active_query_list_.emplace_back(std::move(active_query)); } -void ProxyFilter::chargeQueryStats(const std::string& prefix, +void ProxyFilter::chargeQueryStats(Stats::StatNameVec& names, QueryMessageInfo::QueryType query_type) { - scope_.counter(fmt::format("{}.query.total", prefix)).inc(); + // names come in containing {"collection", collection}. Report stats for 1 or + // 2 variations on this array, and then return with the array in the same + // state it had on entry. Both of these variations by appending {"query", "total"}. + size_t orig_size = names.size(); + ASSERT(names.capacity() - orig_size >= 2); // Ensures the caller has reserved() enough memory. + names.push_back(mongo_stats_->query_); + names.push_back(mongo_stats_->total_); + mongo_stats_->incCounter(names); + + // And now replace "total" with either "scatter_get" or "multi_get" if depending on query_type. if (query_type == QueryMessageInfo::QueryType::ScatterGet) { - scope_.counter(fmt::format("{}.query.scatter_get", prefix)).inc(); + names.back() = mongo_stats_->scatter_get_; + mongo_stats_->incCounter(names); } else if (query_type == QueryMessageInfo::QueryType::MultiGet) { - scope_.counter(fmt::format("{}.query.multi_get", prefix)).inc(); + names.back() = mongo_stats_->multi_get_; + mongo_stats_->incCounter(names); } + names.resize(orig_size); } void ProxyFilter::decodeReply(ReplyMessagePtr&& message) { @@ -208,21 +222,25 @@ void ProxyFilter::decodeReply(ReplyMessagePtr&& message) { } if (!active_query.query_info_.command().empty()) { - std::string stat_prefix = - fmt::format("{}cmd.{}", stat_prefix_, active_query.query_info_.command()); - chargeReplyStats(active_query, stat_prefix, *message); + Stats::StatNameVec names{mongo_stats_->cmd_, + mongo_stats_->getStatName(active_query.query_info_.command())}; + chargeReplyStats(active_query, names, *message); } else { // Collection stats first. - std::string stat_prefix = - fmt::format("{}collection.{}.query", stat_prefix_, active_query.query_info_.collection()); - chargeReplyStats(active_query, stat_prefix, *message); + Stats::StatNameVec names{mongo_stats_->collection_, + mongo_stats_->getStatName(active_query.query_info_.collection()), + mongo_stats_->query_}; + chargeReplyStats(active_query, names, *message); // Callsite stats if we have it. if (!active_query.query_info_.callsite().empty()) { - std::string callsite_stat_prefix = - fmt::format("{}collection.{}.callsite.{}.query", stat_prefix_, - active_query.query_info_.collection(), active_query.query_info_.callsite()); - chargeReplyStats(active_query, callsite_stat_prefix, *message); + // Currently, names == {"collection", collection, "query"} and we are going + // 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_->query_); + chargeReplyStats(active_query, names, *message); } } @@ -270,20 +288,26 @@ void ProxyFilter::onDrainClose() { read_callbacks_->connection().close(Network::ConnectionCloseType::FlushWrite); } -void ProxyFilter::chargeReplyStats(ActiveQuery& active_query, const std::string& prefix, +void ProxyFilter::chargeReplyStats(ActiveQuery& active_query, Stats::StatNameVec& names, const ReplyMessage& message) { uint64_t reply_documents_byte_size = 0; for (const Bson::DocumentSharedPtr& document : message.documents()) { reply_documents_byte_size += document->byteSize(); } - scope_.histogram(fmt::format("{}.reply_num_docs", prefix)) - .recordValue(message.documents().size()); - scope_.histogram(fmt::format("{}.reply_size", prefix)).recordValue(reply_documents_byte_size); - scope_.histogram(fmt::format("{}.reply_time_ms", prefix)) - .recordValue(std::chrono::duration_cast( - time_source_.monotonicTime() - active_query.start_time_) - .count()); + // Write 3 different histograms; appending 3 different suffixes to the name + // that was passed in. Here we overwrite the passed-in names, but we restore + // names to its original state upon return. + const size_t orig_size = names.size(); + names.push_back(mongo_stats_->reply_num_docs_); + mongo_stats_->recordHistogram(names, message.documents().size()); + names[orig_size] = mongo_stats_->reply_size_; + mongo_stats_->recordHistogram(names, reply_documents_byte_size); + names[orig_size] = mongo_stats_->reply_time_ms_; + mongo_stats_->recordHistogram(names, std::chrono::duration_cast( + time_source_.monotonicTime() - active_query.start_time_) + .count()); + names.resize(orig_size); } void ProxyFilter::doDecode(Buffer::Instance& buffer) { diff --git a/source/extensions/filters/network/mongo_proxy/proxy.h b/source/extensions/filters/network/mongo_proxy/proxy.h index f81ef86017ef..da85af19f281 100644 --- a/source/extensions/filters/network/mongo_proxy/proxy.h +++ b/source/extensions/filters/network/mongo_proxy/proxy.h @@ -25,6 +25,7 @@ #include "extensions/filters/common/fault/fault_config.h" #include "extensions/filters/network/mongo_proxy/codec.h" +#include "extensions/filters/network/mongo_proxy/mongo_stats.h" #include "extensions/filters/network/mongo_proxy/utility.h" namespace Envoy { @@ -110,7 +111,7 @@ class ProxyFilter : public Network::Filter, AccessLogSharedPtr access_log, const Filters::Common::Fault::FaultDelayConfigSharedPtr& fault_config, const Network::DrainDecision& drain_decision, TimeSource& time_system, - bool emit_dynamic_metadata); + bool emit_dynamic_metadata, const MongoStatsSharedPtr& stats); ~ProxyFilter() override; virtual DecoderPtr createDecoder(DecoderCallbacks& callbacks) PURE; @@ -164,9 +165,17 @@ class ProxyFilter : public Network::Filter, POOL_HISTOGRAM_PREFIX(scope, prefix))}; } - void chargeQueryStats(const std::string& prefix, QueryMessageInfo::QueryType query_type); - void chargeReplyStats(ActiveQuery& active_query, const std::string& prefix, + // Increment counters related to queries. 'names' is passed by non-const + // reference so the implementation can mutate it without copying, though it + // always restores it to its prior state prior to return. + void chargeQueryStats(Stats::StatNameVec& names, QueryMessageInfo::QueryType query_type); + + // Add samples to histograms related to replies. 'names' is passed by + // non-const reference so the implementation can mutate it without copying, + // though it always restores it to its prior state prior to return. + void chargeReplyStats(ActiveQuery& active_query, Stats::StatNameVec& names, const ReplyMessage& message); + void doDecode(Buffer::Instance& buffer); void logMessage(Message& message, bool full); void onDrainClose(); @@ -176,7 +185,6 @@ class ProxyFilter : public Network::Filter, std::unique_ptr decoder_; std::string stat_prefix_; - Stats::Scope& scope_; MongoProxyStats stats_; Runtime::Loader& runtime_; const Network::DrainDecision& drain_decision_; @@ -191,6 +199,7 @@ class ProxyFilter : public Network::Filter, Event::TimerPtr drain_close_timer_; TimeSource& time_source_; const bool emit_dynamic_metadata_; + MongoStatsSharedPtr mongo_stats_; }; class ProdProxyFilter : public ProxyFilter { diff --git a/source/extensions/stat_sinks/common/statsd/statsd.cc b/source/extensions/stat_sinks/common/statsd/statsd.cc index 64d7238bba07..4dc3a5dae872 100644 --- a/source/extensions/stat_sinks/common/statsd/statsd.cc +++ b/source/extensions/stat_sinks/common/statsd/statsd.cc @@ -13,6 +13,7 @@ #include "common/common/fmt.h" #include "common/common/utility.h" #include "common/config/utility.h" +#include "common/stats/symbol_table_impl.h" namespace Envoy { namespace Extensions { @@ -99,8 +100,9 @@ TcpStatsdSink::TcpStatsdSink(const LocalInfo::LocalInfo& local_info, Upstream::ClusterManager& cluster_manager, Stats::Scope& scope, const std::string& prefix) : prefix_(prefix.empty() ? Statsd::getDefaultPrefix() : prefix), tls_(tls.allocateSlot()), - cluster_manager_(cluster_manager), cx_overflow_stat_(scope.counter("statsd.cx_overflow")) { - + cluster_manager_(cluster_manager), + cx_overflow_stat_(scope.counterFromStatName( + Stats::StatNameManagedStorage("statsd.cx_overflow", scope.symbolTable()).statName())) { Config::Utility::checkClusterAndLocalInfo("tcp statsd", cluster_name, cluster_manager, local_info); cluster_info_ = cluster_manager.get(cluster_name)->info(); diff --git a/source/extensions/transport_sockets/tls/BUILD b/source/extensions/transport_sockets/tls/BUILD index cb8bb9ec02ea..3345a9d87439 100644 --- a/source/extensions/transport_sockets/tls/BUILD +++ b/source/extensions/transport_sockets/tls/BUILD @@ -101,6 +101,7 @@ envoy_cc_library( "//source/common/common:utility_lib", "//source/common/network:address_lib", "//source/common/protobuf:utility_lib", + "//source/common/stats:symbol_table_lib", "//source/extensions/transport_sockets/tls/private_key:private_key_manager_lib", "@envoy_api//envoy/admin/v2alpha:certs_cc", ], diff --git a/source/extensions/transport_sockets/tls/context_impl.cc b/source/extensions/transport_sockets/tls/context_impl.cc index f4795c0db2b1..85928172612e 100644 --- a/source/extensions/transport_sockets/tls/context_impl.cc +++ b/source/extensions/transport_sockets/tls/context_impl.cc @@ -51,7 +51,11 @@ bool cbsContainsU16(CBS& cbs, uint16_t n) { ContextImpl::ContextImpl(Stats::Scope& scope, const Envoy::Ssl::ContextConfig& config, TimeSource& time_source) : scope_(scope), stats_(generateStats(scope)), time_source_(time_source), - tls_max_version_(config.maxProtocolVersion()) { + tls_max_version_(config.maxProtocolVersion()), stat_name_set_(scope.symbolTable()), + ssl_ciphers_(stat_name_set_.add("ssl.ciphers")), + ssl_versions_(stat_name_set_.add("ssl.versions")), + ssl_curves_(stat_name_set_.add("ssl.curves")), + ssl_sigalgs_(stat_name_set_.add("ssl.sigalgs")) { const auto tls_certificates = config.tlsCertificates(); tls_contexts_.resize(std::max(static_cast(1), tls_certificates.size())); @@ -369,6 +373,35 @@ ContextImpl::ContextImpl(Stats::Scope& scope, const Envoy::Ssl::ContextConfig& c } parsed_alpn_protocols_ = parseAlpnProtocols(config.alpnProtocols()); + + // To enumerate the required builtin ciphers, curves, algorithms, and + // versions, uncomment '#define LOG_BUILTIN_STAT_NAMES' below, and run + // bazel test //test/extensions/transport_sockets/tls/... --test_output=streamed + // | grep " Builtin ssl." | sort | uniq + // #define LOG_BUILTIN_STAT_NAMES + // + // TODO(#8035): improve tooling to find any other built-ins needed to avoid + // contention. + + // Ciphers + stat_name_set_.rememberBuiltin("AEAD-AES128-GCM-SHA256"); + stat_name_set_.rememberBuiltin("ECDHE-ECDSA-AES128-GCM-SHA256"); + stat_name_set_.rememberBuiltin("ECDHE-RSA-AES128-GCM-SHA256"); + stat_name_set_.rememberBuiltin("ECDHE-RSA-AES128-SHA"); + stat_name_set_.rememberBuiltin("ECDHE-RSA-CHACHA20-POLY1305"); + + // Curves + stat_name_set_.rememberBuiltin("X25519"); + + // Algorithms + stat_name_set_.rememberBuiltin("ecdsa_secp256r1_sha256"); + stat_name_set_.rememberBuiltin("rsa_pss_rsae_sha256"); + + // Versions + stat_name_set_.rememberBuiltin("TLSv1"); + stat_name_set_.rememberBuiltin("TLSv1.1"); + stat_name_set_.rememberBuiltin("TLSv1.2"); + stat_name_set_.rememberBuiltin("TLSv1.3"); } int ServerContextImpl::alpnSelectCallback(const unsigned char** out, unsigned char* outlen, @@ -477,6 +510,18 @@ int ContextImpl::verifyCertificate(X509* cert, const std::vector& v return 1; } +void ContextImpl::incCounter(const Stats::StatName name, absl::string_view value) const { + Stats::SymbolTable& symbol_table = scope_.symbolTable(); + Stats::SymbolTable::StoragePtr storage = + symbol_table.join({name, stat_name_set_.getStatName(value)}); + scope_.counterFromStatName(Stats::StatName(storage.get())).inc(); + +#ifdef LOG_BUILTIN_STAT_NAMES + std::cerr << absl::StrCat("Builtin ", symbol_table.toString(name), ": ", value, "\n") + << std::flush; +#endif +} + void ContextImpl::logHandshake(SSL* ssl) const { stats_.handshake_.inc(); @@ -484,22 +529,19 @@ void ContextImpl::logHandshake(SSL* ssl) const { stats_.session_reused_.inc(); } - const char* cipher = SSL_get_cipher_name(ssl); - scope_.counter(fmt::format("ssl.ciphers.{}", std::string{cipher})).inc(); - - const char* version = SSL_get_version(ssl); - scope_.counter(fmt::format("ssl.versions.{}", std::string{version})).inc(); + incCounter(ssl_ciphers_, SSL_get_cipher_name(ssl)); + incCounter(ssl_versions_, SSL_get_version(ssl)); uint16_t curve_id = SSL_get_curve_id(ssl); if (curve_id) { - const char* curve = SSL_get_curve_name(curve_id); - scope_.counter(fmt::format("ssl.curves.{}", std::string{curve})).inc(); + // Note: in the unit tests, this curve name is always literal "X25519" + incCounter(ssl_curves_, SSL_get_curve_name(curve_id)); } uint16_t sigalg_id = SSL_get_peer_signature_algorithm(ssl); if (sigalg_id) { const char* sigalg = SSL_get_signature_algorithm_name(sigalg_id, 1 /* include curve */); - scope_.counter(fmt::format("ssl.sigalgs.{}", std::string{sigalg})).inc(); + incCounter(ssl_sigalgs_, sigalg); } bssl::UniquePtr cert(SSL_get_peer_certificate(ssl)); diff --git a/source/extensions/transport_sockets/tls/context_impl.h b/source/extensions/transport_sockets/tls/context_impl.h index ccb984a63efa..ce5007fd5a34 100644 --- a/source/extensions/transport_sockets/tls/context_impl.h +++ b/source/extensions/transport_sockets/tls/context_impl.h @@ -12,6 +12,8 @@ #include "envoy/stats/scope.h" #include "envoy/stats/stats_macros.h" +#include "common/stats/symbol_table_impl.h" + #include "extensions/transport_sockets/tls/context_manager_impl.h" #include "absl/synchronization/mutex.h" @@ -126,6 +128,7 @@ class ContextImpl : public virtual Envoy::Ssl::Context { static SslStats generateStats(Stats::Scope& scope); std::string getCaFileName() const { return ca_file_path_; }; + void incCounter(const Stats::StatName name, absl::string_view value) const; Envoy::Ssl::CertificateDetailsPtr certificateDetails(X509* cert, const std::string& path) const; @@ -167,6 +170,11 @@ class ContextImpl : public virtual Envoy::Ssl::Context { std::string cert_chain_file_path_; TimeSource& time_source_; const unsigned tls_max_version_; + mutable Stats::StatNameSet stat_name_set_; + const Stats::StatName ssl_ciphers_; + const Stats::StatName ssl_versions_; + const Stats::StatName ssl_curves_; + const Stats::StatName ssl_sigalgs_; }; using ContextImplSharedPtr = std::shared_ptr; diff --git a/source/server/BUILD b/source/server/BUILD index 5750846d1dca..169f7d3a3cc3 100644 --- a/source/server/BUILD +++ b/source/server/BUILD @@ -107,6 +107,7 @@ envoy_cc_library( "//source/common/common:minimal_logger_lib", "//source/common/common:thread_lib", "//source/common/event:libevent_lib", + "//source/common/stats:symbol_table_lib", ], ) @@ -228,6 +229,7 @@ envoy_cc_library( "//include/envoy/thread_local:thread_local_interface", "//source/common/common:logger_lib", "//source/common/config:utility_lib", + "//source/common/stats:symbol_table_lib", "//source/server:resource_monitor_config_lib", "@envoy_api//envoy/config/overload/v2alpha:overload_cc", ], diff --git a/source/server/guarddog_impl.cc b/source/server/guarddog_impl.cc index a9e3e583e318..5b0b80163478 100644 --- a/source/server/guarddog_impl.cc +++ b/source/server/guarddog_impl.cc @@ -8,6 +8,7 @@ #include "common/common/assert.h" #include "common/common/fmt.h" #include "common/common/lock_guard.h" +#include "common/stats/symbol_table_impl.h" #include "server/watchdog_impl.h" @@ -30,8 +31,12 @@ GuardDogImpl::GuardDogImpl(Stats::Scope& stats_scope, const Server::Configuratio multikillEnabled() ? multi_kill_timeout_ : min_of_nonfatal, min_of_nonfatal}); }()), - watchdog_miss_counter_(stats_scope.counter("server.watchdog_miss")), - watchdog_megamiss_counter_(stats_scope.counter("server.watchdog_mega_miss")), + watchdog_miss_counter_(stats_scope.counterFromStatName( + Stats::StatNameManagedStorage("server.watchdog_miss", stats_scope.symbolTable()) + .statName())), + watchdog_megamiss_counter_(stats_scope.counterFromStatName( + Stats::StatNameManagedStorage("server.watchdog_mega_miss", stats_scope.symbolTable()) + .statName())), dispatcher_(api.allocateDispatcher()), loop_timer_(dispatcher_->createTimer([this]() { step(); })), run_thread_(true) { start(api); diff --git a/source/server/overload_manager_impl.cc b/source/server/overload_manager_impl.cc index 7795802b6c76..484be6f075a9 100644 --- a/source/server/overload_manager_impl.cc +++ b/source/server/overload_manager_impl.cc @@ -5,6 +5,7 @@ #include "common/common/fmt.h" #include "common/config/utility.h" #include "common/protobuf/utility.h" +#include "common/stats/symbol_table_impl.h" #include "server/resource_monitor_config_impl.h" @@ -33,16 +34,25 @@ class ThresholdTriggerImpl : public OverloadAction::Trigger { absl::optional value_; }; -std::string StatsName(const std::string& a, const std::string& b) { - return absl::StrCat("overload.", a, ".", b); +Stats::Counter& makeCounter(Stats::Scope& scope, absl::string_view a, absl::string_view b) { + Stats::StatNameManagedStorage stat_name(absl::StrCat("overload.", a, ".", b), + scope.symbolTable()); + return scope.counterFromStatName(stat_name.statName()); +} + +Stats::Gauge& makeGauge(Stats::Scope& scope, absl::string_view a, absl::string_view b, + Stats::Gauge::ImportMode import_mode) { + Stats::StatNameManagedStorage stat_name(absl::StrCat("overload.", a, ".", b), + scope.symbolTable()); + return scope.gaugeFromStatName(stat_name.statName(), import_mode); } } // namespace OverloadAction::OverloadAction(const envoy::config::overload::v2alpha::OverloadAction& config, Stats::Scope& stats_scope) - : active_gauge_(stats_scope.gauge(StatsName(config.name(), "active"), - Stats::Gauge::ImportMode::Accumulate)) { + : active_gauge_( + makeGauge(stats_scope, config.name(), "active", Stats::Gauge::ImportMode::Accumulate)) { for (const auto& trigger_config : config.triggers()) { TriggerPtr trigger; @@ -213,9 +223,9 @@ OverloadManagerImpl::Resource::Resource(const std::string& name, ResourceMonitor OverloadManagerImpl& manager, Stats::Scope& stats_scope) : name_(name), monitor_(std::move(monitor)), manager_(manager), pending_update_(false), pressure_gauge_( - stats_scope.gauge(StatsName(name, "pressure"), Stats::Gauge::ImportMode::NeverImport)), - failed_updates_counter_(stats_scope.counter(StatsName(name, "failed_updates"))), - skipped_updates_counter_(stats_scope.counter(StatsName(name, "skipped_updates"))) {} + makeGauge(stats_scope, name, "pressure", Stats::Gauge::ImportMode::NeverImport)), + failed_updates_counter_(makeCounter(stats_scope, name, "failed_updates")), + skipped_updates_counter_(makeCounter(stats_scope, name, "skipped_updates")) {} void OverloadManagerImpl::Resource::update() { if (!pending_update_) { diff --git a/test/extensions/filters/http/fault/fault_filter_test.cc b/test/extensions/filters/http/fault/fault_filter_test.cc index d4f8ad2eb90e..f4e0bc1fcb58 100644 --- a/test/extensions/filters/http/fault/fault_filter_test.cc +++ b/test/extensions/filters/http/fault/fault_filter_test.cc @@ -151,6 +151,7 @@ class FaultFilterTest : public testing::Test { void TestPerFilterConfigFault(const Router::RouteSpecificFilterConfig* route_fault, const Router::RouteSpecificFilterConfig* vhost_fault); + Stats::IsolatedStoreImpl stats_; FaultFilterConfigSharedPtr config_; std::unique_ptr filter_; NiceMock decoder_filter_callbacks_; @@ -158,7 +159,6 @@ class FaultFilterTest : public testing::Test { Http::TestHeaderMapImpl request_headers_; Http::TestHeaderMapImpl response_headers_; Buffer::OwnedImpl data_; - Stats::IsolatedStoreImpl stats_; NiceMock runtime_; Event::MockTimer* timer_{}; Event::SimulatedTimeSystem time_system_; diff --git a/test/extensions/filters/http/ip_tagging/ip_tagging_filter_test.cc b/test/extensions/filters/http/ip_tagging/ip_tagging_filter_test.cc index 50679f090c42..774f4e54c216 100644 --- a/test/extensions/filters/http/ip_tagging/ip_tagging_filter_test.cc +++ b/test/extensions/filters/http/ip_tagging/ip_tagging_filter_test.cc @@ -52,11 +52,11 @@ request_type: internal ~IpTaggingFilterTest() override { filter_->onDestroy(); } + NiceMock stats_; IpTaggingFilterConfigSharedPtr config_; std::unique_ptr filter_; NiceMock filter_callbacks_; Buffer::OwnedImpl data_; - NiceMock stats_; NiceMock runtime_; }; diff --git a/test/extensions/filters/network/mongo_proxy/proxy_test.cc b/test/extensions/filters/network/mongo_proxy/proxy_test.cc index 01d0c8082332..a70c1a6f279c 100644 --- a/test/extensions/filters/network/mongo_proxy/proxy_test.cc +++ b/test/extensions/filters/network/mongo_proxy/proxy_test.cc @@ -8,6 +8,7 @@ #include "extensions/filters/network/mongo_proxy/bson_impl.h" #include "extensions/filters/network/mongo_proxy/codec_impl.h" +#include "extensions/filters/network/mongo_proxy/mongo_stats.h" #include "extensions/filters/network/mongo_proxy/proxy.h" #include "extensions/filters/network/well_known_names.h" @@ -62,7 +63,7 @@ class TestProxyFilter : public ProxyFilter { class MongoProxyFilterTest : public testing::Test { public: - MongoProxyFilterTest() { setup(); } + MongoProxyFilterTest() : mongo_stats_(std::make_shared(store_, "test")) { setup(); } void setup() { ON_CALL(runtime_.snapshot_, featureEnabled("mongo.proxy_enabled", 100)) @@ -82,9 +83,9 @@ class MongoProxyFilterTest : public testing::Test { } void initializeFilter(bool emit_dynamic_metadata = false) { - filter_ = std::make_unique("test.", store_, runtime_, access_log_, - fault_config_, drain_decision_, - dispatcher_.timeSource(), emit_dynamic_metadata); + filter_ = std::make_unique( + "test.", store_, runtime_, access_log_, fault_config_, drain_decision_, + dispatcher_.timeSource(), emit_dynamic_metadata, mongo_stats_); filter_->initializeReadFilterCallbacks(read_filter_callbacks_); filter_->onNewConnection(); @@ -114,6 +115,7 @@ class MongoProxyFilterTest : public testing::Test { Buffer::OwnedImpl fake_data_; NiceMock store_; + MongoStatsSharedPtr mongo_stats_; NiceMock runtime_; NiceMock dispatcher_; std::shared_ptr file_{ diff --git a/test/extensions/transport_sockets/tls/ssl_socket_test.cc b/test/extensions/transport_sockets/tls/ssl_socket_test.cc index 8b6dbb4b0263..4fc6090dde14 100644 --- a/test/extensions/transport_sockets/tls/ssl_socket_test.cc +++ b/test/extensions/transport_sockets/tls/ssl_socket_test.cc @@ -699,7 +699,8 @@ const std::string testUtilV2(const TestUtilOptionsV2& options) { dispatcher->run(Event::Dispatcher::RunType::Block); if (!options.expectedServerStats().empty()) { - EXPECT_EQ(1UL, server_stats_store.counter(options.expectedServerStats()).value()); + EXPECT_EQ(1UL, server_stats_store.counter(options.expectedServerStats()).value()) + << options.expectedServerStats(); } if (!options.expectedClientStats().empty()) { diff --git a/tools/check_format.py b/tools/check_format.py index 2adcc22e8c35..26b3e621848f 100755 --- a/tools/check_format.py +++ b/tools/check_format.py @@ -42,20 +42,6 @@ "./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/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/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") @@ -339,10 +325,6 @@ 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 whitelistedForStdRegex(file_path): return file_path.startswith("./test") or file_path in STD_REGEX_WHITELIST @@ -591,7 +573,6 @@ def checkSourceLine(line, file_path, reportError): 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; use StatName saved during construction") diff --git a/tools/spelling_dictionary.txt b/tools/spelling_dictionary.txt index bb101fa95946..e0ccece9cfc5 100644 --- a/tools/spelling_dictionary.txt +++ b/tools/spelling_dictionary.txt @@ -777,6 +777,7 @@ uint un unacked unary +uncomment unconfigurable unconfigured uncontended @@ -792,6 +793,7 @@ unescaping unindexed uninsantiated uninstantiated +uniq unix unlink unlinked