re: removing histograms, tweaking runtime access (#19972)

Risk Level: low Testing: n/a Docs Changes: n/a Release Notes: inline Part of #19847 Signed-off-by: Alyssa Wilk <[email protected]>
envoyproxy · Feb 16, 2022 · 2484025 · 2484025
1 parent cca3848
commit 2484025
Show file tree

Hide file tree

Showing 5 changed files with 19 additions and 46 deletions.
diff --git a/docs/root/version_history/current.rst b/docs/root/version_history/current.rst
@@ -65,3 +65,4 @@ Deprecated
 ----------
 
 * http: removing support for long-deprecated old style filter names, e.g. envoy.router, envoy.lua.
+* re2: removed undocumented histograms ``re2.program_size`` and ``re2.exceeded_warn_level``.
diff --git a/source/common/common/regex.cc b/source/common/common/regex.cc
@@ -5,6 +5,7 @@
 
 #include "source/common/common/assert.h"
 #include "source/common/common/fmt.h"
+#include "source/common/runtime/runtime_features.h"
 
 namespace Envoy {
 namespace Regex {
@@ -17,42 +18,23 @@ CompiledGoogleReMatcher::CompiledGoogleReMatcher(const std::string& regex,
   }
 
   if (do_program_size_check) {
-    Runtime::Loader* runtime = Runtime::LoaderSingleton::getExisting();
-    if (runtime) {
-      Stats::Scope& root_scope = runtime->getRootScope();
-
-      const uint32_t regex_program_size = static_cast<uint32_t>(regex_.ProgramSize());
-      // TODO(perf): It would be more efficient to create the stats (program size histogram,
-      // warning counter) on startup and not with each regex match.
-      Stats::StatNameManagedStorage program_size_stat_name("re2.program_size",
-                                                           root_scope.symbolTable());
-      Stats::Histogram& program_size_stat = root_scope.histogramFromStatName(
-          program_size_stat_name.statName(), Stats::Histogram::Unit::Unspecified);
-      program_size_stat.recordValue(regex_program_size);
-
-      Stats::StatNameManagedStorage warn_count_stat_name("re2.exceeded_warn_level",
-                                                         root_scope.symbolTable());
-      Stats::Counter& warn_count = root_scope.counterFromStatName(warn_count_stat_name.statName());
-
-      const uint32_t max_program_size_error_level =
-          runtime->snapshot().getInteger("re2.max_program_size.error_level", 100);
-      if (regex_program_size > max_program_size_error_level) {
-        throw EnvoyException(fmt::format("regex '{}' RE2 program size of {} > max program size of "
-                                         "{} set for the error level threshold. Increase "
-                                         "configured max program size if necessary.",
-                                         regex, regex_program_size, max_program_size_error_level));
-      }
+    const uint32_t regex_program_size = static_cast<uint32_t>(regex_.ProgramSize());
+    const uint32_t max_program_size_error_level =
+        Runtime::getInteger("re2.max_program_size.error_level", 100);
+    if (regex_program_size > max_program_size_error_level) {
+      throw EnvoyException(fmt::format("regex '{}' RE2 program size of {} > max program size of "
+                                       "{} set for the error level threshold. Increase "
+                                       "configured max program size if necessary.",
+                                       regex, regex_program_size, max_program_size_error_level));
+    }
 
-      const uint32_t max_program_size_warn_level =
-          runtime->snapshot().getInteger("re2.max_program_size.warn_level", UINT32_MAX);
-      if (regex_program_size > max_program_size_warn_level) {
-        warn_count.inc();
-        ENVOY_LOG_MISC(
-            warn,
-            "regex '{}' RE2 program size of {} > max program size of {} set for the warn "
-            "level threshold. Increase configured max program size if necessary.",
-            regex, regex_program_size, max_program_size_warn_level);
-      }
+    const uint32_t max_program_size_warn_level =
+        Runtime::getInteger("re2.max_program_size.warn_level", UINT32_MAX);
+    if (regex_program_size > max_program_size_warn_level) {
+      ENVOY_LOG_MISC(warn,
+                     "regex '{}' RE2 program size of {} > max program size of {} set for the warn "
+                     "level threshold. Increase configured max program size if necessary.",
+                     regex, regex_program_size, max_program_size_warn_level);
     }
   }
 }

diff --git a/source/common/common/regex.h b/source/common/common/regex.h
@@ -4,7 +4,6 @@
 #include <regex>
 
 #include "envoy/common/regex.h"
-#include "envoy/runtime/runtime.h"
 #include "envoy/type/matcher/v3/regex.pb.h"
 
 #include "source/common/common/assert.h"

diff --git a/source/common/runtime/runtime_features.cc b/source/common/runtime/runtime_features.cc
@@ -22,7 +22,7 @@ bool runtimeFeatureEnabled(absl::string_view feature) {
 }
 
 uint64_t getInteger(absl::string_view feature, uint64_t default_value) {
-  ASSERT(absl::StartsWith(feature, "envoy."));
+  ASSERT(absl::StartsWith(feature, "envoy.") || absl::StartsWith(feature, "re2."));
   if (Runtime::LoaderSingleton::getExisting()) {
     return Runtime::LoaderSingleton::getExisting()->threadsafeSnapshot()->getInteger(
         std::string(feature), default_value);

diff --git a/test/common/common/regex_test.cc b/test/common/common/regex_test.cc
@@ -127,33 +127,24 @@ TEST(Utility, ParseRegex) {
   // Verify that a warning is logged for the warn level max program size.
   {
     TestScopedRuntime scoped_runtime;
-    Envoy::Stats::Counter& warn_count =
-        Runtime::LoaderSingleton::getExisting()->getRootScope().counterFromString(
-            "re2.exceeded_warn_level");
     Runtime::LoaderSingleton::getExisting()->mergeValues(
         {{"re2.max_program_size.warn_level", "1"}});
     envoy::type::matcher::v3::RegexMatcher matcher;
     matcher.set_regex("/asdf/.*");
     matcher.mutable_google_re2();
     EXPECT_NO_THROW(Utility::parseRegex(matcher));
-    EXPECT_EQ(1, warn_count.value());
     EXPECT_LOG_CONTAINS("warn", "> max program size of 1 set for the warn level threshold",
                         Utility::parseRegex(matcher));
-    EXPECT_EQ(2, warn_count.value());
   }
 
   // Verify that no check is performed if the warn level max program size is not set by runtime.
   {
     TestScopedRuntime scoped_runtime;
-    Envoy::Stats::Counter& warn_count =
-        Runtime::LoaderSingleton::getExisting()->getRootScope().counterFromString(
-            "re2.exceeded_warn_level");
     envoy::type::matcher::v3::RegexMatcher matcher;
     matcher.set_regex("/asdf/.*");
     matcher.mutable_google_re2();
     EXPECT_NO_THROW(Utility::parseRegex(matcher));
     EXPECT_LOG_NOT_CONTAINS("warn", "> max program size", Utility::parseRegex(matcher));
-    EXPECT_EQ(0, warn_count.value());
   }
 }
Original file line number	Diff line number	Diff line change
Expand Up		@@ -65,3 +65,4 @@ Deprecated
		----------

		* http: removing support for long-deprecated old style filter names, e.g. envoy.router, envoy.lua.
		* re2: removed undocumented histograms ``re2.program_size`` and ``re2.exceeded_warn_level``.