From 6ffa3ee420ec160800e32a5751d64c5f2816036b Mon Sep 17 00:00:00 2001 From: Joshua Marantz Date: Thu, 31 Oct 2019 10:59:08 -0400 Subject: [PATCH 01/16] add speed & functionality test for using re2 for stats tag matching, mainly for experimenting. Signed-off-by: Joshua Marantz --- test/common/common/BUILD | 11 ++ test/common/common/re_speed_test.cc | 129 +++++++++++++++++++ test/common/stats/tag_extractor_impl_test.cc | 24 +++- 3 files changed, 163 insertions(+), 1 deletion(-) create mode 100644 test/common/common/re_speed_test.cc diff --git a/test/common/common/BUILD b/test/common/common/BUILD index c9024dba5e8d6..736d399827bd9 100644 --- a/test/common/common/BUILD +++ b/test/common/common/BUILD @@ -196,6 +196,17 @@ envoy_cc_test( deps = ["//source/common/common:callback_impl_lib"], ) +envoy_cc_binary( + name = "re_speed_test", + srcs = ["re_speed_test.cc"], + external_deps = ["benchmark"], + deps = [ + "//source/common/common:assert_lib", + "//source/common/common:utility_lib", + "@com_googlesource_code_re2//:re2", + ], +) + envoy_cc_binary( name = "utility_speed_test", srcs = ["utility_speed_test.cc"], diff --git a/test/common/common/re_speed_test.cc b/test/common/common/re_speed_test.cc new file mode 100644 index 0000000000000..1bd5064357ac1 --- /dev/null +++ b/test/common/common/re_speed_test.cc @@ -0,0 +1,129 @@ +// Note: this should be run with --compilation_mode=opt, and would benefit from a +// quiescent system with disabled cstate power management. + +#include + +#include "common/common/assert.h" + +#include "absl/strings/string_view.h" +#include "benchmark/benchmark.h" + +#include "re2/re2.h" + +static const char* cluster_inputs[] = { + "cluster.no_trailing_dot", + "cluster.match.", + "cluster.match.normal", + "cluster.match.and.a.whole.lot.of.things.coming.after.the.matches.really.too.much.stuff", +}; + +static const char cluster_re_pattern[] = "^cluster\\.((.*?)\\.)"; +static const char cluster_re_alt_pattern[] = "^cluster\\.([^\\.]+)\\..*"; + +static void BM_StdRegex(benchmark::State& state) { + std::regex re(cluster_re_pattern); + uint32_t passes = 0; + std::vector inputs; + for (const char* cluster_input : cluster_inputs) { + inputs.push_back(cluster_input); + } + + for (auto _ : state) { + for (const std::string& cluster_input : inputs) { + std::smatch match; + if (std::regex_search(cluster_input, match, re)) { + ASSERT(match.size() >= 3); + ASSERT(match[1] == "match."); + ASSERT(match[2] == "match"); + ++passes; + } + } + } + RELEASE_ASSERT(passes > 0, ""); +} +BENCHMARK(BM_StdRegex); + +static void BM_StdRegexStringView(benchmark::State& state) { + std::regex re(cluster_re_pattern); + std::vector inputs; + for (const char* cluster_input : cluster_inputs) { + inputs.push_back(cluster_input); + } + uint32_t passes = 0; + for (auto _ : state) { + for (absl::string_view cluster_input : inputs) { + std::match_results smatch; + if (std::regex_search(cluster_input.begin(), cluster_input.end(), smatch, re)) { + ASSERT(smatch.size() >= 3); + ASSERT(smatch[1] == "match."); + ASSERT(smatch[2] == "match"); + ++passes; + } + } + } + RELEASE_ASSERT(passes > 0, ""); +} +BENCHMARK(BM_StdRegexStringView); + +static void BM_StdRegexStringViewAltPattern(benchmark::State& state) { + std::regex re(cluster_re_alt_pattern); + std::vector inputs; + for (const char* cluster_input : cluster_inputs) { + inputs.push_back(cluster_input); + } + uint32_t passes = 0; + for (auto _ : state) { + for (absl::string_view cluster_input : inputs) { + std::match_results smatch; + if (std::regex_search(cluster_input.begin(), cluster_input.end(), smatch, re)) { + ASSERT(smatch.size() >= 2); + ASSERT(smatch[1] == "match"); + ++passes; + } + } + } + RELEASE_ASSERT(passes > 0, ""); +} +BENCHMARK(BM_StdRegexStringViewAltPattern); + +static void BM_RE2(benchmark::State& state) { + re2::RE2 re(cluster_re_pattern); + uint32_t passes = 0; + for (auto _ : state) { + for (const char* cluster_input : cluster_inputs) { + re2::StringPiece match1, match2; + if (re2::RE2::PartialMatch(cluster_input, re, &match1, &match2)) { + ASSERT(match1 == "match."); + ASSERT(match2 == "match"); + ++passes; + } + } + } + RELEASE_ASSERT(passes > 0, ""); +} +BENCHMARK(BM_RE2); + +static void BM_RE2_AltPattern(benchmark::State& state) { + re2::RE2 re(cluster_re_alt_pattern); + uint32_t passes = 0; + for (auto _ : state) { + for (const char* cluster_input : cluster_inputs) { + re2::StringPiece match; + if (re2::RE2::PartialMatch(cluster_input, re, &match)) { + ASSERT(match == "match"); + ++passes; + } + } + } + RELEASE_ASSERT(passes > 0, ""); +} +BENCHMARK(BM_RE2_AltPattern); + +// Boilerplate main(), which discovers benchmarks in the same file and runs them. +int main(int argc, char** argv) { + benchmark::Initialize(&argc, argv); + if (benchmark::ReportUnrecognizedArguments(argc, argv)) { + return 1; + } + benchmark::RunSpecifiedBenchmarks(); +} diff --git a/test/common/stats/tag_extractor_impl_test.cc b/test/common/stats/tag_extractor_impl_test.cc index b789ae0aa01a1..102b251aa74c8 100644 --- a/test/common/stats/tag_extractor_impl_test.cc +++ b/test/common/stats/tag_extractor_impl_test.cc @@ -11,11 +11,13 @@ #include "gtest/gtest.h" +#include "re2/re2.h" + namespace Envoy { namespace Stats { TEST(TagExtractorTest, TwoSubexpressions) { - TagExtractorImpl tag_extractor("cluster_name", "^cluster\\.((.+?)\\.)"); + TagExtractorImpl tag_extractor("cluster_name", "^cluster\\.((.*?)\\.)"); EXPECT_EQ("cluster_name", tag_extractor.name()); std::string name = "cluster.test_cluster.upstream_cx_total"; std::vector tags; @@ -28,6 +30,26 @@ TEST(TagExtractorTest, TwoSubexpressions) { EXPECT_EQ("cluster_name", tags.at(0).name_); } +TEST(TagExtractorTest, TwoSubexpressionsRE2) { + re2::RE2 re("^cluster\\.((.*?)\\.)"); + re2::StringPiece match1, match2; + ASSERT_TRUE(re2::RE2::PartialMatch("cluster.test_cluster.upstream_cx_total", re, &match1, + &match2)); + EXPECT_EQ("test_cluster.", std::string(match1)); + EXPECT_EQ("test_cluster", std::string(match2)); + + re2::RE2 alternate_re("^cluster\\.([^\\.]+)\\..*"); + ASSERT_TRUE(re2::RE2::FullMatch("cluster.test_cluster.upstream_cx_total", alternate_re, &match1)); + EXPECT_EQ("test_cluster", std::string(match1)); + + std::regex sr("^cluster\\.((.*?)\\.)"); + std::match_results smatch; + ASSERT_TRUE(std::regex_search("cluster.test_cluster.upstream_cx_total", smatch, sr)); + ASSERT_LE(3, smatch.size()); + EXPECT_EQ("test_cluster.", std::string(smatch[1])); + EXPECT_EQ("test_cluster", std::string(smatch[2])); +} + TEST(TagExtractorTest, SingleSubexpression) { TagExtractorImpl tag_extractor("listner_port", "^listener\\.(\\d+?\\.)"); std::string name = "listener.80.downstream_cx_total"; From fa3573a8874b125bda8c16a9435bc58daa069565 Mon Sep 17 00:00:00 2001 From: Joshua Marantz Date: Thu, 31 Oct 2019 11:01:20 -0400 Subject: [PATCH 02/16] Revert "add speed & functionality test for using re2 for stats tag matching, mainly for experimenting." This reverts commit 6ffa3ee420ec160800e32a5751d64c5f2816036b. Signed-off-by: Joshua Marantz --- test/common/common/BUILD | 11 -- test/common/common/re_speed_test.cc | 129 ------------------- test/common/stats/tag_extractor_impl_test.cc | 24 +--- 3 files changed, 1 insertion(+), 163 deletions(-) delete mode 100644 test/common/common/re_speed_test.cc diff --git a/test/common/common/BUILD b/test/common/common/BUILD index 736d399827bd9..c9024dba5e8d6 100644 --- a/test/common/common/BUILD +++ b/test/common/common/BUILD @@ -196,17 +196,6 @@ envoy_cc_test( deps = ["//source/common/common:callback_impl_lib"], ) -envoy_cc_binary( - name = "re_speed_test", - srcs = ["re_speed_test.cc"], - external_deps = ["benchmark"], - deps = [ - "//source/common/common:assert_lib", - "//source/common/common:utility_lib", - "@com_googlesource_code_re2//:re2", - ], -) - envoy_cc_binary( name = "utility_speed_test", srcs = ["utility_speed_test.cc"], diff --git a/test/common/common/re_speed_test.cc b/test/common/common/re_speed_test.cc deleted file mode 100644 index 1bd5064357ac1..0000000000000 --- a/test/common/common/re_speed_test.cc +++ /dev/null @@ -1,129 +0,0 @@ -// Note: this should be run with --compilation_mode=opt, and would benefit from a -// quiescent system with disabled cstate power management. - -#include - -#include "common/common/assert.h" - -#include "absl/strings/string_view.h" -#include "benchmark/benchmark.h" - -#include "re2/re2.h" - -static const char* cluster_inputs[] = { - "cluster.no_trailing_dot", - "cluster.match.", - "cluster.match.normal", - "cluster.match.and.a.whole.lot.of.things.coming.after.the.matches.really.too.much.stuff", -}; - -static const char cluster_re_pattern[] = "^cluster\\.((.*?)\\.)"; -static const char cluster_re_alt_pattern[] = "^cluster\\.([^\\.]+)\\..*"; - -static void BM_StdRegex(benchmark::State& state) { - std::regex re(cluster_re_pattern); - uint32_t passes = 0; - std::vector inputs; - for (const char* cluster_input : cluster_inputs) { - inputs.push_back(cluster_input); - } - - for (auto _ : state) { - for (const std::string& cluster_input : inputs) { - std::smatch match; - if (std::regex_search(cluster_input, match, re)) { - ASSERT(match.size() >= 3); - ASSERT(match[1] == "match."); - ASSERT(match[2] == "match"); - ++passes; - } - } - } - RELEASE_ASSERT(passes > 0, ""); -} -BENCHMARK(BM_StdRegex); - -static void BM_StdRegexStringView(benchmark::State& state) { - std::regex re(cluster_re_pattern); - std::vector inputs; - for (const char* cluster_input : cluster_inputs) { - inputs.push_back(cluster_input); - } - uint32_t passes = 0; - for (auto _ : state) { - for (absl::string_view cluster_input : inputs) { - std::match_results smatch; - if (std::regex_search(cluster_input.begin(), cluster_input.end(), smatch, re)) { - ASSERT(smatch.size() >= 3); - ASSERT(smatch[1] == "match."); - ASSERT(smatch[2] == "match"); - ++passes; - } - } - } - RELEASE_ASSERT(passes > 0, ""); -} -BENCHMARK(BM_StdRegexStringView); - -static void BM_StdRegexStringViewAltPattern(benchmark::State& state) { - std::regex re(cluster_re_alt_pattern); - std::vector inputs; - for (const char* cluster_input : cluster_inputs) { - inputs.push_back(cluster_input); - } - uint32_t passes = 0; - for (auto _ : state) { - for (absl::string_view cluster_input : inputs) { - std::match_results smatch; - if (std::regex_search(cluster_input.begin(), cluster_input.end(), smatch, re)) { - ASSERT(smatch.size() >= 2); - ASSERT(smatch[1] == "match"); - ++passes; - } - } - } - RELEASE_ASSERT(passes > 0, ""); -} -BENCHMARK(BM_StdRegexStringViewAltPattern); - -static void BM_RE2(benchmark::State& state) { - re2::RE2 re(cluster_re_pattern); - uint32_t passes = 0; - for (auto _ : state) { - for (const char* cluster_input : cluster_inputs) { - re2::StringPiece match1, match2; - if (re2::RE2::PartialMatch(cluster_input, re, &match1, &match2)) { - ASSERT(match1 == "match."); - ASSERT(match2 == "match"); - ++passes; - } - } - } - RELEASE_ASSERT(passes > 0, ""); -} -BENCHMARK(BM_RE2); - -static void BM_RE2_AltPattern(benchmark::State& state) { - re2::RE2 re(cluster_re_alt_pattern); - uint32_t passes = 0; - for (auto _ : state) { - for (const char* cluster_input : cluster_inputs) { - re2::StringPiece match; - if (re2::RE2::PartialMatch(cluster_input, re, &match)) { - ASSERT(match == "match"); - ++passes; - } - } - } - RELEASE_ASSERT(passes > 0, ""); -} -BENCHMARK(BM_RE2_AltPattern); - -// Boilerplate main(), which discovers benchmarks in the same file and runs them. -int main(int argc, char** argv) { - benchmark::Initialize(&argc, argv); - if (benchmark::ReportUnrecognizedArguments(argc, argv)) { - return 1; - } - benchmark::RunSpecifiedBenchmarks(); -} diff --git a/test/common/stats/tag_extractor_impl_test.cc b/test/common/stats/tag_extractor_impl_test.cc index 102b251aa74c8..b789ae0aa01a1 100644 --- a/test/common/stats/tag_extractor_impl_test.cc +++ b/test/common/stats/tag_extractor_impl_test.cc @@ -11,13 +11,11 @@ #include "gtest/gtest.h" -#include "re2/re2.h" - namespace Envoy { namespace Stats { TEST(TagExtractorTest, TwoSubexpressions) { - TagExtractorImpl tag_extractor("cluster_name", "^cluster\\.((.*?)\\.)"); + TagExtractorImpl tag_extractor("cluster_name", "^cluster\\.((.+?)\\.)"); EXPECT_EQ("cluster_name", tag_extractor.name()); std::string name = "cluster.test_cluster.upstream_cx_total"; std::vector tags; @@ -30,26 +28,6 @@ TEST(TagExtractorTest, TwoSubexpressions) { EXPECT_EQ("cluster_name", tags.at(0).name_); } -TEST(TagExtractorTest, TwoSubexpressionsRE2) { - re2::RE2 re("^cluster\\.((.*?)\\.)"); - re2::StringPiece match1, match2; - ASSERT_TRUE(re2::RE2::PartialMatch("cluster.test_cluster.upstream_cx_total", re, &match1, - &match2)); - EXPECT_EQ("test_cluster.", std::string(match1)); - EXPECT_EQ("test_cluster", std::string(match2)); - - re2::RE2 alternate_re("^cluster\\.([^\\.]+)\\..*"); - ASSERT_TRUE(re2::RE2::FullMatch("cluster.test_cluster.upstream_cx_total", alternate_re, &match1)); - EXPECT_EQ("test_cluster", std::string(match1)); - - std::regex sr("^cluster\\.((.*?)\\.)"); - std::match_results smatch; - ASSERT_TRUE(std::regex_search("cluster.test_cluster.upstream_cx_total", smatch, sr)); - ASSERT_LE(3, smatch.size()); - EXPECT_EQ("test_cluster.", std::string(smatch[1])); - EXPECT_EQ("test_cluster", std::string(smatch[2])); -} - TEST(TagExtractorTest, SingleSubexpression) { TagExtractorImpl tag_extractor("listner_port", "^listener\\.(\\d+?\\.)"); std::string name = "listener.80.downstream_cx_total"; From 42ac3d0977170ae9086982ebfb935e3d5146bb02 Mon Sep 17 00:00:00 2001 From: Joshua Marantz Date: Thu, 31 Oct 2019 11:05:43 -0400 Subject: [PATCH 03/16] add re2 experiments & speed test for stats init Signed-off-by: Joshua Marantz --- test/common/common/BUILD | 11 ++ test/common/common/re_speed_test.cc | 129 +++++++++++++++++++ test/common/stats/BUILD | 1 + test/common/stats/tag_extractor_impl_test.cc | 22 ++++ 4 files changed, 163 insertions(+) create mode 100644 test/common/common/re_speed_test.cc diff --git a/test/common/common/BUILD b/test/common/common/BUILD index c9024dba5e8d6..736d399827bd9 100644 --- a/test/common/common/BUILD +++ b/test/common/common/BUILD @@ -196,6 +196,17 @@ envoy_cc_test( deps = ["//source/common/common:callback_impl_lib"], ) +envoy_cc_binary( + name = "re_speed_test", + srcs = ["re_speed_test.cc"], + external_deps = ["benchmark"], + deps = [ + "//source/common/common:assert_lib", + "//source/common/common:utility_lib", + "@com_googlesource_code_re2//:re2", + ], +) + envoy_cc_binary( name = "utility_speed_test", srcs = ["utility_speed_test.cc"], diff --git a/test/common/common/re_speed_test.cc b/test/common/common/re_speed_test.cc new file mode 100644 index 0000000000000..1bd5064357ac1 --- /dev/null +++ b/test/common/common/re_speed_test.cc @@ -0,0 +1,129 @@ +// Note: this should be run with --compilation_mode=opt, and would benefit from a +// quiescent system with disabled cstate power management. + +#include + +#include "common/common/assert.h" + +#include "absl/strings/string_view.h" +#include "benchmark/benchmark.h" + +#include "re2/re2.h" + +static const char* cluster_inputs[] = { + "cluster.no_trailing_dot", + "cluster.match.", + "cluster.match.normal", + "cluster.match.and.a.whole.lot.of.things.coming.after.the.matches.really.too.much.stuff", +}; + +static const char cluster_re_pattern[] = "^cluster\\.((.*?)\\.)"; +static const char cluster_re_alt_pattern[] = "^cluster\\.([^\\.]+)\\..*"; + +static void BM_StdRegex(benchmark::State& state) { + std::regex re(cluster_re_pattern); + uint32_t passes = 0; + std::vector inputs; + for (const char* cluster_input : cluster_inputs) { + inputs.push_back(cluster_input); + } + + for (auto _ : state) { + for (const std::string& cluster_input : inputs) { + std::smatch match; + if (std::regex_search(cluster_input, match, re)) { + ASSERT(match.size() >= 3); + ASSERT(match[1] == "match."); + ASSERT(match[2] == "match"); + ++passes; + } + } + } + RELEASE_ASSERT(passes > 0, ""); +} +BENCHMARK(BM_StdRegex); + +static void BM_StdRegexStringView(benchmark::State& state) { + std::regex re(cluster_re_pattern); + std::vector inputs; + for (const char* cluster_input : cluster_inputs) { + inputs.push_back(cluster_input); + } + uint32_t passes = 0; + for (auto _ : state) { + for (absl::string_view cluster_input : inputs) { + std::match_results smatch; + if (std::regex_search(cluster_input.begin(), cluster_input.end(), smatch, re)) { + ASSERT(smatch.size() >= 3); + ASSERT(smatch[1] == "match."); + ASSERT(smatch[2] == "match"); + ++passes; + } + } + } + RELEASE_ASSERT(passes > 0, ""); +} +BENCHMARK(BM_StdRegexStringView); + +static void BM_StdRegexStringViewAltPattern(benchmark::State& state) { + std::regex re(cluster_re_alt_pattern); + std::vector inputs; + for (const char* cluster_input : cluster_inputs) { + inputs.push_back(cluster_input); + } + uint32_t passes = 0; + for (auto _ : state) { + for (absl::string_view cluster_input : inputs) { + std::match_results smatch; + if (std::regex_search(cluster_input.begin(), cluster_input.end(), smatch, re)) { + ASSERT(smatch.size() >= 2); + ASSERT(smatch[1] == "match"); + ++passes; + } + } + } + RELEASE_ASSERT(passes > 0, ""); +} +BENCHMARK(BM_StdRegexStringViewAltPattern); + +static void BM_RE2(benchmark::State& state) { + re2::RE2 re(cluster_re_pattern); + uint32_t passes = 0; + for (auto _ : state) { + for (const char* cluster_input : cluster_inputs) { + re2::StringPiece match1, match2; + if (re2::RE2::PartialMatch(cluster_input, re, &match1, &match2)) { + ASSERT(match1 == "match."); + ASSERT(match2 == "match"); + ++passes; + } + } + } + RELEASE_ASSERT(passes > 0, ""); +} +BENCHMARK(BM_RE2); + +static void BM_RE2_AltPattern(benchmark::State& state) { + re2::RE2 re(cluster_re_alt_pattern); + uint32_t passes = 0; + for (auto _ : state) { + for (const char* cluster_input : cluster_inputs) { + re2::StringPiece match; + if (re2::RE2::PartialMatch(cluster_input, re, &match)) { + ASSERT(match == "match"); + ++passes; + } + } + } + RELEASE_ASSERT(passes > 0, ""); +} +BENCHMARK(BM_RE2_AltPattern); + +// Boilerplate main(), which discovers benchmarks in the same file and runs them. +int main(int argc, char** argv) { + benchmark::Initialize(&argc, argv); + if (benchmark::ReportUnrecognizedArguments(argc, argv)) { + return 1; + } + benchmark::RunSpecifiedBenchmarks(); +} diff --git a/test/common/stats/BUILD b/test/common/stats/BUILD index d1fb292f653ed..a9743732ceb73 100644 --- a/test/common/stats/BUILD +++ b/test/common/stats/BUILD @@ -159,6 +159,7 @@ envoy_cc_test( "//source/common/stats:tag_extractor_lib", "//source/common/stats:tag_producer_lib", "//test/test_common:utility_lib", + "@com_googlesource_code_re2//:re2", ], ) diff --git a/test/common/stats/tag_extractor_impl_test.cc b/test/common/stats/tag_extractor_impl_test.cc index b789ae0aa01a1..f2d55f7a4d7cd 100644 --- a/test/common/stats/tag_extractor_impl_test.cc +++ b/test/common/stats/tag_extractor_impl_test.cc @@ -11,6 +11,8 @@ #include "gtest/gtest.h" +#include "re2/re2.h" + namespace Envoy { namespace Stats { @@ -28,6 +30,26 @@ TEST(TagExtractorTest, TwoSubexpressions) { EXPECT_EQ("cluster_name", tags.at(0).name_); } +TEST(TagExtractorTest, RE2Variants) { + re2::RE2 re("^cluster\\.((.*?)\\.)"); + re2::StringPiece match1, match2; + ASSERT_TRUE(re2::RE2::PartialMatch("cluster.test_cluster.upstream_cx_total", re, &match1, + &match2)); + EXPECT_EQ("test_cluster.", std::string(match1)); + EXPECT_EQ("test_cluster", std::string(match2)); + + re2::RE2 alternate_re("^cluster\\.([^\\.]+)\\..*"); + ASSERT_TRUE(re2::RE2::FullMatch("cluster.test_cluster.upstream_cx_total", alternate_re, &match1)); + EXPECT_EQ("test_cluster", std::string(match1)); + + std::regex sr("^cluster\\.((.*?)\\.)"); + std::match_results smatch; + ASSERT_TRUE(std::regex_search("cluster.test_cluster.upstream_cx_total", smatch, sr)); + ASSERT_LE(3, smatch.size()); + EXPECT_EQ("test_cluster.", std::string(smatch[1])); + EXPECT_EQ("test_cluster", std::string(smatch[2])); +} + TEST(TagExtractorTest, SingleSubexpression) { TagExtractorImpl tag_extractor("listner_port", "^listener\\.(\\d+?\\.)"); std::string name = "listener.80.downstream_cx_total"; From e9b9c88408d3d3e4747e0f728f6f3e7535c14543 Mon Sep 17 00:00:00 2001 From: Joshua Marantz Date: Thu, 31 Oct 2019 14:33:05 -0400 Subject: [PATCH 04/16] update a single stat tag-extraction RE to (a) be a better pattern and (b) use RE2. This one RE improves startup performance by 10%. Signed-off-by: Joshua Marantz --- source/common/config/well_known_names.cc | 9 ++- source/common/config/well_known_names.h | 4 +- source/common/stats/tag_extractor_impl.cc | 65 ++++++++++++++++++-- source/common/stats/tag_extractor_impl.h | 31 ++++++++-- source/common/stats/tag_producer_impl.cc | 6 +- test/common/common/re_speed_test.cc | 14 +++-- test/common/stats/tag_extractor_impl_test.cc | 22 ++++--- 7 files changed, 121 insertions(+), 30 deletions(-) diff --git a/source/common/config/well_known_names.cc b/source/common/config/well_known_names.cc index a9b1662d20f6a..e9c892fa63b97 100644 --- a/source/common/config/well_known_names.cc +++ b/source/common/config/well_known_names.cc @@ -89,7 +89,7 @@ TagNameValues::TagNameValues() { addRegex(RATELIMIT_PREFIX, R"(^ratelimit\.((.*?)\.)\w+?$)"); // cluster.(.)* - addRegex(CLUSTER_NAME, "^cluster\\.((.*?)\\.)"); + addRe2(CLUSTER_NAME, "^cluster\\.(([^\\.]+)\\.).*"); // listener.[
.]http.(.)* addRegex(HTTP_CONN_MANAGER_PREFIX, R"(^listener(?=\.).*?\.http\.((.*?)\.))", ".http."); @@ -116,7 +116,12 @@ TagNameValues::TagNameValues() { void TagNameValues::addRegex(const std::string& name, const std::string& regex, const std::string& substr) { - descriptor_vec_.emplace_back(Descriptor(name, regex, substr)); + descriptor_vec_.emplace_back(Descriptor{name, regex, substr, false}); +} + +void TagNameValues::addRe2(const std::string& name, const std::string& regex, + const std::string& substr) { + descriptor_vec_.emplace_back(Descriptor{name, regex, substr, true}); } } // namespace Config diff --git a/source/common/config/well_known_names.h b/source/common/config/well_known_names.h index 8a1f7e182eb5f..335aee4722af5 100644 --- a/source/common/config/well_known_names.h +++ b/source/common/config/well_known_names.h @@ -94,11 +94,10 @@ class TagNameValues { * tags, such as "_rq_(\\d)xx$", will probably stay as regexes. */ struct Descriptor { - Descriptor(const std::string& name, const std::string& regex, const std::string& substr = "") - : name_(name), regex_(regex), substr_(substr) {} const std::string name_; const std::string regex_; const std::string substr_; + bool use_re2_; }; // Cluster name tag @@ -160,6 +159,7 @@ class TagNameValues { private: void addRegex(const std::string& name, const std::string& regex, const std::string& substr = ""); + void addRe2(const std::string& name, const std::string& regex, const std::string& substr = ""); // Collection of tag descriptors. std::vector descriptor_vec_; diff --git a/source/common/stats/tag_extractor_impl.cc b/source/common/stats/tag_extractor_impl.cc index a56398895c259..7bb206b5915ff 100644 --- a/source/common/stats/tag_extractor_impl.cc +++ b/source/common/stats/tag_extractor_impl.cc @@ -25,8 +25,7 @@ bool regexStartsWithDot(absl::string_view regex) { TagExtractorImpl::TagExtractorImpl(const std::string& name, const std::string& regex, const std::string& substr) - : name_(name), prefix_(std::string(extractRegexPrefix(regex))), substr_(substr), - regex_(Regex::Utility::parseStdRegex(regex)) {} + : name_(name), prefix_(std::string(extractRegexPrefix(regex))), substr_(substr) {} std::string TagExtractorImpl::extractRegexPrefix(absl::string_view regex) { std::string prefix; @@ -49,7 +48,8 @@ std::string TagExtractorImpl::extractRegexPrefix(absl::string_view regex) { TagExtractorPtr TagExtractorImpl::createTagExtractor(const std::string& name, const std::string& regex, - const std::string& substr) { + const std::string& substr, + bool use_re2) { if (name.empty()) { throw EnvoyException("tag_name cannot be empty"); @@ -59,15 +59,23 @@ TagExtractorPtr TagExtractorImpl::createTagExtractor(const std::string& name, throw EnvoyException(fmt::format( "No regex specified for tag specifier and no default regex for name: '{}'", name)); } - return TagExtractorPtr{new TagExtractorImpl(name, regex, substr)}; + if (use_re2) { + return std::make_unique(name, regex, substr); + } + return std::make_unique(name, regex, substr); } bool TagExtractorImpl::substrMismatch(absl::string_view stat_name) const { return !substr_.empty() && stat_name.find(substr_) == absl::string_view::npos; } -bool TagExtractorImpl::extractTag(absl::string_view stat_name, std::vector& tags, - IntervalSet& remove_characters) const { +TagExtractorStdRegexImpl::TagExtractorStdRegexImpl(const std::string& name, const std::string regex, + const std::string& substr) + : TagExtractorImpl(name, regex, substr), + regex_(Regex::Utility::parseStdRegex(regex)) {} + +bool TagExtractorStdRegexImpl::extractTag(absl::string_view stat_name, std::vector& tags, + IntervalSet& remove_characters) const { PERF_OPERATION(perf); if (substrMismatch(stat_name)) { @@ -105,5 +113,50 @@ bool TagExtractorImpl::extractTag(absl::string_view stat_name, std::vector& return false; } +TagExtractorRe2Impl::TagExtractorRe2Impl(const std::string& name, const std::string regex, + const std::string& substr) + : TagExtractorImpl(name, regex, substr), + regex_(regex) {} + +bool TagExtractorRe2Impl::extractTag(absl::string_view stat_name, std::vector& tags, + IntervalSet& remove_characters) const { + PERF_OPERATION(perf); + + if (substrMismatch(stat_name)) { + PERF_RECORD(perf, "re2-skip-substr", name_); + return false; + } + + // remove_subexpr is the first submatch. It represents the portion of the string to be removed. + re2::StringPiece remove_subexpr, value_subexpr; + + // The regex must match and contain one or more subexpressions (all after the first are ignored). + if (re2::RE2::FullMatch(re2::StringPiece(stat_name.data(), stat_name.size()), regex_, + &remove_subexpr, &value_subexpr) && !remove_subexpr.empty()) { + + // value_subexpr is the optional second submatch. It is usually inside the first submatch + // (remove_subexpr) to allow the expression to strip off extra characters that should be removed + // from the string but also not necessary in the tag value ("." for example). If there is no + // second submatch, then the value_subexpr is the same as the remove_subexpr. + if (value_subexpr.empty()) { + value_subexpr = remove_subexpr; + } + + tags.emplace_back(); + Tag& tag = tags.back(); + tag.name_ = name_; + tag.value_ = std::string(value_subexpr); + + // Determines which characters to remove from stat_name to elide remove_subexpr. + std::string::size_type start = remove_subexpr.data() - stat_name.data(); + std::string::size_type end = remove_subexpr.data() + remove_subexpr.size() - stat_name.data(); + remove_characters.insert(start, end); + PERF_RECORD(perf, "re2-match", name_); + return true; + } + PERF_RECORD(perf, "re2-miss", name_); + return false; +} + } // namespace Stats } // namespace Envoy diff --git a/source/common/stats/tag_extractor_impl.h b/source/common/stats/tag_extractor_impl.h index 138f25b6af7c0..18df096e8d2b7 100644 --- a/source/common/stats/tag_extractor_impl.h +++ b/source/common/stats/tag_extractor_impl.h @@ -8,6 +8,8 @@ #include "absl/strings/string_view.h" +#include "re2/re2.h" + namespace Envoy { namespace Stats { @@ -23,13 +25,11 @@ class TagExtractorImpl : public TagExtractor { * @return TagExtractorPtr newly constructed TagExtractor. */ static TagExtractorPtr createTagExtractor(const std::string& name, const std::string& regex, - const std::string& substr = ""); + const std::string& substr = "", bool is_re2 = false); TagExtractorImpl(const std::string& name, const std::string& regex, const std::string& substr = ""); std::string name() const override { return name_; } - bool extractTag(absl::string_view tag_extracted_name, std::vector& tags, - IntervalSet& remove_characters) const override; absl::string_view prefixToken() const override { return prefix_; } /** @@ -39,7 +39,7 @@ class TagExtractorImpl : public TagExtractor { */ bool substrMismatch(absl::string_view stat_name) const; -private: +protected: /** * Examines a regex string, looking for the pattern: ^alphanumerics_with_underscores\. * Returns "alphanumerics_with_underscores" if that pattern is found, empty-string otherwise. @@ -50,8 +50,31 @@ class TagExtractorImpl : public TagExtractor { const std::string name_; const std::string prefix_; const std::string substr_; +}; + +class TagExtractorStdRegexImpl : public TagExtractorImpl { + public: + TagExtractorStdRegexImpl(const std::string& name, const std::string regex, + const std::string& substr = ""); + + bool extractTag(absl::string_view tag_extracted_name, std::vector& tags, + IntervalSet& remove_characters) const override; + + private: const std::regex regex_; }; +class TagExtractorRe2Impl : public TagExtractorImpl { + public: + TagExtractorRe2Impl(const std::string& name, const std::string regex, + const std::string& substr = ""); + + bool extractTag(absl::string_view tag_extracted_name, std::vector& tags, + IntervalSet& remove_characters) const override; + + private: + re2::RE2 regex_; +}; + } // namespace Stats } // namespace Envoy diff --git a/source/common/stats/tag_producer_impl.cc b/source/common/stats/tag_producer_impl.cc index f03a4b6553e3e..31401136ecee8 100644 --- a/source/common/stats/tag_producer_impl.cc +++ b/source/common/stats/tag_producer_impl.cc @@ -46,7 +46,8 @@ int TagProducerImpl::addExtractorsMatching(absl::string_view name) { for (const auto& desc : Config::TagNames::get().descriptorVec()) { if (desc.name_ == name) { addExtractor( - Stats::TagExtractorImpl::createTagExtractor(desc.name_, desc.regex_, desc.substr_)); + Stats::TagExtractorImpl::createTagExtractor(desc.name_, desc.regex_, desc.substr_, + desc.use_re2_)); ++num_found; } } @@ -102,7 +103,8 @@ TagProducerImpl::addDefaultExtractors(const envoy::config::metrics::v2::StatsCon for (const auto& desc : Config::TagNames::get().descriptorVec()) { names.emplace(desc.name_); addExtractor( - Stats::TagExtractorImpl::createTagExtractor(desc.name_, desc.regex_, desc.substr_)); + Stats::TagExtractorImpl::createTagExtractor(desc.name_, desc.regex_, desc.substr_, + desc.use_re2_)); } } return names; diff --git a/test/common/common/re_speed_test.cc b/test/common/common/re_speed_test.cc index 1bd5064357ac1..1472709f28b77 100644 --- a/test/common/common/re_speed_test.cc +++ b/test/common/common/re_speed_test.cc @@ -18,7 +18,7 @@ static const char* cluster_inputs[] = { }; static const char cluster_re_pattern[] = "^cluster\\.((.*?)\\.)"; -static const char cluster_re_alt_pattern[] = "^cluster\\.([^\\.]+)\\..*"; +static const char cluster_re_alt_pattern[] = "^cluster\\.(([^\\.]+)\\.).*"; static void BM_StdRegex(benchmark::State& state) { std::regex re(cluster_re_pattern); @@ -76,8 +76,9 @@ static void BM_StdRegexStringViewAltPattern(benchmark::State& state) { for (absl::string_view cluster_input : inputs) { std::match_results smatch; if (std::regex_search(cluster_input.begin(), cluster_input.end(), smatch, re)) { - ASSERT(smatch.size() >= 2); - ASSERT(smatch[1] == "match"); + ASSERT(smatch.size() >= 3); + ASSERT(smatch[1] == "match."); + ASSERT(smatch[2] == "match"); ++passes; } } @@ -108,9 +109,10 @@ static void BM_RE2_AltPattern(benchmark::State& state) { uint32_t passes = 0; for (auto _ : state) { for (const char* cluster_input : cluster_inputs) { - re2::StringPiece match; - if (re2::RE2::PartialMatch(cluster_input, re, &match)) { - ASSERT(match == "match"); + re2::StringPiece match1, match2; + if (re2::RE2::PartialMatch(cluster_input, re, &match1, &match2)) { + ASSERT(match1 == "match."); + ASSERT(match2 == "match"); ++passes; } } diff --git a/test/common/stats/tag_extractor_impl_test.cc b/test/common/stats/tag_extractor_impl_test.cc index f2d55f7a4d7cd..acc1c97eb2aef 100644 --- a/test/common/stats/tag_extractor_impl_test.cc +++ b/test/common/stats/tag_extractor_impl_test.cc @@ -17,7 +17,7 @@ namespace Envoy { namespace Stats { TEST(TagExtractorTest, TwoSubexpressions) { - TagExtractorImpl tag_extractor("cluster_name", "^cluster\\.((.+?)\\.)"); + TagExtractorStdRegexImpl tag_extractor("cluster_name", "^cluster\\.((.+?)\\.)"); EXPECT_EQ("cluster_name", tag_extractor.name()); std::string name = "cluster.test_cluster.upstream_cx_total"; std::vector tags; @@ -42,6 +42,12 @@ TEST(TagExtractorTest, RE2Variants) { ASSERT_TRUE(re2::RE2::FullMatch("cluster.test_cluster.upstream_cx_total", alternate_re, &match1)); EXPECT_EQ("test_cluster", std::string(match1)); + re2::RE2 alternate_re2("^cluster\\.(([^\\.]+)\\.).*"); + ASSERT_TRUE(re2::RE2::FullMatch("cluster.test_cluster.upstream_cx_total", alternate_re2, + &match1, &match2)); + EXPECT_EQ("test_cluster.", std::string(match1)); + EXPECT_EQ("test_cluster", std::string(match2)); + std::regex sr("^cluster\\.((.*?)\\.)"); std::match_results smatch; ASSERT_TRUE(std::regex_search("cluster.test_cluster.upstream_cx_total", smatch, sr)); @@ -51,7 +57,7 @@ TEST(TagExtractorTest, RE2Variants) { } TEST(TagExtractorTest, SingleSubexpression) { - TagExtractorImpl tag_extractor("listner_port", "^listener\\.(\\d+?\\.)"); + TagExtractorStdRegexImpl tag_extractor("listner_port", "^listener\\.(\\d+?\\.)"); std::string name = "listener.80.downstream_cx_total"; std::vector tags; IntervalSetImpl remove_characters; @@ -64,24 +70,24 @@ TEST(TagExtractorTest, SingleSubexpression) { } TEST(TagExtractorTest, substrMismatch) { - TagExtractorImpl tag_extractor("listner_port", "^listener\\.(\\d+?\\.)\\.foo\\.", ".foo."); + TagExtractorStdRegexImpl tag_extractor("listner_port", "^listener\\.(\\d+?\\.)\\.foo\\.", ".foo."); EXPECT_TRUE(tag_extractor.substrMismatch("listener.80.downstream_cx_total")); EXPECT_FALSE(tag_extractor.substrMismatch("listener.80.downstream_cx_total.foo.bar")); } TEST(TagExtractorTest, noSubstrMismatch) { - TagExtractorImpl tag_extractor("listner_port", "^listener\\.(\\d+?\\.)\\.foo\\."); + TagExtractorStdRegexImpl tag_extractor("listner_port", "^listener\\.(\\d+?\\.)\\.foo\\."); EXPECT_FALSE(tag_extractor.substrMismatch("listener.80.downstream_cx_total")); EXPECT_FALSE(tag_extractor.substrMismatch("listener.80.downstream_cx_total.foo.bar")); } TEST(TagExtractorTest, EmptyName) { - EXPECT_THROW_WITH_MESSAGE(TagExtractorImpl::createTagExtractor("", "^listener\\.(\\d+?\\.)"), + EXPECT_THROW_WITH_MESSAGE(TagExtractorStdRegexImpl::createTagExtractor("", "^listener\\.(\\d+?\\.)"), EnvoyException, "tag_name cannot be empty"); } TEST(TagExtractorTest, BadRegex) { - EXPECT_THROW_WITH_REGEX(TagExtractorImpl::createTagExtractor("cluster_name", "+invalid"), + EXPECT_THROW_WITH_REGEX(TagExtractorStdRegexImpl::createTagExtractor("cluster_name", "+invalid"), EnvoyException, "Invalid regex '\\+invalid':"); } @@ -373,7 +379,7 @@ TEST(TagExtractorTest, DefaultTagExtractors) { TEST(TagExtractorTest, ExtractRegexPrefix) { TagExtractorPtr tag_extractor; // Keep tag_extractor in this scope to prolong prefix lifetime. auto extractRegexPrefix = [&tag_extractor](const std::string& regex) -> absl::string_view { - tag_extractor = TagExtractorImpl::createTagExtractor("foo", regex); + tag_extractor = TagExtractorStdRegexImpl::createTagExtractor("foo", regex); return tag_extractor->prefixToken(); }; @@ -388,7 +394,7 @@ TEST(TagExtractorTest, ExtractRegexPrefix) { } TEST(TagExtractorTest, CreateTagExtractorNoRegex) { - EXPECT_THROW_WITH_REGEX(TagExtractorImpl::createTagExtractor("no such default tag", ""), + EXPECT_THROW_WITH_REGEX(TagExtractorStdRegexImpl::createTagExtractor("no such default tag", ""), EnvoyException, "^No regex specified for tag specifier and no default"); } From d692af2337be664c2e9ad97fedb61f2888f930e4 Mon Sep 17 00:00:00 2001 From: Joshua Marantz Date: Sun, 3 Nov 2019 13:56:49 -0500 Subject: [PATCH 05/16] clean up names of perf categories. Signed-off-by: Joshua Marantz --- source/common/config/well_known_names.cc | 1 + source/common/stats/tag_extractor_impl.cc | 4 ++-- 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/source/common/config/well_known_names.cc b/source/common/config/well_known_names.cc index e9c892fa63b97..8e3e76d83fe78 100644 --- a/source/common/config/well_known_names.cc +++ b/source/common/config/well_known_names.cc @@ -89,6 +89,7 @@ TagNameValues::TagNameValues() { addRegex(RATELIMIT_PREFIX, R"(^ratelimit\.((.*?)\.)\w+?$)"); // cluster.(.)* + //addRegex(CLUSTER_NAME, "^cluster\\.((.*?)\\.)"); addRe2(CLUSTER_NAME, "^cluster\\.(([^\\.]+)\\.).*"); // listener.[
.]http.(.)* diff --git a/source/common/stats/tag_extractor_impl.cc b/source/common/stats/tag_extractor_impl.cc index 7bb206b5915ff..3d578f18b5946 100644 --- a/source/common/stats/tag_extractor_impl.cc +++ b/source/common/stats/tag_extractor_impl.cc @@ -79,7 +79,7 @@ bool TagExtractorStdRegexImpl::extractTag(absl::string_view stat_name, std::vect PERF_OPERATION(perf); if (substrMismatch(stat_name)) { - PERF_RECORD(perf, "re-skip-substr", name_); + PERF_RECORD(perf, "re-skip", name_); return false; } @@ -123,7 +123,7 @@ bool TagExtractorRe2Impl::extractTag(absl::string_view stat_name, std::vector Date: Sun, 3 Nov 2019 14:00:06 -0500 Subject: [PATCH 06/16] format Signed-off-by: Joshua Marantz --- source/common/config/well_known_names.cc | 4 ++-- source/common/stats/tag_extractor_impl.cc | 12 +++++------- source/common/stats/tag_extractor_impl.h | 9 ++++----- source/common/stats/tag_producer_impl.cc | 10 ++++------ .../filters/network/common/redis/codec_impl.cc | 4 ++-- test/common/common/re_speed_test.cc | 15 ++++++++------- test/common/stats/tag_extractor_impl_test.cc | 17 +++++++++-------- 7 files changed, 34 insertions(+), 37 deletions(-) diff --git a/source/common/config/well_known_names.cc b/source/common/config/well_known_names.cc index 8e3e76d83fe78..d5af51bb3ef95 100644 --- a/source/common/config/well_known_names.cc +++ b/source/common/config/well_known_names.cc @@ -89,8 +89,8 @@ TagNameValues::TagNameValues() { addRegex(RATELIMIT_PREFIX, R"(^ratelimit\.((.*?)\.)\w+?$)"); // cluster.(.)* - //addRegex(CLUSTER_NAME, "^cluster\\.((.*?)\\.)"); - addRe2(CLUSTER_NAME, "^cluster\\.(([^\\.]+)\\.).*"); + // addRegex(CLUSTER_NAME, "^cluster\\.((.*?)\\.)"); + addRe2(CLUSTER_NAME, "^cluster\\.(([^\\.]+)\\.).*"); // listener.[
.]http.(.)* addRegex(HTTP_CONN_MANAGER_PREFIX, R"(^listener(?=\.).*?\.http\.((.*?)\.))", ".http."); diff --git a/source/common/stats/tag_extractor_impl.cc b/source/common/stats/tag_extractor_impl.cc index 3d578f18b5946..d60c261dbc3be 100644 --- a/source/common/stats/tag_extractor_impl.cc +++ b/source/common/stats/tag_extractor_impl.cc @@ -48,8 +48,7 @@ std::string TagExtractorImpl::extractRegexPrefix(absl::string_view regex) { TagExtractorPtr TagExtractorImpl::createTagExtractor(const std::string& name, const std::string& regex, - const std::string& substr, - bool use_re2) { + const std::string& substr, bool use_re2) { if (name.empty()) { throw EnvoyException("tag_name cannot be empty"); @@ -71,8 +70,7 @@ bool TagExtractorImpl::substrMismatch(absl::string_view stat_name) const { TagExtractorStdRegexImpl::TagExtractorStdRegexImpl(const std::string& name, const std::string regex, const std::string& substr) - : TagExtractorImpl(name, regex, substr), - regex_(Regex::Utility::parseStdRegex(regex)) {} + : TagExtractorImpl(name, regex, substr), regex_(Regex::Utility::parseStdRegex(regex)) {} bool TagExtractorStdRegexImpl::extractTag(absl::string_view stat_name, std::vector& tags, IntervalSet& remove_characters) const { @@ -115,8 +113,7 @@ bool TagExtractorStdRegexImpl::extractTag(absl::string_view stat_name, std::vect TagExtractorRe2Impl::TagExtractorRe2Impl(const std::string& name, const std::string regex, const std::string& substr) - : TagExtractorImpl(name, regex, substr), - regex_(regex) {} + : TagExtractorImpl(name, regex, substr), regex_(regex) {} bool TagExtractorRe2Impl::extractTag(absl::string_view stat_name, std::vector& tags, IntervalSet& remove_characters) const { @@ -132,7 +129,8 @@ bool TagExtractorRe2Impl::extractTag(absl::string_view stat_name, std::vector& tags, IntervalSet& remove_characters) const override; - private: +private: const std::regex regex_; }; class TagExtractorRe2Impl : public TagExtractorImpl { - public: +public: TagExtractorRe2Impl(const std::string& name, const std::string regex, const std::string& substr = ""); bool extractTag(absl::string_view tag_extracted_name, std::vector& tags, IntervalSet& remove_characters) const override; - private: +private: re2::RE2 regex_; }; diff --git a/source/common/stats/tag_producer_impl.cc b/source/common/stats/tag_producer_impl.cc index 31401136ecee8..cc6ae13e6943f 100644 --- a/source/common/stats/tag_producer_impl.cc +++ b/source/common/stats/tag_producer_impl.cc @@ -45,9 +45,8 @@ int TagProducerImpl::addExtractorsMatching(absl::string_view name) { int num_found = 0; for (const auto& desc : Config::TagNames::get().descriptorVec()) { if (desc.name_ == name) { - addExtractor( - Stats::TagExtractorImpl::createTagExtractor(desc.name_, desc.regex_, desc.substr_, - desc.use_re2_)); + addExtractor(Stats::TagExtractorImpl::createTagExtractor(desc.name_, desc.regex_, + desc.substr_, desc.use_re2_)); ++num_found; } } @@ -102,9 +101,8 @@ TagProducerImpl::addDefaultExtractors(const envoy::config::metrics::v2::StatsCon if (!config.has_use_all_default_tags() || config.use_all_default_tags().value()) { for (const auto& desc : Config::TagNames::get().descriptorVec()) { names.emplace(desc.name_); - addExtractor( - Stats::TagExtractorImpl::createTagExtractor(desc.name_, desc.regex_, desc.substr_, - desc.use_re2_)); + addExtractor(Stats::TagExtractorImpl::createTagExtractor(desc.name_, desc.regex_, + desc.substr_, desc.use_re2_)); } } return names; diff --git a/source/extensions/filters/network/common/redis/codec_impl.cc b/source/extensions/filters/network/common/redis/codec_impl.cc index ab01d85d6ebba..a63c8a7ab4faa 100644 --- a/source/extensions/filters/network/common/redis/codec_impl.cc +++ b/source/extensions/filters/network/common/redis/codec_impl.cc @@ -313,8 +313,8 @@ RespValue::CompositeArray::CompositeArrayConstIterator::operator++() { return *this; } -bool RespValue::CompositeArray::CompositeArrayConstIterator::operator!=( - const CompositeArrayConstIterator& rhs) const { +bool RespValue::CompositeArray::CompositeArrayConstIterator:: +operator!=(const CompositeArrayConstIterator& rhs) const { return command_ != (rhs.command_) || &array_ != &(rhs.array_) || index_ != rhs.index_ || first_ != rhs.first_; } diff --git a/test/common/common/re_speed_test.cc b/test/common/common/re_speed_test.cc index 1472709f28b77..3300cfc59829b 100644 --- a/test/common/common/re_speed_test.cc +++ b/test/common/common/re_speed_test.cc @@ -1,5 +1,5 @@ -// Note: this should be run with --compilation_mode=opt, and would benefit from a -// quiescent system with disabled cstate power management. +// Note: this should be run with --compilation_mode=opt, and would benefit from +// a quiescent system with disabled cstate power management. #include @@ -7,14 +7,15 @@ #include "absl/strings/string_view.h" #include "benchmark/benchmark.h" - #include "re2/re2.h" +// NOLINT(namespace-envoy) + static const char* cluster_inputs[] = { - "cluster.no_trailing_dot", - "cluster.match.", - "cluster.match.normal", - "cluster.match.and.a.whole.lot.of.things.coming.after.the.matches.really.too.much.stuff", + "cluster.no_trailing_dot", + "cluster.match.", + "cluster.match.normal", + "cluster.match.and.a.whole.lot.of.things.coming.after.the.matches.really.too.much.stuff", }; static const char cluster_re_pattern[] = "^cluster\\.((.*?)\\.)"; diff --git a/test/common/stats/tag_extractor_impl_test.cc b/test/common/stats/tag_extractor_impl_test.cc index acc1c97eb2aef..c0ebaafed2e79 100644 --- a/test/common/stats/tag_extractor_impl_test.cc +++ b/test/common/stats/tag_extractor_impl_test.cc @@ -10,7 +10,6 @@ #include "test/test_common/utility.h" #include "gtest/gtest.h" - #include "re2/re2.h" namespace Envoy { @@ -33,8 +32,8 @@ TEST(TagExtractorTest, TwoSubexpressions) { TEST(TagExtractorTest, RE2Variants) { re2::RE2 re("^cluster\\.((.*?)\\.)"); re2::StringPiece match1, match2; - ASSERT_TRUE(re2::RE2::PartialMatch("cluster.test_cluster.upstream_cx_total", re, &match1, - &match2)); + ASSERT_TRUE( + re2::RE2::PartialMatch("cluster.test_cluster.upstream_cx_total", re, &match1, &match2)); EXPECT_EQ("test_cluster.", std::string(match1)); EXPECT_EQ("test_cluster", std::string(match2)); @@ -43,8 +42,8 @@ TEST(TagExtractorTest, RE2Variants) { EXPECT_EQ("test_cluster", std::string(match1)); re2::RE2 alternate_re2("^cluster\\.(([^\\.]+)\\.).*"); - ASSERT_TRUE(re2::RE2::FullMatch("cluster.test_cluster.upstream_cx_total", alternate_re2, - &match1, &match2)); + ASSERT_TRUE(re2::RE2::FullMatch("cluster.test_cluster.upstream_cx_total", alternate_re2, &match1, + &match2)); EXPECT_EQ("test_cluster.", std::string(match1)); EXPECT_EQ("test_cluster", std::string(match2)); @@ -70,7 +69,8 @@ TEST(TagExtractorTest, SingleSubexpression) { } TEST(TagExtractorTest, substrMismatch) { - TagExtractorStdRegexImpl tag_extractor("listner_port", "^listener\\.(\\d+?\\.)\\.foo\\.", ".foo."); + TagExtractorStdRegexImpl tag_extractor("listner_port", "^listener\\.(\\d+?\\.)\\.foo\\.", + ".foo."); EXPECT_TRUE(tag_extractor.substrMismatch("listener.80.downstream_cx_total")); EXPECT_FALSE(tag_extractor.substrMismatch("listener.80.downstream_cx_total.foo.bar")); } @@ -82,8 +82,9 @@ TEST(TagExtractorTest, noSubstrMismatch) { } TEST(TagExtractorTest, EmptyName) { - EXPECT_THROW_WITH_MESSAGE(TagExtractorStdRegexImpl::createTagExtractor("", "^listener\\.(\\d+?\\.)"), - EnvoyException, "tag_name cannot be empty"); + EXPECT_THROW_WITH_MESSAGE( + TagExtractorStdRegexImpl::createTagExtractor("", "^listener\\.(\\d+?\\.)"), EnvoyException, + "tag_name cannot be empty"); } TEST(TagExtractorTest, BadRegex) { From a4f297c8e0cebcd74c5e7546f6bd850ebca7609e Mon Sep 17 00:00:00 2001 From: Joshua Marantz Date: Tue, 5 Nov 2019 21:38:57 -0500 Subject: [PATCH 07/16] format Signed-off-by: Joshua Marantz --- source/extensions/filters/network/common/redis/codec_impl.cc | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/source/extensions/filters/network/common/redis/codec_impl.cc b/source/extensions/filters/network/common/redis/codec_impl.cc index c7ebc51efa4b2..6b62e228bfa60 100644 --- a/source/extensions/filters/network/common/redis/codec_impl.cc +++ b/source/extensions/filters/network/common/redis/codec_impl.cc @@ -315,8 +315,8 @@ RespValue::CompositeArray::CompositeArrayConstIterator::operator++() { return *this; } -bool RespValue::CompositeArray::CompositeArrayConstIterator:: -operator!=(const CompositeArrayConstIterator& rhs) const { +bool RespValue::CompositeArray::CompositeArrayConstIterator::operator!=( + const CompositeArrayConstIterator& rhs) const { return command_ != (rhs.command_) || &array_ != &(rhs.array_) || index_ != rhs.index_ || first_ != rhs.first_; } From 46a39009463b5dd539557561bf68390dd9557df9 Mon Sep 17 00:00:00 2001 From: Joshua Marantz Date: Mon, 9 Nov 2020 09:27:45 -0500 Subject: [PATCH 08/16] review comments (base-class name and enums). Signed-off-by: Joshua Marantz --- source/common/common/regex.h | 2 ++ source/common/config/BUILD | 1 + source/common/config/well_known_names.cc | 4 ++-- source/common/config/well_known_names.h | 3 ++- source/common/stats/tag_extractor_impl.cc | 25 ++++++++++++----------- source/common/stats/tag_extractor_impl.h | 13 +++++++----- source/common/stats/tag_producer_impl.cc | 10 ++++----- 7 files changed, 33 insertions(+), 25 deletions(-) diff --git a/source/common/common/regex.h b/source/common/common/regex.h index 68cb7ff8074d1..e284b72822780 100644 --- a/source/common/common/regex.h +++ b/source/common/common/regex.h @@ -9,6 +9,8 @@ namespace Envoy { namespace Regex { +enum class Type {Re2, StdRegex}; + /** * Utilities for constructing regular expressions. */ diff --git a/source/common/config/BUILD b/source/common/config/BUILD index cf1ac31142ffb..340c2a6c1e446 100644 --- a/source/common/config/BUILD +++ b/source/common/config/BUILD @@ -450,6 +450,7 @@ envoy_cc_library( hdrs = ["well_known_names.h"], deps = [ "//source/common/common:assert_lib", + "//source/common/common:regex_lib", "//source/common/singleton:const_singleton", ], ) diff --git a/source/common/config/well_known_names.cc b/source/common/config/well_known_names.cc index b893edabc7329..211d19207821c 100644 --- a/source/common/config/well_known_names.cc +++ b/source/common/config/well_known_names.cc @@ -120,12 +120,12 @@ TagNameValues::TagNameValues() { void TagNameValues::addRegex(const std::string& name, const std::string& regex, const std::string& substr) { - descriptor_vec_.emplace_back(Descriptor{name, regex, substr, false}); + descriptor_vec_.emplace_back(Descriptor{name, regex, substr, Regex::Type::StdRegex}); } void TagNameValues::addRe2(const std::string& name, const std::string& regex, const std::string& substr) { - descriptor_vec_.emplace_back(Descriptor{name, regex, substr, true}); + descriptor_vec_.emplace_back(Descriptor{name, regex, substr, Regex::Type::Re2}); } } // namespace Config diff --git a/source/common/config/well_known_names.h b/source/common/config/well_known_names.h index cef045d7b19f7..c5d3baadaef31 100644 --- a/source/common/config/well_known_names.h +++ b/source/common/config/well_known_names.h @@ -6,6 +6,7 @@ #include "envoy/common/exception.h" #include "common/common/assert.h" +#include "common/common/regex.h" #include "common/singleton/const_singleton.h" namespace Envoy { @@ -65,7 +66,7 @@ class TagNameValues { const std::string name_; const std::string regex_; const std::string substr_; - bool use_re2_; + Regex::Type re_type_; }; // Cluster name tag diff --git a/source/common/stats/tag_extractor_impl.cc b/source/common/stats/tag_extractor_impl.cc index d60c261dbc3be..08584e96ae925 100644 --- a/source/common/stats/tag_extractor_impl.cc +++ b/source/common/stats/tag_extractor_impl.cc @@ -23,11 +23,11 @@ bool regexStartsWithDot(absl::string_view regex) { } // namespace -TagExtractorImpl::TagExtractorImpl(const std::string& name, const std::string& regex, +TagExtractorImplBase::TagExtractorImplBase(const std::string& name, const std::string& regex, const std::string& substr) : name_(name), prefix_(std::string(extractRegexPrefix(regex))), substr_(substr) {} -std::string TagExtractorImpl::extractRegexPrefix(absl::string_view regex) { +std::string TagExtractorImplBase::extractRegexPrefix(absl::string_view regex) { std::string prefix; if (absl::StartsWith(regex, "^")) { for (absl::string_view::size_type i = 1; i < regex.size(); ++i) { @@ -46,10 +46,10 @@ std::string TagExtractorImpl::extractRegexPrefix(absl::string_view regex) { return prefix; } -TagExtractorPtr TagExtractorImpl::createTagExtractor(const std::string& name, - const std::string& regex, - const std::string& substr, bool use_re2) { - +TagExtractorPtr TagExtractorImplBase::createTagExtractor(const std::string& name, + const std::string& regex, + const std::string& substr, + Regex::Type re_type) { if (name.empty()) { throw EnvoyException("tag_name cannot be empty"); } @@ -58,19 +58,20 @@ TagExtractorPtr TagExtractorImpl::createTagExtractor(const std::string& name, throw EnvoyException(fmt::format( "No regex specified for tag specifier and no default regex for name: '{}'", name)); } - if (use_re2) { - return std::make_unique(name, regex, substr); + switch (re_type) { + case Regex::Type::Re2: return std::make_unique(name, regex, substr); + case Regex::Type::StdRegex: return std::make_unique( + name, regex, substr); } - return std::make_unique(name, regex, substr); } -bool TagExtractorImpl::substrMismatch(absl::string_view stat_name) const { +bool TagExtractorImplBase::substrMismatch(absl::string_view stat_name) const { return !substr_.empty() && stat_name.find(substr_) == absl::string_view::npos; } TagExtractorStdRegexImpl::TagExtractorStdRegexImpl(const std::string& name, const std::string regex, const std::string& substr) - : TagExtractorImpl(name, regex, substr), regex_(Regex::Utility::parseStdRegex(regex)) {} + : TagExtractorImplBase(name, regex, substr), regex_(Regex::Utility::parseStdRegex(regex)) {} bool TagExtractorStdRegexImpl::extractTag(absl::string_view stat_name, std::vector& tags, IntervalSet& remove_characters) const { @@ -113,7 +114,7 @@ bool TagExtractorStdRegexImpl::extractTag(absl::string_view stat_name, std::vect TagExtractorRe2Impl::TagExtractorRe2Impl(const std::string& name, const std::string regex, const std::string& substr) - : TagExtractorImpl(name, regex, substr), regex_(regex) {} + : TagExtractorImplBase(name, regex, substr), regex_(regex) {} bool TagExtractorRe2Impl::extractTag(absl::string_view stat_name, std::vector& tags, IntervalSet& remove_characters) const { diff --git a/source/common/stats/tag_extractor_impl.h b/source/common/stats/tag_extractor_impl.h index 000ce46067838..0c344e51099d2 100644 --- a/source/common/stats/tag_extractor_impl.h +++ b/source/common/stats/tag_extractor_impl.h @@ -6,13 +6,15 @@ #include "envoy/stats/tag_extractor.h" +#include "common/common/regex.h" + #include "absl/strings/string_view.h" #include "re2/re2.h" namespace Envoy { namespace Stats { -class TagExtractorImpl : public TagExtractor { +class TagExtractorImplBase : public TagExtractor { public: /** * Creates a tag extractor from the regex provided. name and regex must be non-empty. @@ -24,9 +26,10 @@ class TagExtractorImpl : public TagExtractor { * @return TagExtractorPtr newly constructed TagExtractor. */ static TagExtractorPtr createTagExtractor(const std::string& name, const std::string& regex, - const std::string& substr = "", bool is_re2 = false); + const std::string& substr = "", + Regex::Type re_type = Regex::Type::StdRegex); - TagExtractorImpl(const std::string& name, const std::string& regex, + TagExtractorImplBase(const std::string& name, const std::string& regex, const std::string& substr = ""); std::string name() const override { return name_; } absl::string_view prefixToken() const override { return prefix_; } @@ -51,7 +54,7 @@ class TagExtractorImpl : public TagExtractor { const std::string substr_; }; -class TagExtractorStdRegexImpl : public TagExtractorImpl { +class TagExtractorStdRegexImpl : public TagExtractorImplBase { public: TagExtractorStdRegexImpl(const std::string& name, const std::string regex, const std::string& substr = ""); @@ -63,7 +66,7 @@ class TagExtractorStdRegexImpl : public TagExtractorImpl { const std::regex regex_; }; -class TagExtractorRe2Impl : public TagExtractorImpl { +class TagExtractorRe2Impl : public TagExtractorImplBase { public: TagExtractorRe2Impl(const std::string& name, const std::string regex, const std::string& substr = ""); diff --git a/source/common/stats/tag_producer_impl.cc b/source/common/stats/tag_producer_impl.cc index 8549a6626b1bb..5befb7e5f7275 100644 --- a/source/common/stats/tag_producer_impl.cc +++ b/source/common/stats/tag_producer_impl.cc @@ -34,7 +34,7 @@ TagProducerImpl::TagProducerImpl(const envoy::config::metrics::v3::StatsConfig& "No regex specified for tag specifier and no default regex for name: '{}'", name)); } } else { - addExtractor(Stats::TagExtractorImpl::createTagExtractor(name, tag_specifier.regex())); + addExtractor(Stats::TagExtractorImplBase::createTagExtractor(name, tag_specifier.regex())); } } else if (tag_specifier.tag_value_case() == envoy::config::metrics::v3::TagSpecifier::TagValueCase::kFixedValue) { @@ -47,8 +47,8 @@ int TagProducerImpl::addExtractorsMatching(absl::string_view name) { int num_found = 0; for (const auto& desc : Config::TagNames::get().descriptorVec()) { if (desc.name_ == name) { - addExtractor(Stats::TagExtractorImpl::createTagExtractor(desc.name_, desc.regex_, - desc.substr_, desc.use_re2_)); + addExtractor(Stats::TagExtractorImplBase::createTagExtractor(desc.name_, desc.regex_, + desc.substr_, desc.re_type_)); ++num_found; } } @@ -103,8 +103,8 @@ TagProducerImpl::addDefaultExtractors(const envoy::config::metrics::v3::StatsCon if (!config.has_use_all_default_tags() || config.use_all_default_tags().value()) { for (const auto& desc : Config::TagNames::get().descriptorVec()) { names.emplace(desc.name_); - addExtractor(Stats::TagExtractorImpl::createTagExtractor(desc.name_, desc.regex_, - desc.substr_, desc.use_re2_)); + addExtractor(Stats::TagExtractorImplBase::createTagExtractor(desc.name_, desc.regex_, + desc.substr_, desc.re_type_)); } } return names; From 0fa1ca22cb17b9b81591055a68c12efecce26e67 Mon Sep 17 00:00:00 2001 From: Joshua Marantz Date: Mon, 9 Nov 2020 09:32:53 -0500 Subject: [PATCH 09/16] format Signed-off-by: Joshua Marantz --- source/common/common/regex.h | 2 +- source/common/stats/tag_extractor_impl.cc | 9 +++++---- source/common/stats/tag_extractor_impl.h | 2 +- test/common/stats/BUILD | 2 +- 4 files changed, 8 insertions(+), 7 deletions(-) diff --git a/source/common/common/regex.h b/source/common/common/regex.h index e284b72822780..2fdcd52ebc1ce 100644 --- a/source/common/common/regex.h +++ b/source/common/common/regex.h @@ -9,7 +9,7 @@ namespace Envoy { namespace Regex { -enum class Type {Re2, StdRegex}; +enum class Type { Re2, StdRegex }; /** * Utilities for constructing regular expressions. diff --git a/source/common/stats/tag_extractor_impl.cc b/source/common/stats/tag_extractor_impl.cc index 08584e96ae925..889b7d20a9e4d 100644 --- a/source/common/stats/tag_extractor_impl.cc +++ b/source/common/stats/tag_extractor_impl.cc @@ -24,7 +24,7 @@ bool regexStartsWithDot(absl::string_view regex) { } // namespace TagExtractorImplBase::TagExtractorImplBase(const std::string& name, const std::string& regex, - const std::string& substr) + const std::string& substr) : name_(name), prefix_(std::string(extractRegexPrefix(regex))), substr_(substr) {} std::string TagExtractorImplBase::extractRegexPrefix(absl::string_view regex) { @@ -59,9 +59,10 @@ TagExtractorPtr TagExtractorImplBase::createTagExtractor(const std::string& name "No regex specified for tag specifier and no default regex for name: '{}'", name)); } switch (re_type) { - case Regex::Type::Re2: return std::make_unique(name, regex, substr); - case Regex::Type::StdRegex: return std::make_unique( - name, regex, substr); + case Regex::Type::Re2: + return std::make_unique(name, regex, substr); + case Regex::Type::StdRegex: + return std::make_unique(name, regex, substr); } } diff --git a/source/common/stats/tag_extractor_impl.h b/source/common/stats/tag_extractor_impl.h index 0c344e51099d2..ffe18e391bf6b 100644 --- a/source/common/stats/tag_extractor_impl.h +++ b/source/common/stats/tag_extractor_impl.h @@ -30,7 +30,7 @@ class TagExtractorImplBase : public TagExtractor { Regex::Type re_type = Regex::Type::StdRegex); TagExtractorImplBase(const std::string& name, const std::string& regex, - const std::string& substr = ""); + const std::string& substr = ""); std::string name() const override { return name_; } absl::string_view prefixToken() const override { return prefix_; } diff --git a/test/common/stats/BUILD b/test/common/stats/BUILD index 0db87dc11bd42..fae0bc4f5539d 100644 --- a/test/common/stats/BUILD +++ b/test/common/stats/BUILD @@ -208,7 +208,7 @@ envoy_cc_test( "//source/common/stats:tag_producer_lib", "//test/test_common:utility_lib", "@com_googlesource_code_re2//:re2", - "@envoy_api//envoy/config/metrics/v2:pkg_cc_proto", + "@envoy_api//envoy/config/metrics/v3:pkg_cc_proto", ], ) From fad62de9dec70385b49ba700850a45e28372c43f Mon Sep 17 00:00:00 2001 From: Joshua Marantz Date: Mon, 9 Nov 2020 09:51:22 -0500 Subject: [PATCH 10/16] Remove extraneous main. Signed-off-by: Joshua Marantz --- test/common/common/re_speed_test.cc | 9 --------- 1 file changed, 9 deletions(-) diff --git a/test/common/common/re_speed_test.cc b/test/common/common/re_speed_test.cc index 3300cfc59829b..f8dce79f31784 100644 --- a/test/common/common/re_speed_test.cc +++ b/test/common/common/re_speed_test.cc @@ -121,12 +121,3 @@ static void BM_RE2_AltPattern(benchmark::State& state) { RELEASE_ASSERT(passes > 0, ""); } BENCHMARK(BM_RE2_AltPattern); - -// Boilerplate main(), which discovers benchmarks in the same file and runs them. -int main(int argc, char** argv) { - benchmark::Initialize(&argc, argv); - if (benchmark::ReportUnrecognizedArguments(argc, argv)) { - return 1; - } - benchmark::RunSpecifiedBenchmarks(); -} From 1976786bebff0154c0d773d50d29e7b643506dd3 Mon Sep 17 00:00:00 2001 From: Joshua Marantz Date: Mon, 9 Nov 2020 22:13:18 -0500 Subject: [PATCH 11/16] try to fix clang-tidy problems. Signed-off-by: Joshua Marantz --- source/common/stats/BUILD | 1 + source/common/stats/tag_extractor_impl.cc | 2 ++ test/common/stats/thread_local_store_speed_test.cc | 2 ++ 3 files changed, 5 insertions(+) diff --git a/source/common/stats/BUILD b/source/common/stats/BUILD index fb85280639340..edc68f4774e7b 100644 --- a/source/common/stats/BUILD +++ b/source/common/stats/BUILD @@ -195,6 +195,7 @@ envoy_cc_library( hdrs = ["tag_extractor_impl.h"], deps = [ "//include/envoy/stats:stats_interface", + "//source/common/common:assert_lib", "//source/common/common:perf_annotation_lib", "//source/common/common:regex_lib", ], diff --git a/source/common/stats/tag_extractor_impl.cc b/source/common/stats/tag_extractor_impl.cc index 889b7d20a9e4d..980f13b0a4a99 100644 --- a/source/common/stats/tag_extractor_impl.cc +++ b/source/common/stats/tag_extractor_impl.cc @@ -5,6 +5,7 @@ #include "envoy/common/exception.h" +#include "common/common/assert.h" #include "common/common/fmt.h" #include "common/common/perf_annotation.h" #include "common/common/regex.h" @@ -64,6 +65,7 @@ TagExtractorPtr TagExtractorImplBase::createTagExtractor(const std::string& name case Regex::Type::StdRegex: return std::make_unique(name, regex, substr); } + NOT_REACHED_GCOVR_EXCL_LINE; } bool TagExtractorImplBase::substrMismatch(absl::string_view stat_name) const { diff --git a/test/common/stats/thread_local_store_speed_test.cc b/test/common/stats/thread_local_store_speed_test.cc index 3eff52a78d97c..5208fdaed9f5c 100644 --- a/test/common/stats/thread_local_store_speed_test.cc +++ b/test/common/stats/thread_local_store_speed_test.cc @@ -79,6 +79,7 @@ class ThreadLocalStorePerf { // Tests the single-threaded performance of the thread-local-store stats caches // without having initialized tls. +// NOLINTNEXTLINE(readability-identifier-naming) static void BM_StatsNoTls(benchmark::State& state) { Envoy::ThreadLocalStorePerf context; @@ -91,6 +92,7 @@ BENCHMARK(BM_StatsNoTls); // Tests the single-threaded performance of the thread-local-store stats caches // with tls. Note that this test is still single-threaded, and so there's only // one replica of the tls cache. +// NOLINTNEXTLINE(readability-identifier-naming) static void BM_StatsWithTls(benchmark::State& state) { Envoy::ThreadLocalStorePerf context; context.initThreading(); From 8c79d6cf547197834a523686575c7c9d7db7bbcb Mon Sep 17 00:00:00 2001 From: Joshua Marantz Date: Wed, 11 Nov 2020 19:46:44 -0500 Subject: [PATCH 12/16] lint workarounds for benchmark naming convention Signed-off-by: Joshua Marantz --- test/common/common/re_speed_test.cc | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/test/common/common/re_speed_test.cc b/test/common/common/re_speed_test.cc index f8dce79f31784..33367c8f1ffdb 100644 --- a/test/common/common/re_speed_test.cc +++ b/test/common/common/re_speed_test.cc @@ -21,6 +21,7 @@ static const char* cluster_inputs[] = { static const char cluster_re_pattern[] = "^cluster\\.((.*?)\\.)"; static const char cluster_re_alt_pattern[] = "^cluster\\.(([^\\.]+)\\.).*"; +// NOLINTNEXTLINE(readability-identifier-naming) static void BM_StdRegex(benchmark::State& state) { std::regex re(cluster_re_pattern); uint32_t passes = 0; @@ -44,6 +45,7 @@ static void BM_StdRegex(benchmark::State& state) { } BENCHMARK(BM_StdRegex); +// NOLINTNEXTLINE(readability-identifier-naming) static void BM_StdRegexStringView(benchmark::State& state) { std::regex re(cluster_re_pattern); std::vector inputs; @@ -66,6 +68,7 @@ static void BM_StdRegexStringView(benchmark::State& state) { } BENCHMARK(BM_StdRegexStringView); +// NOLINTNEXTLINE(readability-identifier-naming) static void BM_StdRegexStringViewAltPattern(benchmark::State& state) { std::regex re(cluster_re_alt_pattern); std::vector inputs; @@ -88,6 +91,7 @@ static void BM_StdRegexStringViewAltPattern(benchmark::State& state) { } BENCHMARK(BM_StdRegexStringViewAltPattern); +// NOLINTNEXTLINE(readability-identifier-naming) static void BM_RE2(benchmark::State& state) { re2::RE2 re(cluster_re_pattern); uint32_t passes = 0; @@ -105,6 +109,7 @@ static void BM_RE2(benchmark::State& state) { } BENCHMARK(BM_RE2); +// NOLINTNEXTLINE(readability-identifier-naming) static void BM_RE2_AltPattern(benchmark::State& state) { re2::RE2 re(cluster_re_alt_pattern); uint32_t passes = 0; From c4455333bf58a4105bc6da51e2c23884ef20f96e Mon Sep 17 00:00:00 2001 From: Joshua Marantz Date: Thu, 12 Nov 2020 13:03:07 -0500 Subject: [PATCH 13/16] try to fix clang-tidy and windows compiler errors. Signed-off-by: Joshua Marantz --- test/common/common/re_speed_test.cc | 10 +++++----- test/common/stats/tag_extractor_impl_test.cc | 5 +++-- 2 files changed, 8 insertions(+), 7 deletions(-) diff --git a/test/common/common/re_speed_test.cc b/test/common/common/re_speed_test.cc index 33367c8f1ffdb..ea0da6713fdbc 100644 --- a/test/common/common/re_speed_test.cc +++ b/test/common/common/re_speed_test.cc @@ -30,7 +30,7 @@ static void BM_StdRegex(benchmark::State& state) { inputs.push_back(cluster_input); } - for (auto _ : state) { + for (auto _ : state) { // NOLINT for (const std::string& cluster_input : inputs) { std::smatch match; if (std::regex_search(cluster_input, match, re)) { @@ -53,7 +53,7 @@ static void BM_StdRegexStringView(benchmark::State& state) { inputs.push_back(cluster_input); } uint32_t passes = 0; - for (auto _ : state) { + for (auto _ : state) { // NOLINT for (absl::string_view cluster_input : inputs) { std::match_results smatch; if (std::regex_search(cluster_input.begin(), cluster_input.end(), smatch, re)) { @@ -76,7 +76,7 @@ static void BM_StdRegexStringViewAltPattern(benchmark::State& state) { inputs.push_back(cluster_input); } uint32_t passes = 0; - for (auto _ : state) { + for (auto _ : state) { // NOLINT for (absl::string_view cluster_input : inputs) { std::match_results smatch; if (std::regex_search(cluster_input.begin(), cluster_input.end(), smatch, re)) { @@ -95,7 +95,7 @@ BENCHMARK(BM_StdRegexStringViewAltPattern); static void BM_RE2(benchmark::State& state) { re2::RE2 re(cluster_re_pattern); uint32_t passes = 0; - for (auto _ : state) { + for (auto _ : state) { // NOLINT for (const char* cluster_input : cluster_inputs) { re2::StringPiece match1, match2; if (re2::RE2::PartialMatch(cluster_input, re, &match1, &match2)) { @@ -113,7 +113,7 @@ BENCHMARK(BM_RE2); static void BM_RE2_AltPattern(benchmark::State& state) { re2::RE2 re(cluster_re_alt_pattern); uint32_t passes = 0; - for (auto _ : state) { + for (auto _ : state) { // NOLINT for (const char* cluster_input : cluster_inputs) { re2::StringPiece match1, match2; if (re2::RE2::PartialMatch(cluster_input, re, &match1, &match2)) { diff --git a/test/common/stats/tag_extractor_impl_test.cc b/test/common/stats/tag_extractor_impl_test.cc index 9b1e4dd09646f..076032c44ee7d 100644 --- a/test/common/stats/tag_extractor_impl_test.cc +++ b/test/common/stats/tag_extractor_impl_test.cc @@ -48,8 +48,9 @@ TEST(TagExtractorTest, RE2Variants) { EXPECT_EQ("test_cluster", std::string(match2)); std::regex sr("^cluster\\.((.*?)\\.)"); - std::match_results smatch; - ASSERT_TRUE(std::regex_search("cluster.test_cluster.upstream_cx_total", smatch, sr)); + std::match_results smatch; + std::string str("cluster.test_cluster.upstream_cx_total"); + ASSERT_TRUE(std::regex_search(str, smatch, sr)); ASSERT_LE(3, smatch.size()); EXPECT_EQ("test_cluster.", std::string(smatch[1])); EXPECT_EQ("test_cluster", std::string(smatch[2])); From 00a0ba12beee82c38fa728876af49e0ef02c5877 Mon Sep 17 00:00:00 2001 From: Joshua Marantz Date: Thu, 12 Nov 2020 17:15:57 -0500 Subject: [PATCH 14/16] review comments. Signed-off-by: Joshua Marantz --- .bazelversion | 2 +- source/common/config/well_known_names.cc | 1 - source/common/config/well_known_names.h | 2 +- source/common/stats/tag_extractor_impl.cc | 21 ++++++------ source/common/stats/tag_extractor_impl.h | 16 ++++----- source/common/stats/tag_producer_impl.cc | 12 +++---- test/common/common/re_speed_test.cc | 26 +++++++-------- test/common/stats/tag_extractor_impl_test.cc | 35 ++++++-------------- 8 files changed, 51 insertions(+), 64 deletions(-) diff --git a/.bazelversion b/.bazelversion index 40c341bdcdbe8..7c69a55dbb185 100644 --- a/.bazelversion +++ b/.bazelversion @@ -1 +1 @@ -3.6.0 +3.7.0 diff --git a/source/common/config/well_known_names.cc b/source/common/config/well_known_names.cc index 211d19207821c..e8fc767c41a3b 100644 --- a/source/common/config/well_known_names.cc +++ b/source/common/config/well_known_names.cc @@ -92,7 +92,6 @@ TagNameValues::TagNameValues() { addRegex(RATELIMIT_PREFIX, R"(^ratelimit\.((.*?)\.)\w+?$)"); // cluster.(.)* - // addRegex(CLUSTER_NAME, "^cluster\\.((.*?)\\.)"); addRe2(CLUSTER_NAME, "^cluster\\.(([^\\.]+)\\.).*"); // listener.[
.]http.(.)* diff --git a/source/common/config/well_known_names.h b/source/common/config/well_known_names.h index c5d3baadaef31..97ce58fd7265c 100644 --- a/source/common/config/well_known_names.h +++ b/source/common/config/well_known_names.h @@ -66,7 +66,7 @@ class TagNameValues { const std::string name_; const std::string regex_; const std::string substr_; - Regex::Type re_type_; + const Regex::Type re_type_; }; // Cluster name tag diff --git a/source/common/stats/tag_extractor_impl.cc b/source/common/stats/tag_extractor_impl.cc index 980f13b0a4a99..f28922b3983b1 100644 --- a/source/common/stats/tag_extractor_impl.cc +++ b/source/common/stats/tag_extractor_impl.cc @@ -24,8 +24,8 @@ bool regexStartsWithDot(absl::string_view regex) { } // namespace -TagExtractorImplBase::TagExtractorImplBase(const std::string& name, const std::string& regex, - const std::string& substr) +TagExtractorImplBase::TagExtractorImplBase(absl::string_view name, absl::string_view regex, + absl::string_view substr) : name_(name), prefix_(std::string(extractRegexPrefix(regex))), substr_(substr) {} std::string TagExtractorImplBase::extractRegexPrefix(absl::string_view regex) { @@ -47,9 +47,9 @@ std::string TagExtractorImplBase::extractRegexPrefix(absl::string_view regex) { return prefix; } -TagExtractorPtr TagExtractorImplBase::createTagExtractor(const std::string& name, - const std::string& regex, - const std::string& substr, +TagExtractorPtr TagExtractorImplBase::createTagExtractor(absl::string_view name, + absl::string_view regex, + absl::string_view substr, Regex::Type re_type) { if (name.empty()) { throw EnvoyException("tag_name cannot be empty"); @@ -72,9 +72,10 @@ bool TagExtractorImplBase::substrMismatch(absl::string_view stat_name) const { return !substr_.empty() && stat_name.find(substr_) == absl::string_view::npos; } -TagExtractorStdRegexImpl::TagExtractorStdRegexImpl(const std::string& name, const std::string regex, - const std::string& substr) - : TagExtractorImplBase(name, regex, substr), regex_(Regex::Utility::parseStdRegex(regex)) {} +TagExtractorStdRegexImpl::TagExtractorStdRegexImpl(absl::string_view name, absl::string_view regex, + absl::string_view substr) + : TagExtractorImplBase(name, regex, substr), + regex_(Regex::Utility::parseStdRegex(std::string(regex))) {} bool TagExtractorStdRegexImpl::extractTag(absl::string_view stat_name, std::vector& tags, IntervalSet& remove_characters) const { @@ -115,8 +116,8 @@ bool TagExtractorStdRegexImpl::extractTag(absl::string_view stat_name, std::vect return false; } -TagExtractorRe2Impl::TagExtractorRe2Impl(const std::string& name, const std::string regex, - const std::string& substr) +TagExtractorRe2Impl::TagExtractorRe2Impl(absl::string_view name, absl::string_view regex, + absl::string_view substr) : TagExtractorImplBase(name, regex, substr), regex_(regex) {} bool TagExtractorRe2Impl::extractTag(absl::string_view stat_name, std::vector& tags, diff --git a/source/common/stats/tag_extractor_impl.h b/source/common/stats/tag_extractor_impl.h index ffe18e391bf6b..6234b929d4898 100644 --- a/source/common/stats/tag_extractor_impl.h +++ b/source/common/stats/tag_extractor_impl.h @@ -25,12 +25,12 @@ class TagExtractorImplBase : public TagExtractor { * to avoid large numbers of failed regex lookups. * @return TagExtractorPtr newly constructed TagExtractor. */ - static TagExtractorPtr createTagExtractor(const std::string& name, const std::string& regex, - const std::string& substr = "", + static TagExtractorPtr createTagExtractor(absl::string_view name, absl::string_view regex, + absl::string_view substr = "", Regex::Type re_type = Regex::Type::StdRegex); - TagExtractorImplBase(const std::string& name, const std::string& regex, - const std::string& substr = ""); + TagExtractorImplBase(absl::string_view name, absl::string_view regex, + absl::string_view substr = ""); std::string name() const override { return name_; } absl::string_view prefixToken() const override { return prefix_; } @@ -56,8 +56,8 @@ class TagExtractorImplBase : public TagExtractor { class TagExtractorStdRegexImpl : public TagExtractorImplBase { public: - TagExtractorStdRegexImpl(const std::string& name, const std::string regex, - const std::string& substr = ""); + TagExtractorStdRegexImpl(absl::string_view name, absl::string_view regex, + absl::string_view substr = ""); bool extractTag(absl::string_view tag_extracted_name, std::vector& tags, IntervalSet& remove_characters) const override; @@ -68,8 +68,8 @@ class TagExtractorStdRegexImpl : public TagExtractorImplBase { class TagExtractorRe2Impl : public TagExtractorImplBase { public: - TagExtractorRe2Impl(const std::string& name, const std::string regex, - const std::string& substr = ""); + TagExtractorRe2Impl(absl::string_view name, absl::string_view regex, + absl::string_view substr = ""); bool extractTag(absl::string_view tag_extracted_name, std::vector& tags, IntervalSet& remove_characters) const override; diff --git a/source/common/stats/tag_producer_impl.cc b/source/common/stats/tag_producer_impl.cc index 5befb7e5f7275..bfc35b52e40ae 100644 --- a/source/common/stats/tag_producer_impl.cc +++ b/source/common/stats/tag_producer_impl.cc @@ -34,11 +34,11 @@ TagProducerImpl::TagProducerImpl(const envoy::config::metrics::v3::StatsConfig& "No regex specified for tag specifier and no default regex for name: '{}'", name)); } } else { - addExtractor(Stats::TagExtractorImplBase::createTagExtractor(name, tag_specifier.regex())); + addExtractor(TagExtractorImplBase::createTagExtractor(name, tag_specifier.regex())); } } else if (tag_specifier.tag_value_case() == envoy::config::metrics::v3::TagSpecifier::TagValueCase::kFixedValue) { - default_tags_.emplace_back(Stats::Tag{name, tag_specifier.fixed_value()}); + default_tags_.emplace_back(Tag{name, tag_specifier.fixed_value()}); } } } @@ -47,8 +47,8 @@ int TagProducerImpl::addExtractorsMatching(absl::string_view name) { int num_found = 0; for (const auto& desc : Config::TagNames::get().descriptorVec()) { if (desc.name_ == name) { - addExtractor(Stats::TagExtractorImplBase::createTagExtractor(desc.name_, desc.regex_, - desc.substr_, desc.re_type_)); + addExtractor(TagExtractorImplBase::createTagExtractor(desc.name_, desc.regex_, desc.substr_, + desc.re_type_)); ++num_found; } } @@ -103,8 +103,8 @@ TagProducerImpl::addDefaultExtractors(const envoy::config::metrics::v3::StatsCon if (!config.has_use_all_default_tags() || config.use_all_default_tags().value()) { for (const auto& desc : Config::TagNames::get().descriptorVec()) { names.emplace(desc.name_); - addExtractor(Stats::TagExtractorImplBase::createTagExtractor(desc.name_, desc.regex_, - desc.substr_, desc.re_type_)); + addExtractor(TagExtractorImplBase::createTagExtractor(desc.name_, desc.regex_, desc.substr_, + desc.re_type_)); } } return names; diff --git a/test/common/common/re_speed_test.cc b/test/common/common/re_speed_test.cc index ea0da6713fdbc..0db62906e281e 100644 --- a/test/common/common/re_speed_test.cc +++ b/test/common/common/re_speed_test.cc @@ -11,22 +11,22 @@ // NOLINT(namespace-envoy) -static const char* cluster_inputs[] = { +static const char* ClusterInputs[] = { "cluster.no_trailing_dot", "cluster.match.", "cluster.match.normal", "cluster.match.and.a.whole.lot.of.things.coming.after.the.matches.really.too.much.stuff", }; -static const char cluster_re_pattern[] = "^cluster\\.((.*?)\\.)"; -static const char cluster_re_alt_pattern[] = "^cluster\\.(([^\\.]+)\\.).*"; +static const char ClusterRePattern[] = "^cluster\\.((.*?)\\.)"; +static const char ClusterReAltPattern[] = "^cluster\\.(([^\\.]+)\\.).*"; // NOLINTNEXTLINE(readability-identifier-naming) static void BM_StdRegex(benchmark::State& state) { - std::regex re(cluster_re_pattern); + std::regex re(ClusterRePattern); uint32_t passes = 0; std::vector inputs; - for (const char* cluster_input : cluster_inputs) { + for (const char* cluster_input : ClusterInputs) { inputs.push_back(cluster_input); } @@ -47,9 +47,9 @@ BENCHMARK(BM_StdRegex); // NOLINTNEXTLINE(readability-identifier-naming) static void BM_StdRegexStringView(benchmark::State& state) { - std::regex re(cluster_re_pattern); + std::regex re(ClusterRePattern); std::vector inputs; - for (const char* cluster_input : cluster_inputs) { + for (const char* cluster_input : ClusterInputs) { inputs.push_back(cluster_input); } uint32_t passes = 0; @@ -70,9 +70,9 @@ BENCHMARK(BM_StdRegexStringView); // NOLINTNEXTLINE(readability-identifier-naming) static void BM_StdRegexStringViewAltPattern(benchmark::State& state) { - std::regex re(cluster_re_alt_pattern); + std::regex re(ClusterReAltPattern); std::vector inputs; - for (const char* cluster_input : cluster_inputs) { + for (const char* cluster_input : ClusterInputs) { inputs.push_back(cluster_input); } uint32_t passes = 0; @@ -93,10 +93,10 @@ BENCHMARK(BM_StdRegexStringViewAltPattern); // NOLINTNEXTLINE(readability-identifier-naming) static void BM_RE2(benchmark::State& state) { - re2::RE2 re(cluster_re_pattern); + re2::RE2 re(ClusterRePattern); uint32_t passes = 0; for (auto _ : state) { // NOLINT - for (const char* cluster_input : cluster_inputs) { + for (const char* cluster_input : ClusterInputs) { re2::StringPiece match1, match2; if (re2::RE2::PartialMatch(cluster_input, re, &match1, &match2)) { ASSERT(match1 == "match."); @@ -111,10 +111,10 @@ BENCHMARK(BM_RE2); // NOLINTNEXTLINE(readability-identifier-naming) static void BM_RE2_AltPattern(benchmark::State& state) { - re2::RE2 re(cluster_re_alt_pattern); + re2::RE2 re(ClusterReAltPattern); uint32_t passes = 0; for (auto _ : state) { // NOLINT - for (const char* cluster_input : cluster_inputs) { + for (const char* cluster_input : ClusterInputs) { re2::StringPiece match1, match2; if (re2::RE2::PartialMatch(cluster_input, re, &match1, &match2)) { ASSERT(match1 == "match."); diff --git a/test/common/stats/tag_extractor_impl_test.cc b/test/common/stats/tag_extractor_impl_test.cc index 076032c44ee7d..2d7e0e79d0fdb 100644 --- a/test/common/stats/tag_extractor_impl_test.cc +++ b/test/common/stats/tag_extractor_impl_test.cc @@ -30,30 +30,17 @@ TEST(TagExtractorTest, TwoSubexpressions) { } TEST(TagExtractorTest, RE2Variants) { - re2::RE2 re("^cluster\\.((.*?)\\.)"); - re2::StringPiece match1, match2; - ASSERT_TRUE( - re2::RE2::PartialMatch("cluster.test_cluster.upstream_cx_total", re, &match1, &match2)); - EXPECT_EQ("test_cluster.", std::string(match1)); - EXPECT_EQ("test_cluster", std::string(match2)); - - re2::RE2 alternate_re("^cluster\\.([^\\.]+)\\..*"); - ASSERT_TRUE(re2::RE2::FullMatch("cluster.test_cluster.upstream_cx_total", alternate_re, &match1)); - EXPECT_EQ("test_cluster", std::string(match1)); - - re2::RE2 alternate_re2("^cluster\\.(([^\\.]+)\\.).*"); - ASSERT_TRUE(re2::RE2::FullMatch("cluster.test_cluster.upstream_cx_total", alternate_re2, &match1, - &match2)); - EXPECT_EQ("test_cluster.", std::string(match1)); - EXPECT_EQ("test_cluster", std::string(match2)); - - std::regex sr("^cluster\\.((.*?)\\.)"); - std::match_results smatch; - std::string str("cluster.test_cluster.upstream_cx_total"); - ASSERT_TRUE(std::regex_search(str, smatch, sr)); - ASSERT_LE(3, smatch.size()); - EXPECT_EQ("test_cluster.", std::string(smatch[1])); - EXPECT_EQ("test_cluster", std::string(smatch[2])); + TagExtractorRe2Impl tag_extractor("cluster_name", "^cluster\\.(([^\\.]+)\\.).*"); + EXPECT_EQ("cluster_name", tag_extractor.name()); + std::string name = "cluster.test_cluster.upstream_cx_total"; + TagVector tags; + IntervalSetImpl remove_characters; + ASSERT_TRUE(tag_extractor.extractTag(name, tags, remove_characters)); + std::string tag_extracted_name = StringUtil::removeCharacters(name, remove_characters); + EXPECT_EQ("cluster.upstream_cx_total", tag_extracted_name); + ASSERT_EQ(1, tags.size()); + EXPECT_EQ("test_cluster", tags.at(0).value_); + EXPECT_EQ("cluster_name", tags.at(0).name_); } TEST(TagExtractorTest, SingleSubexpression) { From 5cf3b48cc8e42580f508df0710ed16282a799749 Mon Sep 17 00:00:00 2001 From: Joshua Marantz Date: Thu, 12 Nov 2020 17:30:09 -0500 Subject: [PATCH 15/16] remaining nits Signed-off-by: Joshua Marantz --- .bazelversion | 2 +- source/common/stats/tag_extractor_impl.h | 2 +- test/common/stats/BUILD | 1 - test/common/stats/tag_extractor_impl_test.cc | 1 - 4 files changed, 2 insertions(+), 4 deletions(-) diff --git a/.bazelversion b/.bazelversion index 7c69a55dbb185..40c341bdcdbe8 100644 --- a/.bazelversion +++ b/.bazelversion @@ -1 +1 @@ -3.7.0 +3.6.0 diff --git a/source/common/stats/tag_extractor_impl.h b/source/common/stats/tag_extractor_impl.h index 6234b929d4898..4b8fdf38011a4 100644 --- a/source/common/stats/tag_extractor_impl.h +++ b/source/common/stats/tag_extractor_impl.h @@ -75,7 +75,7 @@ class TagExtractorRe2Impl : public TagExtractorImplBase { IntervalSet& remove_characters) const override; private: - re2::RE2 regex_; + const re2::RE2 regex_; }; } // namespace Stats diff --git a/test/common/stats/BUILD b/test/common/stats/BUILD index fae0bc4f5539d..0493f5127b176 100644 --- a/test/common/stats/BUILD +++ b/test/common/stats/BUILD @@ -207,7 +207,6 @@ envoy_cc_test( "//source/common/stats:tag_extractor_lib", "//source/common/stats:tag_producer_lib", "//test/test_common:utility_lib", - "@com_googlesource_code_re2//:re2", "@envoy_api//envoy/config/metrics/v3:pkg_cc_proto", ], ) diff --git a/test/common/stats/tag_extractor_impl_test.cc b/test/common/stats/tag_extractor_impl_test.cc index 2d7e0e79d0fdb..13fd4de172c93 100644 --- a/test/common/stats/tag_extractor_impl_test.cc +++ b/test/common/stats/tag_extractor_impl_test.cc @@ -10,7 +10,6 @@ #include "test/test_common/utility.h" #include "gtest/gtest.h" -#include "re2/re2.h" namespace Envoy { namespace Stats { From b5bac7e4f8c6d13a646e12fee818ae1e3a6348c5 Mon Sep 17 00:00:00 2001 From: Joshua Marantz Date: Sun, 15 Nov 2020 22:28:45 -0500 Subject: [PATCH 16/16] review comments Signed-off-by: Joshua Marantz --- source/common/stats/tag_extractor_impl.cc | 20 ++++++++++---------- source/common/stats/tag_extractor_impl.h | 10 ++++++++++ 2 files changed, 20 insertions(+), 10 deletions(-) diff --git a/source/common/stats/tag_extractor_impl.cc b/source/common/stats/tag_extractor_impl.cc index f28922b3983b1..6aefbdf6cd258 100644 --- a/source/common/stats/tag_extractor_impl.cc +++ b/source/common/stats/tag_extractor_impl.cc @@ -77,6 +77,13 @@ TagExtractorStdRegexImpl::TagExtractorStdRegexImpl(absl::string_view name, absl: : TagExtractorImplBase(name, regex, substr), regex_(Regex::Utility::parseStdRegex(std::string(regex))) {} +std::string& TagExtractorImplBase::addTag(std::vector& tags) const { + tags.emplace_back(); + Tag& tag = tags.back(); + tag.name_ = name_; + return tag.value_; +} + bool TagExtractorStdRegexImpl::extractTag(absl::string_view stat_name, std::vector& tags, IntervalSet& remove_characters) const { PERF_OPERATION(perf); @@ -99,11 +106,7 @@ bool TagExtractorStdRegexImpl::extractTag(absl::string_view stat_name, std::vect // from the string but also not necessary in the tag value ("." for example). If there is no // second submatch, then the value_subexpr is the same as the remove_subexpr. const auto& value_subexpr = match.size() > 2 ? match[2] : remove_subexpr; - - tags.emplace_back(); - Tag& tag = tags.back(); - tag.name_ = name_; - tag.value_ = value_subexpr.str(); + addTag(tags) = value_subexpr.str(); // Determines which characters to remove from stat_name to elide remove_subexpr. std::string::size_type start = remove_subexpr.first - stat_name.begin(); @@ -144,16 +147,13 @@ bool TagExtractorRe2Impl::extractTag(absl::string_view stat_name, std::vector& tags) const; + const std::string name_; const std::string prefix_; const std::string substr_;