metrics/base: log whether drives have seek penalties.

metrics/base: log whether drives have seek penalties.

Only on Win 7+ for now.

R=rvargas@chromium.org,mpearson@chromium.org
BUG=463209
Committed: https://crrev.com/64c37847c51804dc237875e691c8ff95b6d25edd
Cr-Commit-Position: refs/heads/master@{#321056}

[Dan Beam, 2015-03-11 23:40:11]

dbeam@chromium.org changed reviewers:
+ rvargas@chromium.org
- mark@chromium.org

[Dan Beam, 2015-03-12 01:26:43]

rvargas@ for base/ (mark@ mentioned you might be a good reviewer for windows-y
code)

mpearson@ for metrics/

[Mark P, 2015-03-12 16:30:01]

On 2015/03/12 01:26:43, Dan Beam wrote:
> rvargas@ for base/ (mark@ mentioned you might be a good reviewer for windows-y
> code)
> 
> mpearson@ for metrics/

I don't generally work with MetricsProviders.  Can you find another metrics
OWNER such as asvitkine?  If you can't find someone else, I'll do this review.

--mark

[Alexei Svitkine (slow), 2015-03-12 16:31:27]

asvitkine@chromium.org changed reviewers:
+ asvitkine@chromium.org

[Alexei Svitkine (slow), 2015-03-12 16:31:27]

I can review the metrics bits.

[Dan Beam, 2015-03-12 16:38:05]

dbeam@chromium.org changed reviewers:
- mpearson@chromium.org

[Alexei Svitkine (slow), 2015-03-12 16:41:07]

https://codereview.chromium.org/999623002/diff/60001/chrome/browser/metrics/d...
File chrome/browser/metrics/disk_metrics_provider.cc (right):

https://codereview.chromium.org/999623002/diff/60001/chrome/browser/metrics/d...
chrome/browser/metrics/disk_metrics_provider.cc:43:
content::BrowserThread::FILE, FROM_HERE,
Any reason to use FILE thread explicitly rather than blocking pool?

Note: If you move this to the component, you can pass the task runner to the
ctor.

https://codereview.chromium.org/999623002/diff/60001/chrome/browser/metrics/d...
chrome/browser/metrics/disk_metrics_provider.cc:53: if
(metrics_.app_disk.success) {
Nit: You can make a helper that takes metrics_.app_disk and 
system_profile_proto->mutable_hardware()->mutable_app_disk() and then just call
it twice for the two disks.

https://codereview.chromium.org/999623002/diff/60001/chrome/browser/metrics/d...
File chrome/browser/metrics/disk_metrics_provider.h (right):

https://codereview.chromium.org/999623002/diff/60001/chrome/browser/metrics/d...
chrome/browser/metrics/disk_metrics_provider.h:5: #ifndef
CHROME_BROWSER_METRICS_DISK_METRICS_PROVIDER_H_
Can this class live in the component? Since it just depends on base.

https://codereview.chromium.org/999623002/diff/60001/chrome/browser/metrics/d...
chrome/browser/metrics/disk_metrics_provider.h:19: struct DiskMetrics {
Can these be declared inside the class? That way, we're not polluting the global
namespace.

https://codereview.chromium.org/999623002/diff/60001/chrome/browser/metrics/d...
chrome/browser/metrics/disk_metrics_provider.h:36: void GotDiskMetrics(const
DiskMetrics& metrics);
Add a comment.

https://codereview.chromium.org/999623002/diff/60001/components/metrics/proto...
File components/metrics/proto/system_profile.proto (right):

https://codereview.chromium.org/999623002/diff/60001/components/metrics/proto...
components/metrics/proto/system_profile.proto:265: message Disk {
Is Disk the right terminology here or should we use "Drive"? Not sure what the
difference is, just want to double check that we're using the more correct one -
whichever it is.

https://codereview.chromium.org/999623002/diff/60001/components/metrics/proto...
components/metrics/proto/system_profile.proto:268: // The disk that Chrome loads
from.
Nit: "Chrome" -> "the application". (as this proto can be used by other products
potentially.

"loads" -> "was loaded"

Maybe even say "the application executable" to be clear about what this actually
means, since it's different than --user-data-dir below.

[rvargas (doing something else), 2015-03-12 22:01:12]

https://codereview.chromium.org/999623002/diff/60001/base/sys_info.cc
File base/sys_info.cc (right):

https://codereview.chromium.org/999623002/diff/60001/base/sys_info.cc#newcode56
base/sys_info.cc:56: #if !defined(OS_WIN)
Given that this would have to be modified every time a new platform comes to
life, it seems better to pay the price and copy this to all unsupported
platforms.

https://codereview.chromium.org/999623002/diff/60001/base/sys_info.h
File base/sys_info.h (right):

https://codereview.chromium.org/999623002/diff/60001/base/sys_info.h#newcode53
base/sys_info.h:53: // |has_seek_penalty| is NULL.
The second condition sounds like a DCHECK to me.

https://codereview.chromium.org/999623002/diff/60001/base/sys_info_win.cc
File base/sys_info_win.cc (right):

https://codereview.chromium.org/999623002/diff/60001/base/sys_info_win.cc#new...
base/sys_info_win.cc:73: if (!path.IsAbsolute() || !has_seek_penalty)
I would probably also DCHECK the first condition given that it is a precondition
and not something that the caller cannot control.

https://codereview.chromium.org/999623002/diff/60001/base/sys_info_win.cc#new...
base/sys_info_win.cc:79: FilePath::StringType drive = L"\\\\.\\" +
components[0];
This syntax is not supported before win7

https://codereview.chromium.org/999623002/diff/60001/base/sys_info_win.cc#new...
base/sys_info_win.cc:80: win::ScopedHandle handle(CreateFile(
Looks like a job for base::File

https://codereview.chromium.org/999623002/diff/60001/base/sys_info_win.cc#new...
base/sys_info_win.cc:94: query.PropertyId = StorageDeviceSeekPenaltyProperty;
Not supported on XP

https://codereview.chromium.org/999623002/diff/60001/base/sys_info_win.cc#new...
base/sys_info_win.cc:103: &unused_bytes_returned,
Verify that this is >= sizeof(result) before accessing the result.

https://codereview.chromium.org/999623002/diff/60001/base/sys_info_win.cc#new...
base/sys_info_win.cc:108: *has_seek_penalty = !!result.IncursSeekPenalty;
nit: != FALSE? (I hate using "!!" to do this).

[Dan Beam, 2015-03-13 19:27:40]

Patchset #5 (id:80001) has been deleted

[Dan Beam, 2015-03-13 19:31:32]

let me know how long after "rush hour" we should collect these metrics so I can
delay accordingly.

https://codereview.chromium.org/999623002/diff/60001/base/sys_info.cc
File base/sys_info.cc (right):

https://codereview.chromium.org/999623002/diff/60001/base/sys_info.cc#newcode56
base/sys_info.cc:56: #if !defined(OS_WIN)
On 2015/03/12 22:01:11, rvargas wrote:
> Given that this would have to be modified every time a new platform comes to
> life, it seems better to pay the price and copy this to all unsupported
> platforms.

put into _posix.cc for now, which originally was because I had hoped that i'd
only have to do this once for all posix systems but I'm pretty certain that's
wrong :(

https://codereview.chromium.org/999623002/diff/60001/base/sys_info.h
File base/sys_info.h (right):

https://codereview.chromium.org/999623002/diff/60001/base/sys_info.h#newcode53
base/sys_info.h:53: // |has_seek_penalty| is NULL.
On 2015/03/12 22:01:11, rvargas wrote:
> The second condition sounds like a DCHECK to me.

Done.

https://codereview.chromium.org/999623002/diff/60001/base/sys_info_win.cc
File base/sys_info_win.cc (right):

https://codereview.chromium.org/999623002/diff/60001/base/sys_info_win.cc#new...
base/sys_info_win.cc:73: if (!path.IsAbsolute() || !has_seek_penalty)
On 2015/03/12 22:01:12, rvargas wrote:
> I would probably also DCHECK the first condition given that it is a
precondition
> and not something that the caller cannot control.

Done.

https://codereview.chromium.org/999623002/diff/60001/base/sys_info_win.cc#new...
base/sys_info_win.cc:79: FilePath::StringType drive = L"\\\\.\\" +
components[0];
On 2015/03/12 22:01:12, rvargas wrote:
> This syntax is not supported before win7

Acknowledged.

https://codereview.chromium.org/999623002/diff/60001/base/sys_info_win.cc#new...
base/sys_info_win.cc:80: win::ScopedHandle handle(CreateFile(
On 2015/03/12 22:01:12, rvargas wrote:
> Looks like a job for base::File

Done.

https://codereview.chromium.org/999623002/diff/60001/base/sys_info_win.cc#new...
base/sys_info_win.cc:94: query.PropertyId = StorageDeviceSeekPenaltyProperty;
On 2015/03/12 22:01:11, rvargas wrote:
> Not supported on XP

Acknowledged.

https://codereview.chromium.org/999623002/diff/60001/base/sys_info_win.cc#new...
base/sys_info_win.cc:103: &unused_bytes_returned,
On 2015/03/12 22:01:12, rvargas wrote:
> Verify that this is >= sizeof(result) before accessing the result.

Done.

https://codereview.chromium.org/999623002/diff/60001/base/sys_info_win.cc#new...
base/sys_info_win.cc:108: *has_seek_penalty = !!result.IncursSeekPenalty;
On 2015/03/12 22:01:12, rvargas wrote:
> nit: != FALSE? (I hate using "!!" to do this).

why not == TRUE?

https://codereview.chromium.org/999623002/diff/60001/chrome/browser/metrics/d...
File chrome/browser/metrics/disk_metrics_provider.cc (right):

https://codereview.chromium.org/999623002/diff/60001/chrome/browser/metrics/d...
chrome/browser/metrics/disk_metrics_provider.cc:43:
content::BrowserThread::FILE, FROM_HERE,
On 2015/03/12 16:41:06, Alexei Svitkine (slow) wrote:
> Any reason to use FILE thread explicitly rather than blocking pool?
> 
> Note: If you move this to the component, you can pass the task runner to the
> ctor.

can't be in components/ AFAIU because it depends on chrome/*
(common/chrome_paths.h)

https://codereview.chromium.org/999623002/diff/60001/chrome/browser/metrics/d...
chrome/browser/metrics/disk_metrics_provider.cc:53: if
(metrics_.app_disk.success) {
On 2015/03/12 16:41:06, Alexei Svitkine (slow) wrote:
> Nit: You can make a helper that takes metrics_.app_disk and 
> system_profile_proto->mutable_hardware()->mutable_app_disk() and then just
call
> it twice for the two disks.

Done.

https://codereview.chromium.org/999623002/diff/60001/chrome/browser/metrics/d...
File chrome/browser/metrics/disk_metrics_provider.h (right):

https://codereview.chromium.org/999623002/diff/60001/chrome/browser/metrics/d...
chrome/browser/metrics/disk_metrics_provider.h:5: #ifndef
CHROME_BROWSER_METRICS_DISK_METRICS_PROVIDER_H_
On 2015/03/12 16:41:06, Alexei Svitkine (slow) wrote:
> Can this class live in the component? Since it just depends on base.

look at the cc dependencies

https://codereview.chromium.org/999623002/diff/60001/chrome/browser/metrics/d...
chrome/browser/metrics/disk_metrics_provider.h:19: struct DiskMetrics {
On 2015/03/12 16:41:06, Alexei Svitkine (slow) wrote:
> Can these be declared inside the class? That way, we're not polluting the
global
> namespace.

yes, Done.

https://codereview.chromium.org/999623002/diff/60001/chrome/browser/metrics/d...
chrome/browser/metrics/disk_metrics_provider.h:36: void GotDiskMetrics(const
DiskMetrics& metrics);
On 2015/03/12 16:41:06, Alexei Svitkine (slow) wrote:
> Add a comment.

Done.

https://codereview.chromium.org/999623002/diff/60001/components/metrics/proto...
File components/metrics/proto/system_profile.proto (right):

https://codereview.chromium.org/999623002/diff/60001/components/metrics/proto...
components/metrics/proto/system_profile.proto:265: message Disk {
On 2015/03/12 16:41:06, Alexei Svitkine (slow) wrote:
> Is Disk the right terminology here or should we use "Drive"? Not sure what the
> difference is, just want to double check that we're using the more correct one
-
> whichever it is.

I originally did this to avoid confusion with Google Drive.

I guess disk implies it spins.

s/disk/drive/g

https://codereview.chromium.org/999623002/diff/60001/components/metrics/proto...
components/metrics/proto/system_profile.proto:268: // The disk that Chrome loads
from.
On 2015/03/12 16:41:06, Alexei Svitkine (slow) wrote:
> Nit: "Chrome" -> "the application". (as this proto can be used by other
products
> potentially.
> 
> "loads" -> "was loaded"
> 
> Maybe even say "the application executable" to be clear about what this
actually
> means, since it's different than --user-data-dir below.

Done.

[rvargas (doing something else), 2015-03-13 21:03:09]

https://codereview.chromium.org/999623002/diff/100001/base/sys_info_win.cc
File base/sys_info_win.cc (right):

https://codereview.chromium.org/999623002/diff/100001/base/sys_info_win.cc#ne...
base/sys_info_win.cc:74: if (!path.IsAbsolute() || win::GetVersion() <
win::VERSION_WIN7)
I have not heard why allowing callers to use relative paths is a good thing :)

https://codereview.chromium.org/999623002/diff/100001/base/sys_info_win.cc#ne...
base/sys_info_win.cc:79: File drive(FilePath(L"\\\\.\\" + components[0]),
File::FLAG_OPEN);
Still requires if (!drive.IsValid())...

https://codereview.chromium.org/999623002/diff/100001/base/sys_info_win.cc#ne...
base/sys_info_win.cc:91: &bytes_returned,
not defined?

https://codereview.chromium.org/999623002/diff/100001/base/sys_info_win.cc#ne...
base/sys_info_win.cc:96: *has_seek_penalty = result.IncursSeekPenalty == TRUE;
Comparing against TRUE is not a good idea (in general) because TRUE == 1 and
some functions can return another value even though it is supposed to be a bool.

[Dan Beam, 2015-03-13 21:12:23]

Patchset #6 (id:120001) has been deleted

[Dan Beam, 2015-03-13 21:13:58]

Patchset #6 (id:140001) has been deleted

[Dan Beam, 2015-03-13 21:15:51]

https://codereview.chromium.org/999623002/diff/100001/base/sys_info_win.cc
File base/sys_info_win.cc (right):

https://codereview.chromium.org/999623002/diff/100001/base/sys_info_win.cc#ne...
base/sys_info_win.cc:74: if (!path.IsAbsolute() || win::GetVersion() <
win::VERSION_WIN7)
On 2015/03/13 21:03:09, rvargas wrote:
> I have not heard why allowing callers to use relative paths is a good thing :)

Done. (changed to DCHECK)

https://codereview.chromium.org/999623002/diff/100001/base/sys_info_win.cc#ne...
base/sys_info_win.cc:79: File drive(FilePath(L"\\\\.\\" + components[0]),
File::FLAG_OPEN);
On 2015/03/13 21:03:09, rvargas wrote:
> Still requires if (!drive.IsValid())...

Done.

https://codereview.chromium.org/999623002/diff/100001/base/sys_info_win.cc#ne...
base/sys_info_win.cc:91: &bytes_returned,
On 2015/03/13 21:03:09, rvargas wrote:
> not defined?

whoops, Done.

https://codereview.chromium.org/999623002/diff/100001/base/sys_info_win.cc#ne...
base/sys_info_win.cc:96: *has_seek_penalty = result.IncursSeekPenalty == TRUE;
On 2015/03/13 21:03:09, rvargas wrote:
> Comparing against TRUE is not a good idea (in general) because TRUE == 1 and
> some functions can return another value even though it is supposed to be a
bool.

Done.

[rvargas (doing something else), 2015-03-14 00:51:06]

base lgtm on ps9.

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

https://codereview.chromium.org/999623002/diff/220001/chrome/browser/metrics/...
File chrome/browser/metrics/drive_metrics_provider.cc (right):

https://codereview.chromium.org/999623002/diff/220001/chrome/browser/metrics/...
chrome/browser/metrics/drive_metrics_provider.cc:20: void TestSeekPenalty(int
path_service_key,
Nit: Test* can be confusing when it doesn't relate to actual test code. How
about QuerySeekPenalty.

https://codereview.chromium.org/999623002/diff/220001/chrome/browser/metrics/...
chrome/browser/metrics/drive_metrics_provider.cc:28: resp->success =
base::SysInfo::HasSeekPenalty(path, &resp->has_seek_penalty);
Nit: resp -> response or result.

https://codereview.chromium.org/999623002/diff/220001/chrome/browser/metrics/...
chrome/browser/metrics/drive_metrics_provider.cc:40: void
FillDriveMetrics(metrics::SystemProfileProto::Hardware::Drive* drive,
Please add a short comment above each of the methods in the anon namespace.

https://codereview.chromium.org/999623002/diff/220001/chrome/browser/metrics/...
chrome/browser/metrics/drive_metrics_provider.cc:41: const
DriveMetricsProvider::SeekPenaltyResponse& resp) {
Nit: Const arguments should come before non-const ones.

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

https://codereview.chromium.org/999623002/diff/220001/chrome/browser/metrics/...
chrome/browser/metrics/drive_metrics_provider.cc:55: if (!got_metrics_) {
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.

https://codereview.chromium.org/999623002/diff/220001/chrome/browser/metrics/...
File chrome/browser/metrics/drive_metrics_provider.h (right):

https://codereview.chromium.org/999623002/diff/220001/chrome/browser/metrics/...
chrome/browser/metrics/drive_metrics_provider.h:27: struct SeekPenaltyResponse {
Move these to the private section.

https://codereview.chromium.org/999623002/diff/220001/components/metrics/prot...
File components/metrics/proto/system_profile.proto (right):

https://codereview.chromium.org/999623002/diff/220001/components/metrics/prot...
components/metrics/proto/system_profile.proto:266: optional bool
has_seek_penalty = 1;
This definitely needs a comment.

https://codereview.chromium.org/999623002/diff/220001/components/metrics/prot...
components/metrics/proto/system_profile.proto:270: // The drive that the current
--user-data-dir was loaded from.
Nit: I would actually just say "the user data directory" rather than mention the
command-line parameter.

[Dan Beam, 2015-03-16 18:41:03]

Patchset #10 (id:230001) has been deleted

[Dan Beam, 2015-03-16 19:04:24]

Patchset #10 (id:250001) has been deleted

[Dan Beam, 2015-03-16 19:20:24]

https://codereview.chromium.org/999623002/diff/220001/chrome/browser/metrics/...
File chrome/browser/metrics/drive_metrics_provider.cc (right):

https://codereview.chromium.org/999623002/diff/220001/chrome/browser/metrics/...
chrome/browser/metrics/drive_metrics_provider.cc:20: void TestSeekPenalty(int
path_service_key,
On 2015/03/16 16:49:36, Alexei Svitkine (slow) wrote:
> Nit: Test* can be confusing when it doesn't relate to actual test code. How
> about QuerySeekPenalty.

Done.

https://codereview.chromium.org/999623002/diff/220001/chrome/browser/metrics/...
chrome/browser/metrics/drive_metrics_provider.cc:28: resp->success =
base::SysInfo::HasSeekPenalty(path, &resp->has_seek_penalty);
On 2015/03/16 16:49:36, Alexei Svitkine (slow) wrote:
> Nit: resp -> response or result.

Done.

https://codereview.chromium.org/999623002/diff/220001/chrome/browser/metrics/...
chrome/browser/metrics/drive_metrics_provider.cc:40: void
FillDriveMetrics(metrics::SystemProfileProto::Hardware::Drive* drive,
On 2015/03/16 16:49:36, Alexei Svitkine (slow) wrote:
> Please add a short comment above each of the methods in the anon namespace.

Done. (in the .h)

https://codereview.chromium.org/999623002/diff/220001/chrome/browser/metrics/...
chrome/browser/metrics/drive_metrics_provider.cc:41: const
DriveMetricsProvider::SeekPenaltyResponse& resp) {
On 2015/03/16 16:49:36, Alexei Svitkine (slow) wrote:
> Nit: Const arguments should come before non-const ones.
> 
> Also, prefer full words, so |resp| -> |response|.

Done.

https://codereview.chromium.org/999623002/diff/220001/chrome/browser/metrics/...
chrome/browser/metrics/drive_metrics_provider.cc:55: if (!got_metrics_) {
On 2015/03/16 16:49:36, Alexei Svitkine (slow) wrote:
> 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.

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.

https://codereview.chromium.org/999623002/diff/220001/chrome/browser/metrics/...
File chrome/browser/metrics/drive_metrics_provider.h (right):

https://codereview.chromium.org/999623002/diff/220001/chrome/browser/metrics/...
chrome/browser/metrics/drive_metrics_provider.h:27: struct SeekPenaltyResponse {
On 2015/03/16 16:49:36, Alexei Svitkine (slow) wrote:
> Move these to the private section.

they were used in the anonymous namespace, so had to be public.  moved to
private: and changed anon namespace funcs to private static/instance methods.

https://codereview.chromium.org/999623002/diff/220001/components/metrics/prot...
File components/metrics/proto/system_profile.proto (right):

https://codereview.chromium.org/999623002/diff/220001/components/metrics/prot...
components/metrics/proto/system_profile.proto:266: optional bool
has_seek_penalty = 1;
On 2015/03/16 16:49:36, Alexei Svitkine (slow) wrote:
> This definitely needs a comment.

Done.

https://codereview.chromium.org/999623002/diff/220001/components/metrics/prot...
components/metrics/proto/system_profile.proto:270: // The drive that the current
--user-data-dir was loaded from.
On 2015/03/16 16:49:36, Alexei Svitkine (slow) wrote:
> Nit: I would actually just say "the user data directory" rather than mention
the
> command-line parameter.

Done.

https://codereview.chromium.org/999623002/diff/270001/chrome/browser/metrics/...
File chrome/browser/metrics/drive_metrics_provider.cc (right):

https://codereview.chromium.org/999623002/diff/270001/chrome/browser/metrics/...
chrome/browser/metrics/drive_metrics_provider.cc:28:
weak_ptr_factory_.GetWeakPtr()),
this didn't work

https://codereview.chromium.org/999623002/diff/290001/chrome/browser/metrics/...
File chrome/browser/metrics/drive_metrics_provider.cc (right):

https://codereview.chromium.org/999623002/diff/290001/chrome/browser/metrics/...
chrome/browser/metrics/drive_metrics_provider.cc:27:
base::Bind(&DriveMetricsProvider::GetDriveMetrics),
this does work

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

https://codereview.chromium.org/999623002/diff/220001/chrome/browser/metrics/...
File chrome/browser/metrics/drive_metrics_provider.cc (right):

https://codereview.chromium.org/999623002/diff/220001/chrome/browser/metrics/...
chrome/browser/metrics/drive_metrics_provider.cc:55: if (!got_metrics_) {
On 2015/03/16 19:20:23, Dan Beam wrote:
> On 2015/03/16 16:49:36, Alexei Svitkine (slow) wrote:
> > 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.
> 
> 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.

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.

https://codereview.chromium.org/999623002/diff/290001/components/metrics/prot...
File components/metrics/proto/system_profile.proto (right):

https://codereview.chromium.org/999623002/diff/290001/components/metrics/prot...
components/metrics/proto/system_profile.proto:266: // Whether this drive incurs
a time penalty when randomly accessed.
I would expand this to mention the expected value for SSDs vs. spinning disk,
just to make this clear.

[Dan Beam, 2015-03-16 22:23:14]

Patchset #12 (id:310001) has been deleted

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

https://codereview.chromium.org/999623002/diff/220001/chrome/browser/metrics/...
File chrome/browser/metrics/drive_metrics_provider.cc (right):

https://codereview.chromium.org/999623002/diff/220001/chrome/browser/metrics/...
chrome/browser/metrics/drive_metrics_provider.cc:55: if (!got_metrics_) {
On 2015/03/16 19:29:07, Alexei Svitkine (slow) wrote:
> On 2015/03/16 19:20:23, Dan Beam wrote:
> > On 2015/03/16 16:49:36, Alexei Svitkine (slow) wrote:
> > > 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.
> > 
> > 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.
> 
> 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.

Done. (see updated code)

https://codereview.chromium.org/999623002/diff/290001/components/metrics/prot...
File components/metrics/proto/system_profile.proto (right):

https://codereview.chromium.org/999623002/diff/290001/components/metrics/prot...
components/metrics/proto/system_profile.proto:266: // Whether this drive incurs
a time penalty when randomly accessed.
On 2015/03/16 19:29:07, Alexei Svitkine (slow) wrote:
> I would expand this to mention the expected value for SSDs vs. spinning disk,
> just to make this clear.

Done.

[Alexei Svitkine (slow), 2015-03-17 00:15:47]

Thanks, LGTM!

Please wait for the internal protobuf review CL to be submitted before
committing.

[Dan Beam, 2015-03-17 23:46:06]

The CQ bit was checked by dbeam@chromium.org

[Dan Beam, 2015-03-17 23:46:07]

The patchset sent to the CQ was uploaded after l-g-t-m from
rvargas@chromium.org, asvitkine@chromium.org
Link to the patchset: https://codereview.chromium.org/999623002/#ps450001
(title: "DONT_ASSERT_ANYTHING")

[commit-bot: I haz the power, 2015-03-17 23:47:23]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/999623002/450001

[commit-bot: I haz the power, 2015-03-18 02:23:41]

Committed patchset #18 (id:450001)

[commit-bot: I haz the power, 2015-03-18 02:24:24]

Patchset 18 (id:??) landed as
https://crrev.com/64c37847c51804dc237875e691c8ff95b6d25edd
Cr-Commit-Position: refs/heads/master@{#321056}