ProfileProvider: move more code into MetricCollector for better reuse

Move additional logic to the base class MetricCollector for reuse between
different collector implementations, e.g. writing outcomes to UMA histograms,
removal of unknown fields from protos, saving profiles to the local cache.
This change enables making two additional fields private.

Moved tests for discarding unknown fields and for saving perf data and perf
stat protos, to the metric collector unit tests.

BUG=b:110205489
TEST=Unit tests pass

Change-Id: I8877e9abc6ee8d89e6be391063d3ddf7ba231bc0
Reviewed-on: https://chromium-review.googlesource.com/c/1392352Reviewed-by: Ilya Sherman <isherman@chromium.org>
Commit-Queue: Gabriel Marin <gmx@chromium.org>
Cr-Commit-Position: refs/heads/master@{#619836}

chrome/browser/metrics/perf/metric_collector.cc

@@ -4,7 +4,9 @@
 
 #include "chrome/browser/metrics/perf/metric_collector.h"
 
+#include "base/metrics/histogram_functions.h"
 #include "base/rand_util.h"
+#include "base/system/sys_info.h"
 #include "third_party/metrics_proto/sampled_profile.pb.h"
 
 namespace metrics {
@@ -24,21 +26,62 @@ base::TimeDelta RandomTimeDelta(base::TimeDelta max) {
       base::RandGenerator(max.InMicroseconds()));
 }
 
+// PerfDataProto is defined elsewhere with more fields than the definition in
+// Chromium's copy of perf_data.proto. During deserialization, the protobuf
+// data could contain fields that are defined elsewhere but not in
+// perf_data.proto, resulting in some data in |unknown_fields| for the message
+// types within PerfDataProto.
+//
+// This function deletes those dangling unknown fields if they are in messages
+// containing strings. See comments in perf_data.proto describing the fields
+// that have been intentionally left out. Note that all unknown fields will be
+// removed from those messages, not just unknown string fields.
+void RemoveUnknownFieldsFromMessagesWithStrings(PerfDataProto* proto) {
+  // Clean up PerfEvent::MMapEvent and PerfEvent::CommEvent.
+  for (PerfDataProto::PerfEvent& event : *proto->mutable_events()) {
+    if (event.has_comm_event())
+      event.mutable_comm_event()->mutable_unknown_fields()->clear();
+    if (event.has_mmap_event())
+      event.mutable_mmap_event()->mutable_unknown_fields()->clear();
+  }
+  // Clean up PerfBuildID.
+  for (PerfDataProto::PerfBuildID& build_id : *proto->mutable_build_ids()) {
+    build_id.mutable_unknown_fields()->clear();
+  }
+  // Clean up StringMetadata and StringMetadata::StringAndMd5sumPrefix.
+  if (proto->has_string_metadata()) {
+    proto->mutable_string_metadata()->mutable_unknown_fields()->clear();
+    if (proto->string_metadata().has_perf_command_line_whole()) {
+      proto->mutable_string_metadata()
+          ->mutable_perf_command_line_whole()
+          ->mutable_unknown_fields()
+          ->clear();
+    }
+  }
+}
+
 }  // namespace
 
-MetricCollector::MetricCollector() {}
+MetricCollector::MetricCollector(const std::string& uma_histogram)
+    : uma_histogram_(uma_histogram) {}
 
-MetricCollector::MetricCollector(const CollectionParams& collection_params)
-    : collection_params_(collection_params) {}
+MetricCollector::MetricCollector(const std::string& uma_histogram,
+                                 const CollectionParams& collection_params)
+    : collection_params_(collection_params), uma_histogram_(uma_histogram) {}
 
 MetricCollector::~MetricCollector() {
   DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
 }
 
+void MetricCollector::AddToUmaHistogram(CollectionAttemptStatus outcome) const {
+  base::UmaHistogramEnumeration(uma_histogram_, outcome,
+                                CollectionAttemptStatus::NUM_OUTCOMES);
+}
+
 bool MetricCollector::GetSampledProfiles(
     std::vector<SampledProfile>* sampled_profiles) {
   DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
-  if (!ShouldUpload() || cached_profile_data_.empty())
+  if (!ShouldUpload())
     return false;
 
   sampled_profiles->insert(
@@ -50,6 +93,12 @@ bool MetricCollector::GetSampledProfiles(
 }
 
 bool MetricCollector::ShouldUpload() const {
+  if (cached_profile_data_.empty()) {
+    AddToUmaHistogram(CollectionAttemptStatus::NOT_READY_TO_UPLOAD);
+    return false;
+  }
+
+  AddToUmaHistogram(CollectionAttemptStatus::SUCCESS);
   return true;
 }
 
@@ -204,4 +253,56 @@ void MetricCollector::Deactivate() {
   timer_.Stop();
 }
 
+void MetricCollector::SaveSerializedPerfProto(
+    std::unique_ptr<SampledProfile> sampled_profile,
+    PerfProtoType type,
+    const std::string& serialized_proto) {
+  if (serialized_proto.empty()) {
+    AddToUmaHistogram(CollectionAttemptStatus::ILLEGAL_DATA_RETURNED);
+    return;
+  }
+
+  switch (type) {
+    case PerfProtoType::PERF_TYPE_DATA: {
+      PerfDataProto perf_data_proto;
+      if (!perf_data_proto.ParseFromString(serialized_proto)) {
+        AddToUmaHistogram(CollectionAttemptStatus::PROTOBUF_NOT_PARSED);
+        return;
+      }
+      RemoveUnknownFieldsFromMessagesWithStrings(&perf_data_proto);
+      sampled_profile->mutable_perf_data()->Swap(&perf_data_proto);
+      break;
+    }
+    case PerfProtoType::PERF_TYPE_STAT: {
+      PerfStatProto perf_stat_proto;
+      if (!perf_stat_proto.ParseFromString(serialized_proto)) {
+        AddToUmaHistogram(CollectionAttemptStatus::PROTOBUF_NOT_PARSED);
+        return;
+      }
+      sampled_profile->mutable_perf_stat()->Swap(&perf_stat_proto);
+      break;
+    }
+    case PerfProtoType::PERF_TYPE_UNSUPPORTED:
+      AddToUmaHistogram(CollectionAttemptStatus::PROTOBUF_NOT_PARSED);
+      return;
+  }
+
+  sampled_profile->set_ms_after_boot(base::SysInfo::Uptime().InMilliseconds());
+  DCHECK(!login_time_.is_null());
+  sampled_profile->set_ms_after_login(
+      (base::TimeTicks::Now() - login_time_).InMilliseconds());
+
+  // Add the collected data to the container of collected SampledProfiles.
+  cached_profile_data_.resize(cached_profile_data_.size() + 1);
+  cached_profile_data_.back().Swap(sampled_profile.get());
+}
+
+size_t MetricCollector::cached_profile_data_size() const {
+  size_t data_size = 0;
+  for (size_t i = 0; i < cached_profile_data_.size(); ++i) {
+    data_size += cached_profile_data_[i].ByteSize();
+  }
+  return data_size;
+}
+
 }  // namespace metrics

chrome/browser/metrics/perf/metric_collector.h

@@ -25,8 +25,9 @@ class SampledProfile;
 // pointer across threads safely.
 class MetricCollector : public base::SupportsWeakPtr<MetricCollector> {
  public:
-  MetricCollector();
-  explicit MetricCollector(const CollectionParams& collection_params);
+  explicit MetricCollector(const std::string& uma_histogram);
+  explicit MetricCollector(const std::string& uma_histogram,
+                           const CollectionParams& collection_params);
   virtual ~MetricCollector();
 
   // Collector specific initialization.
@@ -52,12 +53,38 @@ class MetricCollector : public base::SupportsWeakPtr<MetricCollector> {
   void OnSessionRestoreDone(int num_tabs_restored);
 
  protected:
+  // Perf proto type.
+  enum class PerfProtoType {
+    PERF_TYPE_DATA,
+    PERF_TYPE_STAT,
+    PERF_TYPE_UNSUPPORTED,
+  };
+
+  // Enumeration representing success and various failure modes for collecting
+  // and sending perf data.
+  enum class CollectionAttemptStatus {
+    SUCCESS,
+    NOT_READY_TO_UPLOAD,
+    NOT_READY_TO_COLLECT,
+    INCOGNITO_ACTIVE,
+    INCOGNITO_LAUNCHED,
+    PROTOBUF_NOT_PARSED,
+    ILLEGAL_DATA_RETURNED,
+    ALREADY_COLLECTING,
+    NUM_OUTCOMES
+  };
+
+  // Saves the given outcome to the uma histogram associated with the collector.
+  void AddToUmaHistogram(CollectionAttemptStatus outcome) const;
+
   const CollectionParams& collection_params() const {
     return collection_params_;
   }
 
   const base::OneShotTimer& timer() const { return timer_; }
 
+  base::TimeTicks login_time() const { return login_time_; }
+
   // Collects perf data after a resume. |sleep_duration| is the duration the
   // system was suspended before resuming. |time_after_resume_ms| is how long
   // ago the system resumed.
@@ -94,14 +121,18 @@ class MetricCollector : public base::SupportsWeakPtr<MetricCollector> {
   // implementation can override this logic.
   virtual bool ShouldUpload() const;
 
-  // Parameters controlling how profiles are collected.
-  CollectionParams collection_params_;
+  // Parses the given serialized perf proto of the given type (data or stat).
+  // If valid, it adds it to the given sampled_profile and stores it in the
+  // local profile data cache.
+  void SaveSerializedPerfProto(std::unique_ptr<SampledProfile> sampled_profile,
+                               PerfProtoType type,
+                               const std::string& serialized_proto);
 
-  // Vector of SampledProfile protobufs containing perf profiles.
-  std::vector<SampledProfile> cached_profile_data_;
+  // Returns the size of the cached profile data.
+  size_t cached_profile_data_size() const;
 
-  // Record of the last login time.
-  base::TimeTicks login_time_;
+  // Parameters controlling how profiles are collected.
+  CollectionParams collection_params_;
 
   SEQUENCE_CHECKER(sequence_checker_);
 
@@ -109,12 +140,22 @@ class MetricCollector : public base::SupportsWeakPtr<MetricCollector> {
   // For scheduling collection of profile data.
   base::OneShotTimer timer_;
 
+  // Vector of SampledProfile protobufs containing perf profiles.
+  std::vector<SampledProfile> cached_profile_data_;
+
+  // Record of the last login time.
+  base::TimeTicks login_time_;
+
   // Record of the start of the upcoming profiling interval.
   base::TimeTicks next_profiling_interval_start_;
 
   // Tracks the last time a session restore was collected.
   base::TimeTicks last_session_restore_collection_time_;
 
+  // Name of the histogram that represents the success and various failure modes
+  // for a collector.
+  std::string uma_histogram_;
+
   DISALLOW_COPY_AND_ASSIGN(MetricCollector);
 };
 

chrome/browser/metrics/perf/perf_events_collector.cc

@@ -4,11 +4,9 @@
 
 #include "chrome/browser/metrics/perf/perf_events_collector.h"
 
-#include "base/metrics/histogram_macros.h"
 #include "base/rand_util.h"
 #include "base/strings/string_number_conversions.h"
 #include "base/strings/string_split.h"
-#include "base/system/sys_info.h"
 #include "chrome/browser/metrics/perf/cpu_identity.h"
 #include "chrome/browser/metrics/perf/perf_output.h"
 #include "chrome/browser/metrics/perf/windowed_incognito_observer.h"
@@ -27,28 +25,9 @@ const char kCWPFieldTrialName[] = "ChromeOSWideProfilingCollection";
 // collecting further perf data. The current value is 4 MB.
 const size_t kCachedPerfDataProtobufSizeThreshold = 4 * 1024 * 1024;
 
-// Enumeration representing success and various failure modes for collecting and
-// sending perf data.
-enum GetPerfDataOutcome {
-  SUCCESS,
-  NOT_READY_TO_UPLOAD,
-  NOT_READY_TO_COLLECT,
-  INCOGNITO_ACTIVE,
-  INCOGNITO_LAUNCHED,
-  PROTOBUF_NOT_PARSED,
-  ILLEGAL_DATA_RETURNED,
-  ALREADY_COLLECTING,
-  NUM_OUTCOMES
-};
-
 // Name of the histogram that represents the success and various failure modes
-// for collecting and sending perf data.
-const char kGetPerfDataOutcomeHistogram[] = "UMA.Perf.GetData";
-
-void AddToPerfHistogram(GetPerfDataOutcome outcome) {
-  UMA_HISTOGRAM_ENUMERATION(kGetPerfDataOutcomeHistogram, outcome,
-                            NUM_OUTCOMES);
-}
+// for the perf collector.
+const char kPerfCollectorOutcomeHistogram[] = "UMA.Perf.GetData";
 
 // Gets parameter named by |key| from the map. If it is present and is an
 // integer, stores the result in |out| and return true. Otherwise return false.
@@ -193,40 +172,6 @@ const std::vector<RandomSelector::WeightAndValue> GetDefaultCommands_x86_64(
   return cmds;
 }
 
-// PerfDataProto is defined elsewhere with more fields than the definition in
-// Chromium's copy of perf_data.proto. During deserialization, the protobuf
-// data could contain fields that are defined elsewhere but not in
-// perf_data.proto, resulting in some data in |unknown_fields| for the message
-// types within PerfDataProto.
-//
-// This function deletes those dangling unknown fields if they are in messages
-// containing strings. See comments in perf_data.proto describing the fields
-// that have been intentionally left out. Note that all unknown fields will be
-// removed from those messages, not just unknown string fields.
-void RemoveUnknownFieldsFromMessagesWithStrings(PerfDataProto* proto) {
-  // Clean up PerfEvent::MMapEvent and PerfEvent::CommEvent.
-  for (PerfDataProto::PerfEvent& event : *proto->mutable_events()) {
-    if (event.has_comm_event())
-      event.mutable_comm_event()->mutable_unknown_fields()->clear();
-    if (event.has_mmap_event())
-      event.mutable_mmap_event()->mutable_unknown_fields()->clear();
-  }
-  // Clean up PerfBuildID.
-  for (PerfDataProto::PerfBuildID& build_id : *proto->mutable_build_ids()) {
-    build_id.mutable_unknown_fields()->clear();
-  }
-  // Clean up StringMetadata and StringMetadata::StringAndMd5sumPrefix.
-  if (proto->has_string_metadata()) {
-    proto->mutable_string_metadata()->mutable_unknown_fields()->clear();
-    if (proto->string_metadata().has_perf_command_line_whole()) {
-      proto->mutable_string_metadata()
-          ->mutable_perf_command_line_whole()
-          ->mutable_unknown_fields()
-          ->clear();
-    }
-  }
-}
-
 }  // namespace
 
 namespace internal {
@@ -253,7 +198,8 @@ std::vector<RandomSelector::WeightAndValue> GetDefaultCommandsForCpu(
 
 }  // namespace internal
 
-PerfCollector::PerfCollector() {}
+PerfCollector::PerfCollector()
+    : MetricCollector(kPerfCollectorOutcomeHistogram) {}
 
 PerfCollector::~PerfCollector() {}
 
@@ -267,16 +213,6 @@ void PerfCollector::Init() {
   MetricCollector::Init();
 }
 
-bool PerfCollector::ShouldUpload() const {
-  if (cached_profile_data_.empty()) {
-    AddToPerfHistogram(NOT_READY_TO_UPLOAD);
-    return false;
-  }
-
-  AddToPerfHistogram(SUCCESS);
-  return true;
-}
-
 namespace internal {
 
 std::string FindBestCpuSpecifierFromParams(
@@ -387,24 +323,22 @@ void PerfCollector::SetCollectionParamsFromVariationParams(
   command_selector_.SetOdds(commands);
 }
 
-PerfCollector::PerfSubcommand PerfCollector::GetPerfSubcommandType(
+MetricCollector::PerfProtoType PerfCollector::GetPerfProtoType(
     const std::vector<std::string>& args) {
   if (args.size() > 1 && args[0] == "perf") {
-    if (args[1] == "record")
-      return PerfSubcommand::PERF_COMMAND_RECORD;
+    if (args[1] == "record" || args[1] == "mem")
+      return PerfProtoType::PERF_TYPE_DATA;
     if (args[1] == "stat")
-      return PerfSubcommand::PERF_COMMAND_STAT;
-    if (args[1] == "mem")
-      return PerfSubcommand::PERF_COMMAND_MEM;
+      return PerfProtoType::PERF_TYPE_STAT;
   }
 
-  return PerfSubcommand::PERF_COMMAND_UNSUPPORTED;
+  return PerfProtoType::PERF_TYPE_UNSUPPORTED;
 }
 
 void PerfCollector::ParseOutputProtoIfValid(
     std::unique_ptr<WindowedIncognitoObserver> incognito_observer,
     std::unique_ptr<SampledProfile> sampled_profile,
-    PerfSubcommand subcommand,
+    PerfProtoType type,
     const std::string& perf_stdout) {
   DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
 
@@ -413,74 +347,30 @@ void PerfCollector::ParseOutputProtoIfValid(
   std::unique_ptr<PerfOutputCall> call_deleter(std::move(perf_output_call_));
 
   if (incognito_observer->incognito_launched()) {
-    AddToPerfHistogram(INCOGNITO_LAUNCHED);
-    return;
-  }
-
-  if (perf_stdout.empty()) {
-    AddToPerfHistogram(ILLEGAL_DATA_RETURNED);
+    AddToUmaHistogram(CollectionAttemptStatus::INCOGNITO_LAUNCHED);
     return;
   }
-
-  switch (subcommand) {
-    case PerfSubcommand::PERF_COMMAND_RECORD:
-    case PerfSubcommand::PERF_COMMAND_MEM: {
-      PerfDataProto perf_data_proto;
-      if (!perf_data_proto.ParseFromString(perf_stdout)) {
-        AddToPerfHistogram(PROTOBUF_NOT_PARSED);
-        return;
-      }
-      RemoveUnknownFieldsFromMessagesWithStrings(&perf_data_proto);
-      sampled_profile->set_ms_after_boot(
-          base::SysInfo::Uptime().InMilliseconds());
-      sampled_profile->mutable_perf_data()->Swap(&perf_data_proto);
-      break;
-    }
-    case PerfSubcommand::PERF_COMMAND_STAT: {
-      PerfStatProto perf_stat_proto;
-      if (!perf_stat_proto.ParseFromString(perf_stdout)) {
-        AddToPerfHistogram(PROTOBUF_NOT_PARSED);
-        return;
-      }
-      sampled_profile->mutable_perf_stat()->Swap(&perf_stat_proto);
-      break;
-    }
-    case PerfSubcommand::PERF_COMMAND_UNSUPPORTED:
-      AddToPerfHistogram(PROTOBUF_NOT_PARSED);
-      return;
-  }
-
-  DCHECK(!login_time_.is_null());
-  sampled_profile->set_ms_after_login(
-      (base::TimeTicks::Now() - login_time_).InMilliseconds());
-
-  // Add the collected data to the container of collected SampledProfiles.
-  cached_profile_data_.resize(cached_profile_data_.size() + 1);
-  cached_profile_data_.back().Swap(sampled_profile.get());
+  SaveSerializedPerfProto(std::move(sampled_profile), type, perf_stdout);
 }
 
 bool PerfCollector::ShouldCollect() const {
   // Only allow one active collection.
   if (perf_output_call_) {
-    AddToPerfHistogram(ALREADY_COLLECTING);
+    AddToUmaHistogram(CollectionAttemptStatus::ALREADY_COLLECTING);
     return false;
   }
 
   // Do not collect further data if we've already collected a substantial amount
   // of data, as indicated by |kCachedPerfDataProtobufSizeThreshold|.
-  size_t cached_perf_data_size = 0;
-  for (size_t i = 0; i < cached_profile_data_.size(); ++i) {
-    cached_perf_data_size += cached_profile_data_[i].ByteSize();
-  }
-  if (cached_perf_data_size >= kCachedPerfDataProtobufSizeThreshold) {
-    AddToPerfHistogram(NOT_READY_TO_COLLECT);
+  if (cached_profile_data_size() >= kCachedPerfDataProtobufSizeThreshold) {
+    AddToUmaHistogram(CollectionAttemptStatus::NOT_READY_TO_COLLECT);
     return false;
   }
 
   // For privacy reasons, Chrome should only collect perf data if there is no
   // incognito session active (or gets spawned during the collection).
   if (BrowserList::IsIncognitoSessionActive()) {
-    AddToPerfHistogram(INCOGNITO_ACTIVE);
+    AddToUmaHistogram(CollectionAttemptStatus::INCOGNITO_ACTIVE);
     return false;
   }
 
@@ -495,14 +385,14 @@ void PerfCollector::CollectProfile(
   std::vector<std::string> command =
       base::SplitString(command_selector_.Select(), kPerfCommandDelimiter,
                         base::KEEP_WHITESPACE, base::SPLIT_WANT_ALL);
-  PerfSubcommand subcommand = GetPerfSubcommandType(command);
+  PerfProtoType type = GetPerfProtoType(command);
 
   perf_output_call_ = std::make_unique<PerfOutputCall>(
       collection_params_.collection_duration, command,
       base::BindOnce(&PerfCollector::ParseOutputProtoIfValid,
                      base::AsWeakPtr<PerfCollector>(this),
                      base::Passed(&incognito_observer),
-                     base::Passed(&sampled_profile), subcommand));
+                     base::Passed(&sampled_profile), type));
 }
 
 }  // namespace metrics

chrome/browser/metrics/perf/perf_events_collector.h

@@ -27,18 +27,9 @@ class PerfCollector : public MetricCollector {
   void Init() override;
 
  protected:
-  // Perf events collection subcommands.
-  enum PerfSubcommand {
-    PERF_COMMAND_RECORD,
-    PERF_COMMAND_STAT,
-    PERF_COMMAND_MEM,
-    PERF_COMMAND_UNSUPPORTED,
-  };
-
-  // Returns one of the above enums given an vector of perf arguments, starting
-  // with "perf" itself in |args[0]|.
-  static PerfSubcommand GetPerfSubcommandType(
-      const std::vector<std::string>& args);
+  // Returns the perf proto type associated with the given vector of perf
+  // arguments, starting with "perf" itself in |args[0]|.
+  static PerfProtoType GetPerfProtoType(const std::vector<std::string>& args);
 
   // Parses a PerfDataProto or PerfStatProto from serialized data |perf_stdout|,
   // if non-empty. Which proto to use depends on |subcommand|. If |perf_stdout|
@@ -48,11 +39,10 @@ class PerfCollector : public MetricCollector {
   void ParseOutputProtoIfValid(
       std::unique_ptr<WindowedIncognitoObserver> incognito_observer,
       std::unique_ptr<SampledProfile> sampled_profile,
-      PerfSubcommand subcommand,
+      PerfProtoType type,
       const std::string& perf_stdout);
 
   // MetricCollector:
-  bool ShouldUpload() const override;
   bool ShouldCollect() const override;
   void CollectProfile(std::unique_ptr<SampledProfile> sampled_profile) override;
 

chrome/browser/metrics/perf/profile_provider_chromeos_unittest.cc

@@ -24,9 +24,6 @@ namespace metrics {
 
 namespace {
 
-const long kMsAfterBoot = 10000;
-const long kMsAfterLogin = 2000;
-
 // Returns sample PerfDataProtos with custom timestamps. The contents don't have
 // to make sense. They just need to constitute a semantically valid protobuf.
 // |proto| is an output parameter that will contain the created protobuf.
@@ -62,18 +59,14 @@ class TestMetricCollector : public MetricCollector {
  public:
   TestMetricCollector() {}
   explicit TestMetricCollector(const CollectionParams& collection_params)
-      : MetricCollector(collection_params) {}
+      : MetricCollector("UMA.CWP.TestData", collection_params) {}
 
   void CollectProfile(
       std::unique_ptr<SampledProfile> sampled_profile) override {
     PerfDataProto perf_data_proto = GetExamplePerfDataProto(TSTAMP);
-    sampled_profile->set_ms_after_boot(kMsAfterBoot);
-    sampled_profile->set_ms_after_login(kMsAfterLogin);
-    sampled_profile->mutable_perf_data()->Swap(&perf_data_proto);
-
-    // Add the collected data to the container of collected SampledProfiles.
-    cached_profile_data_.resize(cached_profile_data_.size() + 1);
-    cached_profile_data_.back().Swap(sampled_profile.get());
+    SaveSerializedPerfProto(std::move(sampled_profile),
+                            PerfProtoType::PERF_TYPE_DATA,
+                            perf_data_proto.SerializeAsString());
   }
 
  private: