diff --git a/envoy/stats/stats_matcher.h b/envoy/stats/stats_matcher.h index fc0c7c1cc84e3..767e57605f8fc 100644 --- a/envoy/stats/stats_matcher.h +++ b/envoy/stats/stats_matcher.h @@ -9,16 +9,58 @@ namespace Envoy { namespace Stats { +class StatName; + class StatsMatcher { public: + // Holds the result of fastRejects(). This contains state that must be then + // passed to slowRejects() for the final 2-phase determination of whether a + // stat can be rejected. + enum class FastResult { + NoMatch, + Rejects, + Matches, + }; + virtual ~StatsMatcher() = default; /** * Take a metric name and report whether or not it should be instantiated. - * @param the name of a Stats::Metric. + * This may need to convert the StatName to a string. This is equivalent to + * calling fastRejects() and then calling slowResults() if necessary. + * + * @param name the name of a Stats::Metric. + * @return bool true if that stat should not be instantiated. + */ + virtual bool rejects(StatName name) const PURE; + + /** + * Takes a metric name and quickly determines whether it can be rejected based + * purely on the StatName. If fastResults(stat_name).rejects() is 'false', we + * will need to check slowRejects as well. It should not be necessary to cache + * the result of fastRejects() -- it's cheap enough to recompute. However we + * should protect slowRejects() by a cache due to its speed and the potential + * need to take a symbol table lock. + * + * @param name the name of a Stats::Metric. + * @return A result indicating whether the stat can be quickly rejected, as + * well as state that is then passed to slowRejects if rejection + * cannot be quickly determined. + */ + virtual FastResult fastRejects(StatName name) const PURE; + + /** + * Takes a metric name and converts it to a string, if needed, to determine + * whether it needs to be rejected. This is intended to be used if + * fastRejects() cannot determine an early rejection. It is a good idea to + * cache the results of this, to avoid the stringification overhead, potential + * regex overhead, plus a global symbol table lock. + * + * @param fast_result the result of fastRejects(), which must be called first. + * @param name the name of a Stats::Metric. * @return bool true if that stat should not be instantiated. */ - virtual bool rejects(const std::string& name) const PURE; + virtual bool slowRejects(FastResult fast_result, StatName name) const PURE; /** * Helps determine whether the matcher needs to be called. This can be used diff --git a/source/common/common/matchers.cc b/source/common/common/matchers.cc index b1970aead08cb..ec93e4bbf11c1 100644 --- a/source/common/common/matchers.cc +++ b/source/common/common/matchers.cc @@ -112,6 +112,16 @@ bool StringMatcherImpl::match(const absl::string_view value) const { } } +bool StringMatcherImpl::getCaseSensitivePrefixMatch(std::string& prefix) const { + if (matcher_.match_pattern_case() == + envoy::type::matcher::v3::StringMatcher::MatchPatternCase::kPrefix && + !matcher_.ignore_case()) { + prefix = matcher_.prefix(); + return true; + } + return false; +} + ListMatcher::ListMatcher(const envoy::type::matcher::v3::ListMatcher& matcher) : matcher_(matcher) { ASSERT(matcher_.match_pattern_case() == envoy::type::matcher::v3::ListMatcher::MatchPatternCase::kOneOf); diff --git a/source/common/common/matchers.h b/source/common/common/matchers.h index bdc284f2d480f..3bcf1a88bf12b 100644 --- a/source/common/common/matchers.h +++ b/source/common/common/matchers.h @@ -85,11 +85,21 @@ class StringMatcherImpl : public ValueMatcher, public StringMatcher { public: explicit StringMatcherImpl(const envoy::type::matcher::v3::StringMatcher& matcher); + // StringMatcher bool match(const absl::string_view value) const override; bool match(const ProtobufWkt::Value& value) const override; const envoy::type::matcher::v3::StringMatcher& matcher() const { return matcher_; } + /** + * Helps applications optimize the case where a matcher is a case-sensitive + * prefix-match. + * + * @param prefix the returned prefix string + * @return true if the matcher is a case-sensitive prefix-match. + */ + bool getCaseSensitivePrefixMatch(std::string& prefix) const; + private: const envoy::type::matcher::v3::StringMatcher matcher_; Regex::CompiledMatcherPtr regex_; diff --git a/source/common/config/utility.cc b/source/common/config/utility.cc index d6d526b792be1..89b752be1940a 100644 --- a/source/common/config/utility.cc +++ b/source/common/config/utility.cc @@ -223,8 +223,9 @@ Utility::createTagProducer(const envoy::config::bootstrap::v3::Bootstrap& bootst } Stats::StatsMatcherPtr -Utility::createStatsMatcher(const envoy::config::bootstrap::v3::Bootstrap& bootstrap) { - return std::make_unique(bootstrap.stats_config()); +Utility::createStatsMatcher(const envoy::config::bootstrap::v3::Bootstrap& bootstrap, + Stats::SymbolTable& symbol_table) { + return std::make_unique(bootstrap.stats_config(), symbol_table); } Stats::HistogramSettingsConstPtr diff --git a/source/common/config/utility.h b/source/common/config/utility.h index bce7b7f688d3b..e4227ff06caec 100644 --- a/source/common/config/utility.h +++ b/source/common/config/utility.h @@ -431,7 +431,8 @@ class Utility { * Create StatsMatcher instance. */ static Stats::StatsMatcherPtr - createStatsMatcher(const envoy::config::bootstrap::v3::Bootstrap& bootstrap); + createStatsMatcher(const envoy::config::bootstrap::v3::Bootstrap& bootstrap, + Stats::SymbolTable& symbol_table); /** * Create HistogramSettings instance. diff --git a/source/common/stats/BUILD b/source/common/stats/BUILD index 8391ecfd2c4d2..f71156c712426 100644 --- a/source/common/stats/BUILD +++ b/source/common/stats/BUILD @@ -223,6 +223,7 @@ envoy_cc_library( srcs = ["stats_matcher_impl.cc"], hdrs = ["stats_matcher_impl.h"], deps = [ + ":symbol_table_lib", "//envoy/stats:stats_interface", "//source/common/common:matchers_lib", "//source/common/protobuf", diff --git a/source/common/stats/stats_matcher_impl.cc b/source/common/stats/stats_matcher_impl.cc index 01b0049771f03..a4440f90494cf 100644 --- a/source/common/stats/stats_matcher_impl.cc +++ b/source/common/stats/stats_matcher_impl.cc @@ -7,12 +7,17 @@ #include "source/common/common/utility.h" +#include "absl/strings/match.h" + namespace Envoy { namespace Stats { // TODO(ambuc): Refactor this into common/matchers.cc, since StatsMatcher is really just a thin // wrapper around what might be called a StringMatcherList. -StatsMatcherImpl::StatsMatcherImpl(const envoy::config::metrics::v3::StatsConfig& config) { +StatsMatcherImpl::StatsMatcherImpl(const envoy::config::metrics::v3::StatsConfig& config, + SymbolTable& symbol_table) + : symbol_table_(symbol_table), stat_name_pool_(std::make_unique(symbol_table)) { + switch (config.stats_matcher().stats_matcher_case()) { case envoy::config::metrics::v3::StatsMatcher::StatsMatcherCase::kRejectAll: // In this scenario, there are no matchers to store. @@ -22,6 +27,7 @@ StatsMatcherImpl::StatsMatcherImpl(const envoy::config::metrics::v3::StatsConfig // If we have an inclusion list, we are being default-exclusive. for (const auto& stats_matcher : config.stats_matcher().inclusion_list().patterns()) { matchers_.push_back(Matchers::StringMatcherImpl(stats_matcher)); + optimizeLastMatcher(); } is_inclusive_ = false; break; @@ -29,6 +35,7 @@ StatsMatcherImpl::StatsMatcherImpl(const envoy::config::metrics::v3::StatsConfig // If we have an exclusion list, we are being default-inclusive. for (const auto& stats_matcher : config.stats_matcher().exclusion_list().patterns()) { matchers_.push_back(Matchers::StringMatcherImpl(stats_matcher)); + optimizeLastMatcher(); } FALLTHRU; default: @@ -38,19 +45,74 @@ StatsMatcherImpl::StatsMatcherImpl(const envoy::config::metrics::v3::StatsConfig } } -bool StatsMatcherImpl::rejects(const std::string& name) const { - // - // is_inclusive_ | match | return - // ---------------+-------+-------- - // T | T | T Default-inclusive and matching an (exclusion) matcher, deny. - // T | F | F Otherwise, allow. - // F | T | F Default-exclusive and matching an (inclusion) matcher, allow. - // F | F | T Otherwise, deny. - // - // This is an XNOR, which can be evaluated by checking for equality. - - return (is_inclusive_ == std::any_of(matchers_.begin(), matchers_.end(), - [&name](auto& matcher) { return matcher.match(name); })); +// If the last string-matcher added is a case-sensitive prefix match, and the +// prefix ends in ".", then this drops that match and adds it to a list of +// prefixes. This is beneficial because token prefixes can be handled more +// efficiently as a StatName without requiring conversion to a string. +// +// In the future, other matcher patterns could be optimized in a similar way, +// such as: +// * suffixes that begin with "." +// * exact-matches +// * substrings that begin and end with "." +// +// These are left unoptimized for the moment to keep the code-change simpler, +// and because we haven't observed an acute performance need to optimize those +// other patterns yet. +void StatsMatcherImpl::optimizeLastMatcher() { + std::string prefix; + if (matchers_.back().getCaseSensitivePrefixMatch(prefix) && absl::EndsWith(prefix, ".") && + prefix.size() > 1) { + prefixes_.push_back(stat_name_pool_->add(prefix.substr(0, prefix.size() - 1))); + matchers_.pop_back(); + } +} + +StatsMatcher::FastResult StatsMatcherImpl::fastRejects(StatName stat_name) const { + if (rejectsAll()) { + return FastResult::Rejects; + } + bool matches = fastRejectMatch(stat_name); + if ((is_inclusive_ || matchers_.empty()) && matches == is_inclusive_) { + // We can short-circuit the slow matchers only if they are empty, or if + // we are in inclusive-mode and we find a match. + return FastResult::Rejects; + } else if (matches) { + return FastResult::Matches; + } + return FastResult::NoMatch; +} + +bool StatsMatcherImpl::fastRejectMatch(StatName stat_name) const { + return std::any_of(prefixes_.begin(), prefixes_.end(), + [stat_name](StatName prefix) { return stat_name.startsWith(prefix); }); +} + +bool StatsMatcherImpl::slowRejects(FastResult fast_result, StatName stat_name) const { + bool match = slowRejectMatch(stat_name); + + if (is_inclusive_ || match || fast_result != FastResult::Matches) { + // is_inclusive_ | match | return + // ---------------+-------+-------- + // T | T | T Default-inclusive and matching an (exclusion) matcher, deny. + // T | F | F Otherwise, allow. + // F | T | F Default-exclusive and matching an (inclusion) matcher, allow. + // F | F | T Otherwise, deny. + // + // This is an XNOR, which can be evaluated by checking for equality. + return match == is_inclusive_; + } + + return false; +} + +bool StatsMatcherImpl::slowRejectMatch(StatName stat_name) const { + if (matchers_.empty()) { + return false; + } + std::string name = symbol_table_->toString(stat_name); + return std::any_of(matchers_.begin(), matchers_.end(), + [&name](auto& matcher) { return matcher.match(name); }); } } // namespace Stats diff --git a/source/common/stats/stats_matcher_impl.h b/source/common/stats/stats_matcher_impl.h index de8ece9a02da2..b9cad0fa35da2 100644 --- a/source/common/stats/stats_matcher_impl.h +++ b/source/common/stats/stats_matcher_impl.h @@ -2,11 +2,13 @@ #include +#include "envoy/common/optref.h" #include "envoy/config/metrics/v3/stats.pb.h" #include "envoy/stats/stats_matcher.h" #include "source/common/common/matchers.h" #include "source/common/protobuf/protobuf.h" +#include "source/common/stats/symbol_table_impl.h" #include "absl/strings/string_view.h" @@ -18,22 +20,40 @@ namespace Stats { */ class StatsMatcherImpl : public StatsMatcher { public: - explicit StatsMatcherImpl(const envoy::config::metrics::v3::StatsConfig& config); + StatsMatcherImpl(const envoy::config::metrics::v3::StatsConfig& config, + SymbolTable& symbol_table); // Default constructor simply allows everything. StatsMatcherImpl() = default; // StatsMatcher - bool rejects(const std::string& name) const override; - bool acceptsAll() const override { return is_inclusive_ && matchers_.empty(); } - bool rejectsAll() const override { return !is_inclusive_ && matchers_.empty(); } + bool rejects(StatName name) const override { + FastResult fast_result = fastRejects(name); + return fast_result == FastResult::Rejects || slowRejects(fast_result, name); + } + FastResult fastRejects(StatName name) const override; + bool slowRejects(FastResult, StatName name) const override; + bool acceptsAll() const override { + return is_inclusive_ && matchers_.empty() && prefixes_.empty(); + } + bool rejectsAll() const override { + return !is_inclusive_ && matchers_.empty() && prefixes_.empty(); + } private: + void optimizeLastMatcher(); + bool fastRejectMatch(StatName name) const; + bool slowRejectMatch(StatName name) const; + // Bool indicating whether or not the StatsMatcher is including or excluding stats by default. See // StatsMatcherImpl::rejects() for much more detail. bool is_inclusive_{true}; + OptRef symbol_table_; + std::unique_ptr stat_name_pool_; + std::vector matchers_; + std::vector prefixes_; }; } // namespace Stats diff --git a/source/common/stats/symbol_table_impl.cc b/source/common/stats/symbol_table_impl.cc index a60fa31d9b426..0b15b66ba4547 100644 --- a/source/common/stats/symbol_table_impl.cc +++ b/source/common/stats/symbol_table_impl.cc @@ -154,6 +154,43 @@ void SymbolTableImpl::Encoding::decodeTokens( } } +bool StatName::startsWith(StatName symbolic_prefix) const { + bool ret = true; + std::vector prefix_symbols; + + // Decode the prefix as a StatNameVec. + SymbolTableImpl::Encoding::decodeTokens( + symbolic_prefix.data(), symbolic_prefix.dataSize(), + [&prefix_symbols](Symbol symbol) { prefix_symbols.push_back(symbol); }, + [&ret](absl::string_view) { ret = false; }); + + // If there any dynamic components, our string_view lambda will be called, and + // then we'll return false for simplicity. We don't have a current need for + // prefixes to be expressed dynamically, and handling that case would add + // complexity. + if (!ret) { + return false; + } + + // Now decode the StatName, matching against the symbols. It's OK for there to + // be dynamic string_view elements after the prefix match. + uint32_t index = 0; + SymbolTableImpl::Encoding::decodeTokens( + data(), dataSize(), + [&prefix_symbols, &index, &ret](Symbol symbol) { + if (index < prefix_symbols.size() && prefix_symbols[index] != symbol) { + ret = false; + } + ++index; + }, + [&prefix_symbols, &index, &ret](absl::string_view) { + if (index < prefix_symbols.size()) { + ret = false; + } + }); + return ret && index >= prefix_symbols.size(); +} + std::vector SymbolTableImpl::decodeStrings(const SymbolTable::Storage array, size_t size) const { std::vector strings; diff --git a/source/common/stats/symbol_table_impl.h b/source/common/stats/symbol_table_impl.h index cee896e649fa3..52cfe3e924198 100644 --- a/source/common/stats/symbol_table_impl.h +++ b/source/common/stats/symbol_table_impl.h @@ -456,6 +456,16 @@ class StatName { */ bool empty() const { return size_and_data_ == nullptr || dataSize() == 0; } + /** + * Determines whether this starts with the prefix. Note: dynamic segments + * are not supported in the current implementation; this matching only works + * for symbolic segments. However it OK for this to include dynamic segments + * following the prefix. + * + * @param symbolic_prefix the prefix, which must not contain dynamic segments. + */ + bool startsWith(StatName symbolic_prefix) const; + private: /** * Casts the raw data as a string_view. Note that this string_view will not diff --git a/source/common/stats/thread_local_store.cc b/source/common/stats/thread_local_store.cc index de7b0c061a191..3b29f8b5d4df5 100644 --- a/source/common/stats/thread_local_store.cc +++ b/source/common/stats/thread_local_store.cc @@ -101,18 +101,13 @@ void ThreadLocalStoreImpl::removeRejectedStats(StatMapClass& map, StatListClass& } } -bool ThreadLocalStoreImpl::rejects(StatName stat_name) const { - ASSERT(!stats_matcher_->acceptsAll()); +StatsMatcher::FastResult ThreadLocalStoreImpl::fastRejects(StatName stat_name) const { + return stats_matcher_->fastRejects(stat_name); +} - // TODO(ambuc): If stats_matcher_ depends on regexes, this operation (on the - // hot path) could become prohibitively expensive. Revisit this usage in the - // future. - // - // Also note that the elaboration of the stat-name into a string is expensive, - // so I think it might be better to move the matcher test until after caching, - // unless its acceptsAll/rejectsAll. - return stats_matcher_->rejectsAll() || - stats_matcher_->rejects(constSymbolTable().toString(stat_name)); +bool ThreadLocalStoreImpl::slowRejects(StatsMatcher::FastResult fast_reject_result, + StatName stat_name) const { + return stats_matcher_->slowRejects(fast_reject_result, stat_name); } std::vector ThreadLocalStoreImpl::counters() const { @@ -437,6 +432,7 @@ class StatNameTagHelper { }; bool ThreadLocalStoreImpl::checkAndRememberRejection(StatName name, + StatsMatcher::FastResult fast_reject_result, StatNameStorageSet& central_rejected_stats, StatNameHashSet* tls_rejected_stats) { if (stats_matcher_->acceptsAll()) { @@ -448,7 +444,7 @@ bool ThreadLocalStoreImpl::checkAndRememberRejection(StatName name, if (iter != central_rejected_stats.end()) { rejected_name = &(*iter); } else { - if (rejects(name)) { + if (slowRejects(fast_reject_result, name)) { auto insertion = central_rejected_stats.insert(StatNameStorage(name, symbolTable())); const StatNameStorage& rejected_name_ref = *(insertion.first); rejected_name = &rejected_name_ref; @@ -468,8 +464,9 @@ StatType& ThreadLocalStoreImpl::ScopeImpl::safeMakeStat( StatName full_stat_name, StatName name_no_tags, const absl::optional& stat_name_tags, StatNameHashMap>& central_cache_map, - StatNameStorageSet& central_rejected_stats, MakeStatFn make_stat, - StatRefMap* tls_cache, StatNameHashSet* tls_rejected_stats, StatType& null_stat) { + StatsMatcher::FastResult fast_reject_result, StatNameStorageSet& central_rejected_stats, + MakeStatFn make_stat, StatRefMap* tls_cache, + StatNameHashSet* tls_rejected_stats, StatType& null_stat) { if (tls_rejected_stats != nullptr && tls_rejected_stats->find(full_stat_name) != tls_rejected_stats->end()) { @@ -491,8 +488,8 @@ StatType& ThreadLocalStoreImpl::ScopeImpl::safeMakeStat( RefcountPtr* central_ref = nullptr; if (iter != central_cache_map.end()) { central_ref = &(iter->second); - } else if (parent_.checkAndRememberRejection(full_stat_name, central_rejected_stats, - tls_rejected_stats)) { + } else if (parent_.checkAndRememberRejection(full_stat_name, fast_reject_result, + central_rejected_stats, tls_rejected_stats)) { return null_stat; } else { StatNameTagHelper tag_helper(parent_, name_no_tags, stat_name_tags); @@ -546,6 +543,11 @@ Counter& ThreadLocalStoreImpl::ScopeImpl::counterFromStatNameWithTags( TagUtility::TagStatNameJoiner joiner(prefix_.statName(), name, stat_name_tags, symbolTable()); Stats::StatName final_stat_name = joiner.nameWithTags(); + StatsMatcher::FastResult fast_reject_result = parent_.fastRejects(final_stat_name); + if (fast_reject_result == StatsMatcher::FastResult::Rejects) { + return parent_.null_counter_; + } + // We now find the TLS cache. This might remain null if we don't have TLS // initialized currently. StatRefMap* tls_cache = nullptr; @@ -558,7 +560,7 @@ Counter& ThreadLocalStoreImpl::ScopeImpl::counterFromStatNameWithTags( return safeMakeStat( final_stat_name, joiner.tagExtractedName(), stat_name_tags, central_cache_->counters_, - central_cache_->rejected_stats_, + fast_reject_result, central_cache_->rejected_stats_, [](Allocator& allocator, StatName name, StatName tag_extracted_name, const StatNameTagVector& tags) -> CounterSharedPtr { return allocator.makeCounter(name, tag_extracted_name, tags); @@ -600,6 +602,11 @@ Gauge& ThreadLocalStoreImpl::ScopeImpl::gaugeFromStatNameWithTags( TagUtility::TagStatNameJoiner joiner(prefix_.statName(), name, stat_name_tags, symbolTable()); StatName final_stat_name = joiner.nameWithTags(); + StatsMatcher::FastResult fast_reject_result = parent_.fastRejects(final_stat_name); + if (fast_reject_result == StatsMatcher::FastResult::Rejects) { + return parent_.null_gauge_; + } + StatRefMap* tls_cache = nullptr; StatNameHashSet* tls_rejected_stats = nullptr; if (!parent_.shutting_down_ && parent_.tls_cache_) { @@ -610,7 +617,7 @@ Gauge& ThreadLocalStoreImpl::ScopeImpl::gaugeFromStatNameWithTags( Gauge& gauge = safeMakeStat( final_stat_name, joiner.tagExtractedName(), stat_name_tags, central_cache_->gauges_, - central_cache_->rejected_stats_, + fast_reject_result, central_cache_->rejected_stats_, [import_mode](Allocator& allocator, StatName name, StatName tag_extracted_name, const StatNameTagVector& tags) -> GaugeSharedPtr { return allocator.makeGauge(name, tag_extracted_name, tags, import_mode); @@ -638,6 +645,11 @@ Histogram& ThreadLocalStoreImpl::ScopeImpl::histogramFromStatNameWithTags( TagUtility::TagStatNameJoiner joiner(prefix_.statName(), name, stat_name_tags, symbolTable()); StatName final_stat_name = joiner.nameWithTags(); + StatsMatcher::FastResult fast_reject_result = parent_.fastRejects(final_stat_name); + if (fast_reject_result == StatsMatcher::FastResult::Rejects) { + return parent_.null_histogram_; + } + StatNameHashMap* tls_cache = nullptr; StatNameHashSet* tls_rejected_stats = nullptr; if (!parent_.shutting_down_ && parent_.tls_cache_) { @@ -658,7 +670,8 @@ Histogram& ThreadLocalStoreImpl::ScopeImpl::histogramFromStatNameWithTags( ParentHistogramImplSharedPtr* central_ref = nullptr; if (iter != central_cache_->histograms_.end()) { central_ref = &iter->second; - } else if (parent_.checkAndRememberRejection(final_stat_name, central_cache_->rejected_stats_, + } else if (parent_.checkAndRememberRejection(final_stat_name, fast_reject_result, + central_cache_->rejected_stats_, tls_rejected_stats)) { return parent_.null_histogram_; } else { @@ -711,6 +724,11 @@ TextReadout& ThreadLocalStoreImpl::ScopeImpl::textReadoutFromStatNameWithTags( TagUtility::TagStatNameJoiner joiner(prefix_.statName(), name, stat_name_tags, symbolTable()); Stats::StatName final_stat_name = joiner.nameWithTags(); + StatsMatcher::FastResult fast_reject_result = parent_.fastRejects(final_stat_name); + if (fast_reject_result == StatsMatcher::FastResult::Rejects) { + return parent_.null_text_readout_; + } + // We now find the TLS cache. This might remain null if we don't have TLS // initialized currently. StatRefMap* tls_cache = nullptr; @@ -723,7 +741,7 @@ TextReadout& ThreadLocalStoreImpl::ScopeImpl::textReadoutFromStatNameWithTags( return safeMakeStat( final_stat_name, joiner.tagExtractedName(), stat_name_tags, central_cache_->text_readouts_, - central_cache_->rejected_stats_, + fast_reject_result, central_cache_->rejected_stats_, [](Allocator& allocator, StatName name, StatName tag_extracted_name, const StatNameTagVector& tags) -> TextReadoutSharedPtr { return allocator.makeTextReadout(name, tag_extracted_name, tags); diff --git a/source/common/stats/thread_local_store.h b/source/common/stats/thread_local_store.h index ecc359841bce0..eaf2946fd99f7 100644 --- a/source/common/stats/thread_local_store.h +++ b/source/common/stats/thread_local_store.h @@ -416,6 +416,7 @@ class ThreadLocalStoreImpl : Logger::Loggable, public StoreRo StatType& safeMakeStat(StatName full_stat_name, StatName name_no_tags, const absl::optional& stat_name_tags, StatNameHashMap>& central_cache_map, + StatsMatcher::FastResult fast_reject_result, StatNameStorageSet& central_rejected_stats, MakeStatFn make_stat, StatRefMap* tls_cache, StatNameHashSet* tls_rejected_stats, StatType& null_stat); @@ -476,11 +477,14 @@ class ThreadLocalStoreImpl : Logger::Loggable, public StoreRo void clearHistogramsFromCaches(); void releaseScopeCrossThread(ScopeImpl* scope); void mergeInternal(PostMergeCb merge_cb); - bool rejects(StatName name) const; + bool slowRejects(StatsMatcher::FastResult fast_reject_result, StatName name) const; + bool rejects(StatName name) const { return stats_matcher_->rejects(name); } + StatsMatcher::FastResult fastRejects(StatName name) const; bool rejectsAll() const { return stats_matcher_->rejectsAll(); } template void removeRejectedStats(StatMapClass& map, StatListClass& list); - bool checkAndRememberRejection(StatName name, StatNameStorageSet& central_rejected_stats, + bool checkAndRememberRejection(StatName name, StatsMatcher::FastResult fast_reject_result, + StatNameStorageSet& central_rejected_stats, StatNameHashSet* tls_rejected_stats); TlsCache& tlsCache() { return **tls_cache_; } diff --git a/source/server/server.cc b/source/server/server.cc index 9dc4000dac77b..14bded52bcf77 100644 --- a/source/server/server.cc +++ b/source/server/server.cc @@ -364,7 +364,8 @@ void InstanceImpl::initialize(const Options& options, // Needs to happen as early as possible in the instantiation to preempt the objects that require // stats. stats_store_.setTagProducer(Config::Utility::createTagProducer(bootstrap_)); - stats_store_.setStatsMatcher(Config::Utility::createStatsMatcher(bootstrap_)); + stats_store_.setStatsMatcher( + Config::Utility::createStatsMatcher(bootstrap_, stats_store_.symbolTable())); stats_store_.setHistogramSettings(Config::Utility::createHistogramSettings(bootstrap_)); const std::string server_stats_prefix = "server."; diff --git a/test/common/stats/BUILD b/test/common/stats/BUILD index e7b87df994711..cc3e4b21f360d 100644 --- a/test/common/stats/BUILD +++ b/test/common/stats/BUILD @@ -282,6 +282,7 @@ envoy_cc_benchmark_binary( ":stat_test_utility_lib", "//source/common/common:thread_lib", "//source/common/event:dispatcher_lib", + "//source/common/stats:stats_matcher_lib", "//source/common/stats:thread_local_store_lib", "//source/common/thread_local:thread_local_lib", "//test/test_common:simulated_time_system_lib", diff --git a/test/common/stats/stat_test_utility.cc b/test/common/stats/stat_test_utility.cc index 9f9438a7bf9a5..3eb489dbfa6cf 100644 --- a/test/common/stats/stat_test_utility.cc +++ b/test/common/stats/stat_test_utility.cc @@ -7,7 +7,8 @@ namespace Envoy { namespace Stats { namespace TestUtil { -void forEachSampleStat(int num_clusters, std::function fn) { +void forEachSampleStat(int num_clusters, bool include_other_stats, + std::function fn) { // These are stats that are repeated for each cluster as of Oct 2018, with a // very basic configuration with no traffic. static const char* cluster_stats[] = {"bind_errors", @@ -95,8 +96,10 @@ void forEachSampleStat(int num_clusters, std::function fn(absl::StrCat("cluster.service_", cluster, ".", cluster_stat)); } } - for (const auto& other_stat : other_stats) { - fn(other_stat); + if (include_other_stats) { + for (const auto& other_stat : other_stats) { + fn(other_stat); + } } } diff --git a/test/common/stats/stat_test_utility.h b/test/common/stats/stat_test_utility.h index 213775d4bb218..ea9e5b15c3776 100644 --- a/test/common/stats/stat_test_utility.h +++ b/test/common/stats/stat_test_utility.h @@ -49,7 +49,8 @@ class TestSymbolTable { * @param num_clusters the number of clusters for which to generate stats. * @param fn the function to call with every stat name. */ -void forEachSampleStat(int num_clusters, std::function fn); +void forEachSampleStat(int num_clusters, bool include_other_stats, + std::function fn); // Tracks memory consumption over a span of time. Test classes instantiate a // MemoryTest object to start measuring heap memory, and call consumedBytes() to @@ -185,7 +186,9 @@ class TestStore : public SymbolTableProvider, public IsolatedStoreImpl { do { \ if (Stats::TestUtil::MemoryTest::mode() != Stats::TestUtil::MemoryTest::Mode::Disabled) { \ EXPECT_LE(consumed_bytes, upper_bound); \ - EXPECT_GT(consumed_bytes, 0); \ + if (upper_bound != 0) { \ + EXPECT_GT(consumed_bytes, 0); \ + } \ } else { \ ENVOY_LOG_MISC( \ info, "Skipping upper-bound memory test against {} bytes as platform lacks tcmalloc", \ diff --git a/test/common/stats/stats_matcher_impl_test.cc b/test/common/stats/stats_matcher_impl_test.cc index 87b806cd9f133..821a47c69b1df 100644 --- a/test/common/stats/stats_matcher_impl_test.cc +++ b/test/common/stats/stats_matcher_impl_test.cc @@ -12,6 +12,8 @@ namespace Stats { class StatsMatcherTest : public testing::Test { protected: + StatsMatcherTest() : pool_(symbol_table_) {} + envoy::type::matcher::v3::StringMatcher* inclusionList() { return stats_config_.mutable_stats_matcher()->mutable_inclusion_list()->add_patterns(); } @@ -21,18 +23,22 @@ class StatsMatcherTest : public testing::Test { void rejectAll(const bool should_reject) { stats_config_.mutable_stats_matcher()->set_reject_all(should_reject); } - void initMatcher() { stats_matcher_impl_ = std::make_unique(stats_config_); } - void expectAccepted(std::vector expected_to_pass) { + void initMatcher() { + stats_matcher_impl_ = std::make_unique(stats_config_, symbol_table_); + } + void expectAccepted(const std::vector& expected_to_pass) { for (const auto& stat_name : expected_to_pass) { - EXPECT_FALSE(stats_matcher_impl_->rejects(stat_name)) << "Accepted: " << stat_name; + EXPECT_FALSE(stats_matcher_impl_->rejects(pool_.add(stat_name))) << "Accepted: " << stat_name; } } - void expectDenied(std::vector expected_to_fail) { + void expectDenied(const std::vector& expected_to_fail) { for (const auto& stat_name : expected_to_fail) { - EXPECT_TRUE(stats_matcher_impl_->rejects(stat_name)) << "Rejected: " << stat_name; + EXPECT_TRUE(stats_matcher_impl_->rejects(pool_.add(stat_name))) << "Rejected: " << stat_name; } } + SymbolTableImpl symbol_table_; + StatNamePool pool_; std::unique_ptr stats_matcher_impl_; private: @@ -56,6 +62,7 @@ TEST_F(StatsMatcherTest, CheckRejectAll) { expectDenied({"foo", "bar", "foo.bar", "foo.bar.baz", "foobarbaz"}); EXPECT_FALSE(stats_matcher_impl_->acceptsAll()); EXPECT_TRUE(stats_matcher_impl_->rejectsAll()); + EXPECT_EQ(StatsMatcher::FastResult::Rejects, stats_matcher_impl_->fastRejects(StatName())); } TEST_F(StatsMatcherTest, CheckNotRejectAll) { @@ -65,6 +72,7 @@ TEST_F(StatsMatcherTest, CheckNotRejectAll) { expectAccepted({"foo", "bar", "foo.bar", "foo.bar.baz", "foobarbaz"}); EXPECT_TRUE(stats_matcher_impl_->acceptsAll()); EXPECT_FALSE(stats_matcher_impl_->rejectsAll()); + EXPECT_EQ(StatsMatcher::FastResult::NoMatch, stats_matcher_impl_->fastRejects(StatName())); } TEST_F(StatsMatcherTest, CheckIncludeAll) { @@ -90,8 +98,7 @@ TEST_F(StatsMatcherTest, CheckIncludeExact) { inclusionList()->set_exact("abc"); initMatcher(); expectAccepted({"abc"}); - expectDenied({"abcd", "abc.d", "d.abc", "dabc", "ab", "ac", "abcc", "Abc", "aBc", "abC", "abc.", - ".abc", "ABC"}); + expectDenied({"abcd", "abc.d", "d.abc", "dabc", "ab", "ac", "abcc", "Abc", "aBc", "abC", "ABC"}); EXPECT_FALSE(stats_matcher_impl_->acceptsAll()); EXPECT_FALSE(stats_matcher_impl_->rejectsAll()); } @@ -99,8 +106,8 @@ TEST_F(StatsMatcherTest, CheckIncludeExact) { TEST_F(StatsMatcherTest, CheckExcludeExact) { exclusionList()->set_exact("abc"); initMatcher(); - expectAccepted({"abcd", "abc.d", "d.abc", "dabc", "ab", "ac", "abcc", "Abc", "aBc", "abC", "abc.", - ".abc", "ABC"}); + expectAccepted( + {"abcd", "abc.d", "d.abc", "dabc", "ab", "ac", "abcc", "Abc", "aBc", "abC", "ABC"}); expectDenied({"abc"}); EXPECT_FALSE(stats_matcher_impl_->acceptsAll()); EXPECT_FALSE(stats_matcher_impl_->rejectsAll()); @@ -117,6 +124,18 @@ TEST_F(StatsMatcherTest, CheckIncludePrefix) { EXPECT_FALSE(stats_matcher_impl_->rejectsAll()); } +TEST_F(StatsMatcherTest, CheckIncludePrefixDot) { + inclusionList()->set_prefix("abc."); + initMatcher(); + expectAccepted({"abc", "abc.foo"}); + expectDenied( + {"abcfoo", "ABC", "ABC.foo", "ABCfoo", "foo", "abb", "a.b.c", "_abc", "foo.abc", "fooabc"}); + EXPECT_FALSE(stats_matcher_impl_->acceptsAll()); + EXPECT_FALSE(stats_matcher_impl_->rejectsAll()); + EXPECT_EQ(StatsMatcher::FastResult::Matches, + stats_matcher_impl_->fastRejects(pool_.add("abc.foo"))); +} + TEST_F(StatsMatcherTest, CheckExcludePrefix) { exclusionList()->set_prefix("abc"); initMatcher(); @@ -283,5 +302,33 @@ TEST_F(StatsMatcherTest, CheckMultipleAssortedExclusionMatchers) { EXPECT_FALSE(stats_matcher_impl_->rejectsAll()); } +TEST_F(StatsMatcherTest, CheckMultipleAssortedInclusionMatchersWithPrefix) { + inclusionList()->MergeFrom(TestUtility::createRegexMatcher(".*envoy.*")); + inclusionList()->set_suffix("requests"); + inclusionList()->set_exact("regex"); + inclusionList()->set_prefix("prefix."); + initMatcher(); + expectAccepted({"envoy.matchers.requests", "requests.for.envoy", "envoyrequests", "regex", + "prefix", "prefix.foo"}); + expectAccepted({"prefix.envoy.matchers.requests", "prefix.requests.for.envoy", + "prefix.envoyrequests", "prefix.regex"}); + expectDenied({"requestsEnvoy", "EnvoyProxy", "foo", "regex_etc"}); + EXPECT_FALSE(stats_matcher_impl_->acceptsAll()); + EXPECT_FALSE(stats_matcher_impl_->rejectsAll()); +} + +TEST_F(StatsMatcherTest, CheckMultipleAssortedExclusionMatchersWithPrefix) { + exclusionList()->MergeFrom(TestUtility::createRegexMatcher(".*envoy.*")); + exclusionList()->set_suffix("requests"); + exclusionList()->set_exact("regex"); + exclusionList()->set_prefix("prefix."); + initMatcher(); + expectAccepted({"requestsEnvoy", "EnvoyProxy", "foo", "regex_etc", "prefixfoo"}); + expectDenied({"envoy.matchers.requests", "requests.for.envoy", "envoyrequests", "regex", "prefix", + "prefix.foo", "prefix.requests.for.envoy"}); + EXPECT_FALSE(stats_matcher_impl_->acceptsAll()); + EXPECT_FALSE(stats_matcher_impl_->rejectsAll()); +} + } // namespace Stats } // namespace Envoy diff --git a/test/common/stats/symbol_table_impl_test.cc b/test/common/stats/symbol_table_impl_test.cc index 0f8dd73fedae5..a54394975983a 100644 --- a/test/common/stats/symbol_table_impl_test.cc +++ b/test/common/stats/symbol_table_impl_test.cc @@ -694,6 +694,20 @@ TEST_F(StatNameTest, StatNameEmptyEquivalent) { EXPECT_NE(empty2.hash(), non_empty.hash()); } +TEST_F(StatNameTest, StartsWith) { + StatName prefix = makeStat("prefix"); + EXPECT_TRUE(prefix.startsWith(prefix)); + EXPECT_TRUE(makeStat("prefix").startsWith(prefix)); + EXPECT_TRUE(makeStat("prefix.foo").startsWith(prefix)); + EXPECT_TRUE(makeStat("prefix.foo.bar").startsWith(prefix)); + EXPECT_FALSE(makeStat("").startsWith(prefix)); + EXPECT_FALSE(makeStat("foo").startsWith(prefix)); + StatNameDynamicPool dynamic(table_); + StatName dynamic_prefix = dynamic.add("prefix"); + EXPECT_FALSE(dynamic_prefix.startsWith(prefix)); + EXPECT_FALSE(dynamic_prefix.startsWith(dynamic_prefix)); +} + TEST_F(StatNameTest, SupportsAbslHash) { EXPECT_TRUE(absl::VerifyTypeImplementsAbslHashCorrectly({ StatName(), @@ -709,7 +723,7 @@ TEST(SymbolTableTest, Memory) { // Tests a stat-name allocation strategy. auto test_memory_usage = [](std::function fn) -> size_t { TestUtil::MemoryTest memory_test; - TestUtil::forEachSampleStat(1000, fn); + TestUtil::forEachSampleStat(1000, true, fn); return memory_test.consumedBytes(); }; diff --git a/test/common/stats/thread_local_store_speed_test.cc b/test/common/stats/thread_local_store_speed_test.cc index 8ce08a84d052e..620af46c698ab 100644 --- a/test/common/stats/thread_local_store_speed_test.cc +++ b/test/common/stats/thread_local_store_speed_test.cc @@ -7,6 +7,7 @@ #include "source/common/common/thread.h" #include "source/common/event/dispatcher_impl.h" #include "source/common/stats/allocator_impl.h" +#include "source/common/stats/stats_matcher_impl.h" #include "source/common/stats/symbol_table_impl.h" #include "source/common/stats/tag_producer_impl.h" #include "source/common/stats/thread_local_store.h" @@ -28,7 +29,7 @@ class ThreadLocalStorePerf { api_(Api::createApiForTest(store_, time_system_)) { store_.setTagProducer(std::make_unique(stats_config_)); - Stats::TestUtil::forEachSampleStat(1000, [this](absl::string_view name) { + Stats::TestUtil::forEachSampleStat(1000, true, [this](absl::string_view name) { stat_names_.push_back(std::make_unique(name, symbol_table_)); }); } @@ -62,6 +63,12 @@ class ThreadLocalStorePerf { store_.initializeThreading(*dispatcher_, *tls_); } + void initPrefixRejections(const std::string& prefix) { + stats_config_.mutable_stats_matcher()->mutable_exclusion_list()->add_patterns()->set_prefix( + prefix); + store_.setStatsMatcher(std::make_unique(stats_config_, symbol_table_)); + } + private: Stats::SymbolTableImpl symbol_table_; Event::SimulatedTimeSystem time_system_; @@ -82,7 +89,7 @@ class ThreadLocalStorePerf { static void BM_StatsNoTls(benchmark::State& state) { Envoy::ThreadLocalStorePerf context; - for (auto _ : state) { + for (auto _ : state) { // NOLINT context.accessCounters(); } } @@ -96,11 +103,35 @@ static void BM_StatsWithTls(benchmark::State& state) { Envoy::ThreadLocalStorePerf context; context.initThreading(); - for (auto _ : state) { + for (auto _ : state) { // NOLINT context.accessCounters(); } } BENCHMARK(BM_StatsWithTls); +// NOLINTNEXTLINE(readability-identifier-naming) +static void BM_StatsWithTlsAndRejectionsWithDot(benchmark::State& state) { + Envoy::ThreadLocalStorePerf context; + context.initThreading(); + context.initPrefixRejections("cluster."); + + for (auto _ : state) { // NOLINT + context.accessCounters(); + } +} +BENCHMARK(BM_StatsWithTlsAndRejectionsWithDot); + +// NOLINTNEXTLINE(readability-identifier-naming) +static void BM_StatsWithTlsAndRejectionsWithoutDot(benchmark::State& state) { + Envoy::ThreadLocalStorePerf context; + context.initThreading(); + context.initPrefixRejections("cluster"); + + for (auto _ : state) { // NOLINT + context.accessCounters(); + } +} +BENCHMARK(BM_StatsWithTlsAndRejectionsWithoutDot); + // TODO(jmarantz): add multi-threaded variant of this test, that aggressively // looks up stats in multiple threads to try to trigger contention issues. diff --git a/test/common/stats/thread_local_store_test.cc b/test/common/stats/thread_local_store_test.cc index b6ee2c6a1a4ff..b723fe3083fb9 100644 --- a/test/common/stats/thread_local_store_test.cc +++ b/test/common/stats/thread_local_store_test.cc @@ -743,6 +743,28 @@ TEST_F(LookupWithStatNameTest, NotFound) { class StatsMatcherTLSTest : public StatsThreadLocalStoreTest { public: envoy::config::metrics::v3::StatsConfig stats_config_; + + ~StatsMatcherTLSTest() override { + tls_.shutdownGlobalThreading(); + store_->shutdownThreading(); + } + + // Adds counters for 1000 clusters and returns the amount of memory consumed. + uint64_t memoryConsumedAddingClusterStats() { + StatNamePool pool(symbol_table_); + std::vector stat_names; + Stats::TestUtil::forEachSampleStat(1000, false, [&pool, &stat_names](absl::string_view name) { + stat_names.push_back(pool.add(name)); + }); + + { + TestUtil::MemoryTest memory_test; + for (StatName stat_name : stat_names) { + store_->counterFromStatName(stat_name); + } + return memory_test.consumedBytes(); + } + } }; TEST_F(StatsMatcherTLSTest, TestNoOpStatImpls) { @@ -750,7 +772,7 @@ TEST_F(StatsMatcherTLSTest, TestNoOpStatImpls) { stats_config_.mutable_stats_matcher()->mutable_exclusion_list()->add_patterns()->set_prefix( "noop"); - store_->setStatsMatcher(std::make_unique(stats_config_)); + store_->setStatsMatcher(std::make_unique(stats_config_, symbol_table_)); // Testing No-op counters, gauges, histograms which match the prefix "noop". @@ -815,9 +837,6 @@ TEST_F(StatsMatcherTLSTest, TestNoOpStatImpls) { Histogram& noop_histogram_2 = store_->histogramFromString("noop_histogram_2", Stats::Histogram::Unit::Unspecified); EXPECT_EQ(&noop_histogram, &noop_histogram_2); - - tls_.shutdownGlobalThreading(); - store_->shutdownThreading(); } // We only test the exclusion list -- the inclusion list is the inverse, and both are tested in @@ -830,7 +849,7 @@ TEST_F(StatsMatcherTLSTest, TestExclusionRegex) { // Will block all stats containing any capital alphanumeric letter. stats_config_.mutable_stats_matcher()->mutable_exclusion_list()->add_patterns()->MergeFrom( TestUtility::createRegexMatcher(".*[A-Z].*")); - store_->setStatsMatcher(std::make_unique(stats_config_)); + store_->setStatsMatcher(std::make_unique(stats_config_, symbol_table_)); // The creation of counters/gauges/histograms which have no uppercase letters should succeed. Counter& lowercase_counter = store_->counterFromString("lowercase_counter"); @@ -875,7 +894,7 @@ TEST_F(StatsMatcherTLSTest, TestExclusionRegex) { // the string "invalid". stats_config_.mutable_stats_matcher()->mutable_exclusion_list()->add_patterns()->set_prefix( "invalid"); - store_->setStatsMatcher(std::make_unique(stats_config_)); + store_->setStatsMatcher(std::make_unique(stats_config_, symbol_table_)); Counter& valid_counter = store_->counterFromString("valid_counter"); valid_counter.inc(); @@ -927,10 +946,40 @@ TEST_F(StatsMatcherTLSTest, TestExclusionRegex) { TextReadout& invalid_string_2 = store_->textReadoutFromString("also_INVLD_string"); invalid_string_2.set("still no"); EXPECT_EQ("", invalid_string_2.value()); +} - // Expected to free lowercase_counter, lowercase_gauge, valid_counter, valid_gauge - tls_.shutdownGlobalThreading(); - store_->shutdownThreading(); +// Rejecting stats of the form "cluster." enables an optimization in the matcher +// infrastructure that performs the rejection without converting from StatName +// to string, obviating the need to memoize the rejection in a set. This saves +// a ton of memory, allowing us to reject thousands of stats without consuming +// memory. Note that the trailing "." is critical so we can compare symbolically. +TEST_F(StatsMatcherTLSTest, RejectPrefixDot) { + store_->initializeThreading(main_thread_dispatcher_, tls_); + stats_config_.mutable_stats_matcher()->mutable_exclusion_list()->add_patterns()->set_prefix( + "cluster."); // Prefix match can be executed symbolically. + store_->setStatsMatcher(std::make_unique(stats_config_, symbol_table_)); + uint64_t mem_consumed = memoryConsumedAddingClusterStats(); + + // No memory is consumed at all while rejecting stats from "prefix." + EXPECT_MEMORY_EQ(mem_consumed, 0); + EXPECT_MEMORY_LE(mem_consumed, 0); +} + +// Repeating the same test but retaining the dot means that the StatsMatcher +// infrastructure requires us remember the rejected StatNames in an ever-growing +// map. That map is needed to avoid taking locks while re-rejecting stats we've +// rejected in the past. +TEST_F(StatsMatcherTLSTest, RejectPrefixNoDot) { + store_->initializeThreading(main_thread_dispatcher_, tls_); + stats_config_.mutable_stats_matcher()->mutable_exclusion_list()->add_patterns()->set_prefix( + "cluster"); // No dot at the end means we have to compare as strings. + store_->setStatsMatcher(std::make_unique(stats_config_, symbol_table_)); + uint64_t mem_consumed = memoryConsumedAddingClusterStats(); + + // Memory is consumed at all while rejecting stats from "prefix" in proportion + // to the number of stat instantiations attempted. + EXPECT_MEMORY_EQ(mem_consumed, 2936480); + EXPECT_MEMORY_LE(mem_consumed, 3500000); } // Tests the logic for caching the stats-matcher results, and in particular the @@ -966,11 +1015,21 @@ class RememberStatsMatcherTest : public testing::TestWithParam { StatsMatcherPtr matcher_ptr(matcher); store_.setStatsMatcher(std::move(matcher_ptr)); - EXPECT_CALL(*matcher, rejects("scope.reject")).WillOnce(Return(true)); - EXPECT_CALL(*matcher, rejects("scope.ok")).WillOnce(Return(false)); - + StatNamePool pool(symbol_table_); + StatsMatcher::FastResult no_fast_rejection = StatsMatcher::FastResult::NoMatch; for (int j = 0; j < 5; ++j) { + EXPECT_CALL(*matcher, fastRejects(pool.add("scope.reject"))) + .WillOnce(Return(no_fast_rejection)); + if (j == 0) { + EXPECT_CALL(*matcher, slowRejects(no_fast_rejection, pool.add("scope.reject"))) + .WillOnce(Return(true)); + } EXPECT_EQ("", lookup_stat("reject")); + EXPECT_CALL(*matcher, fastRejects(pool.add("scope.ok"))).WillOnce(Return(no_fast_rejection)); + if (j == 0) { + EXPECT_CALL(*matcher, slowRejects(no_fast_rejection, pool.add("scope.ok"))) + .WillOnce(Return(false)); + } EXPECT_EQ("scope.ok", lookup_stat("ok")); } } @@ -985,8 +1044,10 @@ class RememberStatsMatcherTest : public testing::TestWithParam { ScopePtr scope = store_.createScope("scope."); + StatNamePool pool(symbol_table_); for (int j = 0; j < 5; ++j) { - // Note: zero calls to reject() are made, as reject-all should short-circuit. + // Note: zero calls to fastReject() or slowReject() are made, as + // reject-all should short-circuit. EXPECT_EQ("", lookup_stat("reject")); } } @@ -998,9 +1059,12 @@ class RememberStatsMatcherTest : public testing::TestWithParam { matcher->accepts_all_ = true; StatsMatcherPtr matcher_ptr(matcher); store_.setStatsMatcher(std::move(matcher_ptr)); + StatNamePool pool(symbol_table_); + StatsMatcher::FastResult no_fast_rejection = StatsMatcher::FastResult::NoMatch; for (int j = 0; j < 5; ++j) { - // Note: zero calls to reject() are made, as accept-all should short-circuit. + EXPECT_CALL(*matcher, fastRejects(pool.add("scope.ok"))).WillOnce(Return(no_fast_rejection)); + // Note: zero calls to slowReject() are made, as accept-all should short-circuit. EXPECT_EQ("scope.ok", lookup_stat("ok")); } } @@ -1109,7 +1173,7 @@ TEST_F(StatsThreadLocalStoreTest, RemoveRejectedStats) { envoy::config::metrics::v3::StatsConfig stats_config; stats_config.mutable_stats_matcher()->mutable_inclusion_list()->add_patterns()->set_exact( "no-such-stat"); - store_->setStatsMatcher(std::make_unique(stats_config)); + store_->setStatsMatcher(std::make_unique(stats_config, symbol_table_)); // They can no longer be found. EXPECT_EQ(0, store_->counters().size()); @@ -1183,7 +1247,7 @@ class StatsThreadLocalStoreTestNoFixture : public testing::Test { TEST_F(StatsThreadLocalStoreTestNoFixture, MemoryWithoutTlsRealSymbolTable) { TestUtil::MemoryTest memory_test; TestUtil::forEachSampleStat( - 100, [this](absl::string_view name) { store_.counterFromString(std::string(name)); }); + 100, true, [this](absl::string_view name) { store_.counterFromString(std::string(name)); }); EXPECT_MEMORY_EQ(memory_test.consumedBytes(), 688080); // July 2, 2020 EXPECT_MEMORY_LE(memory_test.consumedBytes(), 0.75 * million_); } @@ -1192,7 +1256,7 @@ TEST_F(StatsThreadLocalStoreTestNoFixture, MemoryWithTlsRealSymbolTable) { initThreading(); TestUtil::MemoryTest memory_test; TestUtil::forEachSampleStat( - 100, [this](absl::string_view name) { store_.counterFromString(std::string(name)); }); + 100, true, [this](absl::string_view name) { store_.counterFromString(std::string(name)); }); EXPECT_MEMORY_EQ(memory_test.consumedBytes(), 827616); // Sep 25, 2020 EXPECT_MEMORY_LE(memory_test.consumedBytes(), 0.9 * million_); } diff --git a/test/mocks/stats/mocks.h b/test/mocks/stats/mocks.h index b83dd3310c4cc..928f3ab9c7671 100644 --- a/test/mocks/stats/mocks.h +++ b/test/mocks/stats/mocks.h @@ -348,7 +348,9 @@ class MockStatsMatcher : public StatsMatcher { public: MockStatsMatcher(); ~MockStatsMatcher() override; - MOCK_METHOD(bool, rejects, (const std::string& name), (const)); + MOCK_METHOD(bool, rejects, (StatName name), (const)); + MOCK_METHOD(StatsMatcher::FastResult, fastRejects, (StatName name), (const)); + MOCK_METHOD(bool, slowRejects, (FastResult, StatName name), (const)); bool acceptsAll() const override { return accepts_all_; } bool rejectsAll() const override { return rejects_all_; } diff --git a/tools/spelling/spelling_dictionary.txt b/tools/spelling/spelling_dictionary.txt index f5dad4b68e77f..73e89a709bc0a 100644 --- a/tools/spelling/spelling_dictionary.txt +++ b/tools/spelling/spelling_dictionary.txt @@ -797,6 +797,7 @@ megamiss mem memcmp memcpy +memoize mergeable messagename metadata