Stackdriver stats: resource must uniquely identify a timeseries. (#389)

- Don't add the opencensus_task label when a non-default
MonitoredResource is present.

- Refactor exporter code to reduce duplication of effort.

Author: g-easy

opencensus/exporters/stats/stackdriver/internal/stackdriver_exporter.cc

@@ -61,8 +61,8 @@ class Handler : public ::opencensus::stats::StatsExporter::Handler {
   // Returns true if the view has already been registered or registration is
   // successful, and false if the registration fails or the name has already
   // been registered with different parameters.
-  bool MaybeRegisterView(const opencensus::stats::ViewDescriptor& descriptor)
-      EXCLUSIVE_LOCKS_REQUIRED(mu_);
+  bool MaybeRegisterView(const opencensus::stats::ViewDescriptor& descriptor,
+                         bool add_task_label) EXCLUSIVE_LOCKS_REQUIRED(mu_);
 
   const StackdriverOptions opts_;
   mutable absl::Mutex mu_;
@@ -103,13 +103,35 @@ void Handler::ExportViewData(
   absl::MutexLock l(&mu_);
   std::vector<google::monitoring::v3::TimeSeries> time_series;
   for (const auto& datum : data) {
-    if (!MaybeRegisterView(datum.first)) {
-      continue;
+    const opencensus::stats::ViewDescriptor& descriptor = datum.first;
+    const std::string metric_type =
+        MakeType(opts_.metric_name_prefix, descriptor.name());
+    const google::api::MonitoredResource* monitored_resource_for_view =
+        MonitoredResourceForView(descriptor, opts_.monitored_resource,
+                                 opts_.per_metric_monitored_resource);
+
+    const bool is_known_custom_metric = IsKnownCustomMetric(metric_type);
+
+    // If this is a custom metric, add the opencensus_task label so that
+    // different processes produce different timeseries instead of colliding.
+    //
+    // However, if there is a non-default MonitoredResource for this view, it
+    // must already uniquely identify the timeseries, so don't add the
+    // opencensus_task label.
+    const bool add_task_label =
+        is_known_custom_metric && (monitored_resource_for_view == nullptr);
+
+    // Builtin metrics are already defined, skip registration.
+    if (is_known_custom_metric) {
+      // If the view can't be registered, skip it.
+      if (!MaybeRegisterView(descriptor, add_task_label)) {
+        continue;
+      }
     }
-    const auto view_time_series =
-        MakeTimeSeries(opts_.metric_name_prefix, opts_.monitored_resource,
-                       opts_.per_metric_monitored_resource, datum.first,
-                       datum.second, opts_.opencensus_task);
+
+    const auto view_time_series = MakeTimeSeries(
+        opts_.metric_name_prefix, monitored_resource_for_view, descriptor,
+        /*data=*/datum.second, add_task_label, opts_.opencensus_task);
     time_series.insert(time_series.end(), view_time_series.begin(),
                        view_time_series.end());
   }
@@ -156,7 +178,7 @@ void Handler::ExportViewData(
 }
 
 bool Handler::MaybeRegisterView(
-    const opencensus::stats::ViewDescriptor& descriptor) {
+    const opencensus::stats::ViewDescriptor& descriptor, bool add_task_label) {
   const auto& it = registered_descriptors_.find(descriptor.name());
   if (it != registered_descriptors_.end()) {
     if (it->second != descriptor) {
@@ -168,16 +190,10 @@ bool Handler::MaybeRegisterView(
     return true;
   }
 
-  const std::string metric_type =
-      MakeType(opts_.metric_name_prefix, descriptor.name());
-  if (!IsKnownCustomMetric(metric_type)) {
-    // Builtin metrics are already defined, skip the CreateMetricDescriptor RPC.
-    return true;
-  }
   auto request = google::monitoring::v3::CreateMetricDescriptorRequest();
   request.set_name(opts_.project_id);
   SetMetricDescriptor(opts_.project_id, opts_.metric_name_prefix, descriptor,
-                      request.mutable_metric_descriptor());
+                      add_task_label, request.mutable_metric_descriptor());
   ::grpc::ClientContext context;
   context.set_deadline(absl::ToChronoTime(absl::Now() + opts_.rpc_deadline));
   google::api::MetricDescriptor response;

opencensus/exporters/stats/stackdriver/internal/stackdriver_utils.cc

@@ -162,18 +162,31 @@ bool IsKnownCustomMetric(absl::string_view metric_type) {
          absl::StartsWith(metric_type, "external.googleapis.com/");
 }
 
+const google::api::MonitoredResource* MonitoredResourceForView(
+    const opencensus::stats::ViewDescriptor& view_descriptor,
+    const google::api::MonitoredResource& monitored_resource,
+    const std::unordered_map<std::string, google::api::MonitoredResource>&
+        per_metric_monitored_resource) {
+  auto iter = per_metric_monitored_resource.find(view_descriptor.name());
+  if (iter != per_metric_monitored_resource.end()) {
+    return &iter->second;
+  } else if (monitored_resource.type().empty()) {
+    return nullptr;
+  } else {
+    return &monitored_resource;
+  }
+}
+
 void SetMetricDescriptor(
     absl::string_view project_name, absl::string_view metric_name_prefix,
     const opencensus::stats::ViewDescriptor& view_descriptor,
-    google::api::MetricDescriptor* metric_descriptor) {
+    bool add_task_label, google::api::MetricDescriptor* metric_descriptor) {
   metric_descriptor->set_name(
       MakeName(project_name, metric_name_prefix, view_descriptor.name()));
   const std::string metric_type =
       MakeType(metric_name_prefix, view_descriptor.name());
   metric_descriptor->set_type(metric_type);
-  if (IsKnownCustomMetric(metric_type)) {
-    // Custom metrics use this label so that different processes produce
-    // different timeseries instead of colliding.
+  if (add_task_label) {
     SetOpenCensusTaskLabelDescriptor(metric_descriptor->add_labels());
   }
   for (const auto& tag_key : view_descriptor.columns()) {
@@ -194,24 +207,19 @@ void SetMetricDescriptor(
 
 std::vector<google::monitoring::v3::TimeSeries> MakeTimeSeries(
     absl::string_view metric_name_prefix,
-    const google::api::MonitoredResource& monitored_resource,
-    const std::unordered_map<std::string, google::api::MonitoredResource>&
-        per_metric_monitored_resource,
+    const google::api::MonitoredResource* monitored_resource_for_view,
     const opencensus::stats::ViewDescriptor& view_descriptor,
-    const opencensus::stats::ViewData& data,
+    const opencensus::stats::ViewData& data, bool add_task_label,
     absl::string_view opencensus_task) {
   // Set values that are common across all the rows.
   auto base_time_series = google::monitoring::v3::TimeSeries();
   const std::string metric_type =
       MakeType(metric_name_prefix, view_descriptor.name());
   base_time_series.mutable_metric()->set_type(metric_type);
-  auto iter = per_metric_monitored_resource.find(view_descriptor.name());
-  if (iter != per_metric_monitored_resource.end()) {
-    *base_time_series.mutable_resource() = iter->second;
-  } else if (monitored_resource.type().empty()) {
+  if (monitored_resource_for_view == nullptr) {
     base_time_series.mutable_resource()->set_type(kDefaultResourceType);
   } else {
-    *base_time_series.mutable_resource() = monitored_resource;
+    *base_time_series.mutable_resource() = *monitored_resource_for_view;
   }
   auto* interval = base_time_series.add_points()->mutable_interval();
   // Stackdriver doesn't like start_time and end_time being different for GAUGE
@@ -221,9 +229,7 @@ std::vector<google::monitoring::v3::TimeSeries> MakeTimeSeries(
     SetTimestamp(data.start_time(), interval->mutable_start_time());
   }
   SetTimestamp(data.end_time(), interval->mutable_end_time());
-  if (IsKnownCustomMetric(metric_type)) {
-    // Custom metrics use this label so that different processes produce
-    // different timeseries instead of colliding.
+  if (add_task_label) {
     (*base_time_series.mutable_metric()->mutable_labels())[kOpenCensusTaskKey] =
         std::string(opencensus_task);
   }

opencensus/exporters/stats/stackdriver/internal/stackdriver_utils.h

@@ -38,22 +38,29 @@ std::string MakeType(absl::string_view metric_name_prefix,
 // custom (i.e not built-in) Stackdriver metric.
 bool IsKnownCustomMetric(absl::string_view metric_type);
 
+// Returns a pointer to the MonitoredResource proto for this view, or nullptr if
+// the default resource should be used.
+const google::api::MonitoredResource* MonitoredResourceForView(
+    const opencensus::stats::ViewDescriptor& view_descriptor,
+    const google::api::MonitoredResource& monitored_resource,
+    const std::unordered_map<std::string, google::api::MonitoredResource>&
+        per_metric_monitored_resource);
+
 // Populates metric_descriptor. project_name must be in the format
 // "projects/project_id". metric_name_prefix must have a trailing slash, e.g.
 // "custom.googleapis.com/opencensus/".
 void SetMetricDescriptor(
     absl::string_view project_name, absl::string_view metric_name_prefix,
     const opencensus::stats::ViewDescriptor& view_descriptor,
-    google::api::MetricDescriptor* metric_descriptor);
+    bool add_task_label, google::api::MetricDescriptor* metric_descriptor);
 
-// Converts each row of 'data' into TimeSeries.
+// Converts each row of 'data' into a TimeSeries proto.
 std::vector<google::monitoring::v3::TimeSeries> MakeTimeSeries(
     absl::string_view metric_name_prefix,
-    const google::api::MonitoredResource& monitored_resource,
-    const std::unordered_map<std::string, google::api::MonitoredResource>&
-        per_metric_monitored_resource,
+    const google::api::MonitoredResource* monitored_resource_for_view,
     const opencensus::stats::ViewDescriptor& view_descriptor,
-    const opencensus::stats::ViewData& data, absl::string_view opencensus_task);
+    const opencensus::stats::ViewData& data, bool add_task_label,
+    absl::string_view opencensus_task);
 
 // Populates proto based on the given time.
 void SetTimestamp(absl::Time time, google::protobuf::Timestamp* proto);