Reviewer comments

pull/36183/head
Yash Tibrewal 1 year ago
parent c23f45ab8c
commit 948abe7235
  1. 4
      include/grpcpp/ext/otel_plugin.h
  2. 8
      src/cpp/ext/otel/otel_plugin.cc
  3. 4
      src/cpp/ext/otel/otel_plugin.h
  4. 5
      test/cpp/ext/otel/otel_plugin_test.cc

@ -103,9 +103,9 @@ class OpenTelemetryPluginBuilder {
// Methods to manipulate which instruments are enabled in the OpenTelemetry // Methods to manipulate which instruments are enabled in the OpenTelemetry
// Stats Plugin. // Stats Plugin.
OpenTelemetryPluginBuilder& EnableMetrics( OpenTelemetryPluginBuilder& EnableMetrics(
const std::vector<absl::string_view>& metric_names); absl::Span<const absl::string_view> metric_names);
OpenTelemetryPluginBuilder& DisableMetrics( OpenTelemetryPluginBuilder& DisableMetrics(
const std::vector<absl::string_view>& metric_names); absl::Span<const absl::string_view> metric_names);
OpenTelemetryPluginBuilder& DisableAllMetrics(); OpenTelemetryPluginBuilder& DisableAllMetrics();
/// Add a plugin option to add to the opentelemetry plugin being built. At /// Add a plugin option to add to the opentelemetry plugin being built. At
/// present, this type is an opaque type. Ownership of \a option is /// present, this type is an opaque type. Ownership of \a option is

@ -151,7 +151,7 @@ OpenTelemetryPluginBuilderImpl::SetMeterProvider(
} }
OpenTelemetryPluginBuilderImpl& OpenTelemetryPluginBuilderImpl::EnableMetrics( OpenTelemetryPluginBuilderImpl& OpenTelemetryPluginBuilderImpl::EnableMetrics(
const std::vector<absl::string_view>& metric_names) { absl::Span<const absl::string_view> metric_names) {
for (const auto& metric_name : metric_names) { for (const auto& metric_name : metric_names) {
metrics_.emplace(metric_name); metrics_.emplace(metric_name);
} }
@ -159,7 +159,7 @@ OpenTelemetryPluginBuilderImpl& OpenTelemetryPluginBuilderImpl::EnableMetrics(
} }
OpenTelemetryPluginBuilderImpl& OpenTelemetryPluginBuilderImpl::DisableMetrics( OpenTelemetryPluginBuilderImpl& OpenTelemetryPluginBuilderImpl::DisableMetrics(
const std::vector<absl::string_view>& metric_names) { absl::Span<const absl::string_view> metric_names) {
for (const auto& metric_name : metric_names) { for (const auto& metric_name : metric_names) {
metrics_.erase(metric_name); metrics_.erase(metric_name);
} }
@ -612,13 +612,13 @@ OpenTelemetryPluginBuilder::SetGenericMethodAttributeFilter(
} }
OpenTelemetryPluginBuilder& OpenTelemetryPluginBuilder::EnableMetrics( OpenTelemetryPluginBuilder& OpenTelemetryPluginBuilder::EnableMetrics(
const std::vector<absl::string_view>& metric_names) { absl::Span<const absl::string_view> metric_names) {
impl_->EnableMetrics(metric_names); impl_->EnableMetrics(metric_names);
return *this; return *this;
} }
OpenTelemetryPluginBuilder& OpenTelemetryPluginBuilder::DisableMetrics( OpenTelemetryPluginBuilder& OpenTelemetryPluginBuilder::DisableMetrics(
const std::vector<absl::string_view>& metric_names) { absl::Span<const absl::string_view> metric_names) {
impl_->DisableMetrics(metric_names); impl_->DisableMetrics(metric_names);
return *this; return *this;
} }

@ -136,9 +136,9 @@ class OpenTelemetryPluginBuilderImpl {
// grpc.server.call.sent_total_compressed_message_size // grpc.server.call.sent_total_compressed_message_size
// grpc.server.call.rcvd_total_compressed_message_size // grpc.server.call.rcvd_total_compressed_message_size
OpenTelemetryPluginBuilderImpl& EnableMetrics( OpenTelemetryPluginBuilderImpl& EnableMetrics(
const std::vector<absl::string_view>& metric_name); absl::Span<const absl::string_view> metric_names);
OpenTelemetryPluginBuilderImpl& DisableMetrics( OpenTelemetryPluginBuilderImpl& DisableMetrics(
const std::vector<absl::string_view>& metric_name); absl::Span<const absl::string_view> metric_names);
OpenTelemetryPluginBuilderImpl& DisableAllMetrics(); OpenTelemetryPluginBuilderImpl& DisableAllMetrics();
// If set, \a target_selector is called per channel to decide whether to // If set, \a target_selector is called per channel to decide whether to
// collect metrics on that target or not. // collect metrics on that target or not.

@ -1355,16 +1355,17 @@ TEST_F(OpenTelemetryPluginNPCMetricsTest,
TEST(OpenTelemetryPluginMetricsEnablingDisablingTest, TestEnableDisableAPIs) { TEST(OpenTelemetryPluginMetricsEnablingDisablingTest, TestEnableDisableAPIs) {
grpc::internal::OpenTelemetryPluginBuilderImpl builder; grpc::internal::OpenTelemetryPluginBuilderImpl builder;
// First disable all metrics
builder.DisableAllMetrics(); builder.DisableAllMetrics();
EXPECT_TRUE(builder.TestOnlyEnabledMetrics().empty()); EXPECT_TRUE(builder.TestOnlyEnabledMetrics().empty());
// Add in a few metrics
builder.EnableMetrics( builder.EnableMetrics(
{"grpc.test.metric_1", "grpc.test.metric_2", "grpc.test.metric_3"}); {"grpc.test.metric_1", "grpc.test.metric_2", "grpc.test.metric_3"});
EXPECT_THAT( EXPECT_THAT(
builder.TestOnlyEnabledMetrics(), builder.TestOnlyEnabledMetrics(),
::testing::UnorderedElementsAre( ::testing::UnorderedElementsAre(
"grpc.test.metric_1", "grpc.test.metric_2", "grpc.test.metric_3")); "grpc.test.metric_1", "grpc.test.metric_2", "grpc.test.metric_3"));
// Now remove a few metrics
builder.DisableMetrics({"grpc.test.metric_1", "grpc.test.metric_2"}); builder.DisableMetrics({"grpc.test.metric_1", "grpc.test.metric_2"});
EXPECT_THAT(builder.TestOnlyEnabledMetrics(), EXPECT_THAT(builder.TestOnlyEnabledMetrics(),
::testing::UnorderedElementsAre("grpc.test.metric_3")); ::testing::UnorderedElementsAre("grpc.test.metric_3"));

Loading…
Cancel
Save