From a4f76632cb2ee8e9e1890b1a8af1cf5bdceee947 Mon Sep 17 00:00:00 2001 From: Yash Tibrewal Date: Tue, 29 Oct 2024 13:52:01 -0700 Subject: [PATCH] [XdsClient] Add missing authority to XdsClient metrics scope (#38009) Fix for b/365993761. Noticed that XdsClient metrics were not being reported due to authority not being properly set. This solution is not perfect since channels created later can possibly use a different authority, so preferring to use the default authority from the first channel. Closes #38009 COPYBARA_INTEGRATE_REVIEW=https://github.com/grpc/grpc/pull/38009 from yashykt:AddAuthorityToXdsClientMetricsScope 00071efa23ee3ebb5ffe28d3eea62fe7a82a54d9 PiperOrigin-RevId: 691149703 --- include/grpc/support/metrics.h | 6 ++- src/core/xds/grpc/xds_client_grpc.cc | 40 +++++++++++-------- src/core/xds/grpc/xds_client_grpc.h | 3 +- test/cpp/end2end/xds/xds_core_end2end_test.cc | 21 ++++++++-- 4 files changed, 48 insertions(+), 22 deletions(-) diff --git a/include/grpc/support/metrics.h b/include/grpc/support/metrics.h index 62525756620de..55d60d5de19c4 100644 --- a/include/grpc/support/metrics.h +++ b/include/grpc/support/metrics.h @@ -24,7 +24,11 @@ namespace grpc_core { namespace experimental { // Configuration (scope) for a specific client channel to be used for stats -// plugins. +// plugins. For some components like XdsClient where the same XdsClient instance +// can be shared across multiple channels that share the same target name but +// have different default authority and channel arguments, the component uses +// the configuration from the first channel that uses this XdsClient instance to +// determine StatsPluginChannelScope. class StatsPluginChannelScope { public: StatsPluginChannelScope( diff --git a/src/core/xds/grpc/xds_client_grpc.cc b/src/core/xds/grpc/xds_client_grpc.cc index db3bcacb5baaa..2e1328559f2c6 100644 --- a/src/core/xds/grpc/xds_client_grpc.cc +++ b/src/core/xds/grpc/xds_client_grpc.cc @@ -225,6 +225,23 @@ absl::StatusOr GetBootstrapContents(const char* fallback_config) { "not defined"); } +GlobalStatsPluginRegistry::StatsPluginGroup +GetStatsPluginGroupForKeyAndChannelArgs(absl::string_view key, + const ChannelArgs& channel_args) { + if (key == GrpcXdsClient::kServerKey) { + return GlobalStatsPluginRegistry::GetStatsPluginsForServer(channel_args); + } + grpc_event_engine::experimental::ChannelArgsEndpointConfig endpoint_config( + channel_args); + std::string authority = + channel_args.GetOwnedString(GRPC_ARG_DEFAULT_AUTHORITY) + .value_or( + CoreConfiguration::Get().resolver_registry().GetDefaultAuthority( + key)); + experimental::StatsPluginChannelScope scope(key, authority, endpoint_config); + return GlobalStatsPluginRegistry::GetStatsPluginsForChannel(scope); +} + } // namespace absl::StatusOr> GrpcXdsClient::GetOrCreate( @@ -241,7 +258,8 @@ absl::StatusOr> GrpcXdsClient::GetOrCreate( auto channel_args = ChannelArgs::FromC(xds_channel_args); return MakeRefCounted( key, std::move(*bootstrap), channel_args, - MakeRefCounted(channel_args)); + MakeRefCounted(channel_args), + GetStatsPluginGroupForKeyAndChannelArgs(key, args)); } // Otherwise, use the global instance. MutexLock lock(g_mu); @@ -264,7 +282,8 @@ absl::StatusOr> GrpcXdsClient::GetOrCreate( auto channel_args = ChannelArgs::FromC(g_channel_args); auto xds_client = MakeRefCounted( key, std::move(*bootstrap), channel_args, - MakeRefCounted(channel_args)); + MakeRefCounted(channel_args), + GetStatsPluginGroupForKeyAndChannelArgs(key, args)); g_xds_client_map->emplace(xds_client->key(), xds_client.get()); GRPC_TRACE_LOG(xds_client, INFO) << "[xds_client " << xds_client.get() << "] Created xDS client for key " << key; @@ -273,18 +292,6 @@ absl::StatusOr> GrpcXdsClient::GetOrCreate( namespace { -GlobalStatsPluginRegistry::StatsPluginGroup GetStatsPluginGroupForKey( - absl::string_view key) { - if (key == GrpcXdsClient::kServerKey) { - return GlobalStatsPluginRegistry::GetStatsPluginsForServer(ChannelArgs{}); - } - grpc_event_engine::experimental::ChannelArgsEndpointConfig endpoint_config( - ChannelArgs{}); - // TODO(roth): How do we set the authority here? - experimental::StatsPluginChannelScope scope(key, "", endpoint_config); - return GlobalStatsPluginRegistry::GetStatsPluginsForChannel(scope); -} - std::string UserAgentName() { return absl::StrCat("gRPC C-core ", GPR_PLATFORM_STRING, GRPC_XDS_USER_AGENT_NAME_SUFFIX_STRING); @@ -301,7 +308,8 @@ std::string UserAgentVersion() { GrpcXdsClient::GrpcXdsClient( absl::string_view key, std::shared_ptr bootstrap, const ChannelArgs& args, - RefCountedPtr transport_factory) + RefCountedPtr transport_factory, + GlobalStatsPluginRegistry::StatsPluginGroup stats_plugin_group) : XdsClient( bootstrap, transport_factory, grpc_event_engine::experimental::GetDefaultEventEngine(), @@ -315,7 +323,7 @@ GrpcXdsClient::GrpcXdsClient( certificate_provider_store_(MakeOrphanable( static_cast(this->bootstrap()) .certificate_providers())), - stats_plugin_group_(GetStatsPluginGroupForKey(key_)), + stats_plugin_group_(std::move(stats_plugin_group)), registered_metric_callback_(stats_plugin_group_.RegisterCallback( [this](CallbackMetricReporter& reporter) { ReportCallbackMetrics(reporter); diff --git a/src/core/xds/grpc/xds_client_grpc.h b/src/core/xds/grpc/xds_client_grpc.h index f52fdfc1dba38..612a963950a78 100644 --- a/src/core/xds/grpc/xds_client_grpc.h +++ b/src/core/xds/grpc/xds_client_grpc.h @@ -63,7 +63,8 @@ class GrpcXdsClient final : public XdsClient { GrpcXdsClient(absl::string_view key, std::shared_ptr bootstrap, const ChannelArgs& args, - RefCountedPtr transport_factory); + RefCountedPtr transport_factory, + GlobalStatsPluginRegistry::StatsPluginGroup stats_plugin_group); // Helpers for encoding the XdsClient object in channel args. static absl::string_view ChannelArgName() { diff --git a/test/cpp/end2end/xds/xds_core_end2end_test.cc b/test/cpp/end2end/xds/xds_core_end2end_test.cc index df75f8e0923cc..4d486de2b9821 100644 --- a/test/cpp/end2end/xds/xds_core_end2end_test.cc +++ b/test/cpp/end2end/xds/xds_core_end2end_test.cc @@ -1115,10 +1115,23 @@ TEST_P(XdsFederationTest, FederationServer) { class XdsMetricsTest : public XdsEnd2endTest { protected: void SetUp() override { - stats_plugin_ = grpc_core::FakeStatsPluginBuilder() - .UseDisabledByDefaultMetrics(true) - .BuildAndRegister(); - InitClient(); + stats_plugin_ = + grpc_core::FakeStatsPluginBuilder() + .UseDisabledByDefaultMetrics(true) + .SetChannelFilter( + [](const grpc_core::experimental::StatsPluginChannelScope& + scope) { + return scope.target() == absl::StrCat("xds:", kServerName) && + scope.default_authority() == kServerName && + scope.experimental_args().GetString("test_only.arg") == + "test_only.value"; + }) + .BuildAndRegister(); + ChannelArguments args; + args.SetString("test_only.arg", "test_only.value"); + InitClient(/*builder=*/absl::nullopt, /*lb_expected_authority=*/"", + /*xds_resource_does_not_exist_timeout_ms=*/0, + /*balancer_authority_override=*/"", /*args=*/&args); } std::shared_ptr stats_plugin_;