From 52077095e9b24a4cebffe1f37854eb90f791d239 Mon Sep 17 00:00:00 2001 From: Yash Tibrewal Date: Sat, 6 Apr 2024 02:10:50 +0000 Subject: [PATCH] Reviewer comments --- src/core/ext/xds/xds_cluster.cc | 4 ++-- src/core/ext/xds/xds_cluster.h | 4 ++-- .../load_balancing/xds/xds_cluster_impl.cc | 19 +++++++++---------- src/cpp/ext/otel/otel_plugin.h | 3 +-- .../xds/xds_cluster_resource_type_test.cc | 16 ++++++++-------- 5 files changed, 22 insertions(+), 24 deletions(-) diff --git a/src/core/ext/xds/xds_cluster.cc b/src/core/ext/xds/xds_cluster.cc index 572ce831c53..62e36d17fe0 100644 --- a/src/core/ext/xds/xds_cluster.cc +++ b/src/core/ext/xds/xds_cluster.cc @@ -724,11 +724,11 @@ absl::StatusOr> CdsResourceParse( if (google_protobuf_Value_has_string_value(value)) { if (UpbStringToAbsl(google_protobuf_Struct_FieldsEntry_key( fields_entry)) == "service_name") { - cds_update->service_telemetry_label = RefCountedStringValue( + cds_update->service_telemetry_label_ = RefCountedStringValue( UpbStringToAbsl(google_protobuf_Value_string_value(value))); } else if (UpbStringToAbsl(google_protobuf_Struct_FieldsEntry_key( fields_entry)) == "service_namespace") { - cds_update->namespace_telemetry_label = RefCountedStringValue( + cds_update->namespace_telemetry_label_ = RefCountedStringValue( UpbStringToAbsl(google_protobuf_Value_string_value(value))); } } diff --git a/src/core/ext/xds/xds_cluster.h b/src/core/ext/xds/xds_cluster.h index c1694087cb5..cd3fdf0cc24 100644 --- a/src/core/ext/xds/xds_cluster.h +++ b/src/core/ext/xds/xds_cluster.h @@ -101,8 +101,8 @@ struct XdsClusterResource : public XdsResourceType::ResourceData { XdsHealthStatusSet override_host_statuses; - RefCountedStringValue service_telemetry_label; - RefCountedStringValue namespace_telemetry_label; + RefCountedStringValue service_telemetry_label_; + RefCountedStringValue namespace_telemetry_label_; bool operator==(const XdsClusterResource& other) const { return type == other.type && lb_policy_config == other.lb_policy_config && diff --git a/src/core/load_balancing/xds/xds_cluster_impl.cc b/src/core/load_balancing/xds/xds_cluster_impl.cc index 09bfc2999c9..7eaaea4ef21 100644 --- a/src/core/load_balancing/xds/xds_cluster_impl.cc +++ b/src/core/load_balancing/xds/xds_cluster_impl.cc @@ -241,8 +241,8 @@ class XdsClusterImplLb final : public LoadBalancingPolicy { RefCountedPtr call_counter_; uint32_t max_concurrent_requests_; - RefCountedStringValue service_telemetry_label; - RefCountedStringValue namespace_telemetry_label; + RefCountedStringValue service_telemetry_label_; + RefCountedStringValue namespace_telemetry_label_; RefCountedPtr drop_config_; RefCountedPtr drop_stats_; RefCountedPtr picker_; @@ -386,10 +386,10 @@ XdsClusterImplLb::Picker::Picker(XdsClusterImplLb* xds_cluster_impl_lb, : call_counter_(xds_cluster_impl_lb->call_counter_), max_concurrent_requests_( xds_cluster_impl_lb->cluster_resource_->max_concurrent_requests), - service_telemetry_label( - xds_cluster_impl_lb->cluster_resource_->service_telemetry_label), - namespace_telemetry_label( - xds_cluster_impl_lb->cluster_resource_->namespace_telemetry_label), + service_telemetry_label_( + xds_cluster_impl_lb->cluster_resource_->service_telemetry_label_), + namespace_telemetry_label_( + xds_cluster_impl_lb->cluster_resource_->namespace_telemetry_label_), drop_config_(xds_cluster_impl_lb->drop_config_), drop_stats_(xds_cluster_impl_lb->drop_stats_), picker_(std::move(picker)) { @@ -405,13 +405,12 @@ LoadBalancingPolicy::PickResult XdsClusterImplLb::Picker::Pick( auto* call_attempt_tracer = call_state->GetCallAttemptTracer(); if (call_attempt_tracer != nullptr) { call_attempt_tracer->SetOptionalLabel( - ClientCallTracer::CallAttemptTracer::OptionalLabelKey:: - kXdsServiceName, - service_telemetry_label); + ClientCallTracer::CallAttemptTracer::OptionalLabelKey::kXdsServiceName, + service_telemetry_label_); call_attempt_tracer->SetOptionalLabel( ClientCallTracer::CallAttemptTracer::OptionalLabelKey:: kXdsServiceNamespace, - namespace_telemetry_label); + namespace_telemetry_label_); } // Handle EDS drops. const std::string* drop_category; diff --git a/src/cpp/ext/otel/otel_plugin.h b/src/cpp/ext/otel/otel_plugin.h index 81ffda10c6e..3d1fdd3440e 100644 --- a/src/cpp/ext/otel/otel_plugin.h +++ b/src/cpp/ext/otel/otel_plugin.h @@ -364,8 +364,6 @@ class OpenTelemetryPlugin : public grpc_core::StatsPlugin { grpc_core::RegisteredMetricCallback* key_; }; - // StatsPlugin: - // Returns the string form of \a key static absl::string_view OptionalLabelKeyToString( grpc_core::ClientCallTracer::CallAttemptTracer::OptionalLabelKey key); @@ -376,6 +374,7 @@ class OpenTelemetryPlugin : public grpc_core::StatsPlugin { grpc_core::ClientCallTracer::CallAttemptTracer::OptionalLabelKey> OptionalLabelStringToKey(absl::string_view key); + // StatsPlugin: std::pair> IsEnabledForChannel( const OpenTelemetryPluginBuilder::ChannelScope& scope) const override; diff --git a/test/core/xds/xds_cluster_resource_type_test.cc b/test/core/xds/xds_cluster_resource_type_test.cc index 2537fc8af31..b30e2633417 100644 --- a/test/core/xds/xds_cluster_resource_type_test.cc +++ b/test/core/xds/xds_cluster_resource_type_test.cc @@ -1635,8 +1635,8 @@ TEST_F(TelemetryLabelTest, ValidServiceLabelsConfig) { ASSERT_TRUE(decode_result.resource.ok()) << decode_result.resource.status(); auto& resource = static_cast(**decode_result.resource); - EXPECT_EQ(resource.service_telemetry_label.as_string_view(), "abc"); - EXPECT_EQ(resource.namespace_telemetry_label.as_string_view(), "xyz"); + EXPECT_EQ(resource.service_telemetry_label_.as_string_view(), "abc"); + EXPECT_EQ(resource.namespace_telemetry_label_.as_string_view(), "xyz"); } TEST_F(TelemetryLabelTest, MissingMetadataField) { @@ -1651,9 +1651,9 @@ TEST_F(TelemetryLabelTest, MissingMetadataField) { ASSERT_TRUE(decode_result.resource.ok()) << decode_result.resource.status(); auto& resource = static_cast(**decode_result.resource); - EXPECT_THAT(resource.service_telemetry_label.as_string_view(), + EXPECT_THAT(resource.service_telemetry_label_.as_string_view(), ::testing::IsEmpty()); - EXPECT_THAT(resource.namespace_telemetry_label.as_string_view(), + EXPECT_THAT(resource.namespace_telemetry_label_.as_string_view(), ::testing::IsEmpty()); } @@ -1672,9 +1672,9 @@ TEST_F(TelemetryLabelTest, MissingCsmFilterMetadataField) { ASSERT_TRUE(decode_result.resource.ok()) << decode_result.resource.status(); auto& resource = static_cast(**decode_result.resource); - EXPECT_THAT(resource.service_telemetry_label.as_string_view(), + EXPECT_THAT(resource.service_telemetry_label_.as_string_view(), ::testing::IsEmpty()); - EXPECT_THAT(resource.namespace_telemetry_label.as_string_view(), + EXPECT_THAT(resource.namespace_telemetry_label_.as_string_view(), ::testing::IsEmpty()); } @@ -1705,8 +1705,8 @@ TEST_F(TelemetryLabelTest, IgnoreNonServiceLabelEntries) { ASSERT_TRUE(decode_result.resource.ok()) << decode_result.resource.status(); auto& resource = static_cast(**decode_result.resource); - EXPECT_THAT(resource.service_telemetry_label.as_string_view(), "service"); - EXPECT_THAT(resource.namespace_telemetry_label.as_string_view(), + EXPECT_THAT(resource.service_telemetry_label_.as_string_view(), "service"); + EXPECT_THAT(resource.namespace_telemetry_label_.as_string_view(), ::testing::IsEmpty()); }