resolving comments

Author: shubham-up-47

google/cloud/opentelemetry/internal/monitoring_exporter.cc

@@ -31,12 +31,28 @@ std::string FormatProjectFullName(std::string const& project) {
   return absl::StrCat("projects/", project);
 }
 
+otel_internal::ResourceFilterDataFn MakeFilter(Options const& options) {
+  if (!options.has<otel::ResourceFilterDataFnOption>()) {
+    return nullptr;
+  }
+
+  // Get the excluded labels list.
+  auto const& excluded = options.get<otel::ResourceFilterDataFnOption>();
+  if (excluded.empty()) return nullptr;
+
+  // Capture by value to avoid dangling reference in the lambda.
+  return [excluded = std::move(excluded)](std::string const& key) -> bool {
+    return excluded.count(key) > 0;
+  };
+}
+
 }  // namespace
 
 MonitoringExporter::MonitoringExporter(
     std::shared_ptr<monitoring_v3::MetricServiceConnection> conn,
-    otel::MonitoredResourceFromDataFn dynamic_resource_fn,
-    otel::ResourceFilterDataFn resource_filter_fn, Options const& options)
+    otel_internal::MonitoredResourceFromDataFn dynamic_resource_fn,
+    otel_internal::ResourceFilterDataFn resource_filter_fn,
+    Options const& options)
     : client_(std::move(conn)),
       formatter_(options.get<otel::MetricNameFormatterOption>()),
       use_service_time_series_(options.get<otel::ServiceTimeSeriesOption>()),
@@ -48,9 +64,7 @@ MonitoringExporter::MonitoringExporter(
     Project project,
     std::shared_ptr<monitoring_v3::MetricServiceConnection> conn,
     Options const& options)
-    : MonitoringExporter(std::move(conn),
-                         options.get<otel::MonitoredResourceFromDataFnOption>(),
-                         options.get<otel::ResourceFilterDataFnOption>(),
+    : MonitoringExporter(std::move(conn), nullptr, MakeFilter(options),
                          options) {
   project_ = std::move(project);
 }
@@ -140,8 +154,8 @@ Options DefaultOptions(Options o) {
 }
 
 std::unique_ptr<opentelemetry::sdk::metrics::PushMetricExporter>
-MakeMonitoringExporter(otel::MonitoredResourceFromDataFn dynamic_resource_fn,
-                       otel::ResourceFilterDataFn resource_filter_fn,
+MakeMonitoringExporter(MonitoredResourceFromDataFn dynamic_resource_fn,
+                       ResourceFilterDataFn resource_filter_fn,
                        Options options) {
   // TODO(#15321): Determine which options, if any, should be passed along from
   //  the options parameter to MakeMetricServiceConnection.
@@ -154,8 +168,8 @@ MakeMonitoringExporter(otel::MonitoredResourceFromDataFn dynamic_resource_fn,
 
 std::unique_ptr<opentelemetry::sdk::metrics::PushMetricExporter>
 MakeMonitoringExporter(
-    otel::MonitoredResourceFromDataFn dynamic_resource_fn,
-    otel::ResourceFilterDataFn resource_filter_fn,
+    MonitoredResourceFromDataFn dynamic_resource_fn,
+    ResourceFilterDataFn resource_filter_fn,
     std::shared_ptr<monitoring_v3::MetricServiceConnection> conn,
     Options options) {
   options = DefaultOptions(std::move(options));

google/cloud/opentelemetry/internal/monitoring_exporter.h

@@ -33,13 +33,25 @@ namespace cloud {
 namespace otel_internal {
 GOOGLE_CLOUD_CPP_INLINE_NAMESPACE_BEGIN
 
+// For use with dynamic monitored resources, this function constructs the
+// correct MonitoredResource from the PointDataAttributes passed in. This
+// function is called in ToTimeSeriesWithResources.
+using MonitoredResourceFromDataFn =
+    std::function<std::pair<std::string, google::api::MonitoredResource>(
+        opentelemetry::sdk::metrics::PointDataAttributes const&)>;
+
+// For use with dynamic monitored resources, this function is used in ToMetric
+// to indicate which labels should be skipped when populating the labels field
+// of the google::api::Metric proto.
+using ResourceFilterDataFn = std::function<bool(std::string const&)>;
+
 class MonitoringExporter final
     : public opentelemetry::sdk::metrics::PushMetricExporter {
  public:
   MonitoringExporter(
       std::shared_ptr<monitoring_v3::MetricServiceConnection> conn,
-      otel::MonitoredResourceFromDataFn dynamic_resource_fn,
-      otel::ResourceFilterDataFn resource_filter_fn, Options const& options);
+      otel_internal::MonitoredResourceFromDataFn dynamic_resource_fn,
+      otel_internal::ResourceFilterDataFn resource_filter_fn, Options const& options);
 
   MonitoringExporter(
       Project project,
@@ -71,21 +83,21 @@ class MonitoringExporter final
   otel::MetricNameFormatterOption::Type formatter_;
   bool use_service_time_series_;
   absl::optional<google::api::MonitoredResource> mr_proto_;
-  otel::MonitoredResourceFromDataFn dynamic_resource_fn_;
-  otel::ResourceFilterDataFn resource_filter_fn_;
+  otel_internal::MonitoredResourceFromDataFn dynamic_resource_fn_;
+  otel_internal::ResourceFilterDataFn resource_filter_fn_;
 };
 
 Options DefaultOptions(Options o);
 
 std::unique_ptr<opentelemetry::sdk::metrics::PushMetricExporter>
-MakeMonitoringExporter(otel::MonitoredResourceFromDataFn dynamic_resource_fn,
-                       otel::ResourceFilterDataFn resource_filter_fn,
+MakeMonitoringExporter(MonitoredResourceFromDataFn dynamic_resource_fn,
+                       ResourceFilterDataFn resource_filter_fn,
                        Options options = {});
 
 std::unique_ptr<opentelemetry::sdk::metrics::PushMetricExporter>
 MakeMonitoringExporter(
-    otel::MonitoredResourceFromDataFn dynamic_resource_fn,
-    otel::ResourceFilterDataFn resource_filter_fn,
+    MonitoredResourceFromDataFn dynamic_resource_fn,
+    ResourceFilterDataFn resource_filter_fn,
     std::shared_ptr<monitoring_v3::MetricServiceConnection> conn,
     Options options = {});
 

google/cloud/opentelemetry/internal/time_series.cc

@@ -133,7 +133,7 @@ google::api::Metric ToMetric(
     opentelemetry::sdk::metrics::PointAttributes const& attributes,
     opentelemetry::sdk::resource::Resource const* resource,
     std::function<std::string(std::string)> const& name_formatter,
-    otel::ResourceFilterDataFn const& resource_filter_fn) {
+    ResourceFilterDataFn const& resource_filter_fn) {
   auto add_label = [&resource_filter_fn](auto& labels, auto key,
                                          auto const& value) {
     // GCM labels match on the regex: R"([a-zA-Z_][a-zA-Z0-9_]*)".
@@ -304,8 +304,8 @@ std::unordered_map<std::string, std::vector<google::monitoring::v3::TimeSeries>>
 ToTimeSeriesWithResources(
     opentelemetry::sdk::metrics::ResourceMetrics const& data,
     std::function<std::string(std::string)> const& metrics_name_formatter,
-    otel::ResourceFilterDataFn const& resource_filter_fn,
-    otel::MonitoredResourceFromDataFn const& dynamic_resource_fn) {
+    ResourceFilterDataFn const& resource_filter_fn,
+    MonitoredResourceFromDataFn const& dynamic_resource_fn) {
   std::unordered_map<std::string,
                      std::vector<google::monitoring::v3::TimeSeries>>
       tss_map;

google/cloud/opentelemetry/internal/time_series.h

@@ -41,7 +41,7 @@ google::api::Metric ToMetric(
     opentelemetry::sdk::metrics::PointAttributes const& attributes,
     opentelemetry::sdk::resource::Resource const* resource,
     std::function<std::string(std::string)> const& metrics_name_formatter,
-    otel::ResourceFilterDataFn const& resource_filter_fn);
+    ResourceFilterDataFn const& resource_filter_fn);
 
 google::api::Metric ToMetric(
     opentelemetry::sdk::metrics::MetricData const& metric_data,
@@ -94,8 +94,8 @@ std::unordered_map<std::string, std::vector<google::monitoring::v3::TimeSeries>>
 ToTimeSeriesWithResources(
     opentelemetry::sdk::metrics::ResourceMetrics const& data,
     std::function<std::string(std::string)> const& metrics_name_formatter,
-    otel::ResourceFilterDataFn const& resource_filter_fn,
-    otel::MonitoredResourceFromDataFn const& resource_fn);
+    ResourceFilterDataFn const& resource_filter_fn,
+    MonitoredResourceFromDataFn const& resource_fn);
 
 bool IsEmptyTimeSeries(
     opentelemetry::sdk::metrics::ResourceMetrics const& data);

google/cloud/opentelemetry/monitoring_exporter.h

@@ -19,7 +19,6 @@
 #include "google/cloud/opentelemetry/internal/recordable.h"
 #include "google/cloud/project.h"
 #include "google/cloud/version.h"
-#include <opentelemetry/sdk/metrics/data/metric_data.h>
 #include <opentelemetry/sdk/metrics/push_metric_exporter.h>
 #include <functional>
 #include <memory>
@@ -30,18 +29,6 @@ namespace cloud {
 namespace otel {
 GOOGLE_CLOUD_CPP_INLINE_NAMESPACE_BEGIN
 
-// For use with dynamic monitored resources, this function constructs the
-// correct MonitoredResource from the PointDataAttributes passed in. This
-// function is called in ToTimeSeriesWithResources.
-using MonitoredResourceFromDataFn =
-    std::function<std::pair<std::string, google::api::MonitoredResource>(
-        opentelemetry::sdk::metrics::PointDataAttributes const&)>;
-
-// For use with dynamic monitored resources, this function is used in ToMetric
-// to indicate which labels should be skipped when populating the labels field
-// of the google::api::Metric proto.
-using ResourceFilterDataFn = std::function<bool(std::string const&)>;
-
 /**
  * Change formatting for metric names.
  *
@@ -88,24 +75,14 @@ struct MonitoredResourceOption {
   using Type = google::api::MonitoredResource;
 };
 
-/**
- * Override the monitored resource builder.
- *
- * This option is primarily relevant to Google applications and libraries. It
- * can be ignored by external developers.
- */
-struct MonitoredResourceFromDataFnOption {
-  using Type = MonitoredResourceFromDataFn;
-};
-
 /**
  * Filter resource labels.
  *
  * This option is primarily relevant to Google applications and libraries. It
  * can be ignored by external developers.
  */
 struct ResourceFilterDataFnOption {
-  using Type = ResourceFilterDataFn;
+  using Type = std::set<std::string>;
 };
 
 std::unique_ptr<opentelemetry::sdk::metrics::PushMetricExporter>

google/cloud/opentelemetry/monitoring_exporter_test.cc

@@ -264,35 +264,6 @@ TEST(MonitoringExporter, CustomMonitoredResource) {
   EXPECT_EQ(result, opentelemetry::sdk::common::ExportResult::kSuccess);
 }
 
-TEST(MonitoringExporter, CustomMonitoredResourceFromDataFunction) {
-  auto mock =
-      std::make_shared<monitoring_v3_mocks::MockMetricServiceConnection>();
-  EXPECT_CALL(*mock, CreateTimeSeries)
-      .WillOnce(
-          [](google::monitoring::v3::CreateTimeSeriesRequest const& request) {
-            EXPECT_THAT(request.name(), "projects/test-project");
-            EXPECT_THAT(request.time_series(), SizeIs(2));
-            EXPECT_THAT(request.time_series(),
-                        Each(ResourceType("test_resource")));
-            return Status();
-          });
-
-  // Define a function that maps PointDataAttributes to a MonitoredResource.
-  auto resource_fn = [](opentelemetry::sdk::metrics::PointDataAttributes const&) 
-      -> std::pair<std::string, google::api::MonitoredResource> {
-    google::api::MonitoredResource resource;
-    resource.set_type("test_resource");
-    return {"test-project", resource};
-  };
-
-  auto options = Options{}.set<MonitoredResourceFromDataFnOption>(resource_fn);
-  auto exporter =
-      MakeMonitoringExporter(Project("test-project"), std::move(mock), options);
-  auto data = MakeResourceMetrics(/*expected_time_series_count=*/2);
-  auto result = exporter->Export(data);
-  EXPECT_EQ(result, opentelemetry::sdk::common::ExportResult::kSuccess);
-}
-
 TEST(MonitoringExporter, CustomResourceFilterDataFunction) {
   auto mock =
       std::make_shared<monitoring_v3_mocks::MockMetricServiceConnection>();
@@ -304,12 +275,7 @@ TEST(MonitoringExporter, CustomResourceFilterDataFunction) {
             return Status();
           });
 
-  // Define a function that filters out labels starting with "internal_".
-  auto filter_fn = [](std::string const& label) -> bool {
-    return label.rfind("internal_", 0) == 0;
-  };
-
-  auto options = Options{}.set<ResourceFilterDataFnOption>(filter_fn);
+  auto options = Options{}.set<ResourceFilterDataFnOption>({"test_label"});
   auto exporter =
       MakeMonitoringExporter(Project("test-project"), std::move(mock), options);
   auto data = MakeResourceMetrics(/*expected_time_series_count=*/2);

google/cloud/storage/internal/grpc/metrics_exporter_impl.cc

@@ -79,26 +79,6 @@ class ExporterRegistry {
   std::mutex mu_;
 };
 
-otel::ResourceFilterDataFn MakeFilter(Options const& options) {
-  // Check for explicit filter function first.
-  if (options.has<otel::ResourceFilterDataFnOption>()) {
-    return options.get<otel::ResourceFilterDataFnOption>();
-  }
-
-  // Check for excluded labels list.
-  if (!options.has<storage_experimental::GrpcMetricsExcludedLabelsOption>()) {
-    return nullptr;
-  }
-  auto const& excluded =
-      options.get<storage_experimental::GrpcMetricsExcludedLabelsOption>();
-  if (excluded.empty()) return nullptr;
-
-  // Capture by value to avoid dangling reference in the lambda.
-  return [excluded](std::string const& key) -> bool {
-    return excluded.count(key) > 0;
-  };
-}
-
 }  // namespace
 
 absl::optional<ExporterConfig> MakeMeterProviderConfig(
@@ -111,7 +91,10 @@ absl::optional<ExporterConfig> MakeMeterProviderConfig(
   if (!project) return absl::nullopt;
 
   auto exporter_options = MetricsExporterOptions(*project, resource);
-  exporter_options.set<otel::ResourceFilterDataFnOption>(MakeFilter(options));
+  if (options.has<storage_experimental::GrpcMetricsExcludedLabelsOption>()) {
+    exporter_options.set<otel::ResourceFilterDataFnOption>(
+        options.get<storage_experimental::GrpcMetricsExcludedLabelsOption>());
+  }
 
   auto exporter_connection_options = MetricsExporterConnectionOptions(options);
   return ExporterConfig{std::move(*project), std::move(exporter_options),

google/cloud/storage/internal/grpc/metrics_exporter_impl_test.cc

@@ -156,77 +156,6 @@ TEST(GrpcMetricsExporter, ReaderOptionsAreSetFromConfig) {
             std::chrono::milliseconds(expected_timeout));
 }
 
-TEST(GrpcMetricsExporter, MakeFilterWithServiceName) {
-  auto excluded_labels = std::set<std::string>{"service_name"};
-  auto options =
-      TestOptions().set<storage_experimental::GrpcMetricsExcludedLabelsOption>(
-          excluded_labels);
-
-  auto config = MakeMeterProviderConfig(FullResource(), options);
-  ASSERT_TRUE(config.has_value());
-
-  // Test that filter option is set.
-  ASSERT_TRUE(config->exporter_options.has<otel::ResourceFilterDataFnOption>());
-  auto filter_fn =
-      config->exporter_options.get<otel::ResourceFilterDataFnOption>();
-  ASSERT_NE(filter_fn, nullptr);
-
-  // Test filtering behavior.
-  EXPECT_TRUE(filter_fn("service_name"));
-  EXPECT_FALSE(filter_fn("service_version"));
-}
-
-TEST(GrpcMetricsExporter, MakeFilterWithExplicitFilterFunction) {
-  auto explicit_filter = [](std::string const& key) {
-    return key == "explicit_test";
-  };
-  auto excluded_labels = std::set<std::string>{"service_name"};
-
-  // Test when explicit filter function takes precedence.
-  auto options =
-      TestOptions()
-          .set<otel::ResourceFilterDataFnOption>(explicit_filter)
-          .set<storage_experimental::GrpcMetricsExcludedLabelsOption>(
-              excluded_labels);
-  auto config = MakeMeterProviderConfig(FullResource(), options);
-  ASSERT_TRUE(config.has_value());
-
-  auto filter_fn =
-      config->exporter_options.get<otel::ResourceFilterDataFnOption>();
-  ASSERT_NE(filter_fn, nullptr);
-
-  // Explicit filter should take precedence
-  EXPECT_TRUE(filter_fn("explicit_test"));
-  EXPECT_FALSE(filter_fn("service_name"));
-}
-
-TEST(GrpcMetricsExporter, MakeFilterWithEmptyExcludedLabels) {
-  auto excluded_labels = std::set<std::string>{};
-
-  // Test when empty excluded labels returns nullptr.
-  auto options =
-      TestOptions().set<storage_experimental::GrpcMetricsExcludedLabelsOption>(
-          excluded_labels);
-  auto config = MakeMeterProviderConfig(FullResource(), options);
-  ASSERT_TRUE(config.has_value());
-
-  auto filter_fn =
-      config->exporter_options.get<otel::ResourceFilterDataFnOption>();
-  EXPECT_EQ(filter_fn, nullptr);
-}
-
-TEST(GrpcMetricsExporter, MakeFilterWithNoExcludedLabelsOption) {
-  auto options = TestOptions();
-
-  // Test when no excluded labels option returns nullptr.
-  auto config = MakeMeterProviderConfig(FullResource(), options);
-  ASSERT_TRUE(config.has_value());
-
-  auto filter_fn =
-      config->exporter_options.get<otel::ResourceFilterDataFnOption>();
-  EXPECT_EQ(filter_fn, nullptr);
-}
-
 }  // namespace
 GOOGLE_CLOUD_CPP_INLINE_NAMESPACE_END
 }  // namespace storage_internal