Do not modify args for the xds client transport

yashykt · Oct 28, 2024 · 499c631 · 499c631
1 parent 380c2be
commit 499c631
Show file tree

Hide file tree

Showing 3 changed files with 37 additions and 25 deletions.
diff --git 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
@@ -225,6 +225,23 @@ absl::StatusOr<std::string> 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(
+      ChannelArgs{});
+  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<RefCountedPtr<GrpcXdsClient>> GrpcXdsClient::GetOrCreate(
@@ -233,6 +250,11 @@ absl::StatusOr<RefCountedPtr<GrpcXdsClient>> GrpcXdsClient::GetOrCreate(
   // instance for the channel or server instead of using the global instance.
   absl::optional<absl::string_view> bootstrap_config = args.GetString(
       GRPC_ARG_TEST_ONLY_DO_NOT_USE_IN_PROD_XDS_BOOTSTRAP_CONFIG);
+  std::string channel_default_authority =
+      args.GetOwnedString(GRPC_ARG_DEFAULT_AUTHORITY)
+          .value_or(
+              CoreConfiguration::Get().resolver_registry().GetDefaultAuthority(
+                  key));
   if (bootstrap_config.has_value()) {
     auto bootstrap = GrpcXdsBootstrap::Create(*bootstrap_config);
     if (!bootstrap.ok()) return bootstrap.status();
@@ -241,7 +263,8 @@ absl::StatusOr<RefCountedPtr<GrpcXdsClient>> GrpcXdsClient::GetOrCreate(
     auto channel_args = ChannelArgs::FromC(xds_channel_args);
     return MakeRefCounted<GrpcXdsClient>(
         key, std::move(*bootstrap), channel_args,
-        MakeRefCounted<GrpcXdsTransportFactory>(channel_args));
+        MakeRefCounted<GrpcXdsTransportFactory>(channel_args),
+        GetStatsPluginGroupForKeyAndChannelArgs(key, args));
   }
   // Otherwise, use the global instance.
   MutexLock lock(g_mu);
@@ -261,11 +284,11 @@ absl::StatusOr<RefCountedPtr<GrpcXdsClient>> GrpcXdsClient::GetOrCreate(
   auto bootstrap = GrpcXdsBootstrap::Create(*bootstrap_contents);
   if (!bootstrap.ok()) return bootstrap.status();
   // Instantiate XdsClient.
-  auto channel_args =
-      g_channel_args != nullptr ? ChannelArgs::FromC(g_channel_args) : args;
+  auto channel_args = ChannelArgs::FromC(g_channel_args);
   auto xds_client = MakeRefCounted<GrpcXdsClient>(
       key, std::move(*bootstrap), channel_args,
-      MakeRefCounted<GrpcXdsTransportFactory>(channel_args));
+      MakeRefCounted<GrpcXdsTransportFactory>(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;
@@ -274,23 +297,6 @@ absl::StatusOr<RefCountedPtr<GrpcXdsClient>> GrpcXdsClient::GetOrCreate(
 
 namespace {
 
-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(
-      ChannelArgs{});
-  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);
-}
-
 std::string UserAgentName() {
   return absl::StrCat("gRPC C-core ", GPR_PLATFORM_STRING,
                       GRPC_XDS_USER_AGENT_NAME_SUFFIX_STRING);
@@ -307,7 +313,8 @@ std::string UserAgentVersion() {
 GrpcXdsClient::GrpcXdsClient(
     absl::string_view key, std::shared_ptr<GrpcXdsBootstrap> bootstrap,
     const ChannelArgs& args,
-    RefCountedPtr<XdsTransportFactory> transport_factory)
+    RefCountedPtr<XdsTransportFactory> transport_factory,
+    GlobalStatsPluginRegistry::StatsPluginGroup stats_plugin_group)
     : XdsClient(
           bootstrap, transport_factory,
           grpc_event_engine::experimental::GetDefaultEventEngine(),
@@ -321,7 +328,7 @@ GrpcXdsClient::GrpcXdsClient(
       certificate_provider_store_(MakeOrphanable<CertificateProviderStore>(
           static_cast<const GrpcXdsBootstrap&>(this->bootstrap())
               .certificate_providers())),
-      stats_plugin_group_(GetStatsPluginGroupForKeyAndChannelArgs(key_, args)),
+      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
@@ -63,7 +63,8 @@ class GrpcXdsClient final : public XdsClient {
   GrpcXdsClient(absl::string_view key,
                 std::shared_ptr<GrpcXdsBootstrap> bootstrap,
                 const ChannelArgs& args,
-                RefCountedPtr<XdsTransportFactory> transport_factory);
+                RefCountedPtr<XdsTransportFactory> transport_factory,
+                GlobalStatsPluginRegistry::StatsPluginGroup stats_plugin_group);
 
   // Helpers for encoding the XdsClient object in channel args.
   static absl::string_view ChannelArgName() {