ecds: use top level stats prefix (envoyproxy#20396)

Signed-off-by: Kuat Yessenov <kuat@google.com>

docs/root/configuration/overview/extension.rst

@@ -80,8 +80,9 @@ for HTTP filters.
 
 Extension config discovery service has a :ref:`statistics
 <subscription_statistics>` tree rooted at
-*<stat_prefix>.extension_config_discovery.<extension_config_name>*. In addition
-to the common subscription statistics, it also provides the following:
+*extension_config_discovery.<stat_prefix>.<extension_config_name>*. For HTTP
+filters, the value of *<stat_prefix>* is *http_filter*. In addition to the
+common subscription statistics, it also provides the following:
 
 .. csv-table::
  :header: Name, Type, Description

docs/root/version_history/current.rst

@@ -18,6 +18,7 @@ Minor Behavior Changes
 * cryptomb: remove RSA PKCS1 v1.5 padding support.
 * decompressor: decompressor does not duplicate `accept-encoding` header values anymore. This behavioral change can be reverted by setting runtime guard ``envoy.reloadable_features.append_to_accept_content_encoding_only_once`` to false.
 * dynamic_forward_proxy: if a DNS resolution fails, failing immediately with a specific resolution error, rather than finishing up all local filters and failing to select an upstream host.
+* ecds: changed to use ``http_filter`` stat prefix as the metrics root for ECDS subscriptions. This behavior can be temporarily reverted by setting ``envoy.reloadable_features.top_level_ecds_stats`` to false.
 * ext_authz: added requested server name in ext_authz network filter for auth review.
 * file: changed disk based files to truncate files which are not being appended to. This behavioral change can be temporarily reverted by setting runtime guard ``envoy.reloadable_features.append_or_truncate`` to false.
 * grpc: flip runtime guard ``envoy.reloadable_features.enable_grpc_async_client_cache`` to be default enabled. async grpc client created through getOrCreateRawAsyncClient will be cached by default.

envoy/filter/config_provider_manager.h

@@ -52,6 +52,11 @@ template <class FactoryCb> class FilterConfigProviderManager {
  virtual FilterConfigProviderPtr<FactoryCb>
  createStaticFilterConfigProvider(const FactoryCb& config,
  const std::string& filter_config_name) PURE;
+
+ /**
+ * Get the stat prefix for the scope of the filter provider manager.
+ */
+ virtual absl::string_view statPrefix() const PURE;
 };
 
 } // namespace Filter

source/common/filter/config_discovery_impl.cc

@@ -16,6 +16,8 @@
 namespace Envoy {
 namespace Filter {
 
+constexpr absl::string_view HttpStatPrefix = "http_filter.";
+
 namespace {
 void validateTypeUrlHelper(const std::string& type_url,
  const absl::flat_hash_set<std::string> require_type_urls) {
@@ -67,9 +69,7 @@ FilterConfigSubscription::FilterConfigSubscription(
  filter_config_name_(filter_config_name), factory_context_(factory_context),
  init_target_(fmt::format("FilterConfigSubscription init {}", filter_config_name_),
  [this]() { start(); }),
- scope_(factory_context.scope().createScope(stat_prefix + "extension_config_discovery." +
- filter_config_name_ + ".")),
- stat_prefix_(stat_prefix),
+ scope_(factory_context.scope().createScope(stat_prefix)),
  stats_({ALL_EXTENSION_CONFIG_DISCOVERY_STATS(POOL_COUNTER(*scope_))}),
  filter_config_provider_manager_(filter_config_provider_manager),
  subscription_id_(subscription_id) {
@@ -244,6 +244,8 @@ std::tuple<ProtobufTypes::MessagePtr, std::string> HttpFilterConfigProviderManag
  return {std::move(message), factory.name()};
 }
 
+absl::string_view HttpFilterConfigProviderManagerImpl::statPrefix() const { return HttpStatPrefix; }
+
 ProtobufTypes::MessagePtr HttpFilterConfigProviderManagerImpl::getDefaultConfig(
  const ProtobufWkt::Any& proto_config, const std::string& filter_config_name,
  Server::Configuration::FactoryContext& factory_context, bool last_filter_in_filter_chain,

source/common/filter/config_discovery_impl.h

@@ -74,7 +74,7 @@ class DynamicFilterConfigProviderImpl : public DynamicFilterConfigProviderImplBa
  ProtobufTypes::MessagePtr&& default_config,
  bool last_filter_in_filter_chain,
  const std::string& filter_chain_type,
- const std::string& stat_prefix)
+ absl::string_view stat_prefix)
  : DynamicFilterConfigProviderImplBase(subscription, require_type_urls,
  last_filter_in_filter_chain, filter_chain_type),
  stat_prefix_(stat_prefix), factory_context_(factory_context),
@@ -230,7 +230,6 @@ class FilterConfigSubscription
  bool started_{false};
 
  Stats::ScopePtr scope_;
- const std::string stat_prefix_;
  ExtensionConfigDiscoveryStats stats_;
 
  // FilterConfigProviderManagerImplBase maintains active subscriptions in a map.
@@ -300,8 +299,20 @@ class FilterConfigProviderManagerImpl : public FilterConfigProviderManagerImplBa
  const std::string& filter_config_name, Server::Configuration::FactoryContext& factory_context,
  const std::string& stat_prefix, bool last_filter_in_filter_chain,
  const std::string& filter_chain_type) override {
+ std::string subscription_stat_prefix;
+ absl::string_view provider_stat_prefix;
+ if (Runtime::runtimeFeatureEnabled("envoy.reloadable_features.top_level_ecds_stats")) {
+ subscription_stat_prefix =
+ absl::StrCat("extension_config_discovery.", statPrefix(), filter_config_name, ".");
+ provider_stat_prefix = subscription_stat_prefix;
+ } else {
+ subscription_stat_prefix =
+ absl::StrCat(stat_prefix, "extension_config_discovery.", filter_config_name, ".");
+ provider_stat_prefix = stat_prefix;
+ }
+
  auto subscription = getSubscription(config_source.config_source(), filter_config_name,
- factory_context, stat_prefix);
+ factory_context, subscription_stat_prefix);
  // For warming, wait until the subscription receives the first response to indicate readiness.
  // Otherwise, mark ready immediately and start the subscription on initialization. A default
  // config is expected in the latter case.
@@ -323,7 +334,7 @@ class FilterConfigProviderManagerImpl : public FilterConfigProviderManagerImplBa
 
  auto provider = std::make_unique<DynamicFilterConfigProviderImpl<Factory, FactoryCb>>(
  subscription, require_type_urls, factory_context, std::move(default_config),
- last_filter_in_filter_chain, filter_chain_type, stat_prefix);
+ last_filter_in_filter_chain, filter_chain_type, provider_stat_prefix);
 
  // Ensure the subscription starts if it has not already.
  if (config_source.apply_default_config_without_warming()) {
@@ -339,6 +350,8 @@ class FilterConfigProviderManagerImpl : public FilterConfigProviderManagerImplBa
  return std::make_unique<StaticFilterConfigProviderImpl<FactoryCb>>(config, filter_config_name);
  }
 
+ absl::string_view statPrefix() const override PURE;
+
 protected:
  virtual ProtobufTypes::MessagePtr
  getDefaultConfig(const ProtobufWkt::Any& proto_config, const std::string& filter_config_name,
@@ -355,6 +368,8 @@ class HttpFilterConfigProviderManagerImpl
  getMessage(const envoy::config::core::v3::TypedExtensionConfig& filter_config,
  Server::Configuration::ServerFactoryContext& factory_context) const override;
 
+ absl::string_view statPrefix() const override;
+
 protected:
  ProtobufTypes::MessagePtr
  getDefaultConfig(const ProtobufWkt::Any& proto_config, const std::string& filter_config_name,

source/common/runtime/runtime_features.cc

@@ -60,6 +60,7 @@ RUNTIME_GUARD(envoy_reloadable_features_skip_dispatching_frames_for_closed_conne
 RUNTIME_GUARD(envoy_reloadable_features_strict_check_on_ipv4_compat);
 RUNTIME_GUARD(envoy_reloadable_features_support_locality_update_on_eds_cluster_endpoints);
 RUNTIME_GUARD(envoy_reloadable_features_test_feature_true);
+RUNTIME_GUARD(envoy_reloadable_features_top_level_ecds_stats);
 RUNTIME_GUARD(envoy_reloadable_features_udp_listener_updates_filter_chain_in_place);
 RUNTIME_GUARD(envoy_reloadable_features_update_expected_rq_timeout_on_retry);
 RUNTIME_GUARD(envoy_reloadable_features_update_grpc_response_error_tag);

test/common/filter/BUILD

@@ -23,6 +23,7 @@ envoy_cc_test(
  "//test/mocks/thread_local:thread_local_mocks",
  "//test/mocks/upstream:upstream_mocks",
  "//test/test_common:simulated_time_system_lib",
+ "//test/test_common:test_runtime_lib",
  "//test/test_common:utility_lib",
  "@envoy_api//envoy/config/core/v3:pkg_cc_proto",
  "@envoy_api//envoy/service/discovery/v3:pkg_cc_proto",

test/common/filter/config_discovery_impl_test.cc

@@ -20,6 +20,7 @@
 #include "test/mocks/upstream/mocks.h"
 #include "test/test_common/printers.h"
 #include "test/test_common/simulated_time_system.h"
+#include "test/test_common/test_runtime.h"
 #include "test/test_common/utility.h"
 
 #include "absl/strings/substitute.h"
@@ -161,8 +162,10 @@ TEST_F(FilterConfigDiscoveryImplTest, Basic) {
  EXPECT_CALL(init_watcher_, ready());
  callbacks_->onConfigUpdate(decoded_resources.refvec_, response.version_info());
  EXPECT_NE(absl::nullopt, provider_->config());
- EXPECT_EQ(1UL, scope_.counter("xds.extension_config_discovery.foo.config_reload").value());
- EXPECT_EQ(0UL, scope_.counter("xds.extension_config_discovery.foo.config_fail").value());
+ EXPECT_EQ(1UL,
+ scope_.counter("extension_config_discovery.http_filter.foo.config_reload").value());
+ EXPECT_EQ(0UL,
+ scope_.counter("extension_config_discovery.http_filter.foo.config_fail").value());
  }
 
  // 2nd request with same response. Based on hash should not reload config.
@@ -180,18 +183,50 @@ TEST_F(FilterConfigDiscoveryImplTest, Basic) {
  const auto decoded_resources =
  TestUtility::decodeResources<envoy::config::core::v3::TypedExtensionConfig>(response);
  callbacks_->onConfigUpdate(decoded_resources.refvec_, response.version_info());
- EXPECT_EQ(1UL, scope_.counter("xds.extension_config_discovery.foo.config_reload").value());
- EXPECT_EQ(0UL, scope_.counter("xds.extension_config_discovery.foo.config_fail").value());
+ EXPECT_EQ(1UL,
+ scope_.counter("extension_config_discovery.http_filter.foo.config_reload").value());
+ EXPECT_EQ(0UL,
+ scope_.counter("extension_config_discovery.http_filter.foo.config_fail").value());
  }
 }
 
+TEST_F(FilterConfigDiscoveryImplTest, BasicDeprecatedStatPrefix) {
+ TestScopedRuntime scoped_runtime;
+ scoped_runtime.mergeValues({{"envoy.reloadable_features.top_level_ecds_stats", "false"}});
+
+ InSequence s;
+ setup();
+ EXPECT_EQ("foo", provider_->name());
+ EXPECT_EQ(absl::nullopt, provider_->config());
+
+ const std::string response_yaml = R"EOF(
+ version_info: "1"
+ resources:
+ - "@type": type.googleapis.com/envoy.config.core.v3.TypedExtensionConfig
+ name: foo
+ typed_config:
+ "@type": type.googleapis.com/envoy.extensions.filters.http.router.v3.Router
+ )EOF";
+ const auto response =
+ TestUtility::parseYaml<envoy::service::discovery::v3::DiscoveryResponse>(response_yaml);
+ const auto decoded_resources =
+ TestUtility::decodeResources<envoy::config::core::v3::TypedExtensionConfig>(response);
+
+ EXPECT_CALL(init_watcher_, ready());
+ callbacks_->onConfigUpdate(decoded_resources.refvec_, response.version_info());
+ EXPECT_NE(absl::nullopt, provider_->config());
+ EXPECT_EQ(1UL, scope_.counter("xds.extension_config_discovery.foo.config_reload").value());
+ EXPECT_EQ(0UL, scope_.counter("xds.extension_config_discovery.foo.config_fail").value());
+}
+
 TEST_F(FilterConfigDiscoveryImplTest, ConfigFailed) {
  InSequence s;
  setup();
  EXPECT_CALL(init_watcher_, ready());
  callbacks_->onConfigUpdateFailed(Config::ConfigUpdateFailureReason::FetchTimedout, {});
- EXPECT_EQ(0UL, scope_.counter("xds.extension_config_discovery.foo.config_reload").value());
- EXPECT_EQ(1UL, scope_.counter("xds.extension_config_discovery.foo.config_fail").value());
+ EXPECT_EQ(0UL,
+ scope_.counter("extension_config_discovery.http_filter.foo.config_reload").value());
+ EXPECT_EQ(1UL, scope_.counter("extension_config_discovery.http_filter.foo.config_fail").value());
 }
 
 TEST_F(FilterConfigDiscoveryImplTest, TooManyResources) {
@@ -217,7 +252,8 @@ TEST_F(FilterConfigDiscoveryImplTest, TooManyResources) {
  EXPECT_THROW_WITH_MESSAGE(
  callbacks_->onConfigUpdate(decoded_resources.refvec_, response.version_info()),
  EnvoyException, "Unexpected number of resources in ExtensionConfigDS response: 2");
- EXPECT_EQ(0UL, scope_.counter("xds.extension_config_discovery.foo.config_reload").value());
+ EXPECT_EQ(0UL,
+ scope_.counter("extension_config_discovery.http_filter.foo.config_reload").value());
 }
 
 TEST_F(FilterConfigDiscoveryImplTest, WrongName) {
@@ -239,7 +275,8 @@ TEST_F(FilterConfigDiscoveryImplTest, WrongName) {
  EXPECT_THROW_WITH_MESSAGE(
  callbacks_->onConfigUpdate(decoded_resources.refvec_, response.version_info()),
  EnvoyException, "Unexpected resource name in ExtensionConfigDS response: bar");
- EXPECT_EQ(0UL, scope_.counter("xds.extension_config_discovery.foo.config_reload").value());
+ EXPECT_EQ(0UL,
+ scope_.counter("extension_config_discovery.http_filter.foo.config_reload").value());
 }
 
 TEST_F(FilterConfigDiscoveryImplTest, Incremental) {
@@ -268,16 +305,18 @@ version_info: "1"
  EXPECT_CALL(init_watcher_, ready());
  do_xds_response(true);
  EXPECT_NE(absl::nullopt, provider_->config());
- EXPECT_EQ(1UL, scope_.counter("xds.extension_config_discovery.foo.config_reload").value());
- EXPECT_EQ(0UL, scope_.counter("xds.extension_config_discovery.foo.config_fail").value());
+ EXPECT_EQ(1UL,
+ scope_.counter("extension_config_discovery.http_filter.foo.config_reload").value());
+ EXPECT_EQ(0UL, scope_.counter("extension_config_discovery.http_filter.foo.config_fail").value());
 
  // Ensure that we honor resource removals.
  Protobuf::RepeatedPtrField<std::string> remove;
  *remove.Add() = "foo";
  callbacks_->onConfigUpdate({}, remove, "1");
  EXPECT_EQ(absl::nullopt, provider_->config());
- EXPECT_EQ(2UL, scope_.counter("xds.extension_config_discovery.foo.config_reload").value());
- EXPECT_EQ(0UL, scope_.counter("xds.extension_config_discovery.foo.config_fail").value());
+ EXPECT_EQ(2UL,
+ scope_.counter("extension_config_discovery.http_filter.foo.config_reload").value());
+ EXPECT_EQ(0UL, scope_.counter("extension_config_discovery.http_filter.foo.config_fail").value());
 }
 
 TEST_F(FilterConfigDiscoveryImplTest, IncrementalWithDefault) {
@@ -310,26 +349,29 @@ version_info: "1"
  EXPECT_CALL(init_watcher_, ready());
  do_xds_response(false);
  EXPECT_NE(absl::nullopt, provider_->config());
- EXPECT_EQ(1UL, scope_.counter("xds.extension_config_discovery.foo.config_reload").value());
- EXPECT_EQ(0UL, scope_.counter("xds.extension_config_discovery.foo.config_fail").value());
+ EXPECT_EQ(1UL,
+ scope_.counter("extension_config_discovery.http_filter.foo.config_reload").value());
+ EXPECT_EQ(0UL, scope_.counter("extension_config_discovery.http_filter.foo.config_fail").value());
 
  // If we get a removal while a default is configured, we should revert back to the default
  // instead of clearing out the factory.
  Protobuf::RepeatedPtrField<std::string> remove;
  *remove.Add() = "foo";
  callbacks_->onConfigUpdate({}, remove, "1");
  EXPECT_NE(absl::nullopt, provider_->config());
- EXPECT_EQ(2UL, scope_.counter("xds.extension_config_discovery.foo.config_reload").value());
- EXPECT_EQ(0UL, scope_.counter("xds.extension_config_discovery.foo.config_fail").value());
+ EXPECT_EQ(2UL,
+ scope_.counter("extension_config_discovery.http_filter.foo.config_reload").value());
+ EXPECT_EQ(0UL, scope_.counter("extension_config_discovery.http_filter.foo.config_fail").value());
 }
 
 TEST_F(FilterConfigDiscoveryImplTest, ApplyWithoutWarming) {
  InSequence s;
  setup(false);
  EXPECT_EQ("foo", provider_->name());
  EXPECT_NE(absl::nullopt, provider_->config());
- EXPECT_EQ(0UL, scope_.counter("xds.extension_config_discovery.foo.config_reload").value());
- EXPECT_EQ(0UL, scope_.counter("xds.extension_config_discovery.foo.config_fail").value());
+ EXPECT_EQ(0UL,
+ scope_.counter("extension_config_discovery.http_filter.foo.config_reload").value());
+ EXPECT_EQ(0UL, scope_.counter("extension_config_discovery.http_filter.foo.config_fail").value());
 }
 
 TEST_F(FilterConfigDiscoveryImplTest, DualProviders) {
@@ -354,7 +396,8 @@ TEST_F(FilterConfigDiscoveryImplTest, DualProviders) {
  callbacks_->onConfigUpdate(decoded_resources.refvec_, response.version_info());
  EXPECT_NE(absl::nullopt, provider_->config());
  EXPECT_NE(absl::nullopt, provider2->config());
- EXPECT_EQ(1UL, scope_.counter("xds.extension_config_discovery.foo.config_reload").value());
+ EXPECT_EQ(1UL,
+ scope_.counter("extension_config_discovery.http_filter.foo.config_reload").value());
 }
 
 TEST_F(FilterConfigDiscoveryImplTest, DualProvidersInvalid) {
@@ -380,7 +423,8 @@ TEST_F(FilterConfigDiscoveryImplTest, DualProvidersInvalid) {
  EnvoyException,
  "Error: filter config has type URL test.integration.filters.AddBodyFilterConfig but "
  "expect envoy.extensions.filters.http.router.v3.Router.");
- EXPECT_EQ(0UL, scope_.counter("xds.extension_config_discovery.foo.config_reload").value());
+ EXPECT_EQ(0UL,
+ scope_.counter("extension_config_discovery.http_filter.foo.config_reload").value());
 }
 
 // Raise exception when filter is not the last filter in filter chain, but the filter is terminal
@@ -406,7 +450,8 @@ TEST_F(FilterConfigDiscoveryImplTest, TerminalFilterInvalid) {
  EnvoyException,
  "Error: terminal filter named foo of type envoy.filters.http.router must be the last filter "
  "in a http filter chain.");
- EXPECT_EQ(0UL, scope_.counter("xds.extension_config_discovery.foo.config_reload").value());
+ EXPECT_EQ(0UL,
+ scope_.counter("extension_config_discovery.http_filter.foo.config_reload").value());
 }
 
 } // namespace