Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2938263002:
Put BrowserMetrics with embedded profiles into subdir for auto-upload. (Closed)
|
Created:
3 years, 6 months ago by bcwhite Modified:
3 years, 5 months ago Reviewers:
Alexei Svitkine (slow) CC:
chromium-reviews, danakj+watch_chromium.org, asvitkine+watch_chromium.org, vmpstr+watch_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionPut BrowserMetrics with embedded profiles into subdir for auto-upload.
Because the BrowserMetrics file now has an embedded profile, it isn't
sent as part of the startup sequence but a minute or so after. That
means the browser could exit and not send anything with the "previous
run" metrics file being overwritten at the next startup.
Instead, append a date-stamp to the last-run filename and move it to
a subdirectory from which files will be removed only after they are
uploaded.
BUG=695880
Review-Url: https://codereview.chromium.org/2938263002
Cr-Commit-Position: refs/heads/master@{#484116}
Committed: https://chromium.googlesource.com/chromium/src/+/38d8bb9328a2d0a1b7d8373ddd70971c1ce4eeb6
Patch Set 1 #Patch Set 2 : fix build problems on other architectures #Patch Set 3 : use file names instead of modified stamp to check if already done #Patch Set 4 : use BrowserMetrics name as directory name so timestamps will always advance #Patch Set 5 : better handling of incorrect files #Patch Set 6 : fixed build problems on other architectures #Patch Set 7 : rebased #
Total comments: 11
Patch Set 8 : addressed review comments by asvitkine #
Total comments: 4
Patch Set 9 : use std::string::append() where possible #
Messages
Total messages: 67 (58 generated)
The CQ bit was checked by bcwhite@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...) ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
The CQ bit was checked by bcwhite@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...) ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)
Patchset #2 (id:20001) has been deleted
The CQ bit was checked by bcwhite@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by bcwhite@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Patchset #2 (id:40001) has been deleted
The CQ bit was checked by bcwhite@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Patchset #2 (id:60001) has been deleted
Description was changed from ========== Put BrowserMetrics with embedded profiles into subdir for auto-upload. BUG=695880 ========== to ========== Put BrowserMetrics with embedded profiles into subdir for auto-upload. Because the BrowserMetrics file now has an embedded profile, it isn't sent as part of the startup sequence but a minute or so after. That means the browser could exit and not send anything with the "previous run" metrics file being overwritten at the next startup. Instead, append a date-stamp to the last-run filename and move it to a subdirectory from which files will be removed only after they are uploaded. BUG=695880 ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by bcwhite@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by bcwhite@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by bcwhite@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
The CQ bit was checked by bcwhite@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
The CQ bit was checked by bcwhite@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Patchset #6 (id:160001) has been deleted
bcwhite@chromium.org changed reviewers: + asvitkine@chromium.org
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by bcwhite@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm https://codereview.chromium.org/2938263002/diff/200001/base/metrics/persisten... File base/metrics/persistent_histogram_allocator.cc (right): https://codereview.chromium.org/2938263002/diff/200001/base/metrics/persisten... base/metrics/persistent_histogram_allocator.cc:853: .AddExtension(PersistentMemoryAllocator::kFileExtension); Nit: Given this pattern is used a lot, make a helper? MakePersistentMetricsFilePath(dir, name) So then all of these can be more concise. And update the ones above too. https://codereview.chromium.org/2938263002/diff/200001/base/metrics/persisten... File base/metrics/persistent_histogram_allocator.h (right): https://codereview.chromium.org/2938263002/diff/200001/base/metrics/persisten... base/metrics/persistent_histogram_allocator.h:443: const std::string& name, StringPiece especially since you pass in kBrowserMetricsName which isn't a std::string. https://codereview.chromium.org/2938263002/diff/200001/chrome/browser/chrome_... File chrome/browser/chrome_browser_field_trials.cc (right): https://codereview.chromium.org/2938263002/diff/200001/chrome/browser/chrome_... chrome/browser/chrome_browser_field_trials.cc:58: // file and make kBrowserMetricsName local to that file. Friendly ping on this! (Separate CL, but please let's get this done as otherwise this just keeps growing and growing...) https://codereview.chromium.org/2938263002/diff/200001/chrome/browser/chrome_... chrome/browser/chrome_browser_field_trials.cc:88: metrics_dir, upload_dir, ChromeMetricsServiceClient::kBrowserMetricsName, Can you add a comment about expected file locations here? i.e. user-data-dir/BrowserMetrics/BrowserMetrics-active.pma etc. https://codereview.chromium.org/2938263002/diff/200001/components/metrics/fil... File components/metrics/file_metrics_provider_unittest.cc (right): https://codereview.chromium.org/2938263002/diff/200001/components/metrics/fil... components/metrics/file_metrics_provider_unittest.cc:650: for (auto& file_name : file_names) const auto&
The CQ bit was checked by bcwhite@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2938263002/diff/200001/base/metrics/persisten... File base/metrics/persistent_histogram_allocator.cc (right): https://codereview.chromium.org/2938263002/diff/200001/base/metrics/persisten... base/metrics/persistent_histogram_allocator.cc:853: .AddExtension(PersistentMemoryAllocator::kFileExtension); On 2017/06/28 17:30:35, Alexei Svitkine (slow) wrote: > Nit: Given this pattern is used a lot, make a helper? > > MakePersistentMetricsFilePath(dir, name) > > So then all of these can be more concise. And update the ones above too. Done. https://codereview.chromium.org/2938263002/diff/200001/base/metrics/persisten... File base/metrics/persistent_histogram_allocator.h (right): https://codereview.chromium.org/2938263002/diff/200001/base/metrics/persisten... base/metrics/persistent_histogram_allocator.h:443: const std::string& name, On 2017/06/28 17:30:35, Alexei Svitkine (slow) wrote: > StringPiece especially since you pass in kBrowserMetricsName which isn't a > std::string. std::string takes a char* no problem. Inside, though, all conditions lead to the parameter needing to be a std::string so it's better to take it that way in the parameter list and have it converted at most once, possibly never if the caller already has it that way. In the other ConstructFilePaths, the most common case doesn't need a std::string so, combined with most callers having the name as a StringPiece, it's more efficient to have the parameter being a StringPiece. https://codereview.chromium.org/2938263002/diff/200001/chrome/browser/chrome_... File chrome/browser/chrome_browser_field_trials.cc (right): https://codereview.chromium.org/2938263002/diff/200001/chrome/browser/chrome_... chrome/browser/chrome_browser_field_trials.cc:58: // file and make kBrowserMetricsName local to that file. On 2017/06/28 17:30:35, Alexei Svitkine (slow) wrote: > Friendly ping on this! (Separate CL, but please let's get this done as otherwise > this just keeps growing and growing...) Acknowledged. https://codereview.chromium.org/2938263002/diff/200001/chrome/browser/chrome_... chrome/browser/chrome_browser_field_trials.cc:88: metrics_dir, upload_dir, ChromeMetricsServiceClient::kBrowserMetricsName, On 2017/06/28 17:30:35, Alexei Svitkine (slow) wrote: > Can you add a comment about expected file locations here? > > i.e. user-data-dir/BrowserMetrics/BrowserMetrics-active.pma etc. Done. https://codereview.chromium.org/2938263002/diff/200001/components/metrics/fil... File components/metrics/file_metrics_provider_unittest.cc (right): https://codereview.chromium.org/2938263002/diff/200001/components/metrics/fil... components/metrics/file_metrics_provider_unittest.cc:650: for (auto& file_name : file_names) On 2017/06/28 17:30:36, Alexei Svitkine (slow) wrote: > const auto& Done.
lgtm https://codereview.chromium.org/2938263002/diff/200001/base/metrics/persisten... File base/metrics/persistent_histogram_allocator.h (right): https://codereview.chromium.org/2938263002/diff/200001/base/metrics/persisten... base/metrics/persistent_histogram_allocator.h:443: const std::string& name, On 2017/06/28 18:52:08, bcwhite wrote: > On 2017/06/28 17:30:35, Alexei Svitkine (slow) wrote: > > StringPiece especially since you pass in kBrowserMetricsName which isn't a > > std::string. > > std::string takes a char* no problem. Inside, though, all conditions lead to > the parameter needing to be a std::string so it's better to take it that way in > the parameter list and have it converted at most once, possibly never if the > caller already has it that way. > > In the other ConstructFilePaths, the most common case doesn't need a std::string > so, combined with most callers having the name as a StringPiece, it's more > efficient to have the parameter being a StringPiece. Ah right, because you're converting via .as_string() later otherwise. Fair enough. Made a small suggestion about those but otherwise sg. https://codereview.chromium.org/2938263002/diff/220001/base/metrics/persisten... File base/metrics/persistent_histogram_allocator.cc (right): https://codereview.chromium.org/2938263002/diff/220001/base/metrics/persisten... base/metrics/persistent_histogram_allocator.cc:854: MakeMetricsFilePath(active_dir, name + std::string("-active")); Nit: I think name.append("-active") might be more efficient (no extra temporary) and also less verbose. Same in the other function.
https://codereview.chromium.org/2938263002/diff/220001/base/metrics/persisten... File base/metrics/persistent_histogram_allocator.cc (right): https://codereview.chromium.org/2938263002/diff/220001/base/metrics/persisten... base/metrics/persistent_histogram_allocator.cc:854: MakeMetricsFilePath(active_dir, name + std::string("-active")); On 2017/06/28 19:19:09, Alexei Svitkine (slow) wrote: > Nit: I think name.append("-active") might be more efficient (no extra temporary) > and also less verbose. I can't append() because |name| is a "const" ref. Plus, it could append multiple times, once per "if" block, causing wrong names.
https://codereview.chromium.org/2938263002/diff/220001/base/metrics/persisten... File base/metrics/persistent_histogram_allocator.cc (right): https://codereview.chromium.org/2938263002/diff/220001/base/metrics/persisten... base/metrics/persistent_histogram_allocator.cc:854: MakeMetricsFilePath(active_dir, name + std::string("-active")); On 2017/06/28 19:29:51, bcwhite wrote: > On 2017/06/28 19:19:09, Alexei Svitkine (slow) wrote: > > Nit: I think name.append("-active") might be more efficient (no extra > temporary) > > and also less verbose. > > I can't append() because |name| is a "const" ref. Plus, it could append > multiple times, once per "if" block, causing wrong names. Ah right, it doesn't work in this case. It should work in the above function though since .as_string()'s are temporary.
The CQ bit was checked by bcwhite@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2938263002/diff/220001/base/metrics/persisten... File base/metrics/persistent_histogram_allocator.cc (right): https://codereview.chromium.org/2938263002/diff/220001/base/metrics/persisten... base/metrics/persistent_histogram_allocator.cc:854: MakeMetricsFilePath(active_dir, name + std::string("-active")); On 2017/06/28 19:33:00, Alexei Svitkine (slow) wrote: > On 2017/06/28 19:29:51, bcwhite wrote: > > On 2017/06/28 19:19:09, Alexei Svitkine (slow) wrote: > > > Nit: I think name.append("-active") might be more efficient (no extra > > temporary) > > > and also less verbose. > > > > I can't append() because |name| is a "const" ref. Plus, it could append > > multiple times, once per "if" block, causing wrong names. > > Ah right, it doesn't work in this case. It should work in the above function > though since .as_string()'s are temporary. Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by bcwhite@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from asvitkine@chromium.org Link to the patchset: https://codereview.chromium.org/2938263002/#ps240001 (title: "use std::string::append() where possible")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 240001, "attempt_start_ts": 1499178950425420,
"parent_rev": "8505f88fe3482b9d563ea08d2c45e7eba1c22961", "commit_rev":
"38d8bb9328a2d0a1b7d8373ddd70971c1ce4eeb6"}
Message was sent while issue was closed.
Description was changed from ========== Put BrowserMetrics with embedded profiles into subdir for auto-upload. Because the BrowserMetrics file now has an embedded profile, it isn't sent as part of the startup sequence but a minute or so after. That means the browser could exit and not send anything with the "previous run" metrics file being overwritten at the next startup. Instead, append a date-stamp to the last-run filename and move it to a subdirectory from which files will be removed only after they are uploaded. BUG=695880 ========== to ========== Put BrowserMetrics with embedded profiles into subdir for auto-upload. Because the BrowserMetrics file now has an embedded profile, it isn't sent as part of the startup sequence but a minute or so after. That means the browser could exit and not send anything with the "previous run" metrics file being overwritten at the next startup. Instead, append a date-stamp to the last-run filename and move it to a subdirectory from which files will be removed only after they are uploaded. BUG=695880 Review-Url: https://codereview.chromium.org/2938263002 Cr-Commit-Position: refs/heads/master@{#484116} Committed: https://chromium.googlesource.com/chromium/src/+/38d8bb9328a2d0a1b7d8373ddd70... ==========
Message was sent while issue was closed.
Committed patchset #9 (id:240001) as https://chromium.googlesource.com/chromium/src/+/38d8bb9328a2d0a1b7d8373ddd70... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
