metrics/base: log whether drives have seek penalties.

chrome/browser/metrics/drive_metrics_provider.cc

Index: chrome/browser/metrics/drive_metrics_provider.cc
diff --git a/chrome/browser/metrics/drive_metrics_provider.cc b/chrome/browser/metrics/drive_metrics_provider.cc
new file mode 100644
index 0000000000000000000000000000000000000000..9e460e28cb3794f99dbad6316ad94607d797cc84
--- /dev/null
+++ b/chrome/browser/metrics/drive_metrics_provider.cc
@@ -0,0 +1,77 @@
+// Copyright 2015 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 "chrome/browser/metrics/drive_metrics_provider.h"
+
+#include "base/base_paths.h"
+#include "base/bind.h"
+#include "base/files/file_path.h"
+#include "base/location.h"
+#include "base/logging.h"
+#include "base/path_service.h"
+#include "base/sys_info.h"
+#include "chrome/common/chrome_paths.h"
+#include "components/metrics/proto/system_profile.pb.h"
+#include "content/public/browser/browser_thread.h"
+
+namespace {
+
+void TestSeekPenalty(int path_service_key,

[Alexei Svitkine (slow), 2015/03/16 16:49:36]

Nit: Test* can be confusing when it doesn't relate to actual test code. How
about QuerySeekPenalty.

[Dan Beam, 2015/03/16 19:20:23]

Done.

+                     DriveMetricsProvider::SeekPenaltyResponse* resp) {
+  DCHECK(resp);
+
+  base::FilePath path;
+  if (!PathService::Get(path_service_key, &path))
+    return;
+
+  resp->success = base::SysInfo::HasSeekPenalty(path, &resp->has_seek_penalty);

[Alexei Svitkine (slow), 2015/03/16 16:49:36]

Nit: resp -> response or result.

[Dan Beam, 2015/03/16 19:20:23]

Done.

+}
+
+DriveMetricsProvider::DriveMetrics GetDriveMetrics() {
+  DCHECK_CURRENTLY_ON(content::BrowserThread::FILE);
+
+  DriveMetricsProvider::DriveMetrics metrics;
+  TestSeekPenalty(base::FILE_EXE, &metrics.app_drive);
+  TestSeekPenalty(chrome::FILE_LOCAL_STATE, &metrics.user_data_drive);
+  return metrics;
+}
+
+void FillDriveMetrics(metrics::SystemProfileProto::Hardware::Drive* drive,

[Alexei Svitkine (slow), 2015/03/16 16:49:36]

Please add a short comment above each of the methods in the anon namespace.

[Dan Beam, 2015/03/16 19:20:23]

Done. (in the .h)

+                      const DriveMetricsProvider::SeekPenaltyResponse& resp) {

[Alexei Svitkine (slow), 2015/03/16 16:49:36]

Nit: Const arguments should come before non-const ones.

Also, prefer full words, so |resp| -> |response|.

[Dan Beam, 2015/03/16 19:20:23]

Done.

+  if (resp.success)
+    drive->set_has_seek_penalty(resp.has_seek_penalty);
+}
+
+}  // namespace
+
+DriveMetricsProvider::DriveMetricsProvider()
+    : got_metrics_(false), weak_ptr_factory_(this) {}
+
+DriveMetricsProvider::~DriveMetricsProvider() {}
+
+void DriveMetricsProvider::ProvideSystemProfileMetrics(
+    metrics::SystemProfileProto* system_profile_proto) {
+  if (!got_metrics_) {

[Alexei Svitkine (slow), 2015/03/16 16:49:36]

This will make it so the first log will not have this info.

We actually have some other places that need to query some system info on a
background thread to make it available for the first log. We do this in
StartGatheringMetrics in ChromeMetricsServiceClient:

https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/met...

Right now, it runs a bunch of different things in sequence (e.g. get hardware
class, get plugin info, get google update data, get profiler data). You can add
it to that chain.

Though, ideally we can start doing these in parallel - but probably better to do
that in a separate change.

[Dan Beam, 2015/03/16 19:20:23]

rvargas@ mentioned we should gather these metrics later:
https://code.google.com/p/chromium/issues/detail?id=463209#c13
https://codereview.chromium.org/999623002/#msg10

this is where I slapped them for now.  let me know if you have a preferred
delay.

I don't think trying to get them before the first log is necessary or even
desired.

[Alexei Svitkine (slow), 2015/03/16 19:29:07]

I think it would be terribly confusing if some UMA logs had this set and others
didn't. For example, we definitely want this in the first metrics log - since
that's the log that's likely going to have the start up metrics we're analyzing.

Putting it in the initial task seems like the best, since that pattern already
exists. Otherwise, I'm open to doing it in some other way that still makes sure
it happens before PrepareInitialMetricsLog() is called in metrics_service.cc.

[Dan Beam, 2015/03/16 22:25:01]

Done. (see updated code)

+    content::BrowserThread::PostTaskAndReplyWithResult(
+      content::BrowserThread::FILE, FROM_HERE,
+      base::Bind(&GetDriveMetrics),
+      base::Bind(&DriveMetricsProvider::GotDriveMetrics,
+                 weak_ptr_factory_.GetWeakPtr()));
+    return;
+  }
+
+  auto* hardware = system_profile_proto->mutable_hardware();
+  FillDriveMetrics(hardware->mutable_app_drive(), metrics_.app_drive);
+  FillDriveMetrics(hardware->mutable_user_data_drive(),
+                   metrics_.user_data_drive);
+}
+
+DriveMetricsProvider::SeekPenaltyResponse::SeekPenaltyResponse()
+    : success(false) {}
+
+void DriveMetricsProvider::GotDriveMetrics(const DriveMetrics& metrics) {
+  DCHECK(thread_checker_.CalledOnValidThread());
+  got_metrics_ = true;
+  metrics_ = metrics;
+}