Chromium Code Reviews
|
|
Chromium Code Reviews|
Created:
5 years, 9 months ago by Dan Beam Modified:
5 years, 9 months ago Reviewers:
rvargas (doing something else), Alexei Svitkine (slow) CC:
chromium-reviews, erikwright+watch_chromium.org, asvitkine+watch_chromium.org, Lei Zhang Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Descriptionmetrics/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}
Patch Set 1 #Patch Set 2 : nit #Patch Set 3 : FILE thread #Patch Set 4 : nits #
Total comments: 30
Patch Set 5 : slightly after rush hour #
Total comments: 8
Patch Set 6 : whoops #Patch Set 7 : base_unittests #Patch Set 8 : OS_WIN #Patch Set 9 : indent #
Total comments: 18
Patch Set 10 : asvitkine@ review #
Total comments: 1
Patch Set 11 : asvitkine@ review #
Total comments: 3
Patch Set 12 : defer differently #Patch Set 13 : remove debugging #Patch Set 14 : restore review-worthy code #Patch Set 15 : GetLastError #Patch Set 16 : remove unit test #Patch Set 17 : add back crash test #Patch Set 18 : DONT_ASSERT_ANYTHING #
Created: 5 years, 9 months ago
Messages
Total messages: 28 (11 generated)
dbeam@chromium.org changed reviewers: + rvargas@chromium.org - mark@chromium.org
rvargas@ for base/ (mark@ mentioned you might be a good reviewer for windows-y code) mpearson@ for metrics/
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
asvitkine@chromium.org changed reviewers: + asvitkine@chromium.org
I can review the metrics bits.
dbeam@chromium.org changed reviewers: - mpearson@chromium.org
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.
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).
Patchset #5 (id:80001) has been deleted
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.
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.
Patchset #6 (id:120001) has been deleted
Patchset #6 (id:140001) has been deleted
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.
base lgtm on ps9.
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.
Patchset #10 (id:230001) has been deleted
Patchset #10 (id:250001) has been deleted
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
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.
Patchset #12 (id:310001) has been deleted
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.
Thanks, LGTM! Please wait for the internal protobuf review CL to be submitted before committing.
The CQ bit was checked by dbeam@chromium.org
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")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/999623002/450001
Message was sent while issue was closed.
Committed patchset #18 (id:450001)
Message was sent while issue was closed.
Patchset 18 (id:??) landed as https://crrev.com/64c37847c51804dc237875e691c8ff95b6d25edd Cr-Commit-Position: refs/heads/master@{#321056} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
