tags: removing producer exceptions (#33291)

tags: removing more exceptions

Signed-off-by: Alyssa Wilk <[email protected]>

source/common/config/BUILD

@@ -240,19 +240,6 @@ envoy_cc_library(
     ],
 )
 
-envoy_cc_library(
-    name = "stats_utility_lib",
-    srcs = ["stats_utility.cc"],
-    hdrs = ["stats_utility.h"],
-    deps = [
-        "//source/common/stats:histogram_lib",
-        "//source/common/stats:stats_lib",
-        "//source/common/stats:stats_matcher_lib",
-        "//source/common/stats:tag_producer_lib",
-        "@envoy_api//envoy/config/bootstrap/v3:pkg_cc_proto",
-    ],
-)
-
 envoy_cc_library(
     name = "subscription_base_interface",
     hdrs = ["subscription_base.h"],

source/common/stats/tag_producer_impl.cc

@@ -11,13 +11,19 @@
 namespace Envoy {
 namespace Stats {
 
-TagProducerImpl::TagProducerImpl(const envoy::config::metrics::v3::StatsConfig& config)
-    : TagProducerImpl(config, {}) {}
+absl::StatusOr<Stats::TagProducerPtr>
+TagProducerImpl::createTagProducer(const envoy::config::metrics::v3::StatsConfig& config,
+                                   const Stats::TagVector& cli_tags) {
+  absl::Status creation_status;
+  Stats::TagProducerPtr ret(new Stats::TagProducerImpl(config, cli_tags, creation_status));
+  RETURN_IF_NOT_OK(creation_status);
+  return ret;
+}
 
 TagProducerImpl::TagProducerImpl(const envoy::config::metrics::v3::StatsConfig& config,
-                                 const Stats::TagVector& cli_tags) {
+                                 const Stats::TagVector& cli_tags, absl::Status& creation_status) {
   reserveResources(config);
-  addDefaultExtractors(config);
+  creation_status = addDefaultExtractors(config);
 
   for (const auto& cli_tag : cli_tags) {
     addExtractor(std::make_unique<TagExtractorFixedImpl>(cli_tag.name_, cli_tag.value_));
@@ -34,14 +40,18 @@ TagProducerImpl::TagProducerImpl(const envoy::config::metrics::v3::StatsConfig&
             envoy::config::metrics::v3::TagSpecifier::TagValueCase::kRegex) {
 
       if (tag_specifier.regex().empty()) {
-        if (addExtractorsMatching(name) == 0) {
-          throwEnvoyExceptionOrPanic(fmt::format(
-              "No regex specified for tag specifier and no default regex for name: '{}'", name));
+        creation_status = addExtractorsMatching(name);
+        if (!creation_status.ok()) {
+          return;
         }
       } else {
-        addExtractor(THROW_OR_RETURN_VALUE(
-            TagExtractorImplBase::createTagExtractor(name, tag_specifier.regex()),
-            TagExtractorPtr));
+        auto extractor_or_error =
+            TagExtractorImplBase::createTagExtractor(name, tag_specifier.regex());
+        if (!extractor_or_error.ok()) {
+          creation_status = extractor_or_error.status();
+          return;
+        }
+        addExtractor(std::move(extractor_or_error.value()));
       }
     } else if (tag_specifier.tag_value_case() ==
                envoy::config::metrics::v3::TagSpecifier::TagValueCase::kFixedValue) {
@@ -51,14 +61,16 @@ TagProducerImpl::TagProducerImpl(const envoy::config::metrics::v3::StatsConfig&
   }
 }
 
-int TagProducerImpl::addExtractorsMatching(absl::string_view name) {
+absl::Status TagProducerImpl::addExtractorsMatching(absl::string_view name) {
   int num_found = 0;
   for (const auto& desc : Config::TagNames::get().descriptorVec()) {
     if (desc.name_ == name) {
-      addExtractor(THROW_OR_RETURN_VALUE(
-          TagExtractorImplBase::createTagExtractor(desc.name_, desc.regex_, desc.substr_,
-                                                   desc.negative_match_, desc.re_type_),
-          TagExtractorPtr));
+      auto extractor_or_error = TagExtractorImplBase::createTagExtractor(
+          desc.name_, desc.regex_, desc.substr_, desc.negative_match_, desc.re_type_);
+      if (!extractor_or_error.ok()) {
+        return extractor_or_error.status();
+      }
+      addExtractor(std::move(extractor_or_error.value()));
       ++num_found;
     }
   }
@@ -68,7 +80,11 @@ int TagProducerImpl::addExtractorsMatching(absl::string_view name) {
       ++num_found;
     }
   }
-  return num_found;
+  if (num_found == 0) {
+    return absl::InvalidArgumentError(fmt::format(
+        "No regex specified for tag specifier and no default regex for name: '{}'", name));
+  }
+  return absl::OkStatus();
 }
 
 void TagProducerImpl::addExtractor(TagExtractorPtr extractor) {
@@ -136,18 +152,20 @@ void TagProducerImpl::reserveResources(const envoy::config::metrics::v3::StatsCo
   tag_extractors_without_prefix_.reserve(config.stats_tags().size());
 }
 
-void TagProducerImpl::addDefaultExtractors(const envoy::config::metrics::v3::StatsConfig& config) {
+absl::Status
+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(THROW_OR_RETURN_VALUE(
-          TagExtractorImplBase::createTagExtractor(desc.name_, desc.regex_, desc.substr_,
-                                                   desc.negative_match_, desc.re_type_),
-          TagExtractorPtr));
+      auto extractor_or_error = TagExtractorImplBase::createTagExtractor(
+          desc.name_, desc.regex_, desc.substr_, desc.negative_match_, desc.re_type_);
+      RETURN_IF_STATUS_NOT_OK(extractor_or_error);
+      addExtractor(std::move(extractor_or_error.value()));
     }
     for (const auto& desc : Config::TagNames::get().tokenizedDescriptorVec()) {
       addExtractor(std::make_unique<TagExtractorTokensImpl>(desc.name_, desc.pattern_));
     }
   }
+  return absl::OkStatus();
 }
 
 } // namespace Stats

source/common/stats/tag_producer_impl.h

@@ -28,10 +28,16 @@ namespace Stats {
  */
 class TagProducerImpl : public TagProducer {
 public:
-  TagProducerImpl(const envoy::config::metrics::v3::StatsConfig& config,
-                  const Stats::TagVector& cli_tags);
-
-  TagProducerImpl(const envoy::config::metrics::v3::StatsConfig& config);
+  /**
+   * Create TagProducer instance. Check all tag names for conflicts to avoid
+   * unexpected tag name overwriting.
+   * @param config stats config.
+   * @param cli_tags tags that are provided by the cli
+   * @returns a tag producer or an error pointer if a conflict of tag names is found.
+   */
+  static absl::StatusOr<Stats::TagProducerPtr>
+  createTagProducer(const envoy::config::metrics::v3::StatsConfig& config,
+                    const Stats::TagVector& cli_tags);
 
   TagProducerImpl() = default;
 
@@ -46,6 +52,9 @@ class TagProducerImpl : public TagProducer {
   const TagVector& fixedTags() const override { return fixed_tags_; }
 
 private:
+  TagProducerImpl(const envoy::config::metrics::v3::StatsConfig& config,
+                  const Stats::TagVector& cli_tags, absl::Status& creation_status);
+
   friend class DefaultTagRegexTester;
 
   /**
@@ -60,9 +69,9 @@ class TagProducerImpl : public TagProducer {
    * more than one TagExtractor can be used to generate a given tag. The default
    * extractors are specified in common/config/well_known_names.cc.
    * @param name absl::string_view the extractor to add.
-   * @return int the number of matching extractors.
+   * @return Status ok if at least 1 matching extractor was found.
    */
-  int addExtractorsMatching(absl::string_view name);
+  absl::Status addExtractorsMatching(absl::string_view name);
 
   /**
    * Roughly estimate the size of the vectors.
@@ -74,8 +83,9 @@ class TagProducerImpl : public TagProducer {
    * Adds all default extractors from well_known_names.cc into the collection.
    *
    * @param config const envoy::config::metrics::v2::StatsConfig& the config.
+   * @return a status indicating if the default extractors were successfully added.
    */
-  void addDefaultExtractors(const envoy::config::metrics::v3::StatsConfig& config);
+  absl::Status addDefaultExtractors(const envoy::config::metrics::v3::StatsConfig& config);
 
   /**
    * Iterates over every tag extractor that might possibly match stat_name, calling

source/server/BUILD

@@ -442,7 +442,6 @@ envoy_cc_library(
         "//source/common/common:mutex_tracer_lib",
         "//source/common/common:perf_tracing_lib",
         "//source/common/common:utility_lib",
-        "//source/common/config:stats_utility_lib",
         "//source/common/config:utility_lib",
         "//source/common/config:xds_resource_lib",
         "//source/common/grpc:async_client_manager_lib",
@@ -461,6 +460,7 @@ envoy_cc_library(
         "//source/common/secret:secret_manager_impl_lib",
         "//source/common/signal:fatal_error_handler_lib",
         "//source/common/singleton:manager_impl_lib",
+        "//source/common/stats:tag_producer_lib",
         "//source/common/stats:thread_local_store_lib",
         "//source/common/tls:context_lib",
         "//source/common/upstream:cluster_manager_lib",

source/server/config_validation/server.cc

@@ -5,14 +5,14 @@
 #include "envoy/config/bootstrap/v3/bootstrap.pb.h"
 
 #include "source/common/common/utility.h"
-#include "source/common/config/stats_utility.h"
 #include "source/common/config/utility.h"
 #include "source/common/config/well_known_names.h"
 #include "source/common/event/real_time_system.h"
 #include "source/common/listener_manager/listener_info_impl.h"
 #include "source/common/local_info/local_info_impl.h"
 #include "source/common/protobuf/utility.h"
 #include "source/common/singleton/manager_impl.h"
+#include "source/common/stats/tag_producer_impl.h"
 #include "source/common/tls/context_manager_impl.h"
 #include "source/common/version/version.h"
 #include "source/server/admin/admin_factory_context.h"
@@ -101,7 +101,9 @@ void ValidationInstance::initialize(const Options& options,
   regex_engine_ = createRegexEngine(
       bootstrap_, messageValidationContext().staticValidationVisitor(), serverFactoryContext());
 
-  Config::StatsUtility::createTagProducer(bootstrap_, options_.statsTags());
+  auto producer_or_error =
+      Stats::TagProducerImpl::createTagProducer(bootstrap_.stats_config(), options_.statsTags());
+  THROW_IF_STATUS_NOT_OK(producer_or_error, throw);
   if (!bootstrap_.node().user_agent_build_version().has_version()) {
     *bootstrap_.mutable_node()->mutable_user_agent_build_version() = VersionInfo::buildVersion();
   }

source/server/server.cc

@@ -29,7 +29,6 @@
 #include "source/common/common/enum_to_int.h"
 #include "source/common/common/mutex_tracer_impl.h"
 #include "source/common/common/utility.h"
-#include "source/common/config/stats_utility.h"
 #include "source/common/config/utility.h"
 #include "source/common/config/well_known_names.h"
 #include "source/common/config/xds_resource.h"
@@ -48,6 +47,7 @@
 #include "source/common/signal/fatal_error_handler.h"
 #include "source/common/singleton/manager_impl.h"
 #include "source/common/stats/stats_matcher_impl.h"
+#include "source/common/stats/tag_producer_impl.h"
 #include "source/common/stats/thread_local_store.h"
 #include "source/common/stats/timespan_impl.h"
 #include "source/common/tls/context_manager_impl.h"
@@ -497,8 +497,10 @@ absl::Status InstanceBase::initializeOrThrow(Network::Address::InstanceConstShar
 
   // Needs to happen as early as possible in the instantiation to preempt the objects that require
   // stats.
-  stats_store_.setTagProducer(
-      Config::StatsUtility::createTagProducer(bootstrap_, options_.statsTags()));
+  auto producer_or_error =
+      Stats::TagProducerImpl::createTagProducer(bootstrap_.stats_config(), options_.statsTags());
+  RETURN_IF_STATUS_NOT_OK(producer_or_error);
+  stats_store_.setTagProducer(std::move(producer_or_error.value()));
   stats_store_.setStatsMatcher(std::make_unique<Stats::StatsMatcherImpl>(
       bootstrap_.stats_config(), stats_store_.symbolTable(), server_contexts_));
   stats_store_.setHistogramSettings(

test/common/config/BUILD

@@ -126,7 +126,6 @@ envoy_cc_test(
     external_deps = ["abseil_optional"],
     deps = [
         "//source/common/config:api_version_lib",
-        "//source/common/config:stats_utility_lib",
         "//source/common/config:utility_lib",
         "//source/common/config:well_known_names",
         "//source/common/stats:stats_lib",

test/common/config/utility_test.cc

@@ -8,7 +8,6 @@
 
 #include "source/common/common/fmt.h"
 #include "source/common/config/api_version.h"
-#include "source/common/config/stats_utility.h"
 #include "source/common/config/utility.h"
 #include "source/common/config/well_known_names.h"
 #include "source/common/protobuf/protobuf.h"
@@ -57,26 +56,6 @@ TEST(UtilityTest, ConfigSourceInitFetchTimeout) {
   EXPECT_EQ(654, Utility::configSourceInitialFetchTimeout(config_source).count());
 }
 
-TEST(UtilityTest, createTagProducer) {
-  envoy::config::bootstrap::v3::Bootstrap bootstrap;
-  auto producer = StatsUtility::createTagProducer(bootstrap, {});
-  ASSERT_TRUE(producer != nullptr);
-  Stats::TagVector tags;
-  auto extracted_name = producer->produceTags("http.config_test.rq_total", tags);
-  ASSERT_EQ(extracted_name, "http.rq_total");
-  ASSERT_EQ(tags.size(), 1);
-}
-
-TEST(UtilityTest, createTagProducerWithDefaultTgs) {
-  envoy::config::bootstrap::v3::Bootstrap bootstrap;
-  auto producer = StatsUtility::createTagProducer(bootstrap, {{"foo", "bar"}});
-  ASSERT_TRUE(producer != nullptr);
-  Stats::TagVector tags;
-  auto extracted_name = producer->produceTags("http.config_test.rq_total", tags);
-  EXPECT_EQ(extracted_name, "http.rq_total");
-  EXPECT_EQ(tags.size(), 2);
-}
-
 TEST(UtilityTest, CheckFilesystemSubscriptionBackingPath) {
   Api::ApiPtr api = Api::createApiForTest();
 

test/common/stats/tag_extractor_impl_speed_test.cc

@@ -90,7 +90,9 @@ const std::vector<Params> params = {
 
 // NOLINTNEXTLINE(readability-identifier-naming)
 void BM_ExtractTags(benchmark::State& state) {
-  TagProducerImpl tag_extractors{envoy::config::metrics::v3::StatsConfig()};
+  const Stats::TagVector tags;
+  auto tag_extractors =
+      TagProducerImpl::createTagProducer(envoy::config::metrics::v3::StatsConfig(), tags).value();
   const auto idx = state.range(0);
   const auto& p = params[idx];
   absl::string_view str = std::get<0>(p);
@@ -99,7 +101,7 @@ void BM_ExtractTags(benchmark::State& state) {
   for (auto _ : state) {
     UNREFERENCED_PARAMETER(_);
     TagVector tags;
-    tag_extractors.produceTags(str, tags);
+    tag_extractors->produceTags(str, tags);
     RELEASE_ASSERT(tags.size() == tags_size,
                    absl::StrCat("tags.size()=", tags.size(), " tags_size==", tags_size));
   }

test/common/stats/tag_extractor_impl_test.cc

@@ -87,14 +87,17 @@ TEST(TagExtractorTest, BadRegex) {
 
 class DefaultTagRegexTester {
 public:
-  DefaultTagRegexTester() : tag_extractors_(envoy::config::metrics::v3::StatsConfig()) {}
+  DefaultTagRegexTester()
+      : tag_extractors_(std::move(
+            TagProducerImpl::createTagProducer(envoy::config::metrics::v3::StatsConfig(), tags_)
+                .value())) {}
 
   void testRegex(const std::string& stat_name, const std::string& expected_tag_extracted_name,
                  const TagVector& expected_tags) {
 
     // Test forward iteration through the regexes
     TagVector tags;
-    const std::string tag_extracted_name = tag_extractors_.produceTags(stat_name, tags);
+    const std::string tag_extracted_name = tag_extractors_->produceTags(stat_name, tags);
 
     auto cmp = [](const Tag& lhs, const Tag& rhs) {
       return lhs.name_ == rhs.name_ && lhs.value_ == rhs.value_;
@@ -138,10 +141,11 @@ class DefaultTagRegexTester {
     // version does not add in tag_extractors_.default_tags_ into tags. That doesn't matter
     // for this test, however.
     std::list<const TagExtractor*> extractors; // Note push-front is used to reverse order.
-    tag_extractors_.forEachExtractorMatching(metric_name,
-                                             [&extractors](const TagExtractorPtr& tag_extractor) {
-                                               extractors.push_front(tag_extractor.get());
-                                             });
+    reinterpret_cast<const TagProducerImpl*>(tag_extractors_.get())
+        ->forEachExtractorMatching(metric_name,
+                                   [&extractors](const TagExtractorPtr& tag_extractor) {
+                                     extractors.push_front(tag_extractor.get());
+                                   });
 
     IntervalSetImpl<size_t> remove_characters;
     TagExtractionContext tag_extraction_context(metric_name);
@@ -151,8 +155,9 @@ class DefaultTagRegexTester {
     return StringUtil::removeCharacters(metric_name, remove_characters);
   }
 
+  const Stats::TagVector tags_;
   SymbolTableImpl symbol_table_;
-  TagProducerImpl tag_extractors_;
+  TagProducerPtr tag_extractors_;
 };
 
 TEST(TagExtractorTest, DefaultTagExtractors) {