metrics: Add dbus interface for GetRandomPerfOutput

chrome/browser/metrics/perf_provider_chromeos.cc

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include <string>
 
 #include "base/bind.h"
 #include "base/bind_helpers.h"
 #include "base/callback.h"
 #include "base/command_line.h"
@@ skipping 75 matching lines @@
 }
 
 // Returns true if a normal user is logged in. Returns false otherwise (e.g. if
 // logged in as a guest or as a kiosk app).
 bool IsNormalUserLoggedIn() {
   return chromeos::LoginState::Get()->IsUserAuthenticated();
 }
 
 }  // namespace
 
-
 namespace metrics {
 
 // This class must be created and used on the UI thread. It watches for any
 // incognito window being opened from the time it is instantiated to the time it
 // is destroyed.
 class WindowedIncognitoObserver : public chrome::BrowserListObserver {
  public:
   WindowedIncognitoObserver() : incognito_launched_(false) {
     BrowserList::AddObserver(this);
   }
@@ skipping 207 matching lines @@
 
   scoped_ptr<WindowedIncognitoObserver> incognito_observer(
       new WindowedIncognitoObserver);
 
   chromeos::DebugDaemonClient* client =
       chromeos::DBusThreadManager::Get()->GetDebugDaemonClient();
 
   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() {
   scoped_ptr<SampledProfile> sampled_profile(new SampledProfile);
   sampled_profile->set_trigger_event(SampledProfile::PERIODIC_COLLECTION);
 
   CollectIfNecessary(sampled_profile.Pass());
 }
 
 void PerfProvider::CollectPerfDataAfterResume(
@@ skipping 14 matching lines @@
   // Fill out a SampledProfile protobuf that will contain the collected data.
   scoped_ptr<SampledProfile> sampled_profile(new SampledProfile);
   sampled_profile->set_trigger_event(SampledProfile::RESTORE_SESSION);
   sampled_profile->set_ms_after_restore(time_after_restore.InMilliseconds());
   sampled_profile->set_num_tabs_restored(num_tabs_restored);
 
   CollectIfNecessary(sampled_profile.Pass());
   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()) {
     AddToPerfHistogram(INCOGNITO_LAUNCHED);
     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.

 
-  // Fill out remaining fields of the SampledProfile protobuf.
-  collection_data.set_ms_after_boot(
-      perf_data_proto.timestamp_sec() * base::Time::kMillisecondsPerSecond);
+  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);
+  }
+
+  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