From 5c29ac0d749717d90a8ad9096227d9a09b82a495 Mon Sep 17 00:00:00 2001 From: Joshua Marantz Date: Wed, 16 Jun 2021 19:47:09 -0400 Subject: [PATCH 01/16] add API to do a symbolic prefix match on StatNames without taking a lock. Signed-off-by: Joshua Marantz --- source/common/stats/symbol_table_impl.cc | 35 +++++++++++++++++++++ source/common/stats/symbol_table_impl.h | 10 ++++++ test/common/stats/symbol_table_impl_test.cc | 14 +++++++++ 3 files changed, 59 insertions(+) diff --git a/source/common/stats/symbol_table_impl.cc b/source/common/stats/symbol_table_impl.cc index a60fa31d9b426..26c29aef39fa3 100644 --- a/source/common/stats/symbol_table_impl.cc +++ b/source/common/stats/symbol_table_impl.cc @@ -154,6 +154,41 @@ 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, we'll our string_view lambda called, and + // then we'll return false for simplicity. + if (!ret) { + return false; + } + + // Now decode the StatName, matching against the symbols. It's OK for there to + // be 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/test/common/stats/symbol_table_impl_test.cc b/test/common/stats/symbol_table_impl_test.cc index 0f8dd73fedae5..fb828fd7ee9f9 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(), From b134c93b4d73300e82405672a9e0428b4344a56e Mon Sep 17 00:00:00 2001 From: Joshua Marantz Date: Wed, 16 Jun 2021 21:03:33 -0400 Subject: [PATCH 02/16] add prefix-optimized stats_matcher impl. Signed-off-by: Joshua Marantz --- envoy/stats/stats_matcher.h | 4 +- source/common/common/matchers.cc | 10 ++++ source/common/common/matchers.h | 5 ++ source/common/stats/BUILD | 1 + source/common/stats/stats_matcher_impl.cc | 32 ++++++++++-- source/common/stats/stats_matcher_impl.h | 23 ++++++-- source/common/stats/thread_local_store.cc | 3 +- test/common/stats/stats_matcher_impl_test.cc | 55 ++++++++++++++++---- test/mocks/stats/mocks.h | 2 +- 9 files changed, 114 insertions(+), 21 deletions(-) diff --git a/envoy/stats/stats_matcher.h b/envoy/stats/stats_matcher.h index fc0c7c1cc84e3..45b9c5d969e05 100644 --- a/envoy/stats/stats_matcher.h +++ b/envoy/stats/stats_matcher.h @@ -9,6 +9,8 @@ namespace Envoy { namespace Stats { +class StatName; + class StatsMatcher { public: virtual ~StatsMatcher() = default; @@ -18,7 +20,7 @@ class StatsMatcher { * @param 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 rejects(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..97214aa94757c 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 396203ce5b50d..2b275237a57ed 100644 --- a/source/common/common/matchers.h +++ b/source/common/common/matchers.h @@ -80,11 +80,16 @@ 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_; } + // If the matcher is a case-sensitive prefix match, returns the prefix string. + // otherwise, returns an empty string. + bool getCaseSensitivePrefixMatch(std::string& prefix) const; + private: const envoy::type::matcher::v3::StringMatcher matcher_; Regex::CompiledMatcherPtr regex_; 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..36671eb29aef5 100644 --- a/source/common/stats/stats_matcher_impl.cc +++ b/source/common/stats/stats_matcher_impl.cc @@ -7,12 +7,18 @@ #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 +28,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 +36,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,7 +46,16 @@ StatsMatcherImpl::StatsMatcherImpl(const envoy::config::metrics::v3::StatsConfig } } -bool StatsMatcherImpl::rejects(const std::string& name) const { +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(); + } +} + +bool StatsMatcherImpl::rejects(StatName stat_name) const { // // is_inclusive_ | match | return // ---------------+-------+-------- @@ -49,8 +66,15 @@ bool StatsMatcherImpl::rejects(const std::string& name) const { // // 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); })); + bool match = std::any_of(prefixes_.begin(), prefixes_.end(), + [stat_name](StatName prefix) { return stat_name.startsWith(prefix); }); + if (!match && !matchers_.empty()) { + std::string name = symbol_table_->toString(stat_name); + match = std::any_of(matchers_.begin(), matchers_.end(), + [&name](auto& matcher) { return matcher.match(name); }); + } + + return is_inclusive_ == match; } } // namespace Stats diff --git a/source/common/stats/stats_matcher_impl.h b/source/common/stats/stats_matcher_impl.h index de8ece9a02da2..492da88c86735 100644 --- a/source/common/stats/stats_matcher_impl.h +++ b/source/common/stats/stats_matcher_impl.h @@ -3,10 +3,12 @@ #include #include "envoy/config/metrics/v3/stats.pb.h" +#include "envoy/common/optref.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,35 @@ 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; + bool acceptsAll() const override { return is_inclusive_ && matchers_.empty() && + prefixes_.empty(); } + bool rejectsAll() const override { return !is_inclusive_ && matchers_.empty() && + prefixes_.empty(); } + + // Determines whether conversion from StatName to string may be necessary to + // run a match against this set. + bool hasStringMatchers() const { return !matchers_.empty(); } private: + void optimizeLastMatcher(); + // 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/thread_local_store.cc b/source/common/stats/thread_local_store.cc index de7b0c061a191..58290020098aa 100644 --- a/source/common/stats/thread_local_store.cc +++ b/source/common/stats/thread_local_store.cc @@ -111,8 +111,7 @@ bool ThreadLocalStoreImpl::rejects(StatName stat_name) const { // 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)); + return stats_matcher_->rejectsAll() || stats_matcher_->rejects(stat_name); } std::vector ThreadLocalStoreImpl::counters() const { diff --git a/test/common/stats/stats_matcher_impl_test.cc b/test/common/stats/stats_matcher_impl_test.cc index 87b806cd9f133..ec0b4f5c21ac3 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,21 @@ 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: @@ -42,6 +47,7 @@ class StatsMatcherTest : public testing::Test { TEST_F(StatsMatcherTest, CheckDefault) { // With no set fields, everything should be allowed through. initMatcher(); + EXPECT_FALSE(stats_matcher_impl_->hasStringMatchers()); expectAccepted({"foo", "bar", "foo.bar", "foo.bar.baz", "foobarbaz"}); EXPECT_TRUE(stats_matcher_impl_->acceptsAll()); EXPECT_FALSE(stats_matcher_impl_->rejectsAll()); @@ -53,6 +59,7 @@ TEST_F(StatsMatcherTest, CheckRejectAll) { // With reject_all, nothing should be allowed through. rejectAll(true); initMatcher(); + EXPECT_FALSE(stats_matcher_impl_->hasStringMatchers()); expectDenied({"foo", "bar", "foo.bar", "foo.bar.baz", "foobarbaz"}); EXPECT_FALSE(stats_matcher_impl_->acceptsAll()); EXPECT_TRUE(stats_matcher_impl_->rejectsAll()); @@ -62,6 +69,7 @@ TEST_F(StatsMatcherTest, CheckNotRejectAll) { // With !reject_all, everything should be allowed through. rejectAll(false); initMatcher(); + EXPECT_FALSE(stats_matcher_impl_->hasStringMatchers()); expectAccepted({"foo", "bar", "foo.bar", "foo.bar.baz", "foobarbaz"}); EXPECT_TRUE(stats_matcher_impl_->acceptsAll()); EXPECT_FALSE(stats_matcher_impl_->rejectsAll()); @@ -70,6 +78,7 @@ TEST_F(StatsMatcherTest, CheckNotRejectAll) { TEST_F(StatsMatcherTest, CheckIncludeAll) { inclusionList()->MergeFrom(TestUtility::createRegexMatcher(".*")); initMatcher(); + EXPECT_TRUE(stats_matcher_impl_->hasStringMatchers()); expectAccepted({"foo", "bar", "foo.bar", "foo.bar.baz"}); // It really does accept all, but the impl doesn't know it. EXPECT_FALSE(stats_matcher_impl_->acceptsAll()); @@ -79,6 +88,7 @@ TEST_F(StatsMatcherTest, CheckIncludeAll) { TEST_F(StatsMatcherTest, CheckExcludeAll) { exclusionList()->MergeFrom(TestUtility::createRegexMatcher(".*")); initMatcher(); + EXPECT_TRUE(stats_matcher_impl_->hasStringMatchers()); expectDenied({"foo", "bar", "foo.bar", "foo.bar.baz"}); EXPECT_FALSE(stats_matcher_impl_->acceptsAll()); EXPECT_FALSE(stats_matcher_impl_->rejectsAll()); @@ -89,9 +99,9 @@ TEST_F(StatsMatcherTest, CheckExcludeAll) { TEST_F(StatsMatcherTest, CheckIncludeExact) { inclusionList()->set_exact("abc"); initMatcher(); + EXPECT_TRUE(stats_matcher_impl_->hasStringMatchers()); 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 +109,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"}); + EXPECT_TRUE(stats_matcher_impl_->hasStringMatchers()); + 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()); @@ -111,15 +121,28 @@ TEST_F(StatsMatcherTest, CheckExcludeExact) { TEST_F(StatsMatcherTest, CheckIncludePrefix) { inclusionList()->set_prefix("abc"); initMatcher(); + EXPECT_TRUE(stats_matcher_impl_->hasStringMatchers()); expectAccepted({"abc", "abc.foo", "abcfoo"}); expectDenied({"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()); } +TEST_F(StatsMatcherTest, CheckIncludePrefixDot) { + inclusionList()->set_prefix("abc."); + initMatcher(); + EXPECT_FALSE(stats_matcher_impl_->hasStringMatchers()); + 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()); +} + TEST_F(StatsMatcherTest, CheckExcludePrefix) { exclusionList()->set_prefix("abc"); initMatcher(); + EXPECT_TRUE(stats_matcher_impl_->hasStringMatchers()); expectAccepted({"ABC", "ABC.foo", "ABCfoo", "foo", "abb", "a.b.c", "_abc", "foo.abc", "fooabc"}); expectDenied({"abc", "abc.foo", "abcfoo"}); EXPECT_FALSE(stats_matcher_impl_->acceptsAll()); @@ -131,6 +154,7 @@ TEST_F(StatsMatcherTest, CheckExcludePrefix) { TEST_F(StatsMatcherTest, CheckIncludeSuffix) { inclusionList()->set_suffix("abc"); initMatcher(); + EXPECT_TRUE(stats_matcher_impl_->hasStringMatchers()); expectAccepted({"abc", "foo.abc", "fooabc"}); expectDenied({"ABC", "foo.ABC", "fooABC", "foo", "abb", "a.b.c", "abc_", "abc.foo", "abcfoo"}); EXPECT_FALSE(stats_matcher_impl_->acceptsAll()); @@ -140,6 +164,7 @@ TEST_F(StatsMatcherTest, CheckIncludeSuffix) { TEST_F(StatsMatcherTest, CheckExcludeSuffix) { exclusionList()->set_suffix("abc"); initMatcher(); + EXPECT_TRUE(stats_matcher_impl_->hasStringMatchers()); expectAccepted({"ABC", "foo.ABC", "fooABC", "foo", "abb", "a.b.c", "abc_", "abc.foo", "abcfoo"}); expectDenied({"abc", "foo.abc", "fooabc"}); EXPECT_FALSE(stats_matcher_impl_->acceptsAll()); @@ -151,6 +176,7 @@ TEST_F(StatsMatcherTest, CheckExcludeSuffix) { TEST_F(StatsMatcherTest, CheckIncludeRegex) { inclusionList()->MergeFrom(TestUtility::createRegexMatcher(".*envoy.*")); initMatcher(); + EXPECT_TRUE(stats_matcher_impl_->hasStringMatchers()); expectAccepted({"envoy.matchers.requests", "stats.envoy.2xx", "regex.envoy.matchers"}); expectDenied({"foo", "Envoy", "EnvoyProxy"}); EXPECT_FALSE(stats_matcher_impl_->acceptsAll()); @@ -160,6 +186,7 @@ TEST_F(StatsMatcherTest, CheckIncludeRegex) { TEST_F(StatsMatcherTest, CheckExcludeRegex) { exclusionList()->MergeFrom(TestUtility::createRegexMatcher(".*envoy.*")); initMatcher(); + EXPECT_TRUE(stats_matcher_impl_->hasStringMatchers()); expectAccepted({"foo", "Envoy", "EnvoyProxy"}); expectDenied({"envoy.matchers.requests", "stats.envoy.2xx", "regex.envoy.matchers"}); EXPECT_FALSE(stats_matcher_impl_->acceptsAll()); @@ -172,6 +199,7 @@ TEST_F(StatsMatcherTest, CheckMultipleIncludeExact) { inclusionList()->set_exact("foo"); inclusionList()->set_exact("bar"); initMatcher(); + EXPECT_TRUE(stats_matcher_impl_->hasStringMatchers()); expectAccepted({"foo", "bar"}); expectDenied({"foobar", "barfoo", "fo", "ba", "foo.bar"}); EXPECT_FALSE(stats_matcher_impl_->acceptsAll()); @@ -182,6 +210,7 @@ TEST_F(StatsMatcherTest, CheckMultipleExcludeExact) { exclusionList()->set_exact("foo"); exclusionList()->set_exact("bar"); initMatcher(); + EXPECT_TRUE(stats_matcher_impl_->hasStringMatchers()); expectAccepted({"foobar", "barfoo", "fo", "ba", "foo.bar"}); expectDenied({"foo", "bar"}); EXPECT_FALSE(stats_matcher_impl_->acceptsAll()); @@ -194,6 +223,7 @@ TEST_F(StatsMatcherTest, CheckMultipleIncludePrefix) { inclusionList()->set_prefix("foo"); inclusionList()->set_prefix("bar"); initMatcher(); + EXPECT_TRUE(stats_matcher_impl_->hasStringMatchers()); expectAccepted({"foo", "foo.abc", "bar", "bar.abc"}); expectDenied({".foo", "abc.foo", "BAR", "_bar"}); EXPECT_FALSE(stats_matcher_impl_->acceptsAll()); @@ -204,6 +234,7 @@ TEST_F(StatsMatcherTest, CheckMultipleExcludePrefix) { exclusionList()->set_prefix("foo"); exclusionList()->set_prefix("bar"); initMatcher(); + EXPECT_TRUE(stats_matcher_impl_->hasStringMatchers()); expectAccepted({".foo", "abc.foo", "BAR", "_bar"}); expectDenied({"foo", "foo.abc", "bar", "bar.abc"}); EXPECT_FALSE(stats_matcher_impl_->acceptsAll()); @@ -216,6 +247,7 @@ TEST_F(StatsMatcherTest, CheckMultipleIncludeSuffix) { inclusionList()->set_suffix("spam"); inclusionList()->set_suffix("eggs"); initMatcher(); + EXPECT_TRUE(stats_matcher_impl_->hasStringMatchers()); expectAccepted( {"requests.for.spam", "requests.for.eggs", "spam", "eggs", "cannedspam", "fresheggs"}); expectDenied({"Spam", "EGGS", "spam_", "eggs_"}); @@ -227,6 +259,7 @@ TEST_F(StatsMatcherTest, CheckMultipleExcludeSuffix) { exclusionList()->set_suffix("spam"); exclusionList()->set_suffix("eggs"); initMatcher(); + EXPECT_TRUE(stats_matcher_impl_->hasStringMatchers()); expectAccepted({"Spam", "EGGS", "spam_", "eggs_"}); expectDenied( {"requests.for.spam", "requests.for.eggs", "spam", "eggs", "cannedspam", "fresheggs"}); @@ -240,6 +273,7 @@ TEST_F(StatsMatcherTest, CheckMultipleIncludeRegex) { inclusionList()->MergeFrom(TestUtility::createRegexMatcher(".*envoy.*")); inclusionList()->MergeFrom(TestUtility::createRegexMatcher(".*absl.*")); initMatcher(); + EXPECT_TRUE(stats_matcher_impl_->hasStringMatchers()); expectAccepted({"envoy.matchers.requests", "stats.absl.2xx", "absl.envoy.matchers"}); expectDenied({"Abseil", "EnvoyProxy"}); EXPECT_FALSE(stats_matcher_impl_->acceptsAll()); @@ -250,6 +284,7 @@ TEST_F(StatsMatcherTest, CheckMultipleExcludeRegex) { exclusionList()->MergeFrom(TestUtility::createRegexMatcher(".*envoy.*")); exclusionList()->MergeFrom(TestUtility::createRegexMatcher(".*absl.*")); initMatcher(); + EXPECT_TRUE(stats_matcher_impl_->hasStringMatchers()); expectAccepted({"Abseil", "EnvoyProxy"}); expectDenied({"envoy.matchers.requests", "stats.absl.2xx", "absl.envoy.matchers"}); EXPECT_FALSE(stats_matcher_impl_->acceptsAll()); @@ -266,6 +301,7 @@ TEST_F(StatsMatcherTest, CheckMultipleAssortedInclusionMatchers) { inclusionList()->set_suffix("requests"); inclusionList()->set_exact("regex"); initMatcher(); + EXPECT_TRUE(stats_matcher_impl_->hasStringMatchers()); expectAccepted({"envoy.matchers.requests", "requests.for.envoy", "envoyrequests", "regex"}); expectDenied({"requestsEnvoy", "EnvoyProxy", "foo", "regex_etc"}); EXPECT_FALSE(stats_matcher_impl_->acceptsAll()); @@ -277,6 +313,7 @@ TEST_F(StatsMatcherTest, CheckMultipleAssortedExclusionMatchers) { exclusionList()->set_suffix("requests"); exclusionList()->set_exact("regex"); initMatcher(); + EXPECT_TRUE(stats_matcher_impl_->hasStringMatchers()); expectAccepted({"requestsEnvoy", "EnvoyProxy", "foo", "regex_etc"}); expectDenied({"envoy.matchers.requests", "requests.for.envoy", "envoyrequests", "regex"}); EXPECT_FALSE(stats_matcher_impl_->acceptsAll()); diff --git a/test/mocks/stats/mocks.h b/test/mocks/stats/mocks.h index b83dd3310c4cc..ff3930c68a191 100644 --- a/test/mocks/stats/mocks.h +++ b/test/mocks/stats/mocks.h @@ -348,7 +348,7 @@ class MockStatsMatcher : public StatsMatcher { public: MockStatsMatcher(); ~MockStatsMatcher() override; - MOCK_METHOD(bool, rejects, (const std::string& name), (const)); + MOCK_METHOD(bool, rejects, (StatName name), (const)); bool acceptsAll() const override { return accepts_all_; } bool rejectsAll() const override { return rejects_all_; } From 68b7d41181558e303f40cdea611df9107436fe48 Mon Sep 17 00:00:00 2001 From: Joshua Marantz Date: Wed, 16 Jun 2021 21:22:47 -0400 Subject: [PATCH 03/16] Use the new stats-matcher API that optikmizes for prefixes. Signed-off-by: Joshua Marantz --- source/common/common/matchers.cc | 2 +- source/common/config/utility.cc | 5 +++-- source/common/config/utility.h | 3 ++- source/common/stats/stats_matcher_impl.cc | 7 +++---- source/common/stats/stats_matcher_impl.h | 12 +++++++----- source/server/server.cc | 3 ++- test/common/stats/stats_matcher_impl_test.cc | 12 +++++++----- test/common/stats/thread_local_store_test.cc | 13 +++++++------ 8 files changed, 32 insertions(+), 25 deletions(-) diff --git a/source/common/common/matchers.cc b/source/common/common/matchers.cc index 97214aa94757c..ec93e4bbf11c1 100644 --- a/source/common/common/matchers.cc +++ b/source/common/common/matchers.cc @@ -114,7 +114,7 @@ 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 && + envoy::type::matcher::v3::StringMatcher::MatchPatternCase::kPrefix && !matcher_.ignore_case()) { prefix = matcher_.prefix(); return true; diff --git a/source/common/config/utility.cc b/source/common/config/utility.cc index d1743deeb042c..6d2997473cee2 100644 --- a/source/common/config/utility.cc +++ b/source/common/config/utility.cc @@ -227,8 +227,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/stats_matcher_impl.cc b/source/common/stats/stats_matcher_impl.cc index 36671eb29aef5..b10943cfb8939 100644 --- a/source/common/stats/stats_matcher_impl.cc +++ b/source/common/stats/stats_matcher_impl.cc @@ -16,8 +16,7 @@ namespace Stats { // wrapper around what might be called a StringMatcherList. StatsMatcherImpl::StatsMatcherImpl(const envoy::config::metrics::v3::StatsConfig& config, SymbolTable& symbol_table) - : symbol_table_(symbol_table), - stat_name_pool_(std::make_unique(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: @@ -48,8 +47,8 @@ StatsMatcherImpl::StatsMatcherImpl(const envoy::config::metrics::v3::StatsConfig void StatsMatcherImpl::optimizeLastMatcher() { std::string prefix; - if (matchers_.back().getCaseSensitivePrefixMatch(prefix) && - absl::EndsWith(prefix, ".") && prefix.size() > 1) { + 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(); } diff --git a/source/common/stats/stats_matcher_impl.h b/source/common/stats/stats_matcher_impl.h index 492da88c86735..34760c2e60a7d 100644 --- a/source/common/stats/stats_matcher_impl.h +++ b/source/common/stats/stats_matcher_impl.h @@ -2,8 +2,8 @@ #include -#include "envoy/config/metrics/v3/stats.pb.h" #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" @@ -28,10 +28,12 @@ class StatsMatcherImpl : public StatsMatcher { // StatsMatcher bool rejects(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(); } + bool acceptsAll() const override { + return is_inclusive_ && matchers_.empty() && prefixes_.empty(); + } + bool rejectsAll() const override { + return !is_inclusive_ && matchers_.empty() && prefixes_.empty(); + } // Determines whether conversion from StatName to string may be necessary to // run a match against this set. diff --git a/source/server/server.cc b/source/server/server.cc index 5b8520e2fffc4..432b7d055818c 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/stats_matcher_impl_test.cc b/test/common/stats/stats_matcher_impl_test.cc index ec0b4f5c21ac3..005345b30aa5f 100644 --- a/test/common/stats/stats_matcher_impl_test.cc +++ b/test/common/stats/stats_matcher_impl_test.cc @@ -23,8 +23,9 @@ 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_, symbol_table_); } + 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(pool_.add(stat_name))) << "Accepted: " << stat_name; @@ -110,7 +111,8 @@ TEST_F(StatsMatcherTest, CheckExcludeExact) { exclusionList()->set_exact("abc"); initMatcher(); EXPECT_TRUE(stats_matcher_impl_->hasStringMatchers()); - expectAccepted({"abcd", "abc.d", "d.abc", "dabc", "ab", "ac", "abcc", "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()); @@ -133,8 +135,8 @@ TEST_F(StatsMatcherTest, CheckIncludePrefixDot) { initMatcher(); EXPECT_FALSE(stats_matcher_impl_->hasStringMatchers()); expectAccepted({"abc", "abc.foo"}); - expectDenied({"abcfoo", "ABC", "ABC.foo", "ABCfoo", "foo", "abb", "a.b.c", "_abc", "foo.abc", - "fooabc"}); + 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()); } diff --git a/test/common/stats/thread_local_store_test.cc b/test/common/stats/thread_local_store_test.cc index b6ee2c6a1a4ff..3dcf6ae4af60b 100644 --- a/test/common/stats/thread_local_store_test.cc +++ b/test/common/stats/thread_local_store_test.cc @@ -750,7 +750,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". @@ -830,7 +830,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 +875,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(); @@ -966,8 +966,9 @@ 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_); + EXPECT_CALL(*matcher, rejects(pool.add("scope.reject"))).WillOnce(Return(true)); + EXPECT_CALL(*matcher, rejects(pool.add("scope.ok"))).WillOnce(Return(false)); for (int j = 0; j < 5; ++j) { EXPECT_EQ("", lookup_stat("reject")); @@ -1109,7 +1110,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()); From f5068ccdf3923a25d36e06935296bd541eb7f1eb Mon Sep 17 00:00:00 2001 From: Joshua Marantz Date: Wed, 16 Jun 2021 23:13:40 -0400 Subject: [PATCH 04/16] Skip remembering rejected stats when specified cheaply. Signed-off-by: Joshua Marantz --- envoy/stats/stats_matcher.h | 4 ++++ source/common/stats/stats_matcher_impl.h | 5 +---- source/common/stats/thread_local_store.cc | 4 ++++ test/mocks/stats/mocks.h | 1 + 4 files changed, 10 insertions(+), 4 deletions(-) diff --git a/envoy/stats/stats_matcher.h b/envoy/stats/stats_matcher.h index 45b9c5d969e05..b063300059b80 100644 --- a/envoy/stats/stats_matcher.h +++ b/envoy/stats/stats_matcher.h @@ -41,6 +41,10 @@ class StatsMatcher { * rejectsAll() returns false, but rejects() is always true. */ virtual bool rejectsAll() const PURE; + + // Determines whether conversion from StatName to string may be necessary to + // run a match against this set. + virtual bool hasStringMatchers() const PURE; }; using StatsMatcherPtr = std::unique_ptr; diff --git a/source/common/stats/stats_matcher_impl.h b/source/common/stats/stats_matcher_impl.h index 34760c2e60a7d..8d69d93dad205 100644 --- a/source/common/stats/stats_matcher_impl.h +++ b/source/common/stats/stats_matcher_impl.h @@ -34,10 +34,7 @@ class StatsMatcherImpl : public StatsMatcher { bool rejectsAll() const override { return !is_inclusive_ && matchers_.empty() && prefixes_.empty(); } - - // Determines whether conversion from StatName to string may be necessary to - // run a match against this set. - bool hasStringMatchers() const { return !matchers_.empty(); } + bool hasStringMatchers() const override { return !matchers_.empty(); } private: void optimizeLastMatcher(); diff --git a/source/common/stats/thread_local_store.cc b/source/common/stats/thread_local_store.cc index 58290020098aa..85b56408be9a7 100644 --- a/source/common/stats/thread_local_store.cc +++ b/source/common/stats/thread_local_store.cc @@ -448,6 +448,10 @@ bool ThreadLocalStoreImpl::checkAndRememberRejection(StatName name, rejected_name = &(*iter); } else { if (rejects(name)) { + if (!stats_matcher_->hasStringMatchers()) { + // Skip recording of cheaply-matched rejections in the caches. + return true; + } auto insertion = central_rejected_stats.insert(StatNameStorage(name, symbolTable())); const StatNameStorage& rejected_name_ref = *(insertion.first); rejected_name = &rejected_name_ref; diff --git a/test/mocks/stats/mocks.h b/test/mocks/stats/mocks.h index ff3930c68a191..13debb87f066c 100644 --- a/test/mocks/stats/mocks.h +++ b/test/mocks/stats/mocks.h @@ -351,6 +351,7 @@ class MockStatsMatcher : public StatsMatcher { MOCK_METHOD(bool, rejects, (StatName name), (const)); bool acceptsAll() const override { return accepts_all_; } bool rejectsAll() const override { return rejects_all_; } + bool hasStringMatchers() const override { return true; } bool accepts_all_{false}; bool rejects_all_{false}; From bb11ad97f7f9e3385ceb5bfd758ecc74700afc59 Mon Sep 17 00:00:00 2001 From: Joshua Marantz Date: Thu, 17 Jun 2021 12:13:56 -0400 Subject: [PATCH 05/16] add unit tests showing memory benefit, and benchmark tests showing CPU parity Signed-off-by: Joshua Marantz --- source/common/stats/thread_local_store.cc | 14 +++-- source/common/stats/thread_local_store.h | 1 + test/common/stats/BUILD | 1 + test/common/stats/stat_test_utility.cc | 9 ++- test/common/stats/stat_test_utility.h | 3 +- .../stats/thread_local_store_speed_test.cc | 33 +++++++++- test/common/stats/thread_local_store_test.cc | 60 ++++++++++++++++--- tools/spelling/spelling_dictionary.txt | 1 + 8 files changed, 103 insertions(+), 19 deletions(-) diff --git a/source/common/stats/thread_local_store.cc b/source/common/stats/thread_local_store.cc index 85b56408be9a7..c6c5f729a5a00 100644 --- a/source/common/stats/thread_local_store.cc +++ b/source/common/stats/thread_local_store.cc @@ -101,6 +101,11 @@ void ThreadLocalStoreImpl::removeRejectedStats(StatMapClass& map, StatListClass& } } +bool ThreadLocalStoreImpl::fastRejects(StatName stat_name) const { + return stats_matcher_->rejectsAll() || + (!stats_matcher_->hasStringMatchers() && stats_matcher_->rejects(stat_name)); +} + bool ThreadLocalStoreImpl::rejects(StatName stat_name) const { ASSERT(!stats_matcher_->acceptsAll()); @@ -448,10 +453,6 @@ bool ThreadLocalStoreImpl::checkAndRememberRejection(StatName name, rejected_name = &(*iter); } else { if (rejects(name)) { - if (!stats_matcher_->hasStringMatchers()) { - // Skip recording of cheaply-matched rejections in the caches. - return true; - } auto insertion = central_rejected_stats.insert(StatNameStorage(name, symbolTable())); const StatNameStorage& rejected_name_ref = *(insertion.first); rejected_name = &rejected_name_ref; @@ -474,8 +475,9 @@ StatType& ThreadLocalStoreImpl::ScopeImpl::safeMakeStat( 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()) { + if (parent_.fastRejects(full_stat_name) || + (tls_rejected_stats != nullptr && + tls_rejected_stats->find(full_stat_name) != tls_rejected_stats->end())) { return null_stat; } diff --git a/source/common/stats/thread_local_store.h b/source/common/stats/thread_local_store.h index ecc359841bce0..ebc719be6e886 100644 --- a/source/common/stats/thread_local_store.h +++ b/source/common/stats/thread_local_store.h @@ -477,6 +477,7 @@ class ThreadLocalStoreImpl : Logger::Loggable, public StoreRo void releaseScopeCrossThread(ScopeImpl* scope); void mergeInternal(PostMergeCb merge_cb); bool rejects(StatName name) const; + bool fastRejects(StatName name) const; bool rejectsAll() const { return stats_matcher_->rejectsAll(); } template void removeRejectedStats(StatMapClass& map, StatListClass& list); 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..63e6da9c23a10 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 diff --git a/test/common/stats/thread_local_store_speed_test.cc b/test/common/stats/thread_local_store_speed_test.cc index 8ce08a84d052e..52fa183bb9067 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_; @@ -102,5 +109,29 @@ static void BM_StatsWithTls(benchmark::State& state) { } 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) { + 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) { + 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 3dcf6ae4af60b..22600ace662a2 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() { + 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) { @@ -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 @@ -927,10 +946,35 @@ 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(); + EXPECT_MEMORY_EQ(mem_consumed, 240); + EXPECT_MEMORY_LE(mem_consumed, 300); +} + +// 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(); + 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 @@ -1184,7 +1228,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_); } @@ -1193,7 +1237,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/tools/spelling/spelling_dictionary.txt b/tools/spelling/spelling_dictionary.txt index 6291ed0b42e91..2a4b4b36ae742 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 From b6a4edb611f5e1786e3ec359b17c4b9bfa007668 Mon Sep 17 00:00:00 2001 From: Joshua Marantz Date: Thu, 17 Jun 2021 21:04:05 -0400 Subject: [PATCH 06/16] add missing arg. Signed-off-by: Joshua Marantz --- test/common/stats/symbol_table_impl_test.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/common/stats/symbol_table_impl_test.cc b/test/common/stats/symbol_table_impl_test.cc index fb828fd7ee9f9..a54394975983a 100644 --- a/test/common/stats/symbol_table_impl_test.cc +++ b/test/common/stats/symbol_table_impl_test.cc @@ -723,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(); }; From c7a114796f99f7dbd6e3cb200df3791bda259f13 Mon Sep 17 00:00:00 2001 From: Joshua Marantz Date: Thu, 17 Jun 2021 22:42:11 -0400 Subject: [PATCH 07/16] NOLINT for clang-tidy nits Signed-off-by: Joshua Marantz --- test/common/stats/thread_local_store_speed_test.cc | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/test/common/stats/thread_local_store_speed_test.cc b/test/common/stats/thread_local_store_speed_test.cc index 52fa183bb9067..620af46c698ab 100644 --- a/test/common/stats/thread_local_store_speed_test.cc +++ b/test/common/stats/thread_local_store_speed_test.cc @@ -89,7 +89,7 @@ class ThreadLocalStorePerf { static void BM_StatsNoTls(benchmark::State& state) { Envoy::ThreadLocalStorePerf context; - for (auto _ : state) { + for (auto _ : state) { // NOLINT context.accessCounters(); } } @@ -103,7 +103,7 @@ static void BM_StatsWithTls(benchmark::State& state) { Envoy::ThreadLocalStorePerf context; context.initThreading(); - for (auto _ : state) { + for (auto _ : state) { // NOLINT context.accessCounters(); } } @@ -115,7 +115,7 @@ static void BM_StatsWithTlsAndRejectionsWithDot(benchmark::State& state) { context.initThreading(); context.initPrefixRejections("cluster."); - for (auto _ : state) { + for (auto _ : state) { // NOLINT context.accessCounters(); } } @@ -127,7 +127,7 @@ static void BM_StatsWithTlsAndRejectionsWithoutDot(benchmark::State& state) { context.initThreading(); context.initPrefixRejections("cluster"); - for (auto _ : state) { + for (auto _ : state) { // NOLINT context.accessCounters(); } } From 2dd3d5747e3f4ba561f314c97bd268e00d7d78f7 Mon Sep 17 00:00:00 2001 From: Joshua Marantz Date: Fri, 18 Jun 2021 13:38:05 -0400 Subject: [PATCH 08/16] short-circuit more code that doesn't need to run for stats that are rejected by the fast path. Signed-off-by: Joshua Marantz --- envoy/stats/stats_matcher.h | 24 +++++++ source/common/stats/stats_matcher_impl.cc | 68 ++++++++++++++++++-- source/common/stats/stats_matcher_impl.h | 4 ++ source/common/stats/thread_local_store.cc | 39 ++++++----- source/common/stats/thread_local_store.h | 2 +- test/common/stats/stat_test_utility.h | 4 +- test/common/stats/thread_local_store_test.cc | 25 ++++--- test/mocks/stats/mocks.h | 2 + 8 files changed, 134 insertions(+), 34 deletions(-) diff --git a/envoy/stats/stats_matcher.h b/envoy/stats/stats_matcher.h index b063300059b80..eb34fe23d7136 100644 --- a/envoy/stats/stats_matcher.h +++ b/envoy/stats/stats_matcher.h @@ -17,11 +17,35 @@ class StatsMatcher { /** * Take a metric name and report whether or not it should be instantiated. + * The may need to convert the StatName to a string. + * * @param 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 determine whether it can be rejected based + * purely on the StatName. A return of 'false' means we may need to check + * slowRejects as well. + * + * @param the name of a Stats::Metric. + * @return bool true if that stat should not be instantiated, or whether we + * need to check slowRejects. + */ + virtual bool fastRejects(StatName name) const PURE; + + /** + * Takes a metric name and quickly determine whether it can be rejected based + * purely on the StatName. A return of 'false' means we may need to check + * slowRejects as well. + * + * @param the name of a Stats::Metric. + * @return bool true if that stat should not be instantiated, or whether we + * need to check slowRejects. + */ + virtual bool slowRejects(StatName name) const PURE; + /** * Helps determine whether the matcher needs to be called. This can be used * to short-circuit elaboration of stats names. diff --git a/source/common/stats/stats_matcher_impl.cc b/source/common/stats/stats_matcher_impl.cc index b10943cfb8939..0b8c86c20692d 100644 --- a/source/common/stats/stats_matcher_impl.cc +++ b/source/common/stats/stats_matcher_impl.cc @@ -55,7 +55,49 @@ void StatsMatcherImpl::optimizeLastMatcher() { } bool StatsMatcherImpl::rejects(StatName stat_name) const { + if (rejectsAll()) { + return true; + } + + bool match = fastRejectMatch(stat_name) || slowRejectMatch(stat_name); + + // 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_ == match; +} + +bool StatsMatcherImpl::fastRejects(StatName stat_name) const { + if (rejectsAll()) { + return true; + } + + // We can short-circuit the slow matchers only if they are empty, or if + // we are in inclusive-mode and we find a match. + if (is_inclusive_ || matchers_.empty()) { + return fastRejectMatch(stat_name) == is_inclusive_; + } + + // If there are both prefix and string matchers, and we are in exclusive + // mode, then it isn't helpful to run the prefix matchers early, so + // we return false. This forces the caller to eventually then call + // slowRejects(), where we'll handle this case below. + return false; +} + +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(StatName stat_name) const { + bool match = slowRejectMatch(stat_name); + // is_inclusive_ | match | return // ---------------+-------+-------- // T | T | T Default-inclusive and matching an (exclusion) matcher, deny. @@ -65,15 +107,27 @@ bool StatsMatcherImpl::rejects(StatName stat_name) const { // // This is an XNOR, which can be evaluated by checking for equality. - bool match = std::any_of(prefixes_.begin(), prefixes_.end(), - [stat_name](StatName prefix) { return stat_name.startsWith(prefix); }); - if (!match && !matchers_.empty()) { - std::string name = symbol_table_->toString(stat_name); - match = std::any_of(matchers_.begin(), matchers_.end(), - [&name](auto& matcher) { return matcher.match(name); }); + if (is_inclusive_ || match || prefixes_.empty()) { + return match == is_inclusive_; } - return is_inclusive_ == match; + // For exclusive-mode, we may need to re-test the prefixes, as we had to toss + // out that case in fastRejects(). This seems really annoying, but OTOH I + // don't understand the use-case for exclusive-mode, so it's probably not + // worth trying to optimize for it, which would involve carrying through the + // fast-reject match state, which would be a confusing interface. It would + // also be possible to abstradct this into a MatchState object, but that would + // be very cumbersome to mock for use in thread_local_store_test.cc. + return !fastRejectMatch(stat_name); +} + +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 8d69d93dad205..833fb330e6bd3 100644 --- a/source/common/stats/stats_matcher_impl.h +++ b/source/common/stats/stats_matcher_impl.h @@ -28,6 +28,8 @@ class StatsMatcherImpl : public StatsMatcher { // StatsMatcher bool rejects(StatName name) const override; + bool fastRejects(StatName name) const override; + bool slowRejects(StatName name) const override; bool acceptsAll() const override { return is_inclusive_ && matchers_.empty() && prefixes_.empty(); } @@ -38,6 +40,8 @@ class StatsMatcherImpl : public StatsMatcher { 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. diff --git a/source/common/stats/thread_local_store.cc b/source/common/stats/thread_local_store.cc index c6c5f729a5a00..2af400be1cb19 100644 --- a/source/common/stats/thread_local_store.cc +++ b/source/common/stats/thread_local_store.cc @@ -89,7 +89,7 @@ template void ThreadLocalStoreImpl::removeRejectedStats(StatMapClass& map, StatListClass& list) { StatNameVec remove_list; for (auto& stat : map) { - if (rejects(stat.first)) { + if (fastRejects(stat.first) || slowRejects(stat.first)) { remove_list.push_back(stat.first); } } @@ -102,21 +102,11 @@ void ThreadLocalStoreImpl::removeRejectedStats(StatMapClass& map, StatListClass& } bool ThreadLocalStoreImpl::fastRejects(StatName stat_name) const { - return stats_matcher_->rejectsAll() || - (!stats_matcher_->hasStringMatchers() && stats_matcher_->rejects(stat_name)); + return stats_matcher_->fastRejects(stat_name); } -bool ThreadLocalStoreImpl::rejects(StatName stat_name) const { - ASSERT(!stats_matcher_->acceptsAll()); - - // 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(stat_name); +bool ThreadLocalStoreImpl::slowRejects(StatName stat_name) const { + return stats_matcher_->slowRejects(stat_name); } std::vector ThreadLocalStoreImpl::counters() const { @@ -452,7 +442,7 @@ bool ThreadLocalStoreImpl::checkAndRememberRejection(StatName name, if (iter != central_rejected_stats.end()) { rejected_name = &(*iter); } else { - if (rejects(name)) { + if (slowRejects(name)) { auto insertion = central_rejected_stats.insert(StatNameStorage(name, symbolTable())); const StatNameStorage& rejected_name_ref = *(insertion.first); rejected_name = &rejected_name_ref; @@ -475,8 +465,7 @@ StatType& ThreadLocalStoreImpl::ScopeImpl::safeMakeStat( StatNameStorageSet& central_rejected_stats, MakeStatFn make_stat, StatRefMap* tls_cache, StatNameHashSet* tls_rejected_stats, StatType& null_stat) { - if (parent_.fastRejects(full_stat_name) || - (tls_rejected_stats != nullptr && + if ((tls_rejected_stats != nullptr && tls_rejected_stats->find(full_stat_name) != tls_rejected_stats->end())) { return null_stat; } @@ -551,6 +540,10 @@ Counter& ThreadLocalStoreImpl::ScopeImpl::counterFromStatNameWithTags( TagUtility::TagStatNameJoiner joiner(prefix_.statName(), name, stat_name_tags, symbolTable()); Stats::StatName final_stat_name = joiner.nameWithTags(); + if (parent_.fastRejects(final_stat_name)) { + 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; @@ -605,6 +598,10 @@ Gauge& ThreadLocalStoreImpl::ScopeImpl::gaugeFromStatNameWithTags( TagUtility::TagStatNameJoiner joiner(prefix_.statName(), name, stat_name_tags, symbolTable()); StatName final_stat_name = joiner.nameWithTags(); + if (parent_.fastRejects(final_stat_name)) { + return parent_.null_gauge_; + } + StatRefMap* tls_cache = nullptr; StatNameHashSet* tls_rejected_stats = nullptr; if (!parent_.shutting_down_ && parent_.tls_cache_) { @@ -643,6 +640,10 @@ Histogram& ThreadLocalStoreImpl::ScopeImpl::histogramFromStatNameWithTags( TagUtility::TagStatNameJoiner joiner(prefix_.statName(), name, stat_name_tags, symbolTable()); StatName final_stat_name = joiner.nameWithTags(); + if (parent_.fastRejects(final_stat_name)) { + return parent_.null_histogram_; + } + StatNameHashMap* tls_cache = nullptr; StatNameHashSet* tls_rejected_stats = nullptr; if (!parent_.shutting_down_ && parent_.tls_cache_) { @@ -716,6 +717,10 @@ TextReadout& ThreadLocalStoreImpl::ScopeImpl::textReadoutFromStatNameWithTags( TagUtility::TagStatNameJoiner joiner(prefix_.statName(), name, stat_name_tags, symbolTable()); Stats::StatName final_stat_name = joiner.nameWithTags(); + if (parent_.fastRejects(final_stat_name)) { + 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; diff --git a/source/common/stats/thread_local_store.h b/source/common/stats/thread_local_store.h index ebc719be6e886..d1039f3afe4c8 100644 --- a/source/common/stats/thread_local_store.h +++ b/source/common/stats/thread_local_store.h @@ -476,7 +476,7 @@ class ThreadLocalStoreImpl : Logger::Loggable, public StoreRo void clearHistogramsFromCaches(); void releaseScopeCrossThread(ScopeImpl* scope); void mergeInternal(PostMergeCb merge_cb); - bool rejects(StatName name) const; + bool slowRejects(StatName name) const; bool fastRejects(StatName name) const; bool rejectsAll() const { return stats_matcher_->rejectsAll(); } template diff --git a/test/common/stats/stat_test_utility.h b/test/common/stats/stat_test_utility.h index 63e6da9c23a10..ea9e5b15c3776 100644 --- a/test/common/stats/stat_test_utility.h +++ b/test/common/stats/stat_test_utility.h @@ -186,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/thread_local_store_test.cc b/test/common/stats/thread_local_store_test.cc index 22600ace662a2..d0c2bedb01b01 100644 --- a/test/common/stats/thread_local_store_test.cc +++ b/test/common/stats/thread_local_store_test.cc @@ -744,7 +744,7 @@ class StatsMatcherTLSTest : public StatsThreadLocalStoreTest { public: envoy::config::metrics::v3::StatsConfig stats_config_; - ~StatsMatcherTLSTest() { + ~StatsMatcherTLSTest() override { tls_.shutdownGlobalThreading(); store_->shutdownThreading(); } @@ -959,8 +959,8 @@ TEST_F(StatsMatcherTLSTest, RejectPrefixDot) { "cluster."); // Prefix match can be executed symbolically. store_->setStatsMatcher(std::make_unique(stats_config_, symbol_table_)); uint64_t mem_consumed = memoryConsumedAddingClusterStats(); - EXPECT_MEMORY_EQ(mem_consumed, 240); - EXPECT_MEMORY_LE(mem_consumed, 300); + EXPECT_MEMORY_EQ(mem_consumed, 0); + EXPECT_MEMORY_LE(mem_consumed, 0); } // Repeating the same test but retaining the dot means that the StatsMatcher @@ -1011,11 +1011,16 @@ class RememberStatsMatcherTest : public testing::TestWithParam { store_.setStatsMatcher(std::move(matcher_ptr)); StatNamePool pool(symbol_table_); - EXPECT_CALL(*matcher, rejects(pool.add("scope.reject"))).WillOnce(Return(true)); - EXPECT_CALL(*matcher, rejects(pool.add("scope.ok"))).WillOnce(Return(false)); - for (int j = 0; j < 5; ++j) { + EXPECT_CALL(*matcher, fastRejects(pool.add("scope.reject"))).WillOnce(Return(false)); + if (j == 0) { + EXPECT_CALL(*matcher, slowRejects(pool.add("scope.reject"))).WillOnce(Return(true)); + } EXPECT_EQ("", lookup_stat("reject")); + EXPECT_CALL(*matcher, fastRejects(pool.add("scope.ok"))).WillOnce(Return(false)); + if (j == 0) { + EXPECT_CALL(*matcher, slowRejects(pool.add("scope.ok"))).WillOnce(Return(false)); + } EXPECT_EQ("scope.ok", lookup_stat("ok")); } } @@ -1030,8 +1035,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")); } } @@ -1043,9 +1050,11 @@ class RememberStatsMatcherTest : public testing::TestWithParam { matcher->accepts_all_ = true; StatsMatcherPtr matcher_ptr(matcher); store_.setStatsMatcher(std::move(matcher_ptr)); + StatNamePool pool(symbol_table_); 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(false)); + // Note: zero calls to slowReject() are made, as accept-all should short-circuit. EXPECT_EQ("scope.ok", lookup_stat("ok")); } } diff --git a/test/mocks/stats/mocks.h b/test/mocks/stats/mocks.h index 13debb87f066c..58ddc2f1a64f4 100644 --- a/test/mocks/stats/mocks.h +++ b/test/mocks/stats/mocks.h @@ -349,6 +349,8 @@ class MockStatsMatcher : public StatsMatcher { MockStatsMatcher(); ~MockStatsMatcher() override; MOCK_METHOD(bool, rejects, (StatName name), (const)); + MOCK_METHOD(bool, fastRejects, (StatName name), (const)); + MOCK_METHOD(bool, slowRejects, (StatName name), (const)); bool acceptsAll() const override { return accepts_all_; } bool rejectsAll() const override { return rejects_all_; } bool hasStringMatchers() const override { return true; } From 8add54bbdcc17afbeb7a0077bc704772ae115ab3 Mon Sep 17 00:00:00 2001 From: Joshua Marantz Date: Fri, 18 Jun 2021 14:40:54 -0400 Subject: [PATCH 09/16] Fix spelling and simplify the comment. Signed-off-by: Joshua Marantz --- source/common/stats/stats_matcher_impl.cc | 10 +++------- 1 file changed, 3 insertions(+), 7 deletions(-) diff --git a/source/common/stats/stats_matcher_impl.cc b/source/common/stats/stats_matcher_impl.cc index 0b8c86c20692d..8a5af7442f1b3 100644 --- a/source/common/stats/stats_matcher_impl.cc +++ b/source/common/stats/stats_matcher_impl.cc @@ -111,13 +111,9 @@ bool StatsMatcherImpl::slowRejects(StatName stat_name) const { return match == is_inclusive_; } - // For exclusive-mode, we may need to re-test the prefixes, as we had to toss - // out that case in fastRejects(). This seems really annoying, but OTOH I - // don't understand the use-case for exclusive-mode, so it's probably not - // worth trying to optimize for it, which would involve carrying through the - // fast-reject match state, which would be a confusing interface. It would - // also be possible to abstradct this into a MatchState object, but that would - // be very cumbersome to mock for use in thread_local_store_test.cc. + // In exclusive-mode with both prefixes and matches, we must + // skip the fast-path prefix since we invert the results, so we can't + // definitively declare a rejection. So we must check them now. return !fastRejectMatch(stat_name); } From bfd62419f0194359b8222c7c6d4c51e41a781627 Mon Sep 17 00:00:00 2001 From: Joshua Marantz Date: Sat, 19 Jun 2021 11:42:53 -0400 Subject: [PATCH 10/16] cleanup and add tests. Signed-off-by: Joshua Marantz --- source/common/stats/stats_matcher_impl.cc | 18 ------------ source/common/stats/stats_matcher_impl.h | 2 +- test/common/stats/stats_matcher_impl_test.cc | 30 ++++++++++++++++++++ 3 files changed, 31 insertions(+), 19 deletions(-) diff --git a/source/common/stats/stats_matcher_impl.cc b/source/common/stats/stats_matcher_impl.cc index 8a5af7442f1b3..67e95f80b9201 100644 --- a/source/common/stats/stats_matcher_impl.cc +++ b/source/common/stats/stats_matcher_impl.cc @@ -54,24 +54,6 @@ void StatsMatcherImpl::optimizeLastMatcher() { } } -bool StatsMatcherImpl::rejects(StatName stat_name) const { - if (rejectsAll()) { - return true; - } - - bool match = fastRejectMatch(stat_name) || slowRejectMatch(stat_name); - - // 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_ == match; -} - bool StatsMatcherImpl::fastRejects(StatName stat_name) const { if (rejectsAll()) { return true; diff --git a/source/common/stats/stats_matcher_impl.h b/source/common/stats/stats_matcher_impl.h index 833fb330e6bd3..c953927548723 100644 --- a/source/common/stats/stats_matcher_impl.h +++ b/source/common/stats/stats_matcher_impl.h @@ -27,7 +27,7 @@ class StatsMatcherImpl : public StatsMatcher { StatsMatcherImpl() = default; // StatsMatcher - bool rejects(StatName name) const override; + bool rejects(StatName name) const override { return fastRejects(name) || slowRejects(name); } bool fastRejects(StatName name) const override; bool slowRejects(StatName name) const override; bool acceptsAll() const override { diff --git a/test/common/stats/stats_matcher_impl_test.cc b/test/common/stats/stats_matcher_impl_test.cc index 005345b30aa5f..36f3c9c712763 100644 --- a/test/common/stats/stats_matcher_impl_test.cc +++ b/test/common/stats/stats_matcher_impl_test.cc @@ -322,5 +322,35 @@ 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(); + EXPECT_TRUE(stats_matcher_impl_->hasStringMatchers()); + 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(); + EXPECT_TRUE(stats_matcher_impl_->hasStringMatchers()); + 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 From b7382221948a722b558de7eb4f7920c32d1c71af Mon Sep 17 00:00:00 2001 From: Joshua Marantz Date: Sat, 19 Jun 2021 16:25:50 -0400 Subject: [PATCH 11/16] Remove StatMatcher::hasStringMatchers which is no longer needed. Signed-off-by: Joshua Marantz --- envoy/stats/stats_matcher.h | 4 --- source/common/stats/stats_matcher_impl.h | 1 - test/common/stats/stats_matcher_impl_test.cc | 26 -------------------- test/mocks/stats/mocks.h | 1 - 4 files changed, 32 deletions(-) diff --git a/envoy/stats/stats_matcher.h b/envoy/stats/stats_matcher.h index eb34fe23d7136..221b7b8ed23c9 100644 --- a/envoy/stats/stats_matcher.h +++ b/envoy/stats/stats_matcher.h @@ -65,10 +65,6 @@ class StatsMatcher { * rejectsAll() returns false, but rejects() is always true. */ virtual bool rejectsAll() const PURE; - - // Determines whether conversion from StatName to string may be necessary to - // run a match against this set. - virtual bool hasStringMatchers() const PURE; }; using StatsMatcherPtr = std::unique_ptr; diff --git a/source/common/stats/stats_matcher_impl.h b/source/common/stats/stats_matcher_impl.h index c953927548723..5de4d637535c4 100644 --- a/source/common/stats/stats_matcher_impl.h +++ b/source/common/stats/stats_matcher_impl.h @@ -36,7 +36,6 @@ class StatsMatcherImpl : public StatsMatcher { bool rejectsAll() const override { return !is_inclusive_ && matchers_.empty() && prefixes_.empty(); } - bool hasStringMatchers() const override { return !matchers_.empty(); } private: void optimizeLastMatcher(); diff --git a/test/common/stats/stats_matcher_impl_test.cc b/test/common/stats/stats_matcher_impl_test.cc index 36f3c9c712763..6848036a4b777 100644 --- a/test/common/stats/stats_matcher_impl_test.cc +++ b/test/common/stats/stats_matcher_impl_test.cc @@ -48,7 +48,6 @@ class StatsMatcherTest : public testing::Test { TEST_F(StatsMatcherTest, CheckDefault) { // With no set fields, everything should be allowed through. initMatcher(); - EXPECT_FALSE(stats_matcher_impl_->hasStringMatchers()); expectAccepted({"foo", "bar", "foo.bar", "foo.bar.baz", "foobarbaz"}); EXPECT_TRUE(stats_matcher_impl_->acceptsAll()); EXPECT_FALSE(stats_matcher_impl_->rejectsAll()); @@ -60,7 +59,6 @@ TEST_F(StatsMatcherTest, CheckRejectAll) { // With reject_all, nothing should be allowed through. rejectAll(true); initMatcher(); - EXPECT_FALSE(stats_matcher_impl_->hasStringMatchers()); expectDenied({"foo", "bar", "foo.bar", "foo.bar.baz", "foobarbaz"}); EXPECT_FALSE(stats_matcher_impl_->acceptsAll()); EXPECT_TRUE(stats_matcher_impl_->rejectsAll()); @@ -70,7 +68,6 @@ TEST_F(StatsMatcherTest, CheckNotRejectAll) { // With !reject_all, everything should be allowed through. rejectAll(false); initMatcher(); - EXPECT_FALSE(stats_matcher_impl_->hasStringMatchers()); expectAccepted({"foo", "bar", "foo.bar", "foo.bar.baz", "foobarbaz"}); EXPECT_TRUE(stats_matcher_impl_->acceptsAll()); EXPECT_FALSE(stats_matcher_impl_->rejectsAll()); @@ -79,7 +76,6 @@ TEST_F(StatsMatcherTest, CheckNotRejectAll) { TEST_F(StatsMatcherTest, CheckIncludeAll) { inclusionList()->MergeFrom(TestUtility::createRegexMatcher(".*")); initMatcher(); - EXPECT_TRUE(stats_matcher_impl_->hasStringMatchers()); expectAccepted({"foo", "bar", "foo.bar", "foo.bar.baz"}); // It really does accept all, but the impl doesn't know it. EXPECT_FALSE(stats_matcher_impl_->acceptsAll()); @@ -89,7 +85,6 @@ TEST_F(StatsMatcherTest, CheckIncludeAll) { TEST_F(StatsMatcherTest, CheckExcludeAll) { exclusionList()->MergeFrom(TestUtility::createRegexMatcher(".*")); initMatcher(); - EXPECT_TRUE(stats_matcher_impl_->hasStringMatchers()); expectDenied({"foo", "bar", "foo.bar", "foo.bar.baz"}); EXPECT_FALSE(stats_matcher_impl_->acceptsAll()); EXPECT_FALSE(stats_matcher_impl_->rejectsAll()); @@ -100,7 +95,6 @@ TEST_F(StatsMatcherTest, CheckExcludeAll) { TEST_F(StatsMatcherTest, CheckIncludeExact) { inclusionList()->set_exact("abc"); initMatcher(); - EXPECT_TRUE(stats_matcher_impl_->hasStringMatchers()); expectAccepted({"abc"}); expectDenied({"abcd", "abc.d", "d.abc", "dabc", "ab", "ac", "abcc", "Abc", "aBc", "abC", "ABC"}); EXPECT_FALSE(stats_matcher_impl_->acceptsAll()); @@ -110,7 +104,6 @@ TEST_F(StatsMatcherTest, CheckIncludeExact) { TEST_F(StatsMatcherTest, CheckExcludeExact) { exclusionList()->set_exact("abc"); initMatcher(); - EXPECT_TRUE(stats_matcher_impl_->hasStringMatchers()); expectAccepted( {"abcd", "abc.d", "d.abc", "dabc", "ab", "ac", "abcc", "Abc", "aBc", "abC", "ABC"}); expectDenied({"abc"}); @@ -123,7 +116,6 @@ TEST_F(StatsMatcherTest, CheckExcludeExact) { TEST_F(StatsMatcherTest, CheckIncludePrefix) { inclusionList()->set_prefix("abc"); initMatcher(); - EXPECT_TRUE(stats_matcher_impl_->hasStringMatchers()); expectAccepted({"abc", "abc.foo", "abcfoo"}); expectDenied({"ABC", "ABC.foo", "ABCfoo", "foo", "abb", "a.b.c", "_abc", "foo.abc", "fooabc"}); EXPECT_FALSE(stats_matcher_impl_->acceptsAll()); @@ -133,7 +125,6 @@ TEST_F(StatsMatcherTest, CheckIncludePrefix) { TEST_F(StatsMatcherTest, CheckIncludePrefixDot) { inclusionList()->set_prefix("abc."); initMatcher(); - EXPECT_FALSE(stats_matcher_impl_->hasStringMatchers()); expectAccepted({"abc", "abc.foo"}); expectDenied( {"abcfoo", "ABC", "ABC.foo", "ABCfoo", "foo", "abb", "a.b.c", "_abc", "foo.abc", "fooabc"}); @@ -144,7 +135,6 @@ TEST_F(StatsMatcherTest, CheckIncludePrefixDot) { TEST_F(StatsMatcherTest, CheckExcludePrefix) { exclusionList()->set_prefix("abc"); initMatcher(); - EXPECT_TRUE(stats_matcher_impl_->hasStringMatchers()); expectAccepted({"ABC", "ABC.foo", "ABCfoo", "foo", "abb", "a.b.c", "_abc", "foo.abc", "fooabc"}); expectDenied({"abc", "abc.foo", "abcfoo"}); EXPECT_FALSE(stats_matcher_impl_->acceptsAll()); @@ -156,7 +146,6 @@ TEST_F(StatsMatcherTest, CheckExcludePrefix) { TEST_F(StatsMatcherTest, CheckIncludeSuffix) { inclusionList()->set_suffix("abc"); initMatcher(); - EXPECT_TRUE(stats_matcher_impl_->hasStringMatchers()); expectAccepted({"abc", "foo.abc", "fooabc"}); expectDenied({"ABC", "foo.ABC", "fooABC", "foo", "abb", "a.b.c", "abc_", "abc.foo", "abcfoo"}); EXPECT_FALSE(stats_matcher_impl_->acceptsAll()); @@ -166,7 +155,6 @@ TEST_F(StatsMatcherTest, CheckIncludeSuffix) { TEST_F(StatsMatcherTest, CheckExcludeSuffix) { exclusionList()->set_suffix("abc"); initMatcher(); - EXPECT_TRUE(stats_matcher_impl_->hasStringMatchers()); expectAccepted({"ABC", "foo.ABC", "fooABC", "foo", "abb", "a.b.c", "abc_", "abc.foo", "abcfoo"}); expectDenied({"abc", "foo.abc", "fooabc"}); EXPECT_FALSE(stats_matcher_impl_->acceptsAll()); @@ -178,7 +166,6 @@ TEST_F(StatsMatcherTest, CheckExcludeSuffix) { TEST_F(StatsMatcherTest, CheckIncludeRegex) { inclusionList()->MergeFrom(TestUtility::createRegexMatcher(".*envoy.*")); initMatcher(); - EXPECT_TRUE(stats_matcher_impl_->hasStringMatchers()); expectAccepted({"envoy.matchers.requests", "stats.envoy.2xx", "regex.envoy.matchers"}); expectDenied({"foo", "Envoy", "EnvoyProxy"}); EXPECT_FALSE(stats_matcher_impl_->acceptsAll()); @@ -188,7 +175,6 @@ TEST_F(StatsMatcherTest, CheckIncludeRegex) { TEST_F(StatsMatcherTest, CheckExcludeRegex) { exclusionList()->MergeFrom(TestUtility::createRegexMatcher(".*envoy.*")); initMatcher(); - EXPECT_TRUE(stats_matcher_impl_->hasStringMatchers()); expectAccepted({"foo", "Envoy", "EnvoyProxy"}); expectDenied({"envoy.matchers.requests", "stats.envoy.2xx", "regex.envoy.matchers"}); EXPECT_FALSE(stats_matcher_impl_->acceptsAll()); @@ -201,7 +187,6 @@ TEST_F(StatsMatcherTest, CheckMultipleIncludeExact) { inclusionList()->set_exact("foo"); inclusionList()->set_exact("bar"); initMatcher(); - EXPECT_TRUE(stats_matcher_impl_->hasStringMatchers()); expectAccepted({"foo", "bar"}); expectDenied({"foobar", "barfoo", "fo", "ba", "foo.bar"}); EXPECT_FALSE(stats_matcher_impl_->acceptsAll()); @@ -212,7 +197,6 @@ TEST_F(StatsMatcherTest, CheckMultipleExcludeExact) { exclusionList()->set_exact("foo"); exclusionList()->set_exact("bar"); initMatcher(); - EXPECT_TRUE(stats_matcher_impl_->hasStringMatchers()); expectAccepted({"foobar", "barfoo", "fo", "ba", "foo.bar"}); expectDenied({"foo", "bar"}); EXPECT_FALSE(stats_matcher_impl_->acceptsAll()); @@ -225,7 +209,6 @@ TEST_F(StatsMatcherTest, CheckMultipleIncludePrefix) { inclusionList()->set_prefix("foo"); inclusionList()->set_prefix("bar"); initMatcher(); - EXPECT_TRUE(stats_matcher_impl_->hasStringMatchers()); expectAccepted({"foo", "foo.abc", "bar", "bar.abc"}); expectDenied({".foo", "abc.foo", "BAR", "_bar"}); EXPECT_FALSE(stats_matcher_impl_->acceptsAll()); @@ -236,7 +219,6 @@ TEST_F(StatsMatcherTest, CheckMultipleExcludePrefix) { exclusionList()->set_prefix("foo"); exclusionList()->set_prefix("bar"); initMatcher(); - EXPECT_TRUE(stats_matcher_impl_->hasStringMatchers()); expectAccepted({".foo", "abc.foo", "BAR", "_bar"}); expectDenied({"foo", "foo.abc", "bar", "bar.abc"}); EXPECT_FALSE(stats_matcher_impl_->acceptsAll()); @@ -249,7 +231,6 @@ TEST_F(StatsMatcherTest, CheckMultipleIncludeSuffix) { inclusionList()->set_suffix("spam"); inclusionList()->set_suffix("eggs"); initMatcher(); - EXPECT_TRUE(stats_matcher_impl_->hasStringMatchers()); expectAccepted( {"requests.for.spam", "requests.for.eggs", "spam", "eggs", "cannedspam", "fresheggs"}); expectDenied({"Spam", "EGGS", "spam_", "eggs_"}); @@ -261,7 +242,6 @@ TEST_F(StatsMatcherTest, CheckMultipleExcludeSuffix) { exclusionList()->set_suffix("spam"); exclusionList()->set_suffix("eggs"); initMatcher(); - EXPECT_TRUE(stats_matcher_impl_->hasStringMatchers()); expectAccepted({"Spam", "EGGS", "spam_", "eggs_"}); expectDenied( {"requests.for.spam", "requests.for.eggs", "spam", "eggs", "cannedspam", "fresheggs"}); @@ -275,7 +255,6 @@ TEST_F(StatsMatcherTest, CheckMultipleIncludeRegex) { inclusionList()->MergeFrom(TestUtility::createRegexMatcher(".*envoy.*")); inclusionList()->MergeFrom(TestUtility::createRegexMatcher(".*absl.*")); initMatcher(); - EXPECT_TRUE(stats_matcher_impl_->hasStringMatchers()); expectAccepted({"envoy.matchers.requests", "stats.absl.2xx", "absl.envoy.matchers"}); expectDenied({"Abseil", "EnvoyProxy"}); EXPECT_FALSE(stats_matcher_impl_->acceptsAll()); @@ -286,7 +265,6 @@ TEST_F(StatsMatcherTest, CheckMultipleExcludeRegex) { exclusionList()->MergeFrom(TestUtility::createRegexMatcher(".*envoy.*")); exclusionList()->MergeFrom(TestUtility::createRegexMatcher(".*absl.*")); initMatcher(); - EXPECT_TRUE(stats_matcher_impl_->hasStringMatchers()); expectAccepted({"Abseil", "EnvoyProxy"}); expectDenied({"envoy.matchers.requests", "stats.absl.2xx", "absl.envoy.matchers"}); EXPECT_FALSE(stats_matcher_impl_->acceptsAll()); @@ -303,7 +281,6 @@ TEST_F(StatsMatcherTest, CheckMultipleAssortedInclusionMatchers) { inclusionList()->set_suffix("requests"); inclusionList()->set_exact("regex"); initMatcher(); - EXPECT_TRUE(stats_matcher_impl_->hasStringMatchers()); expectAccepted({"envoy.matchers.requests", "requests.for.envoy", "envoyrequests", "regex"}); expectDenied({"requestsEnvoy", "EnvoyProxy", "foo", "regex_etc"}); EXPECT_FALSE(stats_matcher_impl_->acceptsAll()); @@ -315,7 +292,6 @@ TEST_F(StatsMatcherTest, CheckMultipleAssortedExclusionMatchers) { exclusionList()->set_suffix("requests"); exclusionList()->set_exact("regex"); initMatcher(); - EXPECT_TRUE(stats_matcher_impl_->hasStringMatchers()); expectAccepted({"requestsEnvoy", "EnvoyProxy", "foo", "regex_etc"}); expectDenied({"envoy.matchers.requests", "requests.for.envoy", "envoyrequests", "regex"}); EXPECT_FALSE(stats_matcher_impl_->acceptsAll()); @@ -328,7 +304,6 @@ TEST_F(StatsMatcherTest, CheckMultipleAssortedInclusionMatchersWithPrefix) { inclusionList()->set_exact("regex"); inclusionList()->set_prefix("prefix."); initMatcher(); - EXPECT_TRUE(stats_matcher_impl_->hasStringMatchers()); expectAccepted({"envoy.matchers.requests", "requests.for.envoy", "envoyrequests", "regex", "prefix", "prefix.foo"}); expectAccepted({"prefix.envoy.matchers.requests", "prefix.requests.for.envoy", @@ -344,7 +319,6 @@ TEST_F(StatsMatcherTest, CheckMultipleAssortedExclusionMatchersWithPrefix) { exclusionList()->set_exact("regex"); exclusionList()->set_prefix("prefix."); initMatcher(); - EXPECT_TRUE(stats_matcher_impl_->hasStringMatchers()); expectAccepted({"requestsEnvoy", "EnvoyProxy", "foo", "regex_etc", "prefixfoo"}); expectDenied({"envoy.matchers.requests", "requests.for.envoy", "envoyrequests", "regex", "prefix", "prefix.foo", "prefix.requests.for.envoy"}); diff --git a/test/mocks/stats/mocks.h b/test/mocks/stats/mocks.h index 58ddc2f1a64f4..c84f22a65f435 100644 --- a/test/mocks/stats/mocks.h +++ b/test/mocks/stats/mocks.h @@ -353,7 +353,6 @@ class MockStatsMatcher : public StatsMatcher { MOCK_METHOD(bool, slowRejects, (StatName name), (const)); bool acceptsAll() const override { return accepts_all_; } bool rejectsAll() const override { return rejects_all_; } - bool hasStringMatchers() const override { return true; } bool accepts_all_{false}; bool rejects_all_{false}; From 051f4125b462b9c7969ba8f6d04866247fb4aff7 Mon Sep 17 00:00:00 2001 From: Joshua Marantz Date: Sun, 20 Jun 2021 15:16:51 -0400 Subject: [PATCH 12/16] improve commenting Signed-off-by: Joshua Marantz --- envoy/stats/stats_matcher.h | 15 ++++++++------- source/common/common/matchers.h | 9 +++++++-- source/common/stats/stats_matcher_impl.cc | 14 ++++++++++++++ source/common/stats/symbol_table_impl.cc | 8 +++++--- source/common/stats/thread_local_store.cc | 4 ++-- test/common/stats/thread_local_store_test.cc | 5 +++++ 6 files changed, 41 insertions(+), 14 deletions(-) diff --git a/envoy/stats/stats_matcher.h b/envoy/stats/stats_matcher.h index 221b7b8ed23c9..897616b226b86 100644 --- a/envoy/stats/stats_matcher.h +++ b/envoy/stats/stats_matcher.h @@ -19,28 +19,29 @@ class StatsMatcher { * Take a metric name and report whether or not it should be instantiated. * The may need to convert the StatName to a string. * - * @param the name of a Stats::Metric. + * @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 determine whether it can be rejected based - * purely on the StatName. A return of 'false' means we may need to check + * purely on the StatName. A return of 'false' means we will need to check * slowRejects as well. * - * @param the name of a Stats::Metric. + * @param name the name of a Stats::Metric. * @return bool true if that stat should not be instantiated, or whether we * need to check slowRejects. */ virtual bool fastRejects(StatName name) const PURE; /** - * Takes a metric name and quickly determine whether it can be rejected based - * purely on the StatName. A return of 'false' means we may need to check - * slowRejects as well. + * 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() + * returns false. It is a good idea to cache the results of this, to avoid the + * stringification overhead as well as a global symbol table lock. * - * @param the name of a Stats::Metric. + * @param name the name of a Stats::Metric. * @return bool true if that stat should not be instantiated, or whether we * need to check slowRejects. */ diff --git a/source/common/common/matchers.h b/source/common/common/matchers.h index 2b275237a57ed..88e9865c9e6ef 100644 --- a/source/common/common/matchers.h +++ b/source/common/common/matchers.h @@ -86,8 +86,13 @@ class StringMatcherImpl : public ValueMatcher, public StringMatcher { const envoy::type::matcher::v3::StringMatcher& matcher() const { return matcher_; } - // If the matcher is a case-sensitive prefix match, returns the prefix string. - // otherwise, returns an empty string. + /** + * 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: diff --git a/source/common/stats/stats_matcher_impl.cc b/source/common/stats/stats_matcher_impl.cc index 67e95f80b9201..4b56605bf57f9 100644 --- a/source/common/stats/stats_matcher_impl.cc +++ b/source/common/stats/stats_matcher_impl.cc @@ -45,6 +45,20 @@ StatsMatcherImpl::StatsMatcherImpl(const envoy::config::metrics::v3::StatsConfig } } +// 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, ".") && diff --git a/source/common/stats/symbol_table_impl.cc b/source/common/stats/symbol_table_impl.cc index 26c29aef39fa3..0b15b66ba4547 100644 --- a/source/common/stats/symbol_table_impl.cc +++ b/source/common/stats/symbol_table_impl.cc @@ -164,14 +164,16 @@ bool StatName::startsWith(StatName symbolic_prefix) const { [&prefix_symbols](Symbol symbol) { prefix_symbols.push_back(symbol); }, [&ret](absl::string_view) { ret = false; }); - // If there any dynamic components, we'll our string_view lambda called, and - // then we'll return false for simplicity. + // 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 string_view elements after the prefix match. + // be dynamic string_view elements after the prefix match. uint32_t index = 0; SymbolTableImpl::Encoding::decodeTokens( data(), dataSize(), diff --git a/source/common/stats/thread_local_store.cc b/source/common/stats/thread_local_store.cc index 2af400be1cb19..a309854a8af61 100644 --- a/source/common/stats/thread_local_store.cc +++ b/source/common/stats/thread_local_store.cc @@ -465,8 +465,8 @@ StatType& ThreadLocalStoreImpl::ScopeImpl::safeMakeStat( 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())) { + if (tls_rejected_stats != nullptr && + tls_rejected_stats->find(full_stat_name) != tls_rejected_stats->end()) { return null_stat; } diff --git a/test/common/stats/thread_local_store_test.cc b/test/common/stats/thread_local_store_test.cc index d0c2bedb01b01..a5ce531260963 100644 --- a/test/common/stats/thread_local_store_test.cc +++ b/test/common/stats/thread_local_store_test.cc @@ -959,6 +959,8 @@ TEST_F(StatsMatcherTLSTest, RejectPrefixDot) { "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); } @@ -973,6 +975,9 @@ TEST_F(StatsMatcherTLSTest, RejectPrefixNoDot) { "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); } From 1ff784f93156d2be75f0657a4f1818c5ad0e2453 Mon Sep 17 00:00:00 2001 From: Joshua Marantz Date: Thu, 1 Jul 2021 22:15:25 -0400 Subject: [PATCH 13/16] introduce an intermediate result resulted by fastRejects() and required by slowRejects() Signed-off-by: Joshua Marantz --- envoy/stats/stats_matcher.h | 14 +++++- source/common/stats/stats_matcher_impl.cc | 50 +++++++++----------- source/common/stats/stats_matcher_impl.h | 9 ++-- source/common/stats/thread_local_store.cc | 42 +++++++++------- source/common/stats/thread_local_store.h | 9 ++-- test/common/stats/thread_local_store_test.cc | 15 ++++-- test/mocks/stats/mocks.h | 4 +- 7 files changed, 83 insertions(+), 60 deletions(-) diff --git a/envoy/stats/stats_matcher.h b/envoy/stats/stats_matcher.h index 897616b226b86..8d9e207415cd4 100644 --- a/envoy/stats/stats_matcher.h +++ b/envoy/stats/stats_matcher.h @@ -13,6 +13,16 @@ class StatName; class StatsMatcher { public: + struct FastResult { + bool rejects() const { return rejects_; } + bool operator==(const FastResult& that) const { + return rejects_ == that.rejects_ && fast_matches_ == that.fast_matches_; + } + + bool rejects_{false}; + bool fast_matches_{false}; + }; + virtual ~StatsMatcher() = default; /** @@ -33,7 +43,7 @@ class StatsMatcher { * @return bool true if that stat should not be instantiated, or whether we * need to check slowRejects. */ - virtual bool fastRejects(StatName name) const PURE; + virtual FastResult fastRejects(StatName name) const PURE; /** * Takes a metric name and converts it to a string, if needed, to determine @@ -45,7 +55,7 @@ class StatsMatcher { * @return bool true if that stat should not be instantiated, or whether we * need to check slowRejects. */ - virtual bool slowRejects(StatName name) const PURE; + virtual bool slowRejects(FastResult result, StatName name) const PURE; /** * Helps determine whether the matcher needs to be called. This can be used diff --git a/source/common/stats/stats_matcher_impl.cc b/source/common/stats/stats_matcher_impl.cc index 4b56605bf57f9..5fa518836b327 100644 --- a/source/common/stats/stats_matcher_impl.cc +++ b/source/common/stats/stats_matcher_impl.cc @@ -68,22 +68,20 @@ void StatsMatcherImpl::optimizeLastMatcher() { } } -bool StatsMatcherImpl::fastRejects(StatName stat_name) const { +StatsMatcher::FastResult StatsMatcherImpl::fastRejects(StatName stat_name) const { + FastResult result; if (rejectsAll()) { - return true; - } - - // We can short-circuit the slow matchers only if they are empty, or if - // we are in inclusive-mode and we find a match. - if (is_inclusive_ || matchers_.empty()) { - return fastRejectMatch(stat_name) == is_inclusive_; + result.rejects_ = true; + } else { + result.fast_matches_ = fastRejectMatch(stat_name); + if (is_inclusive_ || matchers_.empty()) { + // We can short-circuit the slow matchers only if they are empty, or if + // we are in inclusive-mode and we find a match. + result.rejects_ = result.fast_matches_ == is_inclusive_; + } } - // If there are both prefix and string matchers, and we are in exclusive - // mode, then it isn't helpful to run the prefix matchers early, so - // we return false. This forces the caller to eventually then call - // slowRejects(), where we'll handle this case below. - return false; + return result; } bool StatsMatcherImpl::fastRejectMatch(StatName stat_name) const { @@ -91,26 +89,22 @@ bool StatsMatcherImpl::fastRejectMatch(StatName stat_name) const { [stat_name](StatName prefix) { return stat_name.startsWith(prefix); }); } -bool StatsMatcherImpl::slowRejects(StatName stat_name) const { +bool StatsMatcherImpl::slowRejects(FastResult fast_result, StatName stat_name) const { bool match = slowRejectMatch(stat_name); - // 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. - - if (is_inclusive_ || match || prefixes_.empty()) { + if (is_inclusive_ || match || !fast_result.fast_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_; } - // In exclusive-mode with both prefixes and matches, we must - // skip the fast-path prefix since we invert the results, so we can't - // definitively declare a rejection. So we must check them now. - return !fastRejectMatch(stat_name); + return false; } bool StatsMatcherImpl::slowRejectMatch(StatName stat_name) const { diff --git a/source/common/stats/stats_matcher_impl.h b/source/common/stats/stats_matcher_impl.h index 5de4d637535c4..6bdfe9c9ecf02 100644 --- a/source/common/stats/stats_matcher_impl.h +++ b/source/common/stats/stats_matcher_impl.h @@ -27,9 +27,12 @@ class StatsMatcherImpl : public StatsMatcher { StatsMatcherImpl() = default; // StatsMatcher - bool rejects(StatName name) const override { return fastRejects(name) || slowRejects(name); } - bool fastRejects(StatName name) const override; - bool slowRejects(StatName name) const override; + bool rejects(StatName name) const override { + FastResult fast_result = fastRejects(name); + return fast_result.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(); } diff --git a/source/common/stats/thread_local_store.cc b/source/common/stats/thread_local_store.cc index a309854a8af61..1733cc1554730 100644 --- a/source/common/stats/thread_local_store.cc +++ b/source/common/stats/thread_local_store.cc @@ -89,7 +89,7 @@ template void ThreadLocalStoreImpl::removeRejectedStats(StatMapClass& map, StatListClass& list) { StatNameVec remove_list; for (auto& stat : map) { - if (fastRejects(stat.first) || slowRejects(stat.first)) { + if (rejects(stat.first)) { remove_list.push_back(stat.first); } } @@ -101,12 +101,13 @@ void ThreadLocalStoreImpl::removeRejectedStats(StatMapClass& map, StatListClass& } } -bool ThreadLocalStoreImpl::fastRejects(StatName stat_name) const { +StatsMatcher::FastResult ThreadLocalStoreImpl::fastRejects(StatName stat_name) const { return stats_matcher_->fastRejects(stat_name); } -bool ThreadLocalStoreImpl::slowRejects(StatName stat_name) const { - return stats_matcher_->slowRejects(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 { @@ -431,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()) { @@ -442,7 +444,7 @@ bool ThreadLocalStoreImpl::checkAndRememberRejection(StatName name, if (iter != central_rejected_stats.end()) { rejected_name = &(*iter); } else { - if (slowRejects(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; @@ -462,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()) { @@ -485,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); @@ -540,7 +543,8 @@ Counter& ThreadLocalStoreImpl::ScopeImpl::counterFromStatNameWithTags( TagUtility::TagStatNameJoiner joiner(prefix_.statName(), name, stat_name_tags, symbolTable()); Stats::StatName final_stat_name = joiner.nameWithTags(); - if (parent_.fastRejects(final_stat_name)) { + StatsMatcher::FastResult fast_reject_result = parent_.fastRejects(final_stat_name); + if (fast_reject_result.rejects()) { return parent_.null_counter_; } @@ -556,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); @@ -598,7 +602,8 @@ Gauge& ThreadLocalStoreImpl::ScopeImpl::gaugeFromStatNameWithTags( TagUtility::TagStatNameJoiner joiner(prefix_.statName(), name, stat_name_tags, symbolTable()); StatName final_stat_name = joiner.nameWithTags(); - if (parent_.fastRejects(final_stat_name)) { + StatsMatcher::FastResult fast_reject_result = parent_.fastRejects(final_stat_name); + if (fast_reject_result.rejects()) { return parent_.null_gauge_; } @@ -612,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); @@ -640,7 +645,8 @@ Histogram& ThreadLocalStoreImpl::ScopeImpl::histogramFromStatNameWithTags( TagUtility::TagStatNameJoiner joiner(prefix_.statName(), name, stat_name_tags, symbolTable()); StatName final_stat_name = joiner.nameWithTags(); - if (parent_.fastRejects(final_stat_name)) { + StatsMatcher::FastResult fast_reject_result = parent_.fastRejects(final_stat_name); + if (fast_reject_result.rejects()) { return parent_.null_histogram_; } @@ -664,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 { @@ -717,7 +724,8 @@ TextReadout& ThreadLocalStoreImpl::ScopeImpl::textReadoutFromStatNameWithTags( TagUtility::TagStatNameJoiner joiner(prefix_.statName(), name, stat_name_tags, symbolTable()); Stats::StatName final_stat_name = joiner.nameWithTags(); - if (parent_.fastRejects(final_stat_name)) { + StatsMatcher::FastResult fast_reject_result = parent_.fastRejects(final_stat_name); + if (fast_reject_result.rejects()) { return parent_.null_text_readout_; } @@ -733,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 d1039f3afe4c8..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,12 +477,14 @@ class ThreadLocalStoreImpl : Logger::Loggable, public StoreRo void clearHistogramsFromCaches(); void releaseScopeCrossThread(ScopeImpl* scope); void mergeInternal(PostMergeCb merge_cb); - bool slowRejects(StatName name) const; - bool fastRejects(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/test/common/stats/thread_local_store_test.cc b/test/common/stats/thread_local_store_test.cc index a5ce531260963..bfad7238649ac 100644 --- a/test/common/stats/thread_local_store_test.cc +++ b/test/common/stats/thread_local_store_test.cc @@ -1017,14 +1017,18 @@ class RememberStatsMatcherTest : public testing::TestWithParam { StatNamePool pool(symbol_table_); for (int j = 0; j < 5; ++j) { - EXPECT_CALL(*matcher, fastRejects(pool.add("scope.reject"))).WillOnce(Return(false)); + StatsMatcher::FastResult no_fast_rejection; + EXPECT_CALL(*matcher, fastRejects(pool.add("scope.reject"))) + .WillOnce(Return(no_fast_rejection)); if (j == 0) { - EXPECT_CALL(*matcher, slowRejects(pool.add("scope.reject"))).WillOnce(Return(true)); + 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(false)); + EXPECT_CALL(*matcher, fastRejects(pool.add("scope.ok"))).WillOnce(Return(no_fast_rejection)); if (j == 0) { - EXPECT_CALL(*matcher, slowRejects(pool.add("scope.ok"))).WillOnce(Return(false)); + EXPECT_CALL(*matcher, slowRejects(no_fast_rejection, pool.add("scope.ok"))) + .WillOnce(Return(false)); } EXPECT_EQ("scope.ok", lookup_stat("ok")); } @@ -1057,8 +1061,9 @@ class RememberStatsMatcherTest : public testing::TestWithParam { store_.setStatsMatcher(std::move(matcher_ptr)); StatNamePool pool(symbol_table_); + StatsMatcher::FastResult no_fast_rejection; for (int j = 0; j < 5; ++j) { - EXPECT_CALL(*matcher, fastRejects(pool.add("scope.ok"))).WillOnce(Return(false)); + 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")); } diff --git a/test/mocks/stats/mocks.h b/test/mocks/stats/mocks.h index c84f22a65f435..928f3ab9c7671 100644 --- a/test/mocks/stats/mocks.h +++ b/test/mocks/stats/mocks.h @@ -349,8 +349,8 @@ class MockStatsMatcher : public StatsMatcher { MockStatsMatcher(); ~MockStatsMatcher() override; MOCK_METHOD(bool, rejects, (StatName name), (const)); - MOCK_METHOD(bool, fastRejects, (StatName name), (const)); - MOCK_METHOD(bool, slowRejects, (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_; } From 60e8c00007dd1b88a0169fc183b00fadf127b8a5 Mon Sep 17 00:00:00 2001 From: Joshua Marantz Date: Tue, 6 Jul 2021 16:24:21 -0400 Subject: [PATCH 14/16] Cleanup comments. Signed-off-by: Joshua Marantz --- envoy/stats/stats_matcher.h | 42 +++++++++++++++++++++++++------------ 1 file changed, 29 insertions(+), 13 deletions(-) diff --git a/envoy/stats/stats_matcher.h b/envoy/stats/stats_matcher.h index 8d9e207415cd4..498dedf011ac3 100644 --- a/envoy/stats/stats_matcher.h +++ b/envoy/stats/stats_matcher.h @@ -13,8 +13,18 @@ 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. struct FastResult { + /** + * @return Whether the stat can be quickly rejected without calling slowRejects(). + */ bool rejects() const { return rejects_; } + + /** + * @return whether two FastReults objects are equivalent; this is needed for mocks. + */ bool operator==(const FastResult& that) const { return rejects_ == that.rejects_ && fast_matches_ == that.fast_matches_; } @@ -27,7 +37,8 @@ class StatsMatcher { /** * Take a metric name and report whether or not it should be instantiated. - * The may need to convert the StatName to a string. + * 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. @@ -35,27 +46,32 @@ class StatsMatcher { virtual bool rejects(StatName name) const PURE; /** - * Takes a metric name and quickly determine whether it can be rejected based - * purely on the StatName. A return of 'false' means we will need to check - * slowRejects as well. + * 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 bool true if that stat should not be instantiated, or whether we - * need to check slowRejects. + * @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() - * returns false. It is a good idea to cache the results of this, to avoid the - * stringification overhead as well as a global symbol table lock. + * 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, or whether we - * need to check slowRejects. + * @return bool true if that stat should not be instantiated. */ - virtual bool slowRejects(FastResult result, StatName 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 From fa7c3ca124a3c7d981bacd66839d71697275e4cf Mon Sep 17 00:00:00 2001 From: Joshua Marantz Date: Wed, 7 Jul 2021 09:47:06 -0400 Subject: [PATCH 15/16] Switch from a class to an enum for FastResult; it's a bit cleaner. Signed-off-by: Joshua Marantz --- envoy/stats/stats_matcher.h | 19 ++++------------ source/common/stats/stats_matcher_impl.cc | 23 ++++++++++---------- source/common/stats/stats_matcher_impl.h | 2 +- source/common/stats/thread_local_store.cc | 8 +++---- test/common/stats/thread_local_store_test.cc | 4 ++-- 5 files changed, 22 insertions(+), 34 deletions(-) diff --git a/envoy/stats/stats_matcher.h b/envoy/stats/stats_matcher.h index 498dedf011ac3..767e57605f8fc 100644 --- a/envoy/stats/stats_matcher.h +++ b/envoy/stats/stats_matcher.h @@ -16,21 +16,10 @@ class StatsMatcher { // 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. - struct FastResult { - /** - * @return Whether the stat can be quickly rejected without calling slowRejects(). - */ - bool rejects() const { return rejects_; } - - /** - * @return whether two FastReults objects are equivalent; this is needed for mocks. - */ - bool operator==(const FastResult& that) const { - return rejects_ == that.rejects_ && fast_matches_ == that.fast_matches_; - } - - bool rejects_{false}; - bool fast_matches_{false}; + enum class FastResult { + NoMatch, + Rejects, + Matches, }; virtual ~StatsMatcher() = default; diff --git a/source/common/stats/stats_matcher_impl.cc b/source/common/stats/stats_matcher_impl.cc index 5fa518836b327..b03c7c485138d 100644 --- a/source/common/stats/stats_matcher_impl.cc +++ b/source/common/stats/stats_matcher_impl.cc @@ -69,19 +69,18 @@ void StatsMatcherImpl::optimizeLastMatcher() { } StatsMatcher::FastResult StatsMatcherImpl::fastRejects(StatName stat_name) const { - FastResult result; if (rejectsAll()) { - result.rejects_ = true; - } else { - result.fast_matches_ = fastRejectMatch(stat_name); - if (is_inclusive_ || matchers_.empty()) { - // We can short-circuit the slow matchers only if they are empty, or if - // we are in inclusive-mode and we find a match. - result.rejects_ = result.fast_matches_ == is_inclusive_; - } + return FastResult::NoMatch; } - - return result; + 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 { @@ -92,7 +91,7 @@ bool StatsMatcherImpl::fastRejectMatch(StatName stat_name) const { bool StatsMatcherImpl::slowRejects(FastResult fast_result, StatName stat_name) const { bool match = slowRejectMatch(stat_name); - if (is_inclusive_ || match || !fast_result.fast_matches_) { + if (is_inclusive_ || match || fast_result != FastResult::Matches) { // is_inclusive_ | match | return // ---------------+-------+-------- // T | T | T Default-inclusive and matching an (exclusion) matcher, deny. diff --git a/source/common/stats/stats_matcher_impl.h b/source/common/stats/stats_matcher_impl.h index 6bdfe9c9ecf02..b9cad0fa35da2 100644 --- a/source/common/stats/stats_matcher_impl.h +++ b/source/common/stats/stats_matcher_impl.h @@ -29,7 +29,7 @@ class StatsMatcherImpl : public StatsMatcher { // StatsMatcher bool rejects(StatName name) const override { FastResult fast_result = fastRejects(name); - return fast_result.rejects() || slowRejects(fast_result, name); + return fast_result == FastResult::Rejects || slowRejects(fast_result, name); } FastResult fastRejects(StatName name) const override; bool slowRejects(FastResult, StatName name) const override; diff --git a/source/common/stats/thread_local_store.cc b/source/common/stats/thread_local_store.cc index 1733cc1554730..3b29f8b5d4df5 100644 --- a/source/common/stats/thread_local_store.cc +++ b/source/common/stats/thread_local_store.cc @@ -544,7 +544,7 @@ Counter& ThreadLocalStoreImpl::ScopeImpl::counterFromStatNameWithTags( Stats::StatName final_stat_name = joiner.nameWithTags(); StatsMatcher::FastResult fast_reject_result = parent_.fastRejects(final_stat_name); - if (fast_reject_result.rejects()) { + if (fast_reject_result == StatsMatcher::FastResult::Rejects) { return parent_.null_counter_; } @@ -603,7 +603,7 @@ Gauge& ThreadLocalStoreImpl::ScopeImpl::gaugeFromStatNameWithTags( StatName final_stat_name = joiner.nameWithTags(); StatsMatcher::FastResult fast_reject_result = parent_.fastRejects(final_stat_name); - if (fast_reject_result.rejects()) { + if (fast_reject_result == StatsMatcher::FastResult::Rejects) { return parent_.null_gauge_; } @@ -646,7 +646,7 @@ Histogram& ThreadLocalStoreImpl::ScopeImpl::histogramFromStatNameWithTags( StatName final_stat_name = joiner.nameWithTags(); StatsMatcher::FastResult fast_reject_result = parent_.fastRejects(final_stat_name); - if (fast_reject_result.rejects()) { + if (fast_reject_result == StatsMatcher::FastResult::Rejects) { return parent_.null_histogram_; } @@ -725,7 +725,7 @@ TextReadout& ThreadLocalStoreImpl::ScopeImpl::textReadoutFromStatNameWithTags( Stats::StatName final_stat_name = joiner.nameWithTags(); StatsMatcher::FastResult fast_reject_result = parent_.fastRejects(final_stat_name); - if (fast_reject_result.rejects()) { + if (fast_reject_result == StatsMatcher::FastResult::Rejects) { return parent_.null_text_readout_; } diff --git a/test/common/stats/thread_local_store_test.cc b/test/common/stats/thread_local_store_test.cc index bfad7238649ac..b723fe3083fb9 100644 --- a/test/common/stats/thread_local_store_test.cc +++ b/test/common/stats/thread_local_store_test.cc @@ -1016,8 +1016,8 @@ class RememberStatsMatcherTest : public testing::TestWithParam { 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) { - StatsMatcher::FastResult no_fast_rejection; EXPECT_CALL(*matcher, fastRejects(pool.add("scope.reject"))) .WillOnce(Return(no_fast_rejection)); if (j == 0) { @@ -1061,7 +1061,7 @@ class RememberStatsMatcherTest : public testing::TestWithParam { store_.setStatsMatcher(std::move(matcher_ptr)); StatNamePool pool(symbol_table_); - StatsMatcher::FastResult no_fast_rejection; + StatsMatcher::FastResult no_fast_rejection = StatsMatcher::FastResult::NoMatch; for (int j = 0; j < 5; ++j) { 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. From 8dd325e2634411c3023af909ba71dbd49ab047b3 Mon Sep 17 00:00:00 2001 From: Joshua Marantz Date: Thu, 8 Jul 2021 11:41:36 -0400 Subject: [PATCH 16/16] fast-reject the RejectsAll case, and test for that. Signed-off-by: Joshua Marantz --- source/common/stats/stats_matcher_impl.cc | 2 +- test/common/stats/stats_matcher_impl_test.cc | 4 ++++ 2 files changed, 5 insertions(+), 1 deletion(-) diff --git a/source/common/stats/stats_matcher_impl.cc b/source/common/stats/stats_matcher_impl.cc index b03c7c485138d..a4440f90494cf 100644 --- a/source/common/stats/stats_matcher_impl.cc +++ b/source/common/stats/stats_matcher_impl.cc @@ -70,7 +70,7 @@ void StatsMatcherImpl::optimizeLastMatcher() { StatsMatcher::FastResult StatsMatcherImpl::fastRejects(StatName stat_name) const { if (rejectsAll()) { - return FastResult::NoMatch; + return FastResult::Rejects; } bool matches = fastRejectMatch(stat_name); if ((is_inclusive_ || matchers_.empty()) && matches == is_inclusive_) { diff --git a/test/common/stats/stats_matcher_impl_test.cc b/test/common/stats/stats_matcher_impl_test.cc index 6848036a4b777..821a47c69b1df 100644 --- a/test/common/stats/stats_matcher_impl_test.cc +++ b/test/common/stats/stats_matcher_impl_test.cc @@ -62,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) { @@ -71,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) { @@ -130,6 +132,8 @@ TEST_F(StatsMatcherTest, CheckIncludePrefixDot) { {"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) {