From cbea707ade63cfa4c1c2d32efed393ae1ac5e95d Mon Sep 17 00:00:00 2001 From: wbpcode/wangbaiping Date: Wed, 1 Jul 2026 05:58:16 +0000 Subject: [PATCH] stats: migrate the simple scope creation to new API Signed-off-by: wbpcode/wangbaiping --- source/common/grpc/BUILD | 1 + .../common/grpc/async_client_manager_impl.cc | 8 ++++- .../common/listener_manager/listener_impl.cc | 19 ++++++++++-- source/common/secret/BUILD | 1 + source/common/secret/sds_api.cc | 7 ++++- source/common/tcp_proxy/tcp_proxy.cc | 6 +++- source/common/upstream/upstream_impl.cc | 12 +++++--- .../filters/network/redis_proxy/BUILD | 1 + .../filters/network/redis_proxy/config.cc | 8 +++-- .../grpc/async_client_manager_impl_test.cc | 8 ++--- .../stats/scope_provider_singleton_test.cc | 30 +++++++++---------- .../access_loggers/stats/stats_test.cc | 4 +-- test/mocks/stats/mocks.h | 14 +++++---- 13 files changed, 80 insertions(+), 39 deletions(-) diff --git a/source/common/grpc/BUILD b/source/common/grpc/BUILD index e948de2ea9911..b5ebdc0da9887 100644 --- a/source/common/grpc/BUILD +++ b/source/common/grpc/BUILD @@ -56,6 +56,7 @@ envoy_cc_library( "//envoy/singleton:manager_interface", "//envoy/thread_local:thread_local_interface", "//envoy/upstream:cluster_manager_interface", + "//source/common/config:well_known_names", "@envoy_api//envoy/config/bootstrap/v3:pkg_cc_proto", ] + envoy_select_google_grpc([":google_async_client_lib"]), ) diff --git a/source/common/grpc/async_client_manager_impl.cc b/source/common/grpc/async_client_manager_impl.cc index b6ba792b758a6..5a762aea6e553 100644 --- a/source/common/grpc/async_client_manager_impl.cc +++ b/source/common/grpc/async_client_manager_impl.cc @@ -6,6 +6,7 @@ #include "envoy/stats/scope.h" #include "source/common/common/base64.h" +#include "source/common/config/well_known_names.h" #include "source/common/grpc/async_client_impl.h" #include "source/common/protobuf/utility.h" @@ -84,7 +85,12 @@ GoogleAsyncClientFactoryImpl::GoogleAsyncClientFactoryImpl( Stats::Scope& scope, Server::Configuration::CommonFactoryContext& context, const StatNames& stat_names, absl::Status& creation_status) : google_tls_slot_(google_tls_slot), - scope_(scope.createScope(fmt::format("grpc.{}.", config.google_grpc().stat_prefix()))), + // grpc.().** + scope_(scope.createScopeWithTaggedName( + "grpc", + {Stats::TagStringView{Envoy::Config::TagNames::get().GOOGLE_GRPC_CLIENT_PREFIX, + config.google_grpc().stat_prefix()}}, + fmt::format("grpc.{}.", config.google_grpc().stat_prefix()))), config_(config), factory_context_(context), stat_names_(stat_names) { #ifndef ENVOY_GOOGLE_GRPC UNREFERENCED_PARAMETER(google_tls_slot_); diff --git a/source/common/listener_manager/listener_impl.cc b/source/common/listener_manager/listener_impl.cc index 82ebb4590ec53..be278d5f625d6 100644 --- a/source/common/listener_manager/listener_impl.cc +++ b/source/common/listener_manager/listener_impl.cc @@ -289,9 +289,22 @@ generateListenerStatsScope(const envoy::config::listener::v3::Listener& config, } } - Stats::ScopeSharedPtr scope = stats.createScope("", false, {}, scope_matcher); - Stats::ScopeSharedPtr listener_scope = stats.createScope( - fmt::format("listener.{}.", listenerStatsScope(config)), false, {}, std::move(scope_matcher)); + Stats::ScopeSharedPtr scope = + stats.createScopeWithTaggedName({}, {}, {}, false, {}, scope_matcher); + + // listener.(.)*, but specifically excluding "admin" + const std::string observability_name = listenerStatsScope(config); + Stats::ScopeSharedPtr listener_scope; + if (observability_name != "admin") { + // TODO(wbpcode): for the 'admin' listener, we won't extract the listener address as a tag. + listener_scope = stats.createScopeWithTaggedName( + "listener", + {Stats::TagStringView{Config::TagNames::get().LISTENER_ADDRESS, observability_name}}, + fmt::format("listener.{}.", observability_name), false, {}, std::move(scope_matcher)); + } else { + listener_scope = stats.createScopeWithTaggedName("listener.admin.", {}, {}, false, {}, + std::move(scope_matcher)); + } return std::make_pair(std::move(scope), std::move(listener_scope)); } diff --git a/source/common/secret/BUILD b/source/common/secret/BUILD index 4f7d45c20c6ab..aa3a5b2e8032d 100644 --- a/source/common/secret/BUILD +++ b/source/common/secret/BUILD @@ -61,6 +61,7 @@ envoy_cc_library( "//source/common/config:resource_type_helper_lib", "//source/common/config:utility_lib", "//source/common/config:watched_directory_lib", + "//source/common/config:well_known_names", "//source/common/init:target_lib", "//source/common/protobuf:utility_lib", "//source/common/ssl:certificate_validation_context_config_impl_lib", diff --git a/source/common/secret/sds_api.cc b/source/common/secret/sds_api.cc index 6e377181e88e0..afe2ed9390860 100644 --- a/source/common/secret/sds_api.cc +++ b/source/common/secret/sds_api.cc @@ -6,6 +6,7 @@ #include "source/common/common/assert.h" #include "source/common/config/api_version.h" +#include "source/common/config/well_known_names.h" #include "source/common/grpc/common.h" #include "source/common/protobuf/utility.h" @@ -23,7 +24,11 @@ SdsApi::SdsApi(envoy::config::core::v3::ConfigSource sds_config, absl::string_vi bool warm) : init_target_(fmt::format("SdsApi {}", sds_config_name), [this, warm] { initialize(warm); }), dispatcher_(dispatcher), api_(api), - scope_(stats.createScope(absl::StrCat("sds.", sds_config_name, "."))), + // sds.[.]** + scope_(stats.createScopeWithTaggedName( + "sds", + {Stats::TagStringView{Envoy::Config::TagNames::get().XDS_RESOURCE_NAME, sds_config_name}}, + absl::StrCat("sds.", sds_config_name, "."))), sds_api_stats_(generateStats(*scope_)), resource_type_helper_(validation_visitor, "name"), sds_config_(std::move(sds_config)), sds_config_name_(sds_config_name), clean_up_(std::move(destructor_cb)), subscription_factory_(subscription_factory), diff --git a/source/common/tcp_proxy/tcp_proxy.cc b/source/common/tcp_proxy/tcp_proxy.cc index 36101861b5489..f32c4ba340b4e 100644 --- a/source/common/tcp_proxy/tcp_proxy.cc +++ b/source/common/tcp_proxy/tcp_proxy.cc @@ -134,7 +134,11 @@ OnDemandStats OnDemandConfig::generateStats(Stats::Scope& scope) { Config::SharedConfig::SharedConfig( const envoy::extensions::filters::network::tcp_proxy::v3::TcpProxy& config, Server::Configuration::FactoryContext& context) - : stats_scope_(context.scope().createScope(fmt::format("tcp.{}", config.stat_prefix()))), + : // tcp.(.)* + stats_scope_(context.scope().createScopeWithTaggedName( + "tcp", + {Stats::TagStringView{Envoy::Config::TagNames::get().TCP_PREFIX, config.stat_prefix()}}, + fmt::format("tcp.{}", config.stat_prefix()))), stats_(generateStats(*stats_scope_)), flush_access_log_on_start_(config.access_log_options().flush_access_log_on_start()), proxy_protocol_tlv_merge_policy_(config.proxy_protocol_tlv_merge_policy()) { diff --git a/source/common/upstream/upstream_impl.cc b/source/common/upstream/upstream_impl.cc index cf24d6dfdd1e1..93e0f8976ac70 100644 --- a/source/common/upstream/upstream_impl.cc +++ b/source/common/upstream/upstream_impl.cc @@ -42,6 +42,7 @@ #include "source/common/common/fmt.h" #include "source/common/common/utility.h" #include "source/common/config/utility.h" +#include "source/common/config/well_known_names.h" #include "source/common/http/http1/codec_stats.h" #include "source/common/http/http2/codec_stats.h" #include "source/common/http/utility.h" @@ -465,10 +466,13 @@ generateStatsScope(const envoy::config::cluster::v3::Cluster& config, } } - return stats.createScope(fmt::format("cluster.{}.", (!config.alt_stat_name().empty()) - ? config.alt_stat_name() - : config.name()), - false, {}, std::move(scope_matcher)); + absl::string_view observability_name = + !config.alt_stat_name().empty() ? config.alt_stat_name() : config.name(); + + // cluster.(.)* + return stats.createScopeWithTaggedName( + "cluster", {Stats::TagStringView{Config::TagNames::get().CLUSTER_NAME, observability_name}}, + fmt::format("cluster.{}.", observability_name), false, {}, std::move(scope_matcher)); } // TODO(pianiststickman): this implementation takes a lock on the hot path and puts a copy of the diff --git a/source/extensions/filters/network/redis_proxy/BUILD b/source/extensions/filters/network/redis_proxy/BUILD index 78dd6def6ae91..c648ff751df0e 100644 --- a/source/extensions/filters/network/redis_proxy/BUILD +++ b/source/extensions/filters/network/redis_proxy/BUILD @@ -161,6 +161,7 @@ envoy_cc_extension( ":proxy_filter_lib", ":router_lib", "//envoy/upstream:upstream_interface", + "//source/common/config:well_known_names", "//source/extensions/common/dynamic_forward_proxy:dns_cache_manager_impl", "//source/extensions/common/redis:cluster_refresh_manager_lib", "//source/extensions/filters/network:well_known_names", diff --git a/source/extensions/filters/network/redis_proxy/config.cc b/source/extensions/filters/network/redis_proxy/config.cc index 705e7174cb19e..ff18841b8cc75 100644 --- a/source/extensions/filters/network/redis_proxy/config.cc +++ b/source/extensions/filters/network/redis_proxy/config.cc @@ -4,6 +4,7 @@ #include "envoy/extensions/filters/network/redis_proxy/v3/redis_proxy.pb.h" #include "envoy/extensions/filters/network/redis_proxy/v3/redis_proxy.pb.validate.h" +#include "source/common/config/well_known_names.h" #include "source/extensions/common/dynamic_forward_proxy/dns_cache_manager_impl.h" #include "source/extensions/common/redis/cluster_refresh_manager_impl.h" #include "source/extensions/filters/network/common/redis/aws_iam_authenticator_impl.h" @@ -100,8 +101,11 @@ Network::FilterFactoryCb RedisProxyFilterConfigFactory::createFilterFactoryFromP } } - Stats::ScopeSharedPtr stats_scope = - context.scope().createScope(fmt::format("cluster.{}.redis_cluster", cluster)); + // cluster.(.)* + Stats::ScopeSharedPtr stats_scope = context.scope().createScopeWithTaggedName( + "cluster.redis_cluster", + {Stats::TagStringView{Envoy::Config::TagNames::get().CLUSTER_NAME, cluster}}, + fmt::format("cluster.{}.redis_cluster", cluster)); // Get zone from LocalInfo for fallback when client_zone not explicitly configured const std::string& local_zone = server_context.localInfo().zoneName(); auto conn_pool_ptr = std::make_shared( diff --git a/test/common/grpc/async_client_manager_impl_test.cc b/test/common/grpc/async_client_manager_impl_test.cc index 36fd843713506..2e3dce554c4a7 100644 --- a/test/common/grpc/async_client_manager_impl_test.cc +++ b/test/common/grpc/async_client_manager_impl_test.cc @@ -363,7 +363,7 @@ TEST_F(AsyncClientManagerImplTest, EnvoyGrpcInvalid) { TEST_F(AsyncClientManagerImplTest, GoogleGrpc) { initialize(); - EXPECT_CALL(scope_, createScope_("grpc.foo.")); + EXPECT_CALL(scope_, createScope_("grpc", "grpc.foo.")); envoy::config::core::v3::GrpcService grpc_service; grpc_service.mutable_google_grpc()->set_stat_prefix("foo"); @@ -379,7 +379,7 @@ TEST_F(AsyncClientManagerImplTest, GoogleGrpc) { TEST_F(AsyncClientManagerImplTest, GoogleGrpcIllegalCharsInKey) { initialize(); - EXPECT_CALL(scope_, createScope_("grpc.foo.")); + EXPECT_CALL(scope_, createScope_("grpc", "grpc.foo.")); envoy::config::core::v3::GrpcService grpc_service; grpc_service.mutable_google_grpc()->set_stat_prefix("foo"); @@ -400,7 +400,7 @@ TEST_F(AsyncClientManagerImplTest, GoogleGrpcIllegalCharsInKey) { TEST_F(AsyncClientManagerImplTest, LegalGoogleGrpcChar) { initialize(); - EXPECT_CALL(scope_, createScope_("grpc.foo.")); + EXPECT_CALL(scope_, createScope_("grpc", "grpc.foo.")); envoy::config::core::v3::GrpcService grpc_service; grpc_service.mutable_google_grpc()->set_stat_prefix("foo"); @@ -420,7 +420,7 @@ TEST_F(AsyncClientManagerImplTest, LegalGoogleGrpcChar) { TEST_F(AsyncClientManagerImplTest, GoogleGrpcIllegalCharsInValue) { initialize(); - EXPECT_CALL(scope_, createScope_("grpc.foo.")); + EXPECT_CALL(scope_, createScope_("grpc", "grpc.foo.")); envoy::config::core::v3::GrpcService grpc_service; grpc_service.mutable_google_grpc()->set_stat_prefix("foo"); diff --git a/test/extensions/access_loggers/stats/scope_provider_singleton_test.cc b/test/extensions/access_loggers/stats/scope_provider_singleton_test.cc index 609677db9c343..6d90202f6e275 100644 --- a/test/extensions/access_loggers/stats/scope_provider_singleton_test.cc +++ b/test/extensions/access_loggers/stats/scope_provider_singleton_test.cc @@ -52,8 +52,8 @@ TEST_F(ScopeProviderSingletonTest, GetScopeCachesAndReturnsSameScope) { std::shared_ptr newly_created_scope; - EXPECT_CALL(mock_store_.mockScope(), createScope_(_)) - .WillOnce(Invoke([&](const std::string& name) { + EXPECT_CALL(mock_store_.mockScope(), createScope_(_, _)) + .WillOnce(Invoke([&](const std::string& name, const std::string&) { auto scope_name_storage = std::make_unique(name, mock_store_.symbolTable()); newly_created_scope = std::shared_ptr( @@ -84,8 +84,8 @@ TEST_F(ScopeProviderSingletonTest, GetScopeCachesAndReturnsSameScope) { scope_config3.mutable_max_histograms()->set_value(30); std::shared_ptr newly_created_scope3; - EXPECT_CALL(mock_store_.mockScope(), createScope_(_)) - .WillOnce(Invoke([&](const std::string& name) { + EXPECT_CALL(mock_store_.mockScope(), createScope_(_, _)) + .WillOnce(Invoke([&](const std::string& name, const std::string&) { auto scope_name_storage = std::make_unique(name, mock_store_.symbolTable()); newly_created_scope3 = std::shared_ptr( @@ -111,9 +111,9 @@ TEST_F(ScopeProviderSingletonTest, CleansUpExpiredScopesOnNextGetScope) { EXPECT_CALL(factory_context_, statsScope()).WillRepeatedly(ReturnRef(mock_store_.mockScope())); // First request should trigger createScope_ - EXPECT_CALL(mock_store_.mockScope(), createScope_(_)) + EXPECT_CALL(mock_store_.mockScope(), createScope_(_, _)) .Times(1) - .WillOnce(Invoke([&](const std::string& name) { + .WillOnce(Invoke([&](const std::string& name, const std::string&) { auto scope_name_storage = std::make_unique(name, mock_store_.symbolTable()); return std::shared_ptr( @@ -124,7 +124,7 @@ TEST_F(ScopeProviderSingletonTest, CleansUpExpiredScopesOnNextGetScope) { EXPECT_NE(returned_scope, nullptr); // While returned_scope is alive, requesting again should not trigger createScope_ - EXPECT_CALL(mock_store_.mockScope(), createScope_(_)).Times(0); + EXPECT_CALL(mock_store_.mockScope(), createScope_(_, _)).Times(0); auto returned_scope2 = Stats::ScopeProviderSingleton::getScope(factory_context_, scope_config); EXPECT_EQ(returned_scope, returned_scope2); @@ -136,9 +136,9 @@ TEST_F(ScopeProviderSingletonTest, CleansUpExpiredScopesOnNextGetScope) { returned_scope2.reset(); // Next request should trigger a new createScope_ because it was deleted - EXPECT_CALL(mock_store_.mockScope(), createScope_(_)) + EXPECT_CALL(mock_store_.mockScope(), createScope_(_, _)) .Times(1) - .WillOnce(Invoke([&](const std::string& name) { + .WillOnce(Invoke([&](const std::string& name, const std::string&) { auto scope_name_storage = std::make_unique(name, mock_store_.symbolTable()); return std::shared_ptr( @@ -159,9 +159,9 @@ TEST_F(ScopeProviderSingletonTest, GetScopeWithSharingDisabledDoesNotCache) { EXPECT_CALL(factory_context_, statsScope()).WillRepeatedly(ReturnRef(mock_store_.mockScope())); // First request should create scope - EXPECT_CALL(mock_store_.mockScope(), createScope_(_)) + EXPECT_CALL(mock_store_.mockScope(), createScope_(_, _)) .Times(1) - .WillOnce(Invoke([&](const std::string& name) { + .WillOnce(Invoke([&](const std::string& name, const std::string&) { auto scope_name_storage = std::make_unique(name, mock_store_.symbolTable()); return std::shared_ptr( @@ -172,9 +172,9 @@ TEST_F(ScopeProviderSingletonTest, GetScopeWithSharingDisabledDoesNotCache) { EXPECT_NE(returned_scope1, nullptr); // Second request should also create scope since sharing is disabled - EXPECT_CALL(mock_store_.mockScope(), createScope_(_)) + EXPECT_CALL(mock_store_.mockScope(), createScope_(_, _)) .Times(1) - .WillOnce(Invoke([&](const std::string& name) { + .WillOnce(Invoke([&](const std::string& name, const std::string&) { auto scope_name_storage = std::make_unique(name, mock_store_.symbolTable()); return std::shared_ptr( @@ -197,8 +197,8 @@ TEST_F(ScopeProviderSingletonTest, SupportsGetSharedAndCopying) { EXPECT_CALL(factory_context_.server_context_, mainThreadDispatcher()) .WillRepeatedly(ReturnRef(factory_context_.server_context_.dispatcher_)); - EXPECT_CALL(mock_store_.mockScope(), createScope_(_)) - .WillOnce(Invoke([&](const std::string& name) { + EXPECT_CALL(mock_store_.mockScope(), createScope_(_, _)) + .WillOnce(Invoke([&](const std::string& name, const std::string&) { auto scope_name_storage = std::make_unique(name, mock_store_.symbolTable()); return std::shared_ptr( diff --git a/test/extensions/access_loggers/stats/stats_test.cc b/test/extensions/access_loggers/stats/stats_test.cc index 971f98e17e7e4..1601f49947c9a 100644 --- a/test/extensions/access_loggers/stats/stats_test.cc +++ b/test/extensions/access_loggers/stats/stats_test.cc @@ -129,8 +129,8 @@ class StatsAccessLoggerTest : public testing::Test { .WillByDefault(testing::ReturnRef(store_.mockScope())); ON_CALL(context_, serverScope()).WillByDefault(testing::ReturnRef(store_.mockScope())); - EXPECT_CALL(store_.mockScope(), createScope_(_)) - .WillRepeatedly(Invoke([this](const std::string& name) { + EXPECT_CALL(store_.mockScope(), createScope_(_, _)) + .WillRepeatedly(Invoke([this](const std::string& name, const std::string&) { auto scope_name_storage = std::make_unique(name, context_.store_.symbolTable()); auto scope = std::make_shared>( diff --git a/test/mocks/stats/mocks.h b/test/mocks/stats/mocks.h index 7cd7024d41f27..79ca8f3188b61 100644 --- a/test/mocks/stats/mocks.h +++ b/test/mocks/stats/mocks.h @@ -298,21 +298,23 @@ class MockScope : public TestUtil::TestScope { MockScope(StatName prefix, MockStore& store); ScopeSharedPtr createScopeWithTaggedName(absl::string_view base_name, TagStringViewSpan, - absl::string_view, bool evictable, + absl::string_view tagged_name, bool evictable, const ScopeStatsLimitSettings& limits, StatsMatcherSharedPtr) override { checkCreateScopeArgs(evictable, limits); - return ScopeSharedPtr(createScope_(std::string(base_name))); + return ScopeSharedPtr(createScope_(std::string(base_name), std::string(tagged_name))); } - ScopeSharedPtr scopeFromTaggedName(StatName base_name, StatNameTagSpan, StatName, bool evictable, - const ScopeStatsLimitSettings& limits, + ScopeSharedPtr scopeFromTaggedName(StatName base_name, StatNameTagSpan, StatName tagged_name, + bool evictable, const ScopeStatsLimitSettings& limits, StatsMatcherSharedPtr) override { checkCreateScopeArgs(evictable, limits); - return createScope_(symbolTable().toString(base_name)); + return createScope_(symbolTable().toString(base_name), + tagged_name.empty() ? std::string() : symbolTable().toString(tagged_name)); } MOCK_METHOD(void, checkCreateScopeArgs, (bool, const ScopeStatsLimitSettings&)); - MOCK_METHOD(ScopeSharedPtr, createScope_, (const std::string& name)); + MOCK_METHOD(ScopeSharedPtr, createScope_, + (const std::string& base_name, const std::string& tagged_name)); MOCK_METHOD(CounterOptConstRef, findCounter, (StatName), (const)); MOCK_METHOD(GaugeOptConstRef, findGauge, (StatName), (const)); MOCK_METHOD(HistogramOptConstRef, findHistogram, (StatName), (const));