metrics: Use absolute interval-based perf collection

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"
 #include "base/compiler_specific.h"
 #include "base/metrics/histogram.h"
 #include "base/rand_util.h"
 #include "base/strings/string_number_conversions.h"
 #include "base/threading/sequenced_worker_pool.h"
 #include "chrome/browser/metrics/perf_provider_chromeos.h"
 #include "chrome/browser/profiles/profile.h"
 #include "chrome/browser/ui/browser.h"
 #include "chrome/browser/ui/browser_list.h"
 #include "chrome/browser/ui/browser_list_observer.h"
 #include "chrome/common/chrome_switches.h"
 #include "chromeos/dbus/dbus_thread_manager.h"
 #include "chromeos/dbus/debug_daemon_client.h"
 
 namespace {
 
-// Default time in seconds between invocations of perf.
-// This period is roughly 6.5 hours.
-// This is chosen to be relatively prime with the number of seconds in:
-// - one minute (60)
-// - one hour (3600)
-// - one day (86400)
-const size_t kPerfCommandIntervalDefaultSeconds = 23093;
-
-// The first collection interval is different from the interval above. This is
-// because we want to collect the first profile quickly after Chrome is started.
-// If this period is too long, the user will log off and Chrome will be killed
-// before it is triggered. The following 2 variables determine the upper and
-// lower bound on the interval.
-// The reason we do not always want to collect the initial profile after a fixed
-// period is to not over-represent task X in the profile where task X always
-// runs at a fixed period after start-up. By selecting a period randomly between
-// a lower and upper bound, we will hopefully collect a more fair profile.
-const size_t kPerfCommandStartIntervalLowerBoundMinutes = 10;
-
-const size_t kPerfCommandStartIntervalUpperBoundMinutes = 20;
+// Partition time since the epoch into successive intervals of this size. In
+// each interval that hasn't passed, pick a random time to collect a profile.
+// This interval is twenty-four hours.

[Ilya Sherman, 2014/05/29 22:56:49]

nit: Please update this comment.

[Simon Que, 2014/05/30 01:21:45]

Done.

+const size_t kPerfProfilingIntervalMs = 24 * 60 * 60 * 1000;
 
 // Default time in seconds perf is run for.
 const size_t kPerfCommandDurationDefaultSeconds = 2;
 
 // Limit the total size of protobufs that can be cached, so they don't take up
 // too much memory. If the size of cached protobufs exceeds this value, stop
 // 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
@@ skipping 55 matching lines @@
   virtual void OnBrowserAdded(Browser* browser) OVERRIDE {
     if (browser->profile()->IsOffTheRecord())
       incognito_launched_ = true;
   }
 
   bool incognito_launched_;
 };
 
 PerfProvider::PerfProvider()
       : login_observer_(this),
+        next_profiling_interval_start_(base::TimeTicks::Now()),
         weak_factory_(this) {
   // Register the login observer with LoginState.
   chromeos::LoginState::Get()->AddObserver(&login_observer_);
 
   // Check the login state. At the time of writing, this class is instantiated
   // before login. A subsequent login would activate the profiling. However,
   // that behavior may change in the future so that the user is already logged
   // when this class is instantiated. By calling LoggedInStateChanged() here,
   // PerfProvider will recognize that the system is already logged in.
   login_observer_.LoggedInStateChanged();
 }
 
 PerfProvider::~PerfProvider() {
   chromeos::LoginState::Get()->RemoveObserver(&login_observer_);
 }
 
-bool PerfProvider::GetPerfData(std::vector<PerfDataProto>* perf_data) {
+bool PerfProvider::GetPerfData(std::vector<SampledProfile>* perf_data) {
   DCHECK(CalledOnValidThread());
   if (cached_perf_data_.empty()) {
     AddToPerfHistogram(NOT_READY_TO_UPLOAD);
     return false;
   }
 
   perf_data->swap(cached_perf_data_);
   cached_perf_data_.clear();
 
   AddToPerfHistogram(SUCCESS);
   return true;
 }
 
 PerfProvider::LoginObserver::LoginObserver(PerfProvider* perf_provider)
     : perf_provider_(perf_provider) {}
 
 void PerfProvider::LoginObserver::LoggedInStateChanged() {
   if (IsNormalUserLoggedIn())
     perf_provider_->Activate();
   else
     perf_provider_->Deactivate();
 }
 
 void PerfProvider::Activate() {

[Ilya Sherman, 2014/05/29 22:56:49]

nit: It might be wise to rename this method to something like
"OnUserDidLogIn()", now that it directly sets login_time_ internally.  As is, it
would be easy to accidentally start calling Activate() from some other trigger
as well, without realizing that it has this side-effect.

[Simon Que, 2014/05/30 01:21:45]

Done.

-  size_t collection_interval_minutes = base::RandInt(
-      kPerfCommandStartIntervalLowerBoundMinutes,
-      kPerfCommandStartIntervalUpperBoundMinutes);
-  ScheduleCollection(base::TimeDelta::FromMinutes(collection_interval_minutes));
+  login_time_ = base::TimeTicks::Now();
+  ScheduleCollection();
 }
 
 void PerfProvider::Deactivate() {
   // Stop the timer, but leave |cached_perf_data_| intact.
   timer_.Stop();
 }
 
-void PerfProvider::ScheduleCollection(const base::TimeDelta& interval) {
+void PerfProvider::ScheduleCollection() {
   DCHECK(CalledOnValidThread());
   if (timer_.IsRunning())
     return;
 
-  timer_.Start(FROM_HERE, interval, this,
-               &PerfProvider::CollectIfNecessaryAndReschedule);
+  // Pick a random time in the current interval.
+  base::TimeTicks scheduled_time =
+      next_profiling_interval_start_ +
+      base::TimeDelta::FromMilliseconds(
+          base::RandGenerator(kPerfProfilingIntervalMs));
+
+  // If the scheduled time has already passed in the time it took to make the
+  // above calculations, trigger the collection event immediately.
+  base::TimeTicks now = base::TimeTicks::Now();
+  if (scheduled_time < now) {
+    scheduled_time = now;
+  }

[Ilya Sherman, 2014/05/29 22:56:49]

nit: No need for curly braces.

[Simon Que, 2014/05/30 01:21:45]

Done.

+  timer_.Start(FROM_HERE, scheduled_time - now, this,
+               &PerfProvider::DoPeriodicCollection);
 }
 
-void PerfProvider::CollectIfNecessary() {
+void PerfProvider::CollectIfNecessary(
+    SampledProfile::TriggerEvent trigger_event) {
   DCHECK(CalledOnValidThread());
 
   // 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_perf_data_.size(); ++i) {
     cached_perf_data_size += cached_perf_data_[i].ByteSize();
   }
   if (cached_perf_data_size >= kCachedPerfDataProtobufSizeThreshold) {
     AddToPerfHistogram(NOT_READY_TO_COLLECT);
@@ skipping 12 matching lines @@
 
   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(&incognito_observer),
+                                 trigger_event));
 }
 
-void PerfProvider::CollectIfNecessaryAndReschedule() {
-  CollectIfNecessary();
-  ScheduleCollection(
-      base::TimeDelta::FromSeconds(kPerfCommandIntervalDefaultSeconds));
+void PerfProvider::DoPeriodicCollection() {
+  CollectIfNecessary(SampledProfile::PERIODIC_COLLECTION);
+
+  // Update the profiling interval tracker to the start of the next interval.
+  next_profiling_interval_start_ +=
+      base::TimeDelta::FromMilliseconds(kPerfProfilingIntervalMs);

[Ilya Sherman, 2014/05/29 22:56:49]

nit: I think it would be slightly better to reset this within
ScheduleCollection().  That way, if somebody were to accidentally call
ScheduleCollection() multiple times, it wouldn't doubly-schedule collections
during a single interval; and if the code were to be refactored to call
ScheduleCollection() from somewhere else, it wouldn't be possible to forget to
update the next_profile_interval_start_.

[Simon Que, 2014/05/30 01:21:45]

Done.

+  ScheduleCollection();
 }
 
 void PerfProvider::ParseProtoIfValid(
     scoped_ptr<WindowedIncognitoObserver> incognito_observer,
+    SampledProfile::TriggerEvent trigger_event,
     const std::vector<uint8>& data) {
   DCHECK(CalledOnValidThread());
 
   if (incognito_observer->incognito_launched()) {
     AddToPerfHistogram(INCOGNITO_LAUNCHED);
     return;
   }
 
   PerfDataProto perf_data_proto;
   if (!perf_data_proto.ParseFromArray(data.data(), data.size())) {
     AddToPerfHistogram(PROTOBUF_NOT_PARSED);
     return;
   }
 
-  // Append a new PerfDataProto to the |cached_perf_data_| vector and swap in
-  // the contents.
+  // Populate a profile collection protobuf with the collected perf data and
+  // extra metadata.
   cached_perf_data_.resize(cached_perf_data_.size() + 1);
-  cached_perf_data_.back().Swap(&perf_data_proto);
+  SampledProfile& collection_data = cached_perf_data_.back();
+  collection_data.set_trigger_event(trigger_event);
+  collection_data.set_ms_after_boot(perf_data_proto.timestamp_sec());

[Ilya Sherman, 2014/05/29 22:56:49]

You are still assigning seconds to a millisecond field.

[Simon Que, 2014/05/30 01:21:45]

Done.

+  collection_data.
+      set_ms_after_login((base::TimeTicks::Now() - login_time_)

[Ilya Sherman, 2014/05/29 22:56:49]

nit: Please add a DCHECK that login_time_ is non-null.

[Simon Que, 2014/05/30 01:21:45]

Done.

+          .InMilliseconds());
+  *collection_data.mutable_perf_data() = perf_data_proto;
 }
 
 }  // namespace metrics