tag extractor: removing exceptions (#33191)

Signed-off-by: Alyssa Wilk <[email protected]>
envoyproxy · Mar 29, 2024 · 7bccb0e · 7bccb0e
1 parent 2ccab2a
commit 7bccb0e
Show file tree

Hide file tree

Showing 5 changed files with 32 additions and 25 deletions.
diff --git a/envoy/common/exception.h b/envoy/common/exception.h
@@ -69,7 +69,7 @@ class EnvoyException : public std::runtime_error {
 
 template <class Type> Type returnOrThrow(absl::StatusOr<Type> type_or_error) {
   THROW_IF_STATUS_NOT_OK(type_or_error, throw);
-  return type_or_error.value();
+  return std::move(type_or_error.value());
 }
 
 #define THROW_OR_RETURN_VALUE(expression, type) returnOrThrow<type>(expression)

diff --git a/source/common/stats/tag_extractor_impl.cc b/source/common/stats/tag_extractor_impl.cc
@@ -66,17 +66,16 @@ std::string TagExtractorImplBase::extractRegexPrefix(absl::string_view regex) {
   return prefix;
 }
 
-TagExtractorPtr TagExtractorImplBase::createTagExtractor(absl::string_view name,
-                                                         absl::string_view regex,
-                                                         absl::string_view substr,
-                                                         absl::string_view negative_match,
-                                                         Regex::Type re_type) {
+absl::StatusOr<TagExtractorPtr>
+TagExtractorImplBase::createTagExtractor(absl::string_view name, absl::string_view regex,
+                                         absl::string_view substr, absl::string_view negative_match,
+                                         Regex::Type re_type) {
   if (name.empty()) {
-    throwEnvoyExceptionOrPanic("tag_name cannot be empty");
+    return absl::InvalidArgumentError("tag_name cannot be empty");
   }
 
   if (regex.empty()) {
-    throwEnvoyExceptionOrPanic(fmt::format(
+    return absl::InvalidArgumentError(fmt::format(
         "No regex specified for tag specifier and no default regex for name: '{}'", name));
   }
   switch (re_type) {

diff --git a/source/common/stats/tag_extractor_impl.h b/source/common/stats/tag_extractor_impl.h
@@ -64,12 +64,12 @@ class TagExtractorImplBase : public TagExtractor {
    *               in order to match the regex. This is an optional performance tweak
    *               to avoid large numbers of failed regex lookups.
    * @param re_type the regular expression syntax used (Regex::Type::StdRegex or Regex::Type::Re2).
-   * @return TagExtractorPtr newly constructed TagExtractor.
+   * @return TagExtractorPtr newly constructed TagExtractor or a failure status.
    */
-  static TagExtractorPtr createTagExtractor(absl::string_view name, absl::string_view regex,
-                                            absl::string_view substr = "",
-                                            absl::string_view negative_match = "",
-                                            Regex::Type re_type = Regex::Type::StdRegex);
+  static absl::StatusOr<TagExtractorPtr>
+  createTagExtractor(absl::string_view name, absl::string_view regex, absl::string_view substr = "",
+                     absl::string_view negative_match = "",
+                     Regex::Type re_type = Regex::Type::StdRegex);
 
   TagExtractorImplBase(absl::string_view name, absl::string_view regex,
                        absl::string_view substr = "");

diff --git a/source/common/stats/tag_producer_impl.cc b/source/common/stats/tag_producer_impl.cc
@@ -39,7 +39,9 @@ TagProducerImpl::TagProducerImpl(const envoy::config::metrics::v3::StatsConfig&
               "No regex specified for tag specifier and no default regex for name: '{}'", name));
         }
       } else {
-        addExtractor(TagExtractorImplBase::createTagExtractor(name, tag_specifier.regex()));
+        addExtractor(THROW_OR_RETURN_VALUE(
+            TagExtractorImplBase::createTagExtractor(name, tag_specifier.regex()),
+            TagExtractorPtr));
       }
     } else if (tag_specifier.tag_value_case() ==
                envoy::config::metrics::v3::TagSpecifier::TagValueCase::kFixedValue) {
@@ -53,8 +55,10 @@ int TagProducerImpl::addExtractorsMatching(absl::string_view name) {
   int num_found = 0;
   for (const auto& desc : Config::TagNames::get().descriptorVec()) {
     if (desc.name_ == name) {
-      addExtractor(TagExtractorImplBase::createTagExtractor(desc.name_, desc.regex_, desc.substr_,
-                                                            desc.negative_match_, desc.re_type_));
+      addExtractor(THROW_OR_RETURN_VALUE(
+          TagExtractorImplBase::createTagExtractor(desc.name_, desc.regex_, desc.substr_,
+                                                   desc.negative_match_, desc.re_type_),
+          TagExtractorPtr));
       ++num_found;
     }
   }
@@ -135,8 +139,10 @@ void TagProducerImpl::reserveResources(const envoy::config::metrics::v3::StatsCo
 void TagProducerImpl::addDefaultExtractors(const envoy::config::metrics::v3::StatsConfig& config) {
   if (!config.has_use_all_default_tags() || config.use_all_default_tags().value()) {
     for (const auto& desc : Config::TagNames::get().descriptorVec()) {
-      addExtractor(TagExtractorImplBase::createTagExtractor(desc.name_, desc.regex_, desc.substr_,
-                                                            desc.negative_match_, desc.re_type_));
+      addExtractor(THROW_OR_RETURN_VALUE(
+          TagExtractorImplBase::createTagExtractor(desc.name_, desc.regex_, desc.substr_,
+                                                   desc.negative_match_, desc.re_type_),
+          TagExtractorPtr));
     }
     for (const auto& desc : Config::TagNames::get().tokenizedDescriptorVec()) {
       addExtractor(std::make_unique<TagExtractorTokensImpl>(desc.name_, desc.pattern_));

diff --git a/test/common/stats/tag_extractor_impl_test.cc b/test/common/stats/tag_extractor_impl_test.cc
@@ -74,14 +74,15 @@ TEST(TagExtractorTest, noSubstrMismatch) {
 }
 
 TEST(TagExtractorTest, EmptyName) {
-  EXPECT_THROW_WITH_MESSAGE(
-      TagExtractorStdRegexImpl::createTagExtractor("", "^listener\\.(\\d+?\\.)"), EnvoyException,
+  EXPECT_EQ(
+      TagExtractorStdRegexImpl::createTagExtractor("", "^listener\\.(\\d+?\\.)").status().message(),
       "tag_name cannot be empty");
 }
 
 TEST(TagExtractorTest, BadRegex) {
-  EXPECT_THROW_WITH_REGEX(TagExtractorStdRegexImpl::createTagExtractor("cluster_name", "+invalid"),
-                          EnvoyException, "Invalid regex '\\+invalid':");
+  EXPECT_THROW_WITH_REGEX(
+      TagExtractorStdRegexImpl::createTagExtractor("cluster_name", "+invalid").IgnoreError(),
+      EnvoyException, "Invalid regex '\\+invalid':");
 }
 
 class DefaultTagRegexTester {
@@ -489,7 +490,7 @@ TEST(TagExtractorTest, ExtAuthzTagExtractors) {
 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 = TagExtractorStdRegexImpl::createTagExtractor("foo", regex);
+    tag_extractor = TagExtractorStdRegexImpl::createTagExtractor("foo", regex).value();
     return tag_extractor->prefixToken();
   };
 
@@ -504,8 +505,9 @@ TEST(TagExtractorTest, ExtractRegexPrefix) {
 }
 
 TEST(TagExtractorTest, CreateTagExtractorNoRegex) {
-  EXPECT_THROW_WITH_REGEX(TagExtractorStdRegexImpl::createTagExtractor("no such default tag", ""),
-                          EnvoyException, "^No regex specified for tag specifier and no default");
+  EXPECT_THAT(
+      TagExtractorStdRegexImpl::createTagExtractor("no such default tag", "").status().message(),
+      testing::ContainsRegex("^No regex specified for tag specifier and no default"));
 }
 
 class TagExtractorTokensTest : public testing::Test {