metrics: Add dbus interface for GetRandomPerfOutput

chrome/browser/metrics/perf_provider_chromeos.cc

Index: chrome/browser/metrics/perf_provider_chromeos.cc
diff --git a/chrome/browser/metrics/perf_provider_chromeos.cc b/chrome/browser/metrics/perf_provider_chromeos.cc
index 6bb48a3bef87c1f86d7d55ec51eae18ae9894c05..ac05c8a17e6a455b6c77414c45b72116c4bfc087 100644
--- a/chrome/browser/metrics/perf_provider_chromeos.cc
+++ b/chrome/browser/metrics/perf_provider_chromeos.cc
@@ -93,7 +93,6 @@ bool IsNormalUserLoggedIn() {
 
 }  // namespace
 
-
 namespace metrics {
 
 // This class must be created and used on the UI thread. It watches for any
@@ -321,11 +320,11 @@ void PerfProvider::CollectIfNecessary(
   base::TimeDelta collection_duration = base::TimeDelta::FromSeconds(
       kPerfCommandDurationDefaultSeconds);
 
-  client->GetPerfData(collection_duration.InSeconds(),
-                      base::Bind(&PerfProvider::ParseProtoIfValid,
-                                 weak_factory_.GetWeakPtr(),
-                                 base::Passed(&incognito_observer),
-                                 base::Passed(&sampled_profile)));
+  client->GetPerfOutput(
+      collection_duration.InSeconds(),
+      base::Bind(&PerfProvider::ParseOutputProtoIfValid,
+                 weak_factory_.GetWeakPtr(), base::Passed(&incognito_observer),
+                 base::Passed(&sampled_profile)));
 }
 
 void PerfProvider::DoPeriodicCollection() {
@@ -360,10 +359,12 @@ void PerfProvider::CollectPerfDataAfterSessionRestore(
   last_session_restore_collection_time_ = base::TimeTicks::Now();
 }
 
-void PerfProvider::ParseProtoIfValid(
+void PerfProvider::ParseOutputProtoIfValid(
     scoped_ptr<WindowedIncognitoObserver> incognito_observer,
-    scoped_ptr<SampledProfile> sampled_profile,
-    const std::vector<uint8>& data) {
+    scoped_ptr<SampledProfile> sampled_profile_ptr,
+    int result,
+    const std::vector<uint8>& perf_data,
+    const std::vector<uint8>& perf_stat) {
   DCHECK(CalledOnValidThread());
 
   if (incognito_observer->incognito_launched()) {
@@ -371,29 +372,44 @@ void PerfProvider::ParseProtoIfValid(
     return;
   }
 
-  PerfDataProto perf_data_proto;
-  if (!perf_data_proto.ParseFromArray(data.data(), data.size())) {
+  if (result != 0 || (perf_data.empty() || perf_stat.empty())) {

[Ilya Sherman, 2015/06/27 05:44:24]

nit: Should the second || be an &&?  If so, please add a regression test in
addition to fixing the bug.

[Simon Que, 2015/06/30 05:28:09]

Done.

     AddToPerfHistogram(PROTOBUF_NOT_PARSED);
     return;
   }
 
-  // Populate a profile collection protobuf with the collected perf data and
-  // extra metadata.
-  cached_perf_data_.resize(cached_perf_data_.size() + 1);
-  SampledProfile& collection_data = cached_perf_data_.back();
-  collection_data.Swap(sampled_profile.get());
+  // If output data in either PerfDataProto or PerfStatProto format was
+  // returned, store it in the corresponding field in SampledProfile.
+  SampledProfile& sampled_profile = *sampled_profile_ptr.get();

[Ilya Sherman, 2015/06/27 05:44:24]

Erm, why don't you just use the arrow operator?  What am I missing?

[Simon Que, 2015/06/30 05:50:48]

Done.

+
+  if (!perf_data.empty()) {
+    PerfDataProto perf_data_proto;
+    if (!perf_data_proto.ParseFromArray(perf_data.data(), perf_data.size())) {
+      AddToPerfHistogram(PROTOBUF_NOT_PARSED);
+      return;

[Ilya Sherman, 2015/06/27 05:44:24]

Is it appropriate to discard the perf_stat array if the perf_data one can't be
parsed?  If so, why isn't the reverse also true?

[Simon Que, 2015/06/30 05:50:48]

Good question. That's easy enough to deal with, but I need to consult my
teammates about it and see what the histogram behavior should be if one succeeds
but the other fails.

+    }
+    sampled_profile.set_ms_after_boot(perf_data_proto.timestamp_sec() *
+                                      base::Time::kMillisecondsPerSecond);
+    sampled_profile.mutable_perf_data()->Swap(&perf_data_proto);
+  }
 
-  // Fill out remaining fields of the SampledProfile protobuf.
-  collection_data.set_ms_after_boot(
-      perf_data_proto.timestamp_sec() * base::Time::kMillisecondsPerSecond);
+  if (!perf_stat.empty()) {
+    PerfStatProto perf_stat_proto;
+    if (!perf_stat_proto.ParseFromArray(perf_stat.data(), perf_stat.size())) {
+      AddToPerfHistogram(PROTOBUF_NOT_PARSED);
+      return;
+    }
+    // TODO(sque): Do we want to store |sampled_profile.ms_after_boot| for perf
+    // stat?

[Ilya Sherman, 2015/06/27 05:44:24]

Are you planning to address this TODO as part of this CL, or to commit it?  I'd
encourage you to decide sooner rather than later, unless there's data that
you're waiting for.  (FWIW, there's probably not much cost to setting it.)

[Simon Que, 2015/06/30 05:50:48]

This is actually wrong. The timestamp is time since epoch, not since boot.

I'm thinking that we should instead call
BootTimesRecorder::Stats::GetCurrentStats().

[Ilya Sherman, 2015/07/01 03:09:01]

Sorry, what is actually wrong?  Is the ms_after_boot data being set incorrectly
for the PerfDataProto?  It should be easy enough to translate a timestamp since
epoch to a timestamp since boot.

+    sampled_profile.mutable_perf_stat()->Swap(&perf_stat_proto);
+  }
 
   DCHECK(!login_time_.is_null());
-  collection_data.
-      set_ms_after_login((base::TimeTicks::Now() - login_time_)
-          .InMilliseconds());
+  sampled_profile.set_ms_after_login(
+      (base::TimeTicks::Now() - login_time_).InMilliseconds());
 
-  // Finally, store the perf data itself.
-  collection_data.mutable_perf_data()->Swap(&perf_data_proto);
+  // Add the collected data to the container of collected SampledProfiles.
+  cached_perf_data_.resize(cached_perf_data_.size() + 1);
+  cached_perf_data_.back().Swap(&sampled_profile);
 }
 
 }  // namespace metrics