From c5a8e5af643c7ad0ebf99d30d3702ad938151fac Mon Sep 17 00:00:00 2001 From: Yash Tibrewal Date: Fri, 12 Jan 2024 08:25:17 -0800 Subject: [OTel] Add back server_selector that got deleted by a merge fiasco (#35532) It looks like this ended up getting deleted in https://github.com/grpc/grpc/pull/34350 probably when merging. Also, the `Init` method in the otel test library is getting unwieldy. I'm going to send out a follow-up PR to convert this into a builder instead. Closes #35532 COPYBARA_INTEGRATE_REVIEW=https://github.com/grpc/grpc/pull/35532 from yashykt:OTelPluginBuilderFix 372bf26338cb89b440fbb470a1fe3388d0002070 PiperOrigin-RevId: 597846622 --- src/core/lib/channel/call_tracer.cc | 5 +++ src/core/lib/channel/call_tracer.h | 3 ++ src/cpp/ext/otel/otel_plugin.cc | 1 + test/cpp/ext/otel/otel_plugin_test.cc | 68 ++++++++++++++++++++++++++++++++++ test/cpp/ext/otel/otel_test_library.cc | 7 +++- test/cpp/ext/otel/otel_test_library.h | 4 ++ 6 files changed, 86 insertions(+), 2 deletions(-) diff --git a/src/core/lib/channel/call_tracer.cc b/src/core/lib/channel/call_tracer.cc index d98a944b59..83a8ec8b22 100644 --- a/src/core/lib/channel/call_tracer.cc +++ b/src/core/lib/channel/call_tracer.cc @@ -60,6 +60,11 @@ void ServerCallTracerFactory::RegisterGlobal(ServerCallTracerFactory* factory) { g_server_call_tracer_factory_ = factory; } +void ServerCallTracerFactory::TestOnlyReset() { + delete g_server_call_tracer_factory_; + g_server_call_tracer_factory_ = nullptr; +} + absl::string_view ServerCallTracerFactory::ChannelArgName() { return kServerCallTracerFactoryChannelArgName; } diff --git a/src/core/lib/channel/call_tracer.h b/src/core/lib/channel/call_tracer.h index 34c8342dc3..6116a26a20 100644 --- a/src/core/lib/channel/call_tracer.h +++ b/src/core/lib/channel/call_tracer.h @@ -201,6 +201,9 @@ class ServerCallTracerFactory { // this for the lifetime of the process. static void RegisterGlobal(ServerCallTracerFactory* factory); + // Deletes any previous registered ServerCallTracerFactory. + static void TestOnlyReset(); + static absl::string_view ChannelArgName(); }; diff --git a/src/cpp/ext/otel/otel_plugin.cc b/src/cpp/ext/otel/otel_plugin.cc index 7f7ba290e4..8f3593e94f 100644 --- a/src/cpp/ext/otel/otel_plugin.cc +++ b/src/cpp/ext/otel/otel_plugin.cc @@ -246,6 +246,7 @@ void OpenTelemetryPluginBuilderImpl::BuildAndRegisterGlobal() { g_otel_plugin_state_->labels_injector = std::move(labels_injector_); g_otel_plugin_state_->target_attribute_filter = std::move(target_attribute_filter_); + g_otel_plugin_state_->server_selector = std::move(server_selector_); g_otel_plugin_state_->generic_method_attribute_filter = std::move(generic_method_attribute_filter_); g_otel_plugin_state_->meter_provider = std::move(meter_provider); diff --git a/test/cpp/ext/otel/otel_plugin_test.cc b/test/cpp/ext/otel/otel_plugin_test.cc index 541e49d417..e5eff6e7da 100644 --- a/test/cpp/ext/otel/otel_plugin_test.cc +++ b/test/cpp/ext/otel/otel_plugin_test.cc @@ -358,6 +358,54 @@ TEST_F(OpenTelemetryPluginEnd2EndTest, TargetSelectorReturnsFalse) { ASSERT_TRUE(data.empty()); } +// Test that a server selector returning true records metrics on the server. +TEST_F(OpenTelemetryPluginEnd2EndTest, ServerSelectorReturnsTrue) { + Init({grpc::OpenTelemetryPluginBuilder:: + kServerCallDurationInstrumentName}, /*resource=*/ + opentelemetry::sdk::resource::Resource::Create({}), + /*labels_injector=*/nullptr, + /*test_no_meter_provider=*/false, + /*labels_to_inject=*/{}, + /*target_selector=*/absl::AnyInvocable(), + /*server_selector=*/ + [](const grpc_core::ChannelArgs& /*channel_args*/) { return true; }); + SendRPC(); + const char* kMetricName = "grpc.server.call.duration"; + auto data = ReadCurrentMetricsData( + [&](const absl::flat_hash_map< + std::string, + std::vector>& + data) { return !data.contains(kMetricName); }); + ASSERT_EQ(data[kMetricName].size(), 1); + const auto& server_attributes = + data[kMetricName][0].attributes.GetAttributes(); + EXPECT_EQ(server_attributes.size(), 2); + EXPECT_EQ(absl::get(server_attributes.at("grpc.method")), + kMethodName); + EXPECT_EQ(absl::get(server_attributes.at("grpc.status")), "OK"); +} + +// Test that a server selector returning false does not record metrics on the +// server. +TEST_F(OpenTelemetryPluginEnd2EndTest, ServerSelectorReturnsFalse) { + Init({grpc::OpenTelemetryPluginBuilder:: + kServerCallDurationInstrumentName}, /*resource=*/ + opentelemetry::sdk::resource::Resource::Create({}), + /*labels_injector=*/nullptr, + /*test_no_meter_provider=*/false, + /*labels_to_inject=*/{}, + /*target_selector=*/absl::AnyInvocable(), + /*server_selector=*/ + [](const grpc_core::ChannelArgs& /*channel_args*/) { return false; }); + SendRPC(); + auto data = ReadCurrentMetricsData( + [&](const absl::flat_hash_map< + std::string, + std::vector>& + /*data*/) { return false; }); + ASSERT_TRUE(data.empty()); +} + // Test that a target attribute filter returning true records metrics with the // target as is on the channel. TEST_F(OpenTelemetryPluginEnd2EndTest, TargetAttributeFilterReturnsTrue) { @@ -368,6 +416,8 @@ TEST_F(OpenTelemetryPluginEnd2EndTest, TargetAttributeFilterReturnsTrue) { /*test_no_meter_provider=*/false, /*labels_to_inject=*/{}, /*target_selector=*/absl::AnyInvocable(), + /*server_selector=*/ + absl::AnyInvocable(), /*target_attribute_filter=*/[](absl::string_view /*target*/) { return true; }); @@ -407,6 +457,8 @@ TEST_F(OpenTelemetryPluginEnd2EndTest, TargetAttributeFilterReturnsFalse) { /*test_no_meter_provider=*/false, /*labels_to_inject=*/{}, /*target_selector=*/absl::AnyInvocable(), + /*server_selector=*/ + absl::AnyInvocable(), /*target_attribute_filter=*/ [server_address = canonical_server_address_]( absl::string_view /*target*/) { return false; }); @@ -475,6 +527,8 @@ TEST_F(OpenTelemetryPluginEnd2EndTest, /*test_no_meter_provider=*/false, /*labels_to_inject=*/{}, /*target_selector=*/absl::AnyInvocable(), + /*server_selector=*/ + absl::AnyInvocable(), /*target_attribute_filter=*/ absl::AnyInvocable(), /*generic_method_attribute_filter=*/ @@ -515,6 +569,8 @@ TEST_F(OpenTelemetryPluginEnd2EndTest, /*test_no_meter_provider=*/false, /*labels_to_inject=*/{}, /*target_selector=*/absl::AnyInvocable(), + /*server_selector=*/ + absl::AnyInvocable(), /*target_attribute_filter=*/ absl::AnyInvocable(), /*generic_method_attribute_filter=*/ @@ -583,6 +639,8 @@ TEST_F(OpenTelemetryPluginEnd2EndTest, /*test_no_meter_provider=*/false, /*labels_to_inject=*/{}, /*target_selector=*/absl::AnyInvocable(), + /*server_selector=*/ + absl::AnyInvocable(), /*target_attribute_filter=*/ absl::AnyInvocable(), /*generic_method_attribute_filter=*/ @@ -622,6 +680,8 @@ TEST_F(OpenTelemetryPluginEnd2EndTest, /*test_no_meter_provider=*/false, /*labels_to_inject=*/{}, /*target_selector=*/absl::AnyInvocable(), + /*server_selector=*/ + absl::AnyInvocable(), /*target_attribute_filter=*/ absl::AnyInvocable(), /*generic_method_attribute_filter=*/ @@ -757,6 +817,8 @@ TEST_F(OpenTelemetryPluginOptionEnd2EndTest, Basic) { /*test_no_meter_provider=*/false, /*labels_to_inject=*/{}, /*target_selector=*/absl::AnyInvocable(), + /*server_selector=*/ + absl::AnyInvocable(), /*target_attribute_filter=*/ absl::AnyInvocable(), /*generic_method_attribute_filter=*/ @@ -799,6 +861,8 @@ TEST_F(OpenTelemetryPluginOptionEnd2EndTest, ClientOnlyPluginOption) { /*test_no_meter_provider=*/false, /*labels_to_inject=*/{}, /*target_selector=*/absl::AnyInvocable(), + /*server_selector=*/ + absl::AnyInvocable(), /*target_attribute_filter=*/ absl::AnyInvocable(), /*generic_method_attribute_filter=*/ @@ -842,6 +906,8 @@ TEST_F(OpenTelemetryPluginOptionEnd2EndTest, ServerOnlyPluginOption) { /*test_no_meter_provider=*/false, /*labels_to_inject=*/{}, /*target_selector=*/absl::AnyInvocable(), + /*server_selector=*/ + absl::AnyInvocable(), /*target_attribute_filter=*/ absl::AnyInvocable(), /*generic_method_attribute_filter=*/ @@ -899,6 +965,8 @@ TEST_F(OpenTelemetryPluginOptionEnd2EndTest, /*test_no_meter_provider=*/false, /*labels_to_inject=*/{}, /*target_selector=*/absl::AnyInvocable(), + /*server_selector=*/ + absl::AnyInvocable(), /*target_attribute_filter=*/ absl::AnyInvocable(), /*generic_method_attribute_filter=*/ diff --git a/test/cpp/ext/otel/otel_test_library.cc b/test/cpp/ext/otel/otel_test_library.cc index 6f8e55a96d..19777d4d59 100644 --- a/test/cpp/ext/otel/otel_test_library.cc +++ b/test/cpp/ext/otel/otel_test_library.cc @@ -90,6 +90,9 @@ void OpenTelemetryPluginEnd2EndTest::Init( const std::map& labels_to_inject, absl::AnyInvocable target_selector, + absl::AnyInvocable + server_selector, absl::AnyInvocable target_attribute_filter, absl::AnyInvocable @@ -121,6 +124,7 @@ void OpenTelemetryPluginEnd2EndTest::Init( } ot_builder.SetLabelsInjector(std::move(labels_injector)); ot_builder.SetTargetSelector(std::move(target_selector)); + ot_builder.SetServerSelector(std::move(server_selector)); ot_builder.SetTargetAttributeFilter(std::move(target_attribute_filter)); ot_builder.SetGenericMethodAttributeFilter( std::move(generic_method_attribute_filter)); @@ -160,8 +164,7 @@ void OpenTelemetryPluginEnd2EndTest::Init( void OpenTelemetryPluginEnd2EndTest::TearDown() { server_->Shutdown(); grpc_shutdown_blocking(); - delete grpc_core::ServerCallTracerFactory::Get(grpc_core::ChannelArgs()); - grpc_core::ServerCallTracerFactory::RegisterGlobal(nullptr); + grpc_core::ServerCallTracerFactory::TestOnlyReset(); } void OpenTelemetryPluginEnd2EndTest::ResetStub( diff --git a/test/cpp/ext/otel/otel_test_library.h b/test/cpp/ext/otel/otel_test_library.h index 0f6edfd157..0108145a57 100644 --- a/test/cpp/ext/otel/otel_test_library.h +++ b/test/cpp/ext/otel/otel_test_library.h @@ -66,6 +66,10 @@ class OpenTelemetryPluginEnd2EndTest : public ::testing::Test { const std::map& labels_to_inject = {}, absl::AnyInvocable target_selector = absl::AnyInvocable(), + absl::AnyInvocable + server_selector = absl::AnyInvocable< + bool(const grpc_core::ChannelArgs& /*channel_args*/) const>(), absl::AnyInvocable target_attribute_filter = absl::AnyInvocable(), -- cgit v1.2.3