[XdsClient] Add missing authority to XdsClient metrics scope (grpc#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 grpc#38009 COPYBARA_INTEGRATE_REVIEW=grpc#38009 from yashykt:AddAuthorityToXdsClientMetricsScope 00071ef PiperOrigin-RevId: 691149703
yashykt · Oct 29, 2024 · a4f7663 · a4f7663
1 parent 62e547b
commit a4f7663
Show file tree

Hide file tree

Showing 4 changed files with 48 additions and 22 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(
+      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<RefCountedPtr<GrpcXdsClient>> GrpcXdsClient::GetOrCreate(
@@ -241,7 +258,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);
@@ -264,7 +282,8 @@ absl::StatusOr<RefCountedPtr<GrpcXdsClient>> GrpcXdsClient::GetOrCreate(
   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;
@@ -273,18 +292,6 @@ absl::StatusOr<RefCountedPtr<GrpcXdsClient>> 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<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(),
@@ -315,7 +323,7 @@ GrpcXdsClient::GrpcXdsClient(
       certificate_provider_store_(MakeOrphanable<CertificateProviderStore>(
           static_cast<const GrpcXdsBootstrap&>(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
@@ -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() {

diff --git 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<grpc_core::FakeStatsPlugin> stats_plugin_;