From 8f2245d0a7be54a74654159f0ba4ea9b62df3229 Mon Sep 17 00:00:00 2001 From: Yash Tibrewal Date: Thu, 8 Feb 2024 17:01:00 -0800 Subject: [PATCH] [CSM] Disable metrics recording when CsmObservability goes out of scope (#35835) As discussed, this change adds scoping to `CsmObservability` such that when that object goes out of scope, new channels and servers don't record metrics. In the documentation, I've talked about how existing channels/servers are going to continue to record metrics but i've left room for us to change that behavior in the future. The current way of doing this is through a global bool since there can only be one plugin right now, but we'll change this to use the global stats plugin registry in the future. Closes #35835 COPYBARA_INTEGRATE_REVIEW=https://github.com/grpc/grpc/pull/35835 from yashykt:DisableCsmObsOnScope 33a7c2f7bc59809c135ecc14eacf6cae8e64d8d1 PiperOrigin-RevId: 605468117 --- include/grpcpp/ext/csm_observability.h | 13 +++++--- src/cpp/ext/csm/csm_observability.cc | 28 ++++++++++++++++- src/cpp/ext/csm/csm_observability.h | 6 ++++ test/cpp/ext/csm/csm_observability_test.cc | 36 +++++++++++++++++++++- 4 files changed, 77 insertions(+), 6 deletions(-) diff --git a/include/grpcpp/ext/csm_observability.h b/include/grpcpp/ext/csm_observability.h index 706807032d7..86463acf5f7 100644 --- a/include/grpcpp/ext/csm_observability.h +++ b/include/grpcpp/ext/csm_observability.h @@ -38,16 +38,21 @@ class OpenTelemetryPluginBuilderImpl; namespace experimental { -// This is a no-op at present, but in the future, this object would be useful -// for performing cleanup. +// This object maintains state around the registered CsmObservability plugin. +// The application is responsible for retaining this object until it has closed +// all channels and servers that are recording metrics. class CsmObservability { public: CsmObservability() = default; + ~CsmObservability(); // Disable copy constructor and copy-assignment operator. CsmObservability(const CsmObservability&) = delete; CsmObservability& operator=(const CsmObservability&) = delete; - CsmObservability(CsmObservability&&) = default; - CsmObservability& operator=(CsmObservability&&) = default; + CsmObservability(CsmObservability&&) noexcept; + CsmObservability& operator=(CsmObservability&&) noexcept; + + private: + bool valid_ = true; }; // CsmObservabilityBuilder configures observability for all service mesh traffic diff --git a/src/cpp/ext/csm/csm_observability.cc b/src/cpp/ext/csm/csm_observability.cc index 0bb2590daaf..0b503455efe 100644 --- a/src/cpp/ext/csm/csm_observability.cc +++ b/src/cpp/ext/csm/csm_observability.cc @@ -45,11 +45,17 @@ namespace grpc { namespace internal { +namespace { +std::atomic g_csm_plugin_enabled(false); +} + bool CsmServerSelector(const grpc_core::ChannelArgs& args) { - return args.GetBool(GRPC_ARG_XDS_ENABLED_SERVER).value_or(false); + return g_csm_plugin_enabled && + args.GetBool(GRPC_ARG_XDS_ENABLED_SERVER).value_or(false); } bool CsmChannelTargetSelector(absl::string_view target) { + if (!g_csm_plugin_enabled) return false; auto uri = grpc_core::URI::Parse(target); if (!uri.ok()) { gpr_log(GPR_ERROR, "Failed to parse URI: %s", std::string(target).c_str()); @@ -96,6 +102,25 @@ class CsmOpenTelemetryPluginOption namespace experimental { +// +// CsmObservability +// + +CsmObservability::~CsmObservability() { + if (valid_) { + internal::g_csm_plugin_enabled = false; + } +} + +CsmObservability::CsmObservability(CsmObservability&& other) noexcept { + other.valid_ = false; +} +CsmObservability& CsmObservability::operator=( + CsmObservability&& other) noexcept { + other.valid_ = false; + return *this; +} + // // CsmObservabilityBuilder // @@ -132,6 +157,7 @@ absl::StatusOr CsmObservabilityBuilder::BuildAndRegister() { builder_->AddPluginOption( std::make_unique()); auto status = builder_->BuildAndRegisterGlobal(); + internal::g_csm_plugin_enabled = true; if (!status.ok()) { return status; } diff --git a/src/cpp/ext/csm/csm_observability.h b/src/cpp/ext/csm/csm_observability.h index d882f230177..1f908138f34 100644 --- a/src/cpp/ext/csm/csm_observability.h +++ b/src/cpp/ext/csm/csm_observability.h @@ -23,6 +23,8 @@ #include "absl/strings/string_view.h" +#include "src/core/lib/channel/channel_args.h" + namespace grpc { namespace internal { @@ -30,6 +32,10 @@ namespace internal { // Returns true if the channel is a CSM channel. bool CsmChannelTargetSelector(absl::string_view target); +// EXPOSED FOR TESTING PURPOSES ONLY +// Returns true if the server is a CSM server. +bool CsmServerSelector(const grpc_core::ChannelArgs& args); + } // namespace internal } // namespace grpc diff --git a/test/cpp/ext/csm/csm_observability_test.cc b/test/cpp/ext/csm/csm_observability_test.cc index 69073ff8f92..34af9edd58b 100644 --- a/test/cpp/ext/csm/csm_observability_test.cc +++ b/test/cpp/ext/csm/csm_observability_test.cc @@ -24,7 +24,7 @@ #include #include -#include "src/core/lib/gprpp/env.h" +#include "src/core/ext/xds/xds_enabled_server.h" #include "test/core/util/test_config.h" namespace grpc { @@ -41,6 +41,7 @@ TEST(GsmDependencyTest, GoogleCloudOpenTelemetryDependency) { } TEST(CsmChannelTargetSelectorTest, NonXdsTargets) { + auto obs = experimental::CsmObservabilityBuilder().BuildAndRegister(); EXPECT_FALSE(internal::CsmChannelTargetSelector("foo.bar.google.com")); EXPECT_FALSE(internal::CsmChannelTargetSelector("dns:///foo.bar.google.com")); EXPECT_FALSE( @@ -50,19 +51,52 @@ TEST(CsmChannelTargetSelectorTest, NonXdsTargets) { } TEST(CsmChannelTargetSelectorTest, XdsTargets) { + auto obs = experimental::CsmObservabilityBuilder().BuildAndRegister(); EXPECT_TRUE(internal::CsmChannelTargetSelector("xds:///foo")); EXPECT_TRUE(internal::CsmChannelTargetSelector("xds:///foo.bar")); } TEST(CsmChannelTargetSelectorTest, XdsTargetsWithNonTDAuthority) { + auto obs = experimental::CsmObservabilityBuilder().BuildAndRegister(); EXPECT_FALSE(internal::CsmChannelTargetSelector("xds://authority/foo")); } TEST(CsmChannelTargetSelectorTest, XdsTargetsWithTDAuthority) { + auto obs = experimental::CsmObservabilityBuilder().BuildAndRegister(); EXPECT_TRUE(internal::CsmChannelTargetSelector( "xds://traffic-director-global.xds.googleapis.com/foo")); } +TEST(CsmChannelTargetSelectorTest, CsmObservabilityOutOfScope) { + { auto obs = experimental::CsmObservabilityBuilder().BuildAndRegister(); } + // When CsmObservability goes out of scope, the target selector should return + // false as well. + EXPECT_FALSE(internal::CsmChannelTargetSelector("foo.bar.google.com")); + EXPECT_FALSE(internal::CsmChannelTargetSelector("xds:///foo")); + EXPECT_FALSE(internal::CsmChannelTargetSelector( + "xds://traffic-director-global.xds.googleapis.com/foo")); +} + +TEST(CsmServerSelectorTest, ChannelArgsWithoutXdsServerArg) { + auto obs = experimental::CsmObservabilityBuilder().BuildAndRegister(); + EXPECT_FALSE(internal::CsmServerSelector(grpc_core::ChannelArgs())); +} + +TEST(CsmServerSelectorTest, ChannelArgsWithXdsServerArg) { + auto obs = experimental::CsmObservabilityBuilder().BuildAndRegister(); + EXPECT_TRUE(internal::CsmServerSelector( + grpc_core::ChannelArgs().Set(GRPC_ARG_XDS_ENABLED_SERVER, true))); +} + +TEST(CsmServerSelectorTest, CsmObservabilityOutOfScope) { + { auto obs = experimental::CsmObservabilityBuilder().BuildAndRegister(); } + // When CsmObservability goes out of scope, the server selector should return + // false as well. + EXPECT_FALSE(internal::CsmServerSelector(grpc_core::ChannelArgs())); + EXPECT_FALSE(internal::CsmServerSelector( + grpc_core::ChannelArgs().Set(GRPC_ARG_XDS_ENABLED_SERVER, true))); +} + } // namespace } // namespace testing } // namespace grpc