From 387c8941179fcc25de5f01aa5342bfd6e32adfa5 Mon Sep 17 00:00:00 2001 From: Yash Tibrewal Date: Tue, 6 Feb 2024 12:17:17 -0800 Subject: [PATCH] [CSM] Remove experimental CSM PluginOption API in favor of CsmObservability API (#35812) Also update interop tests to use `CsmObservability` API Closes #35812 COPYBARA_INTEGRATE_REVIEW=https://github.com/grpc/grpc/pull/35812 from yashykt:UpdateCsmInterop dc99764aead307a0a2b93e6bfbf76b9ca79e8581 PiperOrigin-RevId: 604726881 --- include/grpcpp/ext/csm_observability.h | 25 ++++++++++------------ src/cpp/ext/csm/BUILD | 1 + src/cpp/ext/csm/csm_observability.cc | 7 +----- test/cpp/ext/csm/csm_observability_test.cc | 8 ------- test/cpp/interop/xds_interop_client.cc | 16 +++++++------- test/cpp/interop/xds_interop_server.cc | 16 +++++++------- 6 files changed, 29 insertions(+), 44 deletions(-) diff --git a/include/grpcpp/ext/csm_observability.h b/include/grpcpp/ext/csm_observability.h index a3b50a72387..706807032d7 100644 --- a/include/grpcpp/ext/csm_observability.h +++ b/include/grpcpp/ext/csm_observability.h @@ -26,7 +26,7 @@ #include "absl/functional/any_invocable.h" #include "absl/status/statusor.h" #include "absl/strings/string_view.h" -#include "opentelemetry/sdk/metrics/meter_provider.h" +#include "opentelemetry/metrics/meter_provider.h" #include @@ -40,7 +40,15 @@ namespace experimental { // This is a no-op at present, but in the future, this object would be useful // for performing cleanup. -class CsmObservability {}; +class CsmObservability { + public: + CsmObservability() = default; + // Disable copy constructor and copy-assignment operator. + CsmObservability(const CsmObservability&) = delete; + CsmObservability& operator=(const CsmObservability&) = delete; + CsmObservability(CsmObservability&&) = default; + CsmObservability& operator=(CsmObservability&&) = default; +}; // CsmObservabilityBuilder configures observability for all service mesh traffic // for a binary running on CSM. @@ -49,8 +57,7 @@ class CsmObservabilityBuilder { CsmObservabilityBuilder(); ~CsmObservabilityBuilder(); CsmObservabilityBuilder& SetMeterProvider( - std::shared_ptr - meter_provider); + std::shared_ptr meter_provider); // If set, \a target_attribute_filter is called per channel to decide whether // to record the target attribute on client or to replace it with "other". // This helps reduce the cardinality on metrics in cases where many channels @@ -90,16 +97,6 @@ class CsmObservabilityBuilder { std::unique_ptr builder_; }; -/// Creates an OpenTelemetryPluginOption that would add additional labels on -/// gRPC metrics to enhance observability for CSM users. -/// -/// Sample Usage - -/// OpenTelemetryPluginBuilder() -/// .SetMeterProvider(provider) -/// .AddPluginOption(MakeCsmOpenTelemetryPluginOption()) -/// .BuildAndRegisterGlobal(); -std::unique_ptr MakeCsmOpenTelemetryPluginOption(); - } // namespace experimental } // namespace grpc diff --git a/src/cpp/ext/csm/BUILD b/src/cpp/ext/csm/BUILD index f6599816485..25aa1bf21dc 100644 --- a/src/cpp/ext/csm/BUILD +++ b/src/cpp/ext/csm/BUILD @@ -46,6 +46,7 @@ grpc_cc_library( "absl/types:optional", "absl/types:variant", "google_cloud_cpp:opentelemetry", + "otel/api", "otel/sdk/src/metrics", "otel/sdk:headers", "upb_base_lib", diff --git a/src/cpp/ext/csm/csm_observability.cc b/src/cpp/ext/csm/csm_observability.cc index b012ce04fec..0bb2590daaf 100644 --- a/src/cpp/ext/csm/csm_observability.cc +++ b/src/cpp/ext/csm/csm_observability.cc @@ -107,8 +107,7 @@ CsmObservabilityBuilder::CsmObservabilityBuilder() CsmObservabilityBuilder::~CsmObservabilityBuilder() = default; CsmObservabilityBuilder& CsmObservabilityBuilder::SetMeterProvider( - std::shared_ptr - meter_provider) { + std::shared_ptr meter_provider) { builder_->SetMeterProvider(meter_provider); return *this; } @@ -139,9 +138,5 @@ absl::StatusOr CsmObservabilityBuilder::BuildAndRegister() { return CsmObservability(); } -std::unique_ptr MakeCsmOpenTelemetryPluginOption() { - return std::make_unique(); -} - } // namespace experimental } // namespace grpc diff --git a/test/cpp/ext/csm/csm_observability_test.cc b/test/cpp/ext/csm/csm_observability_test.cc index 60517193aa2..69073ff8f92 100644 --- a/test/cpp/ext/csm/csm_observability_test.cc +++ b/test/cpp/ext/csm/csm_observability_test.cc @@ -63,14 +63,6 @@ TEST(CsmChannelTargetSelectorTest, XdsTargetsWithTDAuthority) { "xds://traffic-director-global.xds.googleapis.com/foo")); } -TEST(CsmPluginOptionTest, Basic) { - EXPECT_EQ( - OpenTelemetryPluginBuilder() - .AddPluginOption(experimental::MakeCsmOpenTelemetryPluginOption()) - .BuildAndRegisterGlobal(), - absl::OkStatus()); -} - } // namespace } // namespace testing } // namespace grpc diff --git a/test/cpp/interop/xds_interop_client.cc b/test/cpp/interop/xds_interop_client.cc index 5c2eac7aa81..76dab581fa3 100644 --- a/test/cpp/interop/xds_interop_client.cc +++ b/test/cpp/interop/xds_interop_client.cc @@ -427,7 +427,7 @@ void RunTestLoop(std::chrono::duration duration_per_query, GPR_UNREACHABLE_CODE(thread.join()); } -void EnableCsmObservability() { +grpc::experimental::CsmObservability EnableCsmObservability() { gpr_log(GPR_DEBUG, "Registering Prometheus exporter"); opentelemetry::exporter::metrics::PrometheusExporterOptions opts; // default was "localhost:9464" which causes connection issue across GKE @@ -438,12 +438,11 @@ void EnableCsmObservability() { auto meter_provider = std::make_shared(); meter_provider->AddMetricReader(std::move(prometheus_exporter)); - assert(grpc::OpenTelemetryPluginBuilder() - .AddPluginOption( - grpc::experimental::MakeCsmOpenTelemetryPluginOption()) - .SetMeterProvider(std::move(meter_provider)) - .BuildAndRegisterGlobal() - .ok()); + auto observability = grpc::experimental::CsmObservabilityBuilder() + .SetMeterProvider(std::move(meter_provider)) + .BuildAndRegister(); + assert(observability.ok()); + return *std::move(observability); } void RunServer(const int port, StatsWatchers* stats_watchers, @@ -554,8 +553,9 @@ int main(int argc, char** argv) { } BuildRpcConfigsFromFlags(&rpc_config_queue); + grpc::experimental::CsmObservability observability; if (absl::GetFlag(FLAGS_enable_csm_observability)) { - EnableCsmObservability(); + observability = EnableCsmObservability(); } std::chrono::duration duration_per_query = diff --git a/test/cpp/interop/xds_interop_server.cc b/test/cpp/interop/xds_interop_server.cc index 8a66e173bc2..80637dfa050 100644 --- a/test/cpp/interop/xds_interop_server.cc +++ b/test/cpp/interop/xds_interop_server.cc @@ -43,7 +43,7 @@ ABSL_FLAG(bool, secure_mode, false, ABSL_FLAG(bool, enable_csm_observability, false, "Whether to enable CSM Observability"); -void EnableCsmObservability() { +grpc::experimental::CsmObservability EnableCsmObservability() { gpr_log(GPR_DEBUG, "Registering Prometheus exporter"); opentelemetry::exporter::metrics::PrometheusExporterOptions opts; // default was "localhost:9464" which causes connection issue across GKE @@ -54,12 +54,11 @@ void EnableCsmObservability() { auto meter_provider = std::make_shared(); meter_provider->AddMetricReader(std::move(prometheus_exporter)); - assert(grpc::OpenTelemetryPluginBuilder() - .AddPluginOption( - grpc::experimental::MakeCsmOpenTelemetryPluginOption()) - .SetMeterProvider(std::move(meter_provider)) - .BuildAndRegisterGlobal() - .ok()); + auto observability = grpc::experimental::CsmObservabilityBuilder() + .SetMeterProvider(std::move(meter_provider)) + .BuildAndRegister(); + assert(observability.ok()); + return *std::move(observability); } int main(int argc, char** argv) { @@ -82,8 +81,9 @@ int main(int argc, char** argv) { } grpc::EnableDefaultHealthCheckService(false); bool enable_csm_observability = absl::GetFlag(FLAGS_enable_csm_observability); + grpc::experimental::CsmObservability observability; if (enable_csm_observability) { - EnableCsmObservability(); + observability = EnableCsmObservability(); } grpc::testing::RunServer(absl::GetFlag(FLAGS_secure_mode), enable_csm_observability, port, maintenance_port,