[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 33a7c2f7bc
PiperOrigin-RevId: 605468117
This commit is contained in:
Yash Tibrewal 2024-02-08 17:01:00 -08:00 committed by Copybara-Service
parent 21cb320080
commit 8f2245d0a7
4 changed files with 77 additions and 6 deletions

View File

@ -38,16 +38,21 @@ class OpenTelemetryPluginBuilderImpl;
namespace experimental { namespace experimental {
// This is a no-op at present, but in the future, this object would be useful // This object maintains state around the registered CsmObservability plugin.
// for performing cleanup. // The application is responsible for retaining this object until it has closed
// all channels and servers that are recording metrics.
class CsmObservability { class CsmObservability {
public: public:
CsmObservability() = default; CsmObservability() = default;
~CsmObservability();
// Disable copy constructor and copy-assignment operator. // Disable copy constructor and copy-assignment operator.
CsmObservability(const CsmObservability&) = delete; CsmObservability(const CsmObservability&) = delete;
CsmObservability& operator=(const CsmObservability&) = delete; CsmObservability& operator=(const CsmObservability&) = delete;
CsmObservability(CsmObservability&&) = default; CsmObservability(CsmObservability&&) noexcept;
CsmObservability& operator=(CsmObservability&&) = default; CsmObservability& operator=(CsmObservability&&) noexcept;
private:
bool valid_ = true;
}; };
// CsmObservabilityBuilder configures observability for all service mesh traffic // CsmObservabilityBuilder configures observability for all service mesh traffic

View File

@ -45,11 +45,17 @@ namespace grpc {
namespace internal { namespace internal {
namespace {
std::atomic<bool> g_csm_plugin_enabled(false);
}
bool CsmServerSelector(const grpc_core::ChannelArgs& args) { 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) { bool CsmChannelTargetSelector(absl::string_view target) {
if (!g_csm_plugin_enabled) return false;
auto uri = grpc_core::URI::Parse(target); auto uri = grpc_core::URI::Parse(target);
if (!uri.ok()) { if (!uri.ok()) {
gpr_log(GPR_ERROR, "Failed to parse URI: %s", std::string(target).c_str()); gpr_log(GPR_ERROR, "Failed to parse URI: %s", std::string(target).c_str());
@ -96,6 +102,25 @@ class CsmOpenTelemetryPluginOption
namespace experimental { 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 // CsmObservabilityBuilder
// //
@ -132,6 +157,7 @@ absl::StatusOr<CsmObservability> CsmObservabilityBuilder::BuildAndRegister() {
builder_->AddPluginOption( builder_->AddPluginOption(
std::make_unique<grpc::internal::CsmOpenTelemetryPluginOption>()); std::make_unique<grpc::internal::CsmOpenTelemetryPluginOption>());
auto status = builder_->BuildAndRegisterGlobal(); auto status = builder_->BuildAndRegisterGlobal();
internal::g_csm_plugin_enabled = true;
if (!status.ok()) { if (!status.ok()) {
return status; return status;
} }

View File

@ -23,6 +23,8 @@
#include "absl/strings/string_view.h" #include "absl/strings/string_view.h"
#include "src/core/lib/channel/channel_args.h"
namespace grpc { namespace grpc {
namespace internal { namespace internal {
@ -30,6 +32,10 @@ namespace internal {
// Returns true if the channel is a CSM channel. // Returns true if the channel is a CSM channel.
bool CsmChannelTargetSelector(absl::string_view target); 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 internal
} // namespace grpc } // namespace grpc

View File

@ -24,7 +24,7 @@
#include <grpcpp/ext/csm_observability.h> #include <grpcpp/ext/csm_observability.h>
#include <grpcpp/ext/otel_plugin.h> #include <grpcpp/ext/otel_plugin.h>
#include "src/core/lib/gprpp/env.h" #include "src/core/ext/xds/xds_enabled_server.h"
#include "test/core/util/test_config.h" #include "test/core/util/test_config.h"
namespace grpc { namespace grpc {
@ -41,6 +41,7 @@ TEST(GsmDependencyTest, GoogleCloudOpenTelemetryDependency) {
} }
TEST(CsmChannelTargetSelectorTest, NonXdsTargets) { TEST(CsmChannelTargetSelectorTest, NonXdsTargets) {
auto obs = experimental::CsmObservabilityBuilder().BuildAndRegister();
EXPECT_FALSE(internal::CsmChannelTargetSelector("foo.bar.google.com")); EXPECT_FALSE(internal::CsmChannelTargetSelector("foo.bar.google.com"));
EXPECT_FALSE(internal::CsmChannelTargetSelector("dns:///foo.bar.google.com")); EXPECT_FALSE(internal::CsmChannelTargetSelector("dns:///foo.bar.google.com"));
EXPECT_FALSE( EXPECT_FALSE(
@ -50,19 +51,52 @@ TEST(CsmChannelTargetSelectorTest, NonXdsTargets) {
} }
TEST(CsmChannelTargetSelectorTest, XdsTargets) { TEST(CsmChannelTargetSelectorTest, XdsTargets) {
auto obs = experimental::CsmObservabilityBuilder().BuildAndRegister();
EXPECT_TRUE(internal::CsmChannelTargetSelector("xds:///foo")); EXPECT_TRUE(internal::CsmChannelTargetSelector("xds:///foo"));
EXPECT_TRUE(internal::CsmChannelTargetSelector("xds:///foo.bar")); EXPECT_TRUE(internal::CsmChannelTargetSelector("xds:///foo.bar"));
} }
TEST(CsmChannelTargetSelectorTest, XdsTargetsWithNonTDAuthority) { TEST(CsmChannelTargetSelectorTest, XdsTargetsWithNonTDAuthority) {
auto obs = experimental::CsmObservabilityBuilder().BuildAndRegister();
EXPECT_FALSE(internal::CsmChannelTargetSelector("xds://authority/foo")); EXPECT_FALSE(internal::CsmChannelTargetSelector("xds://authority/foo"));
} }
TEST(CsmChannelTargetSelectorTest, XdsTargetsWithTDAuthority) { TEST(CsmChannelTargetSelectorTest, XdsTargetsWithTDAuthority) {
auto obs = experimental::CsmObservabilityBuilder().BuildAndRegister();
EXPECT_TRUE(internal::CsmChannelTargetSelector( EXPECT_TRUE(internal::CsmChannelTargetSelector(
"xds://traffic-director-global.xds.googleapis.com/foo")); "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
} // namespace testing } // namespace testing
} // namespace grpc } // namespace grpc