Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1537743006:
Persist setup metrics and have Chrome report them during UMA upload. (Closed)
|
Created:
5 years ago by bcwhite Modified:
4 years, 10 months ago CC:
chromium-reviews, grt+watch_chromium.org, asvitkine+watch_chromium.org, wfh+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@shared-histograms Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionPersist setup metrics and have Chrome report them during UMA upload.
BUG=546019
Committed: https://crrev.com/34c6bbf85c990ead28d334b3803093b614c958c6
Cr-Commit-Position: refs/heads/master@{#376549}
Patch Set 1 #
Total comments: 12
Patch Set 2 : rewrite to be a generic file-metrics service #Patch Set 3 : rebased #Patch Set 4 : fixed some compile problems under clang #Patch Set 5 : moved allocator create/destroy to be beside snapshot (simpler code) #
Total comments: 102
Patch Set 6 : address (many) comments by Greg #
Total comments: 65
Patch Set 7 : addressed numerous review comments by Greg #Patch Set 8 : fixed test #
Total comments: 1
Patch Set 9 : use real browser task-runner and fix various other problems #Patch Set 10 : more tests and hard-code allocator name #Patch Set 11 : fixed some build problems #Patch Set 12 : rebased #Patch Set 13 : test needs to clear out statistics-recorder before releasing histogram memory #
Total comments: 52
Patch Set 14 : addressed review comments by Greg #
Total comments: 2
Patch Set 15 : addressed more review comments by Greg #
Total comments: 12
Patch Set 16 : addressed review comments by Greg #
Total comments: 34
Patch Set 17 : addressed review comments by Alexei #Patch Set 18 : rebased #
Total comments: 6
Patch Set 19 : addressed review comments by Alexei & Greg #
Total comments: 7
Patch Set 20 : move AccessResult to .h and add histogram to .xml #
Total comments: 34
Patch Set 21 : addressed review comments by Alexei #Patch Set 22 : simplify taking ownership of histograms during snapshots #
Total comments: 13
Patch Set 23 : addressed final review comments by Alexei #Messages
Total messages: 67 (21 generated)
bcwhite@chromium.org changed reviewers: + asvitkine@chromium.org
Here's the initial concept. It compiles and the unittest passes but I've done zero checking to see that it actually works as intended. Next week...
grt@chromium.org changed reviewers: + grt@chromium.org
I am really, really excited to see this! https://codereview.chromium.org/1537743006/diff/1/chrome/installer/setup/setu... File chrome/installer/setup/setup_constants.h (right): https://codereview.chromium.org/1537743006/diff/1/chrome/installer/setup/setu... chrome/installer/setup/setup_constants.h:10: #include <stdint.h> both wchar_t and char (the two types used in this file) are keywords in C++-03, so i don't think this is needed. https://codereview.chromium.org/1537743006/diff/1/chrome/installer/util/googl... File chrome/installer/util/google_update_constants.h (right): https://codereview.chromium.org/1537743006/diff/1/chrome/installer/util/googl... chrome/installer/util/google_update_constants.h:86: // Name of allocator (and associated file) for storing histograms to be is this related to Chrome's integration with Google Update? if not, util_constants.{cc,h} is a better place for it.
https://codereview.chromium.org/1537743006/diff/1/chrome/installer/setup/setu... File chrome/installer/setup/setup_constants.h (right): https://codereview.chromium.org/1537743006/diff/1/chrome/installer/setup/setu... chrome/installer/setup/setup_constants.h:10: #include <stdint.h> On 2015/12/18 21:02:50, grt wrote: > both wchar_t and char (the two types used in this file) are keywords in C++-03, > so i don't think this is needed. Odd. Not sure why that showed up. Done. https://codereview.chromium.org/1537743006/diff/1/chrome/installer/util/googl... File chrome/installer/util/google_update_constants.h (right): https://codereview.chromium.org/1537743006/diff/1/chrome/installer/util/googl... chrome/installer/util/google_update_constants.h:86: // Name of allocator (and associated file) for storing histograms to be On 2015/12/18 21:02:50, grt wrote: > is this related to Chrome's integration with Google Update? if not, > util_constants.{cc,h} is a better place for it. Done.
https://codereview.chromium.org/1537743006/diff/1/chrome/browser/metrics/chro... File chrome/browser/metrics/chrome_setup_metrics_provider.cc (right): https://codereview.chromium.org/1537743006/diff/1/chrome/browser/metrics/chro... chrome/browser/metrics/chrome_setup_metrics_provider.cc:40: base::Bind(&ChromeSetupMetricsProvider::CheckForSetupMetrics, In Chromium code, it's best practice to not have the same object instance be accessed from multiple threads. You can search chromium-dev for discussions on this topic. Often, a good structure for the code is to have the work that happens on the other thread be done in a free standing function in an anon namespace that will pass its results back to the UI thread object via a callback. https://codereview.chromium.org/1537743006/diff/1/chrome/browser/metrics/chro... chrome/browser/metrics/chrome_setup_metrics_provider.cc:69: metrics_service_->RemovePersistentMemorySegment(metrics_allocator_.get()); Can this functionality be done by a different helper class than metrics service? MetricsService is already pretty complex, so if we can avoid adding more things directly to it, it would be preferable. https://codereview.chromium.org/1537743006/diff/1/chrome/browser/metrics/chro... File chrome/browser/metrics/chrome_setup_metrics_provider.h (right): https://codereview.chromium.org/1537743006/diff/1/chrome/browser/metrics/chro... chrome/browser/metrics/chrome_setup_metrics_provider.h:17: Nit: No need for blank lines within namespace for forward decls. https://codereview.chromium.org/1537743006/diff/1/chrome/browser/metrics/chro... chrome/browser/metrics/chrome_setup_metrics_provider.h:21: }; Nit: No ;
https://codereview.chromium.org/1537743006/diff/1/chrome/browser/metrics/chro... File chrome/browser/metrics/chrome_setup_metrics_provider.cc (right): https://codereview.chromium.org/1537743006/diff/1/chrome/browser/metrics/chro... chrome/browser/metrics/chrome_setup_metrics_provider.cc:40: base::Bind(&ChromeSetupMetricsProvider::CheckForSetupMetrics, On 2015/12/21 16:54:53, Alexei Svitkine (slow) wrote: > In Chromium code, it's best practice to not have the same object instance be > accessed from multiple threads. You can search chromium-dev for discussions on > this topic. > > Often, a good structure for the code is to have the work that happens on the > other thread be done in a free standing function in an anon namespace that will > pass its results back to the UI thread object via a callback. Okay. I'm rewriting this to support multiple files, both once-written and constantly-updated. I'll keep that in mind. https://codereview.chromium.org/1537743006/diff/1/chrome/browser/metrics/chro... chrome/browser/metrics/chrome_setup_metrics_provider.cc:69: metrics_service_->RemovePersistentMemorySegment(metrics_allocator_.get()); On 2015/12/21 16:54:53, Alexei Svitkine (slow) wrote: > Can this functionality be done by a different helper class than metrics service? > MetricsService is already pretty complex, so if we can avoid adding more things > directly to it, it would be preferable. Not really. MetricsService is what owns the HSM and flattener, hence it controls how everything gets merged. https://codereview.chromium.org/1537743006/diff/1/chrome/browser/metrics/chro... File chrome/browser/metrics/chrome_setup_metrics_provider.h (right): https://codereview.chromium.org/1537743006/diff/1/chrome/browser/metrics/chro... chrome/browser/metrics/chrome_setup_metrics_provider.h:17: On 2015/12/21 16:54:53, Alexei Svitkine (slow) wrote: > Nit: No need for blank lines within namespace for forward decls. Done. https://codereview.chromium.org/1537743006/diff/1/chrome/browser/metrics/chro... chrome/browser/metrics/chrome_setup_metrics_provider.h:21: }; On 2015/12/21 16:54:53, Alexei Svitkine (slow) wrote: > Nit: No ; Done.
Rewrite to a general service is done. It still needs support for read-only files (current test seg-faults trying to delta it) but I need validation on previous CLs before I add support for that.
Patchset #2 (id:20001) has been deleted
Patchset #2 (id:40001) has been deleted
On 2016/01/28 21:05:24, bcwhite wrote: > Rewrite to a general service is done. It still needs support for read-only > files (current test seg-faults trying to delta it) but I need validation on > previous CLs before I add support for that. Would you like some feedback on this CR now, or should I hold off until the pending work is done?
On 2016/02/03 15:41:02, grt wrote: > On 2016/01/28 21:05:24, bcwhite wrote: > > Rewrite to a general service is done. It still needs support for read-only > > files (current test seg-faults trying to delta it) but I need validation on > > previous CLs before I add support for that. > > Would you like some feedback on this CR now, or should I hold off until the > pending work is done? I don't think this depends too much on pending CLs outside of their API. Comment, please!
First round of review comments. I'll hold off on reviewing more until you've had a chance to digest these. I'm happy to discuss anything in person. https://codereview.chromium.org/1537743006/diff/120001/base/metrics/histogram... File base/metrics/histogram_snapshot_manager.h (right): https://codereview.chromium.org/1537743006/diff/120001/base/metrics/histogram... base/metrics/histogram_snapshot_manager.h:91: void PrepareDeltaTakingOwnership(HistogramBase* histogram); take a scoped_ptr rather than a raw ptr so that the ownership transfer is explicit https://codereview.chromium.org/1537743006/diff/120001/chrome/installer/setup... File chrome/installer/setup/installer_crash_reporting.cc (right): https://codereview.chromium.org/1537743006/diff/120001/chrome/installer/setup... chrome/installer/setup/installer_crash_reporting.cc:124: #if defined(OS_MACOSX) || defined(OS_WIN) this file is exclusively OS_WIN, so there's no need for the platform-specific conditionals here. https://codereview.chromium.org/1537743006/diff/120001/chrome/installer/setup... File chrome/installer/setup/setup_main.cc (right): https://codereview.chromium.org/1537743006/diff/120001/chrome/installer/setup... chrome/installer/setup/setup_main.cc:1628: // Create a persistent space for any histograms that can be reported later how about creating an installer_metrics.{cc,h} pair (or somesuch) with two functions, one each for this block of code and the one below? https://codereview.chromium.org/1537743006/diff/120001/chrome/installer/setup... chrome/installer/setup/setup_main.cc:1629: // by the Chrome process. The "id" is a timestamp so that Chrome can tell nit: The comment on line 1795 refers to the browser rather than Chrome. If it truly is the browser process that is responsible for picking up the data and reporting it (I think this is true), please harmonize both comments to "...by the browser process...". https://codereview.chromium.org/1537743006/diff/120001/chrome/installer/setup... chrome/installer/setup/setup_main.cc:1794: // Save all the persistent histograms to a single file on disk for reading using a single file means that we'll get only a random subset of installer UMA data. is it possible to not lose data in some reasonable way? multiple files, perhaps? https://codereview.chromium.org/1537743006/diff/120001/chrome/installer/setup... chrome/installer/setup/setup_main.cc:1813: if (base::ReplaceFile(tmp_file_path, file_path, nullptr)) { this will fail so long as chrome.exe has the |file_path| mapped in memory since FileMetricsProvider::CheckAndMapNewMetrics opens the file without SHARE_DELETE. https://codereview.chromium.org/1537743006/diff/120001/components/metrics/fil... File components/metrics/file_metrics_provider.cc (right): https://codereview.chromium.org/1537743006/diff/120001/components/metrics/fil... components/metrics/file_metrics_provider.cc:31: : metrics_found_(0), nit: remove this initializer https://codereview.chromium.org/1537743006/diff/120001/components/metrics/fil... components/metrics/file_metrics_provider.cc:34: // Start a background check for pending setup metrics so it can be uploaded is it appropriate for this to be generalized so that it isn't hardcoded to be specific to setup metrics? otherwise, should FileMetricsProvider be called SetupMetricsProvider? https://codereview.chromium.org/1537743006/diff/120001/components/metrics/fil... components/metrics/file_metrics_provider.cc:41: // posted on the Worker Pool or because the owned Memory Allocator has the destruction problems are avoided by following my advice in ScheduleFilesCheck. https://codereview.chromium.org/1537743006/diff/120001/components/metrics/fil... components/metrics/file_metrics_provider.cc:55: if (!prefs_key.empty()) { && pref_service_ since it could be null as per the comment above https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/bro... https://codereview.chromium.org/1537743006/diff/120001/components/metrics/fil... components/metrics/file_metrics_provider.cc:62: metrics_files_.push_back(scoped_ptr<FileInformation>(file)); nit: scoped_ptr<FileInformation>(file) -> make_scoped_ptr(file) https://codereview.chromium.org/1537743006/diff/120001/components/metrics/fil... components/metrics/file_metrics_provider.cc:68: prefs->RegisterInt64Pref(metrics::prefs::kSetupMetricsLastSeenPrefix + nit: indentation https://codereview.chromium.org/1537743006/diff/120001/components/metrics/fil... components/metrics/file_metrics_provider.cc:78: for (iter = metrics_found_.begin(); iter != metrics_found_.end();) { nit: define |iter| here with "auto iter = ..." https://codereview.chromium.org/1537743006/diff/120001/components/metrics/fil... components/metrics/file_metrics_provider.cc:80: const FileInformation* file = temp->get(); nit: omit this local var and use temp-> directly below https://codereview.chromium.org/1537743006/diff/120001/components/metrics/fil... components/metrics/file_metrics_provider.cc:82: metrics_files_.push_back(std::move(*temp)); i believe this line and the next can be replaced with: metrics_files_.splice(metrics_files_.end(), metrics_found_, temp); https://codereview.chromium.org/1537743006/diff/120001/components/metrics/fil... components/metrics/file_metrics_provider.cc:99: for (auto& file_iter : metrics_found_) { nit: |file_iter| isn't an iterator here, but rather a reference to a scoped_ptr<FileInformation>. i think |file| or |file_ptr| are both more appropriate names for it. i'm leaning toward the former since we don't generally put a _ptr suffix on variables of type scoped_ptr<>. https://codereview.chromium.org/1537743006/diff/120001/components/metrics/fil... components/metrics/file_metrics_provider.cc:100: FileInformation* file = file_iter.get(); nit: omit this local var and use the auto'd |file| below https://codereview.chromium.org/1537743006/diff/120001/components/metrics/fil... components/metrics/file_metrics_provider.cc:107: LOG(ERROR) << "Metrics file \"" << file->path.AsUTF8Unsafe() file->path.value() should be fine here https://codereview.chromium.org/1537743006/diff/120001/components/metrics/fil... components/metrics/file_metrics_provider.cc:114: file->allocator.reset(new base::FilePersistentMemoryAllocator( change FilePersistentMemoryAllocator so that it takes a scoped_ptr rather than a raw ptr, and use std::move(file->mapped) here. this makes the ownership transfer explicit and extremely difficult to get wrong. https://codereview.chromium.org/1537743006/diff/120001/components/metrics/fil... components/metrics/file_metrics_provider.cc:118: if (file->allocator) { remove this condition as it will always be true on the basis of lines 102, 112, and 114 https://codereview.chromium.org/1537743006/diff/120001/components/metrics/fil... components/metrics/file_metrics_provider.cc:122: base::HistogramBase* histogram = use scoped_ptr<base::HistogramBase> rather than a naked pointer since ownership is transferred https://codereview.chromium.org/1537743006/diff/120001/components/metrics/fil... components/metrics/file_metrics_provider.cc:123: base::GetNextPersistentHistogram(file->allocator.get(), &hist_iter); please update this and the related functions to return a scoped_ptr rather than a naked pointer. this makes the ownership explicit in the API https://codereview.chromium.org/1537743006/diff/120001/components/metrics/fil... components/metrics/file_metrics_provider.cc:126: if (file->type == FILE_HISTOGRAMS_ATOMIC) { nit: omit braces https://codereview.chromium.org/1537743006/diff/120001/components/metrics/fil... components/metrics/file_metrics_provider.cc:144: if (!file->prefs_key.empty()) { && pref_service_ https://codereview.chromium.org/1537743006/diff/120001/components/metrics/fil... components/metrics/file_metrics_provider.cc:152: void FileMetricsProvider::CheckAndMapNewMetricFiles( nit: often a suffix such as "OnBlockingPool" is added to isolated functions like this that run on a special thread. https://codereview.chromium.org/1537743006/diff/120001/components/metrics/fil... components/metrics/file_metrics_provider.cc:176: file->mapped->Initialize(file->path); if (file->mapped->Initialize(file->path)) return true; file->mapped.reset(); return false; https://codereview.chromium.org/1537743006/diff/120001/components/metrics/fil... components/metrics/file_metrics_provider.cc:188: bool posted = base::WorkerPool::PostTask( This should use the BrowserThread BlockingPool rather than the WorkerPool. One way to do this is to have ChromeMetricsServiceClient::Initialize inject either a TaskRunner or a SequencedWorkerPool obtained from content::BrowserThread::GetBlockingPool(). Please read the WorkerShutdown comments in sequenced_worker_pool.h to decide what shutdown semantics are called for (SKIP_ON_SHUTDOWN would be good, I think). Also, consider using a SequenceToken so that file checks are always serialized. You could use GetSequencedTaskRunnerWithShutdownBehavior with a random token during initialization and hold on to the resulting SequencedTaskRunner in the instance. Additionally, it's cleaner to use PostTaskAndReply so that CheckAndMapNewMetricFiles is run on an IO-safe thread, and RecordFileInformation is run back on the UI thread. In this case, bind |check_list| as an unretained pointer for the first callback and as an owned pointer for the second. This has the benefit of allowing you to do away with the lock since the instance will only be mutated on the UI thread. I suggest adding a base::ThreadChecker member to the class, and sprinkling DCHECK(thread_checker_.CalledOnValidThread()); at the top of all of the functions so that it's clear that the class is single-threaded and that things will 'splode if anyone ever forgets. https://codereview.chromium.org/1537743006/diff/120001/components/metrics/fil... components/metrics/file_metrics_provider.cc:193: base::Unretained(this))), This will lead to a use-after-free if the metrics service is shut down while a file check is running. WeakPtr is commonly used to deal with this. Add a base::WeakPtrFactory instance member, then call weak_factory_.GetWeakPtr() here when binding. https://codereview.chromium.org/1537743006/diff/120001/components/metrics/fil... components/metrics/file_metrics_provider.cc:195: DCHECK(posted); I don't think this is needed. false from PostTask* is generally an indication that Chrome is shutting down, which probably isn't a reason to crash the browser. https://codereview.chromium.org/1537743006/diff/120001/components/metrics/fil... components/metrics/file_metrics_provider.cc:206: if (iter->mapped) { nit: omit braces https://codereview.chromium.org/1537743006/diff/120001/components/metrics/fil... components/metrics/file_metrics_provider.cc:207: metrics_found_.push_back(std::move(iter)); use splice to move the element. you'll have to change from the range-based loop to an iterator-based one like you have on line 78, but this is a small price to pay. for example: metrics_found_.splice(metrics_found_.end(), *files, temp); https://codereview.chromium.org/1537743006/diff/120001/components/metrics/fil... components/metrics/file_metrics_provider.cc:209: metrics_files_.push_back(std::move(iter)); metrics_files_.splice(metrics_files_.end(), *files, temp); https://codereview.chromium.org/1537743006/diff/120001/components/metrics/fil... File components/metrics/file_metrics_provider.h (right): https://codereview.chromium.org/1537743006/diff/120001/components/metrics/fil... components/metrics/file_metrics_provider.h:1: // Copyright 2015 The Chromium Authors. All rights reserved. 2016 :-) https://codereview.chromium.org/1537743006/diff/120001/components/metrics/fil... components/metrics/file_metrics_provider.h:8: #include <atomic> unused https://codereview.chromium.org/1537743006/diff/120001/components/metrics/fil... components/metrics/file_metrics_provider.h:30: // FileMetricsProvider gathers and logs histograms written to files on disk. Please add some more details. For example, how often does it check for file(s), what file(s) does it look for? Does it poll for them? It looks like a scan is made for files each time the metrics service opens a new log. How often is that? Also good to explain what RegisterFile does and how preferences are used to track things. https://codereview.chromium.org/1537743006/diff/120001/components/metrics/fil... components/metrics/file_metrics_provider.h:46: std::string prefs_key; #include <string> above https://codereview.chromium.org/1537743006/diff/120001/components/metrics/fil... components/metrics/file_metrics_provider.h:47: base::Time last_seen; #include "base/time/time.h" https://codereview.chromium.org/1537743006/diff/120001/components/metrics/fil... components/metrics/file_metrics_provider.h:51: FileInformation(); the ctor and dtor should come before the data members as per https://google.github.io/styleguide/cppguide.html#Declaration_Order https://codereview.chromium.org/1537743006/diff/120001/components/metrics/fil... components/metrics/file_metrics_provider.h:59: typedef std::list<scoped_ptr<FileInformation>> FileInformationList; nit: using FileInformationList = ...; https://codereview.chromium.org/1537743006/diff/120001/components/metrics/fil... components/metrics/file_metrics_provider.h:65: void RegisterFile(const base::FilePath& path, FileType type, please document https://codereview.chromium.org/1537743006/diff/120001/components/metrics/fil... components/metrics/file_metrics_provider.h:66: const std::string& prefs_key); nit: base::StringPiece prefs_key https://codereview.chromium.org/1537743006/diff/120001/components/metrics/fil... components/metrics/file_metrics_provider.h:68: static void RegisterPrefs(PrefRegistrySimple* prefs, const std::string& key); nit: base::StringPiece key https://codereview.chromium.org/1537743006/diff/120001/components/metrics/fil... components/metrics/file_metrics_provider.h:71: void OnDidCreateMetricsLog() override; prefer placing the inherited methods in the private: section unless they need to be part of this class's public API (see https://groups.google.com/a/chromium.org/d/msg/chromium-dev/CwzjcWd9cYM/TcUUl... and https://groups.google.com/a/chromium.org/d/msg/chromium-dev/MkBEHBx3g_4/_wP74... for various discussions). https://codereview.chromium.org/1537743006/diff/120001/components/metrics/fil... components/metrics/file_metrics_provider.h:80: base::Callback<void(FileInformationList*)> callback); note: pass callbacks by constref, although this callback should go away; see comment in ScheduleFilesCheck in .cc file https://codereview.chromium.org/1537743006/diff/120001/components/metrics/fil... components/metrics/file_metrics_provider.h:81: static bool CheckAndMapNewMetrics(FileMetricsProvider::FileInformation* file); "FileMetricsProvider::" not needed here or in definition https://codereview.chromium.org/1537743006/diff/120001/components/metrics/fil... components/metrics/file_metrics_provider.h:87: base::Lock lock_; Is this lock needed? It looks like everything would happen on the UI thread once the call to RecordFileInformation is moved there. Or is there something about the interaction with the metrics service that makes this need to be threadsafe? https://codereview.chromium.org/1537743006/diff/120001/components/metrics/fil... components/metrics/file_metrics_provider.h:88: FileInformationList metrics_files_; what's the advantage of having two lists? could the loop in RecordHistogramSnapshots just begin with: if (!file->mapped) continue; // Another attempt will be made on the next ScheduleFilesCheck.
Covered many of the comments. Changing which pool the thread runs is something to come. https://codereview.chromium.org/1537743006/diff/120001/base/metrics/histogram... File base/metrics/histogram_snapshot_manager.h (right): https://codereview.chromium.org/1537743006/diff/120001/base/metrics/histogram... base/metrics/histogram_snapshot_manager.h:91: void PrepareDeltaTakingOwnership(HistogramBase* histogram); On 2016/02/04 15:36:42, grt wrote: > take a scoped_ptr rather than a raw ptr so that the ownership transfer is > explicit These call through to the private PrepareSamples which *doesn't* (and can't) take a scoped_ptr. I'll have to release the scoped_ptr in the implementation to pass a raw pointer to that method. Okay? https://codereview.chromium.org/1537743006/diff/120001/chrome/installer/setup... File chrome/installer/setup/installer_crash_reporting.cc (right): https://codereview.chromium.org/1537743006/diff/120001/chrome/installer/setup... chrome/installer/setup/installer_crash_reporting.cc:124: #if defined(OS_MACOSX) || defined(OS_WIN) On 2016/02/04 15:36:42, grt wrote: > this file is exclusively OS_WIN, so there's no need for the platform-specific > conditionals here. This whole file will not be in the final submit. It was just necessary for it to compile due to an incomplete port to CrashPad that is currently being resolved external to this CL. https://codereview.chromium.org/1537743006/diff/120001/chrome/installer/setup... File chrome/installer/setup/setup_main.cc (right): https://codereview.chromium.org/1537743006/diff/120001/chrome/installer/setup... chrome/installer/setup/setup_main.cc:1628: // Create a persistent space for any histograms that can be reported later On 2016/02/04 15:36:42, grt wrote: > how about creating an installer_metrics.{cc,h} pair (or somesuch) with two > functions, one each for this block of code and the one below? Done. https://codereview.chromium.org/1537743006/diff/120001/chrome/installer/setup... chrome/installer/setup/setup_main.cc:1629: // by the Chrome process. The "id" is a timestamp so that Chrome can tell On 2016/02/04 15:36:42, grt wrote: > nit: The comment on line 1795 refers to the browser rather than Chrome. If it > truly is the browser process that is responsible for picking up the data and > reporting it (I think this is true), please harmonize both comments to "...by > the browser process...". Done. https://codereview.chromium.org/1537743006/diff/120001/chrome/installer/setup... chrome/installer/setup/setup_main.cc:1794: // Save all the persistent histograms to a single file on disk for reading On 2016/02/04 15:36:42, grt wrote: > using a single file means that we'll get only a random subset of installer UMA > data. is it possible to not lose data in some reasonable way? multiple files, > perhaps? All things are possible... for a cost. Metrics are reported at launch and every 30 minutes after that. We'll only fail to report data if there are multiple runs between those times. Once read/write memory-mapped files are supported (a few months down my timeline), it may be possible to switch to that which will mean that no records will get lost no matter how many runs occur. https://codereview.chromium.org/1537743006/diff/120001/chrome/installer/setup... chrome/installer/setup/setup_main.cc:1813: if (base::ReplaceFile(tmp_file_path, file_path, nullptr)) { On 2016/02/04 15:36:42, grt wrote: > this will fail so long as chrome.exe has the |file_path| mapped in memory since > FileMetricsProvider::CheckAndMapNewMetrics opens the file without SHARE_DELETE. Basically part of the multiple-runs problem. FileMetricsProvider only keep the file mapped until the values are sent, after which it releases it. https://codereview.chromium.org/1537743006/diff/120001/components/metrics/fil... File components/metrics/file_metrics_provider.cc (right): https://codereview.chromium.org/1537743006/diff/120001/components/metrics/fil... components/metrics/file_metrics_provider.cc:31: : metrics_found_(0), On 2016/02/04 15:36:42, grt wrote: > nit: remove this initializer Leftover from a previous design. Odd that it isn't a compile error. https://codereview.chromium.org/1537743006/diff/120001/components/metrics/fil... components/metrics/file_metrics_provider.cc:34: // Start a background check for pending setup metrics so it can be uploaded On 2016/02/04 15:36:42, grt wrote: > is it appropriate for this to be generalized so that it isn't hardcoded to be > specific to setup metrics? otherwise, should FileMetricsProvider be called > SetupMetricsProvider? I started it as something specific for Setup but then others were asking if it could be used to pass in other metrics from other locations so I rewrote it to be more generic, basically handling a list of files rather than one file. https://codereview.chromium.org/1537743006/diff/120001/components/metrics/fil... components/metrics/file_metrics_provider.cc:55: if (!prefs_key.empty()) { On 2016/02/04 15:36:43, grt wrote: > && pref_service_ > since it could be null as per the comment above > https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/bro... Done. https://codereview.chromium.org/1537743006/diff/120001/components/metrics/fil... components/metrics/file_metrics_provider.cc:62: metrics_files_.push_back(scoped_ptr<FileInformation>(file)); On 2016/02/04 15:36:43, grt wrote: > nit: scoped_ptr<FileInformation>(file) -> make_scoped_ptr(file) Done. https://codereview.chromium.org/1537743006/diff/120001/components/metrics/fil... components/metrics/file_metrics_provider.cc:68: prefs->RegisterInt64Pref(metrics::prefs::kSetupMetricsLastSeenPrefix + On 2016/02/04 15:36:43, grt wrote: > nit: indentation Done. https://codereview.chromium.org/1537743006/diff/120001/components/metrics/fil... components/metrics/file_metrics_provider.cc:78: for (iter = metrics_found_.begin(); iter != metrics_found_.end();) { On 2016/02/04 15:36:43, grt wrote: > nit: define |iter| here with "auto iter = ..." Done. https://codereview.chromium.org/1537743006/diff/120001/components/metrics/fil... components/metrics/file_metrics_provider.cc:80: const FileInformation* file = temp->get(); On 2016/02/04 15:36:43, grt wrote: > nit: omit this local var and use temp-> directly below I'd rather not. It looks ugly with all the temp->get()-> and doesn't self-document what is being iterated. https://codereview.chromium.org/1537743006/diff/120001/components/metrics/fil... components/metrics/file_metrics_provider.cc:82: metrics_files_.push_back(std::move(*temp)); On 2016/02/04 15:36:43, grt wrote: > i believe this line and the next can be replaced with: > metrics_files_.splice(metrics_files_.end(), metrics_found_, temp); Done. https://codereview.chromium.org/1537743006/diff/120001/components/metrics/fil... components/metrics/file_metrics_provider.cc:99: for (auto& file_iter : metrics_found_) { On 2016/02/04 15:36:43, grt wrote: > nit: |file_iter| isn't an iterator here, but rather a reference to a > scoped_ptr<FileInformation>. i think |file| or |file_ptr| are both more > appropriate names for it. i'm leaning toward the former since we don't generally > put a _ptr suffix on variables of type scoped_ptr<>. Done. https://codereview.chromium.org/1537743006/diff/120001/components/metrics/fil... components/metrics/file_metrics_provider.cc:100: FileInformation* file = file_iter.get(); On 2016/02/04 15:36:43, grt wrote: > nit: omit this local var and use the auto'd |file| below Done. https://codereview.chromium.org/1537743006/diff/120001/components/metrics/fil... components/metrics/file_metrics_provider.cc:107: LOG(ERROR) << "Metrics file \"" << file->path.AsUTF8Unsafe() On 2016/02/04 15:36:43, grt wrote: > file->path.value() should be fine here Done. https://codereview.chromium.org/1537743006/diff/120001/components/metrics/fil... components/metrics/file_metrics_provider.cc:114: file->allocator.reset(new base::FilePersistentMemoryAllocator( On 2016/02/04 15:36:43, grt wrote: > change FilePersistentMemoryAllocator so that it takes a scoped_ptr rather than a > raw ptr, and use std::move(file->mapped) here. this makes the ownership transfer > explicit and extremely difficult to get wrong. Doing that in a different CL: https://codereview.chromium.org/1660143009 https://codereview.chromium.org/1537743006/diff/120001/components/metrics/fil... components/metrics/file_metrics_provider.cc:118: if (file->allocator) { On 2016/02/04 15:36:43, grt wrote: > remove this condition as it will always be true on the basis of lines 102, 112, > and 114 Done. https://codereview.chromium.org/1537743006/diff/120001/components/metrics/fil... components/metrics/file_metrics_provider.cc:126: if (file->type == FILE_HISTOGRAMS_ATOMIC) { On 2016/02/04 15:36:43, grt wrote: > nit: omit braces Done. https://codereview.chromium.org/1537743006/diff/120001/components/metrics/fil... components/metrics/file_metrics_provider.cc:144: if (!file->prefs_key.empty()) { On 2016/02/04 15:36:42, grt wrote: > && pref_service_ Done. https://codereview.chromium.org/1537743006/diff/120001/components/metrics/fil... components/metrics/file_metrics_provider.cc:152: void FileMetricsProvider::CheckAndMapNewMetricFiles( On 2016/02/04 15:36:43, grt wrote: > nit: often a suffix such as "OnBlockingPool" is added to isolated functions like > this that run on a special thread. Done. https://codereview.chromium.org/1537743006/diff/120001/components/metrics/fil... components/metrics/file_metrics_provider.cc:176: file->mapped->Initialize(file->path); On 2016/02/04 15:36:43, grt wrote: > if (file->mapped->Initialize(file->path)) > return true; > file->mapped.reset(); > return false; Done. https://codereview.chromium.org/1537743006/diff/120001/components/metrics/fil... components/metrics/file_metrics_provider.cc:207: metrics_found_.push_back(std::move(iter)); On 2016/02/04 15:36:43, grt wrote: > use splice to move the element. you'll have to change from the range-based loop > to an iterator-based one like you have on line 78, but this is a small price to > pay. for example: > metrics_found_.splice(metrics_found_.end(), *files, temp); Each move in that case has to delete the value from the head of the vector, shifting all the other ones. O(N^2) This way allows just the internal pointer to be moved and then the entire vector is deleted by the caller. The code back on line #78 has the same issue but only applies to files that were found and need to be recorded whereas this is every file. Still, it could do the same, moving the pointers and then .clear-ing the vector at the end. https://codereview.chromium.org/1537743006/diff/120001/components/metrics/fil... components/metrics/file_metrics_provider.cc:209: metrics_files_.push_back(std::move(iter)); On 2016/02/04 15:36:43, grt wrote: > metrics_files_.splice(metrics_files_.end(), *files, temp); Done. https://codereview.chromium.org/1537743006/diff/120001/components/metrics/fil... File components/metrics/file_metrics_provider.h (right): https://codereview.chromium.org/1537743006/diff/120001/components/metrics/fil... components/metrics/file_metrics_provider.h:1: // Copyright 2015 The Chromium Authors. All rights reserved. On 2016/02/04 15:36:44, grt wrote: > 2016 :-) Done. https://codereview.chromium.org/1537743006/diff/120001/components/metrics/fil... components/metrics/file_metrics_provider.h:8: #include <atomic> On 2016/02/04 15:36:44, grt wrote: > unused Done. https://codereview.chromium.org/1537743006/diff/120001/components/metrics/fil... components/metrics/file_metrics_provider.h:30: // FileMetricsProvider gathers and logs histograms written to files on disk. On 2016/02/04 15:36:44, grt wrote: > Please add some more details. For example, how often does it check for file(s), > what file(s) does it look for? Does it poll for them? It looks like a scan is > made for files each time the metrics service opens a new log. How often is that? > Also good to explain what RegisterFile does and how preferences are used to > track things. Done. https://codereview.chromium.org/1537743006/diff/120001/components/metrics/fil... components/metrics/file_metrics_provider.h:46: std::string prefs_key; On 2016/02/04 15:36:43, grt wrote: > #include <string> above Done. https://codereview.chromium.org/1537743006/diff/120001/components/metrics/fil... components/metrics/file_metrics_provider.h:47: base::Time last_seen; On 2016/02/04 15:36:44, grt wrote: > #include "base/time/time.h" Done. https://codereview.chromium.org/1537743006/diff/120001/components/metrics/fil... components/metrics/file_metrics_provider.h:51: FileInformation(); On 2016/02/04 15:36:43, grt wrote: > the ctor and dtor should come before the data members as per > https://google.github.io/styleguide/cppguide.html#Declaration_Order Done. https://codereview.chromium.org/1537743006/diff/120001/components/metrics/fil... components/metrics/file_metrics_provider.h:65: void RegisterFile(const base::FilePath& path, FileType type, On 2016/02/04 15:36:43, grt wrote: > please document Done. https://codereview.chromium.org/1537743006/diff/120001/components/metrics/fil... components/metrics/file_metrics_provider.h:66: const std::string& prefs_key); On 2016/02/04 15:36:44, grt wrote: > nit: base::StringPiece prefs_key Done. https://codereview.chromium.org/1537743006/diff/120001/components/metrics/fil... components/metrics/file_metrics_provider.h:68: static void RegisterPrefs(PrefRegistrySimple* prefs, const std::string& key); On 2016/02/04 15:36:43, grt wrote: > nit: base::StringPiece key Done. https://codereview.chromium.org/1537743006/diff/120001/components/metrics/fil... components/metrics/file_metrics_provider.h:71: void OnDidCreateMetricsLog() override; On 2016/02/04 15:36:43, grt wrote: > prefer placing the inherited methods in the private: section unless they need to > be part of this class's public API (see > https://groups.google.com/a/chromium.org/d/msg/chromium-dev/CwzjcWd9cYM/TcUUl... > and > https://groups.google.com/a/chromium.org/d/msg/chromium-dev/MkBEHBx3g_4/_wP74... > for various discussions). Done. https://codereview.chromium.org/1537743006/diff/120001/components/metrics/fil... components/metrics/file_metrics_provider.h:81: static bool CheckAndMapNewMetrics(FileMetricsProvider::FileInformation* file); On 2016/02/04 15:36:43, grt wrote: > "FileMetricsProvider::" not needed here or in definition Done. https://codereview.chromium.org/1537743006/diff/120001/components/metrics/fil... components/metrics/file_metrics_provider.h:88: FileInformationList metrics_files_; On 2016/02/04 15:36:44, grt wrote: > what's the advantage of having two lists? could the loop in > RecordHistogramSnapshots just begin with: > if (!file->mapped) > continue; // Another attempt will be made on the next ScheduleFilesCheck. Unmapped files (i.e. those to be checked for something new) are extracted from this object and passed to the worker task so as to avoid having that "anonymous" task point back to data owned by another object. If they're mixed in the same vector with mapped files, the separation would have to happen at every check. All this multi-thread stuff is a pain. It would be nice if metric collection wasn't on the UI thread in the first place, but that's a battle for a later date...
Patchset #6 (id:140001) has been deleted
https://codereview.chromium.org/1537743006/diff/120001/base/metrics/histogram... File base/metrics/histogram_snapshot_manager.h (right): https://codereview.chromium.org/1537743006/diff/120001/base/metrics/histogram... base/metrics/histogram_snapshot_manager.h:91: void PrepareDeltaTakingOwnership(HistogramBase* histogram); On 2016/02/04 21:51:13, bcwhite wrote: > On 2016/02/04 15:36:42, grt wrote: > > take a scoped_ptr rather than a raw ptr so that the ownership transfer is > > explicit > > These call through to the private PrepareSamples which *doesn't* (and can't) > take a scoped_ptr. I'll have to release the scoped_ptr in the implementation to > pass a raw pointer to that method. Okay? SGTM. https://codereview.chromium.org/1537743006/diff/120001/chrome/installer/setup... File chrome/installer/setup/installer_crash_reporting.cc (right): https://codereview.chromium.org/1537743006/diff/120001/chrome/installer/setup... chrome/installer/setup/installer_crash_reporting.cc:124: #if defined(OS_MACOSX) || defined(OS_WIN) On 2016/02/04 21:51:13, bcwhite wrote: > On 2016/02/04 15:36:42, grt wrote: > > this file is exclusively OS_WIN, so there's no need for the platform-specific > > conditionals here. > > This whole file will not be in the final submit. It was just necessary for it > to compile due to an incomplete port to CrashPad that is currently being > resolved external to this CL. Gotcha. That other CL landed, so you should be able to rebase to pick it up now. https://codereview.chromium.org/1537743006/diff/120001/chrome/installer/setup... File chrome/installer/setup/setup_main.cc (right): https://codereview.chromium.org/1537743006/diff/120001/chrome/installer/setup... chrome/installer/setup/setup_main.cc:1794: // Save all the persistent histograms to a single file on disk for reading On 2016/02/04 21:51:13, bcwhite wrote: > On 2016/02/04 15:36:42, grt wrote: > > using a single file means that we'll get only a random subset of installer UMA > > data. is it possible to not lose data in some reasonable way? multiple files, > > perhaps? > > All things are possible... for a cost. Metrics are reported at launch and every > 30 minutes after that. We'll only fail to report data if there are multiple > runs between those times. > > Once read/write memory-mapped files are supported (a few months down my > timeline), it may be possible to switch to that which will mean that no records > will get lost no matter how many runs occur. Okay. Getting some data is better than none, so I'm okay with this for now. Is there a way to measure how much data we're losing in the current scheme? Please open a new crbug issue for this (r/w mapped files so there's no data loss) so that it isn't forgotten about. https://codereview.chromium.org/1537743006/diff/120001/chrome/installer/setup... chrome/installer/setup/setup_main.cc:1813: if (base::ReplaceFile(tmp_file_path, file_path, nullptr)) { On 2016/02/04 21:51:13, bcwhite wrote: > On 2016/02/04 15:36:42, grt wrote: > > this will fail so long as chrome.exe has the |file_path| mapped in memory > since > > FileMetricsProvider::CheckAndMapNewMetrics opens the file without > SHARE_DELETE. > > Basically part of the multiple-runs problem. FileMetricsProvider only keep the > file mapped until the values are sent, after which it releases it. Right now that window is pretty big (~30m), right? Are you planning to change the Chrome side so that it reads the file during the scanning phase rather than keeping it mapped until histograms are collected? https://codereview.chromium.org/1537743006/diff/120001/components/metrics/fil... File components/metrics/file_metrics_provider.cc (right): https://codereview.chromium.org/1537743006/diff/120001/components/metrics/fil... components/metrics/file_metrics_provider.cc:34: // Start a background check for pending setup metrics so it can be uploaded On 2016/02/04 21:51:14, bcwhite wrote: > On 2016/02/04 15:36:42, grt wrote: > > is it appropriate for this to be generalized so that it isn't hardcoded to be > > specific to setup metrics? otherwise, should FileMetricsProvider be called > > SetupMetricsProvider? > > I started it as something specific for Setup but then others were asking if it > could be used to pass in other metrics from other locations so I rewrote it to > be more generic, basically handling a list of files rather than one file. In that case, could you update this and other references to "setup" in the comments and constants? https://codereview.chromium.org/1537743006/diff/120001/components/metrics/fil... components/metrics/file_metrics_provider.cc:207: metrics_found_.push_back(std::move(iter)); On 2016/02/04 21:51:14, bcwhite wrote: > On 2016/02/04 15:36:43, grt wrote: > > use splice to move the element. you'll have to change from the range-based > loop > > to an iterator-based one like you have on line 78, but this is a small price > to > > pay. for example: > > metrics_found_.splice(metrics_found_.end(), *files, temp); > > Each move in that case has to delete the value from the head of the vector, > shifting all the other ones. metrics_files_ and metrics_found_ are both linked lists. Using splice is the most efficient, as it just does some simple pointer juggling with the list items. I believe that anything else will be more costly. https://codereview.chromium.org/1537743006/diff/120001/components/metrics/fil... File components/metrics/file_metrics_provider.h (right): https://codereview.chromium.org/1537743006/diff/120001/components/metrics/fil... components/metrics/file_metrics_provider.h:59: typedef std::list<scoped_ptr<FileInformation>> FileInformationList; On 2016/02/04 15:36:44, grt wrote: > nit: using FileInformationList = ...; ping (as per http://chromium-cpp.appspot.com/#core-whitelist) https://codereview.chromium.org/1537743006/diff/160001/base/metrics/histogram... File base/metrics/histogram_snapshot_manager.h (right): https://codereview.chromium.org/1537743006/diff/160001/base/metrics/histogram... base/metrics/histogram_snapshot_manager.h:116: std::vector<const HistogramBase*> owned_histograms_; use std::vector<scoped_ptr<HistogramBase>> here. clearing the vector will automagically delete all of the histograms. https://codereview.chromium.org/1537743006/diff/160001/chrome/browser/metrics... File chrome/browser/metrics/chrome_metrics_service_client.cc (right): https://codereview.chromium.org/1537743006/diff/160001/chrome/browser/metrics... chrome/browser/metrics/chrome_metrics_service_client.cc:348: base::CommandLine::ForCurrentProcess()->GetProgram().DirName() use base::PathService::Get(base::DIR_EXE, &dir) instead of GetProgram().DirName() https://codereview.chromium.org/1537743006/diff/160001/chrome/installer/setup... File chrome/installer/setup/installer_metrics.cc (right): https://codereview.chromium.org/1537743006/diff/160001/chrome/installer/setup... chrome/installer/setup/installer_metrics.cc:15: void BeginPersistentHistogramStorage(const char* name) { what's the advantage of taking |name| as an arg rather than hardcoding kSetupHistogramAllocatorName below? https://codereview.chromium.org/1537743006/diff/160001/chrome/installer/setup... chrome/installer/setup/installer_metrics.cc:21: void EndPersistentHistogramStorage(const base::FilePath& target_path) { nit: indentation https://codereview.chromium.org/1537743006/diff/160001/chrome/installer/setup... chrome/installer/setup/installer_metrics.cc:36: base::File histograms(tmp_file_path, #include "base/files/file.h" https://codereview.chromium.org/1537743006/diff/160001/chrome/installer/setup... chrome/installer/setup/installer_metrics.cc:39: base::PersistentMemoryAllocator* alloc = is this the same as |allocator| on line 25? https://codereview.chromium.org/1537743006/diff/160001/chrome/installer/setup... chrome/installer/setup/installer_metrics.cc:41: int used = static_cast<int>(alloc->used()); base::checked_cast? https://codereview.chromium.org/1537743006/diff/160001/chrome/installer/setup... chrome/installer/setup/installer_metrics.cc:48: << file_path.AsUTF8Unsafe(); .value() instead of AsUTF8Unsafe() https://codereview.chromium.org/1537743006/diff/160001/chrome/installer/setup... chrome/installer/setup/installer_metrics.cc:53: base::DeleteFile(tmp_file_path, false); move this up one line so that the file is only deleted if CreateTemporaryFileInDir returns true. may as well put an additional histograms.Close(); above it to handle the case where the file was opened but the write/flush failed. https://codereview.chromium.org/1537743006/diff/160001/chrome/installer/setup... File chrome/installer/setup/installer_metrics.h (right): https://codereview.chromium.org/1537743006/diff/160001/chrome/installer/setup... chrome/installer/setup/installer_metrics.h:1: // Copyright (c) 2016 The Chromium Authors. All rights reserved. nit: no (c) in copyright notices for new files (http://www.chromium.org/developers/coding-style#TOC-File-headers) https://codereview.chromium.org/1537743006/diff/160001/chrome/installer/setup... chrome/installer/setup/installer_metrics.h:14: // Create a persistent space for any histograms that can be reported later nit: "Creates..." as per https://google.github.io/styleguide/cppguide.html#Function_Comments https://codereview.chromium.org/1537743006/diff/160001/chrome/installer/setup... chrome/installer/setup/installer_metrics.h:18: // Save all the persistent histograms to a single file on disk for reading nit: "Saves..." https://codereview.chromium.org/1537743006/diff/160001/components/metrics/fil... File components/metrics/file_metrics_provider.cc (right): https://codereview.chromium.org/1537743006/diff/160001/components/metrics/fil... components/metrics/file_metrics_provider.cc:71: void FileMetricsProvider::OnDidCreateMetricsLog() { nit: move these two functions down below RecordFileAsSeen so that the order in the .cc file matches that in the class definition. looks like some other functions need to be reordered as well. https://codereview.chromium.org/1537743006/diff/160001/components/metrics/fil... components/metrics/file_metrics_provider.cc:169: return false; the |mapped| scoped_ptr must also be reset since its validity is equated with "is the file mapped" elsewhere. https://codereview.chromium.org/1537743006/diff/160001/components/metrics/fil... File components/metrics/file_metrics_provider.h (right): https://codereview.chromium.org/1537743006/diff/160001/components/metrics/fil... components/metrics/file_metrics_provider.h:32: // Any number of files can be registered and the will be polled once per "and the will be polled" https://codereview.chromium.org/1537743006/diff/160001/components/metrics/fil... components/metrics/file_metrics_provider.h:50: // data written to it. Because the file will be actively watched by *this* nit: it -> them since the subject is "Such files" https://codereview.chromium.org/1537743006/diff/160001/components/metrics/fil... components/metrics/file_metrics_provider.h:52: // that do not support delete-while-open (e.g. Windows). Windows does, sorta. If all openers use FLAG_SHARE_DELETE when opening the file, then any opener can also use FLAG_DELETE_ON_CLOSE to trigger the file's evaporation once it's no longer in use. https://codereview.chromium.org/1537743006/diff/160001/components/metrics/fil... components/metrics/file_metrics_provider.h:83: // metadata must persist across process restarts, registry entries are used regarding "registry entries", do you mean "preference values"? AFAICT, the Windows registry isn't used. https://codereview.chromium.org/1537743006/diff/160001/components/metrics/fil... File components/metrics/file_metrics_provider_unittest.cc (right): https://codereview.chromium.org/1537743006/diff/160001/components/metrics/fil... components/metrics/file_metrics_provider_unittest.cc:1: // Copyright 2015 The Chromium Authors. All rights reserved. 2016 https://codereview.chromium.org/1537743006/diff/160001/components/metrics/fil... components/metrics/file_metrics_provider_unittest.cc:23: const char* kMetricsName = "TestMetrics"; nit: const char kMetricsName[] = https://codereview.chromium.org/1537743006/diff/160001/components/metrics/fil... components/metrics/file_metrics_provider_unittest.cc:63: nit: omit blank line https://codereview.chromium.org/1537743006/diff/160001/components/metrics/fil... components/metrics/file_metrics_provider_unittest.cc:90: TEST_F(FileMetricsProviderTest, AccessMetrics) { this looks like a "white box" test that is tightly coupled to the implementation, which makes impl changes more difficult. is it possible to instead make a test that uses a mock/fake MetricsService to ensure that metrics are properly read from the file at the right time? https://codereview.chromium.org/1537743006/diff/160001/components/metrics/met... File components/metrics/metrics_provider.h (right): https://codereview.chromium.org/1537743006/diff/160001/components/metrics/met... components/metrics/metrics_provider.h:11: nit: omit empty lines for a single forward decl like this https://codereview.chromium.org/1537743006/diff/160001/components/metrics/met... components/metrics/metrics_provider.h:73: // snapshot manager. PrepareDeltas() will have been already called and nit "will have already been called" https://codereview.chromium.org/1537743006/diff/160001/components/metrics/met... File components/metrics/metrics_service.cc (right): https://codereview.chromium.org/1537743006/diff/160001/components/metrics/met... components/metrics/metrics_service.cc:1111: for (size_t i = 0; i < metrics_providers_.size(); ++i) { will this work: for (auto& provider : metrics_providers_) provider->RecordHistogramSnapshots(&histogram_snapshot_manager_);
https://codereview.chromium.org/1537743006/diff/160001/chrome/browser/metrics... File chrome/browser/metrics/chrome_metrics_service_client.cc (right): https://codereview.chromium.org/1537743006/diff/160001/chrome/browser/metrics... chrome/browser/metrics/chrome_metrics_service_client.cc:350: .AddExtension(L".pma"), Can this logic be somewhere outside this file? Seems like an implementation detail of what we're trying to watch - maybe make a Create*() method for this provider somewhere?
I've started the switch to taking a passed task-runner but seem to have hit a bug in the TestSimpleTaskRunner that is causing a DCHECK when calling PostTaskAndReply. Still investigating. Still missing is the updated instantiation of the FileMetricsFrovider in the main code. https://codereview.chromium.org/1537743006/diff/120001/base/metrics/histogram... File base/metrics/histogram_snapshot_manager.h (right): https://codereview.chromium.org/1537743006/diff/120001/base/metrics/histogram... base/metrics/histogram_snapshot_manager.h:91: void PrepareDeltaTakingOwnership(HistogramBase* histogram); On 2016/02/08 18:09:18, grt wrote: > On 2016/02/04 21:51:13, bcwhite wrote: > > On 2016/02/04 15:36:42, grt wrote: > > > take a scoped_ptr rather than a raw ptr so that the ownership transfer is > > > explicit > > > > These call through to the private PrepareSamples which *doesn't* (and can't) > > take a scoped_ptr. I'll have to release the scoped_ptr in the implementation > to > > pass a raw pointer to that method. Okay? > > SGTM. Done. https://codereview.chromium.org/1537743006/diff/120001/chrome/installer/setup... File chrome/installer/setup/installer_crash_reporting.cc (right): https://codereview.chromium.org/1537743006/diff/120001/chrome/installer/setup... chrome/installer/setup/installer_crash_reporting.cc:124: #if defined(OS_MACOSX) || defined(OS_WIN) On 2016/02/08 18:09:18, grt wrote: > On 2016/02/04 21:51:13, bcwhite wrote: > > On 2016/02/04 15:36:42, grt wrote: > > > this file is exclusively OS_WIN, so there's no need for the > platform-specific > > > conditionals here. > > > > This whole file will not be in the final submit. It was just necessary for it > > to compile due to an incomplete port to CrashPad that is currently being > > resolved external to this CL. > > Gotcha. That other CL landed, so you should be able to rebase to pick it up now. Done. https://codereview.chromium.org/1537743006/diff/120001/chrome/installer/setup... File chrome/installer/setup/setup_main.cc (right): https://codereview.chromium.org/1537743006/diff/120001/chrome/installer/setup... chrome/installer/setup/setup_main.cc:1794: // Save all the persistent histograms to a single file on disk for reading On 2016/02/08 18:09:18, grt wrote: > On 2016/02/04 21:51:13, bcwhite wrote: > > On 2016/02/04 15:36:42, grt wrote: > > > using a single file means that we'll get only a random subset of installer > UMA > > > data. is it possible to not lose data in some reasonable way? multiple > files, > > > perhaps? > > > > All things are possible... for a cost. Metrics are reported at launch and > every > > 30 minutes after that. We'll only fail to report data if there are multiple > > runs between those times. > > > > Once read/write memory-mapped files are supported (a few months down my > > timeline), it may be possible to switch to that which will mean that no > records > > will get lost no matter how many runs occur. > > Okay. Getting some data is better than none, so I'm okay with this for now. Is > there a way to measure how much data we're losing in the current scheme? Please > open a new crbug issue for this (r/w mapped files so there's no data loss) so > that it isn't forgotten about. I've got some ideas on how to extend this but it needs more thought. It's complicated by the privilege separation (i.e. Chrome can't change, delete, or rename files created by setup). https://codereview.chromium.org/1537743006/diff/120001/chrome/installer/setup... chrome/installer/setup/setup_main.cc:1813: if (base::ReplaceFile(tmp_file_path, file_path, nullptr)) { On 2016/02/08 18:09:18, grt wrote: > On 2016/02/04 21:51:13, bcwhite wrote: > > On 2016/02/04 15:36:42, grt wrote: > > > this will fail so long as chrome.exe has the |file_path| mapped in memory > > since > > > FileMetricsProvider::CheckAndMapNewMetrics opens the file without > > SHARE_DELETE. > > > > Basically part of the multiple-runs problem. FileMetricsProvider only keep > the > > file mapped until the values are sent, after which it releases it. > > Right now that window is pretty big (~30m), right? Are you planning to change > the Chrome side so that it reads the file during the scanning phase rather than > keeping it mapped until histograms are collected? Yes. Either I'll pre-read it into an alternate memory buffer or do everything off the UI thread or... something else. https://codereview.chromium.org/1537743006/diff/120001/components/metrics/fil... File components/metrics/file_metrics_provider.cc (right): https://codereview.chromium.org/1537743006/diff/120001/components/metrics/fil... components/metrics/file_metrics_provider.cc:34: // Start a background check for pending setup metrics so it can be uploaded On 2016/02/08 18:09:18, grt wrote: > On 2016/02/04 21:51:14, bcwhite wrote: > > On 2016/02/04 15:36:42, grt wrote: > > > is it appropriate for this to be generalized so that it isn't hardcoded to > be > > > specific to setup metrics? otherwise, should FileMetricsProvider be called > > > SetupMetricsProvider? > > > > I started it as something specific for Setup but then others were asking if it > > could be used to pass in other metrics from other locations so I rewrote it to > > be more generic, basically handling a list of files rather than one file. > > In that case, could you update this and other references to "setup" in the > comments and constants? Done. https://codereview.chromium.org/1537743006/diff/120001/components/metrics/fil... components/metrics/file_metrics_provider.cc:122: base::HistogramBase* histogram = On 2016/02/04 15:36:43, grt wrote: > use scoped_ptr<base::HistogramBase> rather than a naked pointer since ownership > is transferred Done. https://codereview.chromium.org/1537743006/diff/120001/components/metrics/fil... components/metrics/file_metrics_provider.cc:123: base::GetNextPersistentHistogram(file->allocator.get(), &hist_iter); On 2016/02/04 15:36:43, grt wrote: > please update this and the related functions to return a scoped_ptr rather than > a naked pointer. this makes the ownership explicit in the API Okay. It doesn't belong here, though. I'll do it in a different CL. https://codereview.chromium.org/1537743006/diff/120001/components/metrics/fil... components/metrics/file_metrics_provider.cc:206: if (iter->mapped) { On 2016/02/04 15:36:43, grt wrote: > nit: omit braces Done. https://codereview.chromium.org/1537743006/diff/120001/components/metrics/fil... File components/metrics/file_metrics_provider.h (right): https://codereview.chromium.org/1537743006/diff/120001/components/metrics/fil... components/metrics/file_metrics_provider.h:59: typedef std::list<scoped_ptr<FileInformation>> FileInformationList; On 2016/02/04 15:36:44, grt wrote: > nit: using FileInformationList = ...; Right. I had wanted to talk with you about the reasons for that in person. Thanks for the link. Done. https://codereview.chromium.org/1537743006/diff/120001/components/metrics/fil... components/metrics/file_metrics_provider.h:80: base::Callback<void(FileInformationList*)> callback); On 2016/02/04 15:36:44, grt wrote: > note: pass callbacks by constref, although this callback should go away; see > comment in ScheduleFilesCheck in .cc file Done. https://codereview.chromium.org/1537743006/diff/160001/base/metrics/histogram... File base/metrics/histogram_snapshot_manager.h (right): https://codereview.chromium.org/1537743006/diff/160001/base/metrics/histogram... base/metrics/histogram_snapshot_manager.h:116: std::vector<const HistogramBase*> owned_histograms_; On 2016/02/08 18:09:18, grt wrote: > use std::vector<scoped_ptr<HistogramBase>> here. clearing the vector will > automagically delete all of the histograms. Done. Is this new? A lot of code uses STL utils to clear pointers. https://codereview.chromium.org/1537743006/diff/160001/chrome/browser/metrics... File chrome/browser/metrics/chrome_metrics_service_client.cc (right): https://codereview.chromium.org/1537743006/diff/160001/chrome/browser/metrics... chrome/browser/metrics/chrome_metrics_service_client.cc:348: base::CommandLine::ForCurrentProcess()->GetProgram().DirName() On 2016/02/08 18:09:18, grt wrote: > use base::PathService::Get(base::DIR_EXE, &dir) instead of > GetProgram().DirName() Done. https://codereview.chromium.org/1537743006/diff/160001/chrome/installer/setup... File chrome/installer/setup/installer_metrics.cc (right): https://codereview.chromium.org/1537743006/diff/160001/chrome/installer/setup... chrome/installer/setup/installer_metrics.cc:15: void BeginPersistentHistogramStorage(const char* name) { On 2016/02/08 18:09:19, grt wrote: > what's the advantage of taking |name| as an arg rather than hardcoding > kSetupHistogramAllocatorName below? Each file needs a different preference name because each has a different "last access" time. https://codereview.chromium.org/1537743006/diff/160001/chrome/installer/setup... chrome/installer/setup/installer_metrics.cc:21: void EndPersistentHistogramStorage(const base::FilePath& target_path) { On 2016/02/08 18:09:18, grt wrote: > nit: indentation Done. https://codereview.chromium.org/1537743006/diff/160001/chrome/installer/setup... chrome/installer/setup/installer_metrics.cc:36: base::File histograms(tmp_file_path, On 2016/02/08 18:09:18, grt wrote: > #include "base/files/file.h" Done. https://codereview.chromium.org/1537743006/diff/160001/chrome/installer/setup... chrome/installer/setup/installer_metrics.cc:39: base::PersistentMemoryAllocator* alloc = On 2016/02/08 18:09:18, grt wrote: > is this the same as |allocator| on line 25? Heh. That's what happens when you add code at the top of a file. Done. https://codereview.chromium.org/1537743006/diff/160001/chrome/installer/setup... chrome/installer/setup/installer_metrics.cc:41: int used = static_cast<int>(alloc->used()); On 2016/02/08 18:09:19, grt wrote: > base::checked_cast? Should be size_t. https://codereview.chromium.org/1537743006/diff/160001/chrome/installer/setup... chrome/installer/setup/installer_metrics.cc:48: << file_path.AsUTF8Unsafe(); On 2016/02/08 18:09:18, grt wrote: > .value() instead of AsUTF8Unsafe() Done. https://codereview.chromium.org/1537743006/diff/160001/chrome/installer/setup... chrome/installer/setup/installer_metrics.cc:53: base::DeleteFile(tmp_file_path, false); On 2016/02/08 18:09:19, grt wrote: > move this up one line so that the file is only deleted if > CreateTemporaryFileInDir returns true. may as well put an additional > histograms.Close(); above it to handle the case where the file was opened but > the write/flush failed. Done. https://codereview.chromium.org/1537743006/diff/160001/chrome/installer/setup... File chrome/installer/setup/installer_metrics.h (right): https://codereview.chromium.org/1537743006/diff/160001/chrome/installer/setup... chrome/installer/setup/installer_metrics.h:1: // Copyright (c) 2016 The Chromium Authors. All rights reserved. On 2016/02/08 18:09:19, grt wrote: > nit: no (c) in copyright notices for new files > (http://www.chromium.org/developers/coding-style#TOC-File-headers) Done. https://codereview.chromium.org/1537743006/diff/160001/chrome/installer/setup... chrome/installer/setup/installer_metrics.h:14: // Create a persistent space for any histograms that can be reported later On 2016/02/08 18:09:19, grt wrote: > nit: "Creates..." as per > https://google.github.io/styleguide/cppguide.html#Function_Comments Done. https://codereview.chromium.org/1537743006/diff/160001/chrome/installer/setup... chrome/installer/setup/installer_metrics.h:18: // Save all the persistent histograms to a single file on disk for reading On 2016/02/08 18:09:19, grt wrote: > nit: "Saves..." Done. https://codereview.chromium.org/1537743006/diff/160001/components/metrics/fil... File components/metrics/file_metrics_provider.cc (right): https://codereview.chromium.org/1537743006/diff/160001/components/metrics/fil... components/metrics/file_metrics_provider.cc:71: void FileMetricsProvider::OnDidCreateMetricsLog() { On 2016/02/08 18:09:19, grt wrote: > nit: move these two functions down below RecordFileAsSeen so that the order in > the .cc file matches that in the class definition. looks like some other > functions need to be reordered as well. Done. https://codereview.chromium.org/1537743006/diff/160001/components/metrics/fil... components/metrics/file_metrics_provider.cc:169: return false; On 2016/02/08 18:09:19, grt wrote: > the |mapped| scoped_ptr must also be reset since its validity is equated with > "is the file mapped" elsewhere. Done. https://codereview.chromium.org/1537743006/diff/160001/components/metrics/fil... File components/metrics/file_metrics_provider.h (right): https://codereview.chromium.org/1537743006/diff/160001/components/metrics/fil... components/metrics/file_metrics_provider.h:32: // Any number of files can be registered and the will be polled once per On 2016/02/08 18:09:19, grt wrote: > "and the will be polled" Done. https://codereview.chromium.org/1537743006/diff/160001/components/metrics/fil... components/metrics/file_metrics_provider.h:50: // data written to it. Because the file will be actively watched by *this* On 2016/02/08 18:09:19, grt wrote: > nit: it -> them since the subject is "Such files" Done. https://codereview.chromium.org/1537743006/diff/160001/components/metrics/fil... components/metrics/file_metrics_provider.h:83: // metadata must persist across process restarts, registry entries are used On 2016/02/08 18:09:19, grt wrote: > regarding "registry entries", do you mean "preference values"? AFAICT, the > Windows registry isn't used. Done. Was thinking in terms of "PrefRegistry". https://codereview.chromium.org/1537743006/diff/160001/components/metrics/fil... File components/metrics/file_metrics_provider_unittest.cc (right): https://codereview.chromium.org/1537743006/diff/160001/components/metrics/fil... components/metrics/file_metrics_provider_unittest.cc:1: // Copyright 2015 The Chromium Authors. All rights reserved. On 2016/02/08 18:09:19, grt wrote: > 2016 Done. https://codereview.chromium.org/1537743006/diff/160001/components/metrics/fil... components/metrics/file_metrics_provider_unittest.cc:23: const char* kMetricsName = "TestMetrics"; On 2016/02/08 18:09:19, grt wrote: > nit: const char kMetricsName[] = Done. https://codereview.chromium.org/1537743006/diff/160001/components/metrics/fil... components/metrics/file_metrics_provider_unittest.cc:63: On 2016/02/08 18:09:19, grt wrote: > nit: omit blank line Done. https://codereview.chromium.org/1537743006/diff/160001/components/metrics/fil... components/metrics/file_metrics_provider_unittest.cc:90: TEST_F(FileMetricsProviderTest, AccessMetrics) { On 2016/02/08 18:09:19, grt wrote: > this looks like a "white box" test that is tightly coupled to the > implementation, which makes impl changes more difficult. is it possible to > instead make a test that uses a mock/fake MetricsService to ensure that metrics > are properly read from the file at the right time? Done. https://codereview.chromium.org/1537743006/diff/160001/components/metrics/met... File components/metrics/metrics_provider.h (right): https://codereview.chromium.org/1537743006/diff/160001/components/metrics/met... components/metrics/metrics_provider.h:11: On 2016/02/08 18:09:19, grt wrote: > nit: omit empty lines for a single forward decl like this Done. https://codereview.chromium.org/1537743006/diff/160001/components/metrics/met... components/metrics/metrics_provider.h:73: // snapshot manager. PrepareDeltas() will have been already called and On 2016/02/08 18:09:19, grt wrote: > nit "will have already been called" Done. https://codereview.chromium.org/1537743006/diff/160001/components/metrics/met... File components/metrics/metrics_service.cc (right): https://codereview.chromium.org/1537743006/diff/160001/components/metrics/met... components/metrics/metrics_service.cc:1111: for (size_t i = 0; i < metrics_providers_.size(); ++i) { On 2016/02/08 18:09:19, grt wrote: > will this work: > for (auto& provider : metrics_providers_) > provider->RecordHistogramSnapshots(&histogram_snapshot_manager_); Seems to work. I'll updated it elsewhere in the file, too.
https://codereview.chromium.org/1537743006/diff/160001/chrome/installer/setup... File chrome/installer/setup/installer_metrics.cc (right): https://codereview.chromium.org/1537743006/diff/160001/chrome/installer/setup... chrome/installer/setup/installer_metrics.cc:41: int used = static_cast<int>(alloc->used()); On 2016/02/09 21:08:45, bcwhite wrote: > On 2016/02/08 18:09:19, grt wrote: > > base::checked_cast? > > Should be size_t. Scratch that. Write() returns int. Added comment about how Allocator is internally restricted to 2GB in size.
https://codereview.chromium.org/1537743006/diff/120001/chrome/installer/setup... File chrome/installer/setup/setup_main.cc (right): https://codereview.chromium.org/1537743006/diff/120001/chrome/installer/setup... chrome/installer/setup/setup_main.cc:1794: // Save all the persistent histograms to a single file on disk for reading On 2016/02/09 21:08:45, bcwhite wrote: > On 2016/02/08 18:09:18, grt wrote: > > On 2016/02/04 21:51:13, bcwhite wrote: > > > On 2016/02/04 15:36:42, grt wrote: > > > > using a single file means that we'll get only a random subset of installer > > UMA > > > > data. is it possible to not lose data in some reasonable way? multiple > > files, > > > > perhaps? > > > > > > All things are possible... for a cost. Metrics are reported at launch and > > every > > > 30 minutes after that. We'll only fail to report data if there are multiple > > > runs between those times. > > > > > > Once read/write memory-mapped files are supported (a few months down my > > > timeline), it may be possible to switch to that which will mean that no > > records > > > will get lost no matter how many runs occur. > > > > Okay. Getting some data is better than none, so I'm okay with this for now. Is > > there a way to measure how much data we're losing in the current scheme? > Please > > open a new crbug issue for this (r/w mapped files so there's no data loss) so > > that it isn't forgotten about. > > I've got some ideas on how to extend this but it needs more thought. It's > complicated by the privilege separation (i.e. Chrome can't change, delete, or > rename files created by setup). For a system-level install, setup.exe and chrome.exe can interact via ClientStateMedium. Perhaps chrome.exe can relay its collection state to setup.exe by way of a value there. Happy to brainstorm with you when the time is right. https://codereview.chromium.org/1537743006/diff/160001/base/metrics/histogram... File base/metrics/histogram_snapshot_manager.h (right): https://codereview.chromium.org/1537743006/diff/160001/base/metrics/histogram... base/metrics/histogram_snapshot_manager.h:116: std::vector<const HistogramBase*> owned_histograms_; On 2016/02/09 21:08:45, bcwhite wrote: > On 2016/02/08 18:09:18, grt wrote: > > use std::vector<scoped_ptr<HistogramBase>> here. clearing the vector will > > automagically delete all of the histograms. > > Done. Is this new? A lot of code uses STL utils to clear pointers. It is fairly new, yes: https://groups.google.com/a/chromium.org/d/msg/chromium-dev/PizeR1qUJdk/JQd0a... https://codereview.chromium.org/1537743006/diff/160001/chrome/installer/setup... File chrome/installer/setup/installer_metrics.cc (right): https://codereview.chromium.org/1537743006/diff/160001/chrome/installer/setup... chrome/installer/setup/installer_metrics.cc:15: void BeginPersistentHistogramStorage(const char* name) { On 2016/02/09 21:08:45, bcwhite wrote: > On 2016/02/08 18:09:19, grt wrote: > > what's the advantage of taking |name| as an arg rather than hardcoding > > kSetupHistogramAllocatorName below? > > Each file needs a different preference name because each has a different "last > access" time. Will setup_main.cc eventually call this multiple times with different names? https://codereview.chromium.org/1537743006/diff/160001/chrome/installer/setup... chrome/installer/setup/installer_metrics.cc:41: int used = static_cast<int>(alloc->used()); On 2016/02/09 21:08:45, bcwhite wrote: > On 2016/02/08 18:09:19, grt wrote: > > base::checked_cast? > > Should be size_t. File::Write takes its |size| arg as an int and returns an int. It's probably simplest for |used| to be an int, so either use checked_cast (so that the process crashes if |used| is ever impossibly large) or do something else safe to convert the size_t (an unsigned 64-bit int on x64) to int (a signed 32-bit int everywhere we care about). https://codereview.chromium.org/1537743006/diff/200001/chrome/installer/setup... File chrome/installer/setup/installer_crash_reporting.cc (right): https://codereview.chromium.org/1537743006/diff/200001/chrome/installer/setup... chrome/installer/setup/installer_crash_reporting.cc:126: #if defined(OS_MACOSX) || defined(OS_WIN) stale line
Just uploaded new version that is running from end-to-end. Ran into some difficulties with closing the mapped file (needs to be done on I/O thread) which complicated things. https://codereview.chromium.org/1537743006/diff/160001/chrome/installer/setup... File chrome/installer/setup/installer_metrics.cc (right): https://codereview.chromium.org/1537743006/diff/160001/chrome/installer/setup... chrome/installer/setup/installer_metrics.cc:15: void BeginPersistentHistogramStorage(const char* name) { On 2016/02/10 16:01:53, grt wrote: > On 2016/02/09 21:08:45, bcwhite wrote: > > On 2016/02/08 18:09:19, grt wrote: > > > what's the advantage of taking |name| as an arg rather than hardcoding > > > kSetupHistogramAllocatorName below? > > > > Each file needs a different preference name because each has a different "last > > access" time. > > Will setup_main.cc eventually call this multiple times with different names? Probably not. Just seemed a better abstraction. Would you prefer it hard-coded?
Patchset #9 (id:220001) has been deleted
https://codereview.chromium.org/1537743006/diff/160001/chrome/installer/setup... File chrome/installer/setup/installer_metrics.cc (right): https://codereview.chromium.org/1537743006/diff/160001/chrome/installer/setup... chrome/installer/setup/installer_metrics.cc:15: void BeginPersistentHistogramStorage(const char* name) { On 2016/02/10 16:11:24, bcwhite wrote: > On 2016/02/10 16:01:53, grt wrote: > > On 2016/02/09 21:08:45, bcwhite wrote: > > > On 2016/02/08 18:09:19, grt wrote: > > > > what's the advantage of taking |name| as an arg rather than hardcoding > > > > kSetupHistogramAllocatorName below? > > > > > > Each file needs a different preference name because each has a different > "last > > > access" time. > > > > Will setup_main.cc eventually call this multiple times with different names? > > Probably not. Just seemed a better abstraction. Would you prefer it > hard-coded? Sine this file is for exclusive use by the installer, I don't see the benefit to passing in the value.
https://codereview.chromium.org/1537743006/diff/160001/chrome/installer/setup... File chrome/installer/setup/installer_metrics.cc (right): https://codereview.chromium.org/1537743006/diff/160001/chrome/installer/setup... chrome/installer/setup/installer_metrics.cc:15: void BeginPersistentHistogramStorage(const char* name) { On 2016/02/10 20:28:46, grt wrote: > On 2016/02/10 16:11:24, bcwhite wrote: > > On 2016/02/10 16:01:53, grt wrote: > > > On 2016/02/09 21:08:45, bcwhite wrote: > > > > On 2016/02/08 18:09:19, grt wrote: > > > > > what's the advantage of taking |name| as an arg rather than hardcoding > > > > > kSetupHistogramAllocatorName below? > > > > > > > > Each file needs a different preference name because each has a different > > "last > > > > access" time. > > > > > > Will setup_main.cc eventually call this multiple times with different names? > > > > Probably not. Just seemed a better abstraction. Would you prefer it > > hard-coded? > > Sine this file is for exclusive use by the installer, I don't see the benefit to > passing in the value. Done.
Patchset #12 (id:300001) has been deleted
Patchset #12 (id:320001) has been deleted
Patchset #13 (id:360001) has been deleted
Patchset #13 (id:380001) has been deleted
Patchset #13 (id:400001) has been deleted
Patchset #13 (id:420001) has been deleted
Patchset #13 (id:440001) has been deleted
Patchset #13 (id:460001) has been deleted
Patchset #13 (id:480001) has been deleted
There are some testing changes I'm going to split into another CL but the rest is ready for re-review.
almost there! https://codereview.chromium.org/1537743006/diff/160001/chrome/browser/metrics... File chrome/browser/metrics/chrome_metrics_service_client.cc (right): https://codereview.chromium.org/1537743006/diff/160001/chrome/browser/metrics... chrome/browser/metrics/chrome_metrics_service_client.cc:350: .AddExtension(L".pma"), On 2016/02/09 19:48:15, Alexei Svitkine wrote: > Can this logic be somewhere outside this file? Seems like an implementation > detail of what we're trying to watch - maybe make a Create*() method for this > provider somewhere? ping https://codereview.chromium.org/1537743006/diff/160001/components/metrics/fil... File components/metrics/file_metrics_provider.h (right): https://codereview.chromium.org/1537743006/diff/160001/components/metrics/fil... components/metrics/file_metrics_provider.h:52: // that do not support delete-while-open (e.g. Windows). On 2016/02/08 18:09:19, grt wrote: > Windows does, sorta. If all openers use FLAG_SHARE_DELETE when opening the file, > then any opener can also use FLAG_DELETE_ON_CLOSE to trigger the file's > evaporation once it's no longer in use. Please update the comment here, as Windows does support delete-while-open. https://codereview.chromium.org/1537743006/diff/500001/base/metrics/histogram... File base/metrics/histogram_persistence.cc (right): https://codereview.chromium.org/1537743006/diff/500001/base/metrics/histogram... base/metrics/histogram_persistence.cc:488: for (;;) { fyi for the future: "while (true)" is more common for infloops in src/chrome. https://codereview.chromium.org/1537743006/diff/500001/base/metrics/histogram... File base/metrics/histogram_snapshot_manager.cc (right): https://codereview.chromium.org/1537743006/diff/500001/base/metrics/histogram... base/metrics/histogram_snapshot_manager.cc:14: const int kNewInconsistency = (int)0x80000000; nit: replace c-style cast with a C++ cast https://codereview.chromium.org/1537743006/diff/500001/base/metrics/histogram... base/metrics/histogram_snapshot_manager.cc:70: for (auto& iter : known_histograms_) { |iter| isn't an iterator, but rather a pair of a histogram name's hash digest and a SampleInfo. Please give this a more obvious name, such as digest_and_info or something. https://codereview.chromium.org/1537743006/diff/500001/base/metrics/histogram... base/metrics/histogram_snapshot_manager.cc:95: sample_info->histogram = nullptr; just curious: should |inconsistencies| also be cleared since the underlying HistogramBase object is destroyed on line 98? https://codereview.chromium.org/1537743006/diff/500001/base/metrics/histogram... File base/metrics/histogram_snapshot_manager.h (right): https://codereview.chromium.org/1537743006/diff/500001/base/metrics/histogram... base/metrics/histogram_snapshot_manager.h:83: SampleInfo() : histogram(nullptr), move the initializers down to the members themselves as per "Non-Static Class Member Initializers" in http://chromium-cpp.appspot.com/#core-whitelist https://codereview.chromium.org/1537743006/diff/500001/base/metrics/histogram... base/metrics/histogram_snapshot_manager.h:100: int inconsistencies; use unsigned types for bitfields since some shifts are undefined for signed types. even if you're not doing any shifting, it's a simple rule of thumb to follow globally. uint32_t is the same size as |int|, so that seems like a good choice here. https://codereview.chromium.org/1537743006/diff/500001/base/metrics/statistic... File base/metrics/statistics_recorder.cc (right): https://codereview.chromium.org/1537743006/diff/500001/base/metrics/statistic... base/metrics/statistics_recorder.cc:396: // This leaks a lot of objects but they've allready been annotated for such. "already" https://codereview.chromium.org/1537743006/diff/500001/chrome/browser/metrics... File chrome/browser/metrics/chrome_metrics_service_client.cc (right): https://codereview.chromium.org/1537743006/diff/500001/chrome/browser/metrics... chrome/browser/metrics/chrome_metrics_service_client.cc:348: new metrics::FileMetricsProvider(content::BrowserThread::GetBlockingPool(), passing in the SequencedWorkerPool itself like this will result in the call to CheckAndMapNewMetricFilesOnTaskRunner blocking shutdown. i think it's better to use: content::BrowserThread::GetBlockingPool()->GetTaskRunnerWithShutdownBehavior(base::SequencedWorkerPool::CONTINUE_ON_SHUTDOWN) so that the task is abandoned if the browser is shut down while it's running. https://codereview.chromium.org/1537743006/diff/500001/chrome/installer/setup... File chrome/installer/setup/installer_metrics.cc (right): https://codereview.chromium.org/1537743006/diff/500001/chrome/installer/setup... chrome/installer/setup/installer_metrics.cc:44: // Allocator doesn't support more than 2GB so can never overflow. 1GB? (kSegmentMaxSize is 1 << 30) https://codereview.chromium.org/1537743006/diff/500001/components/metrics/fil... File components/metrics/file_metrics_provider.cc (right): https://codereview.chromium.org/1537743006/diff/500001/components/metrics/fil... components/metrics/file_metrics_provider.cc:110: base::Unretained(this), base::Owned(check_list))); IIUC, this FileMetricsProvider instance will be deleted during shutdown in the MetricsService's dtor. if i'm reading the tea leaves, this happens in BrowserProcessImpl::StartTearDown. the reply task here could potentially be run after that point, leading to a use-after-free. to protect against this, add a WeakPtrFactory member to this class, and bind the instance with weak_factory_.GetWeakPtr() rather than Unretained. this way, the reply is safely dropped if it is run after the provider is destroyed. https://codereview.chromium.org/1537743006/diff/500001/components/metrics/fil... components/metrics/file_metrics_provider.cc:111: DCHECK(posted); i don't think this is a condition that should break developers (by causing their browser to crash). it simply means that the browser is in its shutdown phase, which is valid, no? https://codereview.chromium.org/1537743006/diff/500001/components/metrics/fil... components/metrics/file_metrics_provider.cc:115: // Move each processed file to either the "to-read" list (for processing) or DCHECK_CURRENTLY_ON(content::BrowserThread::UI); at the top of the function https://codereview.chromium.org/1537743006/diff/500001/components/metrics/fil... components/metrics/file_metrics_provider.cc:137: // Move finished metric files back to list of monitored files. DCHECK_CURRENTLY_ON(content::BrowserThread::UI); at the top of the function https://codereview.chromium.org/1537743006/diff/500001/components/metrics/fil... components/metrics/file_metrics_provider.cc:154: for (auto& file : files_to_read_) { DCHECK_CURRENTLY_ON(content::BrowserThread::UI); at the top of the function https://codereview.chromium.org/1537743006/diff/500001/components/metrics/fil... components/metrics/file_metrics_provider.cc:185: for (;;) { nit: while (true) is more common https://codereview.chromium.org/1537743006/diff/500001/components/metrics/fil... components/metrics/file_metrics_provider.cc:194: histogram_count++; although either pre- or post-increment is allowed for fundamental types like this (https://google.github.io/styleguide/cppguide.html#Preincrement_and_Predecrement), it's easier to read/review code when postincrement is a rare exception used only when needed. so please use ++histogram_count here. https://codereview.chromium.org/1537743006/diff/500001/components/metrics/fil... components/metrics/file_metrics_provider.cc:196: VLOG(1) << "Reported " << histogram_count << " histograms from " production logging carries weight in the form of the strings and the code required for the logging. unless there is a compelling reason for this to be present in the product, please use DVLOG. (note that the installer is one exception where production logging is extraordinarily useful and is allowed/encouraged.) https://codereview.chromium.org/1537743006/diff/500001/components/metrics/fil... components/metrics/file_metrics_provider.cc:203: file->allocator_to_release.swap(file->allocator); since the allocator isn't released until the next pass through OnDidCreateMetricsLog, doesn't this mean that chrome keeps the installer's metrics file open pretty much all the time? this will prevent the installer from providing new metrics at all while chrome is in use. i had thought this was going to change so that OnDidCreateMetricsLog would trigger an immediate read of the metrics file into a buffer so that the file could be closed immediately. https://codereview.chromium.org/1537743006/diff/500001/components/metrics/fil... File components/metrics/file_metrics_provider.h (right): https://codereview.chromium.org/1537743006/diff/500001/components/metrics/fil... components/metrics/file_metrics_provider.h:57: // declaration) so it can be known to tests. this is no longer true. is now a good time to move it into the .cc file? https://codereview.chromium.org/1537743006/diff/500001/components/metrics/fil... components/metrics/file_metrics_provider.h:66: scoped_ptr<base::MemoryMappedFile> mapped; i think it would be useful to document the various valid states for these members. it's a bit complicated. https://codereview.chromium.org/1537743006/diff/500001/components/metrics/fil... File components/metrics/file_metrics_provider_unittest.cc (right): https://codereview.chromium.org/1537743006/diff/500001/components/metrics/fil... components/metrics/file_metrics_provider_unittest.cc:92: ThreadTaskRunnerHandle reply_runner_; nit: call this thread_task_runner_handle_. https://codereview.chromium.org/1537743006/diff/500001/components/metrics/fil... components/metrics/file_metrics_provider_unittest.cc:119: EXPECT_TRUE(writer.IsValid()); should this be ASSERT_TRUE? is there any point in the test proceeding if this fails? same for the Write call below. https://codereview.chromium.org/1537743006/diff/500001/components/metrics/met... File components/metrics/metrics_pref_names.cc (right): https://codereview.chromium.org/1537743006/diff/500001/components/metrics/met... components/metrics/metrics_pref_names.cc:59: // The last-seen timestamp of setup metrics file. should this be something like // The prefix of the last-seen timestamps for persistent histogram files. Values are named for the files themselves.
https://codereview.chromium.org/1537743006/diff/160001/chrome/browser/metrics... File chrome/browser/metrics/chrome_metrics_service_client.cc (right): https://codereview.chromium.org/1537743006/diff/160001/chrome/browser/metrics... chrome/browser/metrics/chrome_metrics_service_client.cc:350: .AddExtension(L".pma"), On 2016/02/09 19:48:15, Alexei Svitkine wrote: > Can this logic be somewhere outside this file? Seems like an implementation > detail of what we're trying to watch - maybe make a Create*() method for this > provider somewhere? How about just a constant that has both the file name and extension together? The alternative would be a function that takes a dir-name and adds both. https://codereview.chromium.org/1537743006/diff/160001/components/metrics/fil... File components/metrics/file_metrics_provider.h (right): https://codereview.chromium.org/1537743006/diff/160001/components/metrics/fil... components/metrics/file_metrics_provider.h:52: // that do not support delete-while-open (e.g. Windows). On 2016/02/08 18:09:19, grt wrote: > Windows does, sorta. If all openers use FLAG_SHARE_DELETE when opening the file, > then any opener can also use FLAG_DELETE_ON_CLOSE to trigger the file's > evaporation once it's no longer in use. Done, but differently since on reflection, the file should probably not be deleted anyway. https://codereview.chromium.org/1537743006/diff/500001/base/metrics/histogram... File base/metrics/histogram_persistence.cc (right): https://codereview.chromium.org/1537743006/diff/500001/base/metrics/histogram... base/metrics/histogram_persistence.cc:488: for (;;) { On 2016/02/15 15:42:39, grt wrote: > fyi for the future: "while (true)" is more common for infloops in src/chrome. Done. https://codereview.chromium.org/1537743006/diff/500001/base/metrics/histogram... File base/metrics/histogram_snapshot_manager.cc (right): https://codereview.chromium.org/1537743006/diff/500001/base/metrics/histogram... base/metrics/histogram_snapshot_manager.cc:14: const int kNewInconsistency = (int)0x80000000; On 2016/02/15 15:42:39, grt wrote: > nit: replace c-style cast with a C++ cast Done. https://codereview.chromium.org/1537743006/diff/500001/base/metrics/histogram... base/metrics/histogram_snapshot_manager.cc:70: for (auto& iter : known_histograms_) { On 2016/02/15 15:42:39, grt wrote: > |iter| isn't an iterator, but rather a pair of a histogram name's hash digest > and a SampleInfo. Please give this a more obvious name, such as digest_and_info > or something. Done. https://codereview.chromium.org/1537743006/diff/500001/base/metrics/histogram... base/metrics/histogram_snapshot_manager.cc:95: sample_info->histogram = nullptr; On 2016/02/15 15:42:39, grt wrote: > just curious: should |inconsistencies| also be cleared since the underlying > HistogramBase object is destroyed on line 98? It needs to remain to prevent multiple reports of the inconsistency. The next time a snapshot is created, the inconsistency will still be present but we don't want to continuously report such to the server. https://codereview.chromium.org/1537743006/diff/500001/base/metrics/histogram... File base/metrics/histogram_snapshot_manager.h (right): https://codereview.chromium.org/1537743006/diff/500001/base/metrics/histogram... base/metrics/histogram_snapshot_manager.h:83: SampleInfo() : histogram(nullptr), On 2016/02/15 15:42:39, grt wrote: > move the initializers down to the members themselves as per "Non-Static Class > Member Initializers" in http://chromium-cpp.appspot.com/#core-whitelist Cool. I didn't even know about that. https://codereview.chromium.org/1537743006/diff/500001/base/metrics/histogram... base/metrics/histogram_snapshot_manager.h:100: int inconsistencies; On 2016/02/15 15:42:39, grt wrote: > use unsigned types for bitfields since some shifts are undefined for signed > types. even if you're not doing any shifting, it's a simple rule of thumb to > follow globally. uint32_t is the same size as |int|, so that seems like a good > choice here. Okay but I'll do it elsewhere, either in the parent CL that defined this or 3rd CL since it also goes into existing code. https://codereview.chromium.org/1537743006/diff/500001/base/metrics/statistic... File base/metrics/statistics_recorder.cc (right): https://codereview.chromium.org/1537743006/diff/500001/base/metrics/statistic... base/metrics/statistics_recorder.cc:396: // This leaks a lot of objects but they've allready been annotated for such. On 2016/02/15 15:42:39, grt wrote: > "already" Done. https://codereview.chromium.org/1537743006/diff/500001/chrome/browser/metrics... File chrome/browser/metrics/chrome_metrics_service_client.cc (right): https://codereview.chromium.org/1537743006/diff/500001/chrome/browser/metrics... chrome/browser/metrics/chrome_metrics_service_client.cc:348: new metrics::FileMetricsProvider(content::BrowserThread::GetBlockingPool(), On 2016/02/15 15:42:39, grt wrote: > passing in the SequencedWorkerPool itself like this will result in the call to > CheckAndMapNewMetricFilesOnTaskRunner blocking shutdown. i think it's better to > use: > > content::BrowserThread::GetBlockingPool()->GetTaskRunnerWithShutdownBehavior(base::SequencedWorkerPool::CONTINUE_ON_SHUTDOWN) > so that the task is abandoned if the browser is shut down while it's running. Done. https://codereview.chromium.org/1537743006/diff/500001/chrome/installer/setup... File chrome/installer/setup/installer_metrics.cc (right): https://codereview.chromium.org/1537743006/diff/500001/chrome/installer/setup... chrome/installer/setup/installer_metrics.cc:44: // Allocator doesn't support more than 2GB so can never overflow. On 2016/02/15 15:42:39, grt wrote: > 1GB? (kSegmentMaxSize is 1 << 30) Still technically true! :-D Done. https://codereview.chromium.org/1537743006/diff/500001/components/metrics/fil... File components/metrics/file_metrics_provider.cc (right): https://codereview.chromium.org/1537743006/diff/500001/components/metrics/fil... components/metrics/file_metrics_provider.cc:110: base::Unretained(this), base::Owned(check_list))); On 2016/02/15 15:42:39, grt wrote: > IIUC, this FileMetricsProvider instance will be deleted during shutdown in the > MetricsService's dtor. if i'm reading the tea leaves, this happens in > BrowserProcessImpl::StartTearDown. the reply task here could potentially be run > after that point, leading to a use-after-free. to protect against this, add a > WeakPtrFactory member to this class, and bind the instance with > weak_factory_.GetWeakPtr() rather than Unretained. this way, the reply is safely > dropped if it is run after the provider is destroyed. Done. https://codereview.chromium.org/1537743006/diff/500001/components/metrics/fil... components/metrics/file_metrics_provider.cc:111: DCHECK(posted); On 2016/02/15 15:42:39, grt wrote: > i don't think this is a condition that should break developers (by causing their > browser to crash). it simply means that the browser is in its shutdown phase, > which is valid, no? Done. https://codereview.chromium.org/1537743006/diff/500001/components/metrics/fil... components/metrics/file_metrics_provider.cc:115: // Move each processed file to either the "to-read" list (for processing) or On 2016/02/15 15:42:39, grt wrote: > DCHECK_CURRENTLY_ON(content::BrowserThread::UI); > at the top of the function This is in "components". Can it access "content"? Also, this breaks the tests because neither TestSimpleTaskRunner nor the test thread match this check. https://codereview.chromium.org/1537743006/diff/500001/components/metrics/fil... components/metrics/file_metrics_provider.cc:185: for (;;) { On 2016/02/15 15:42:39, grt wrote: > nit: while (true) is more common Done. https://codereview.chromium.org/1537743006/diff/500001/components/metrics/fil... components/metrics/file_metrics_provider.cc:194: histogram_count++; On 2016/02/15 15:42:39, grt wrote: > although either pre- or post-increment is allowed for fundamental types like > this > (https://google.github.io/styleguide/cppguide.html#Preincrement_and_Predecrement), > it's easier to read/review code when postincrement is a rare exception used only > when needed. so please use ++histogram_count here. Done. https://codereview.chromium.org/1537743006/diff/500001/components/metrics/fil... components/metrics/file_metrics_provider.cc:196: VLOG(1) << "Reported " << histogram_count << " histograms from " On 2016/02/15 15:42:39, grt wrote: > production logging carries weight in the form of the strings and the code > required for the logging. unless there is a compelling reason for this to be > present in the product, please use DVLOG. (note that the installer is one > exception where production logging is extraordinarily useful and is > allowed/encouraged.) Done. https://codereview.chromium.org/1537743006/diff/500001/components/metrics/fil... components/metrics/file_metrics_provider.cc:203: file->allocator_to_release.swap(file->allocator); On 2016/02/15 15:42:39, grt wrote: > since the allocator isn't released until the next pass through > OnDidCreateMetricsLog, doesn't this mean that chrome keeps the installer's > metrics file open pretty much all the time? this will prevent the installer from > providing new metrics at all while chrome is in use. i had thought this was > going to change so that OnDidCreateMetricsLog would trigger an immediate read of > the metrics file into a buffer so that the file could be closed immediately. The call to OnDidCreateMetricsLog should be called every 30 minutes. As it happens, the RecordHistogramSnapshots is called at the end of the period, just before OnDidCreateMetricsLog is called again. This doesn't end up changing how long the file is open. Also, the file is only open if it's been modified so most of the time it will not be open. Yes, that is the plan. Or to move it to its own thread where it call all be done in RecordHistogramSnapshots. https://codereview.chromium.org/1537743006/diff/500001/components/metrics/fil... File components/metrics/file_metrics_provider.h (right): https://codereview.chromium.org/1537743006/diff/500001/components/metrics/fil... components/metrics/file_metrics_provider.h:57: // declaration) so it can be known to tests. On 2016/02/15 15:42:39, grt wrote: > this is no longer true. is now a good time to move it into the .cc file? Done. https://codereview.chromium.org/1537743006/diff/500001/components/metrics/fil... components/metrics/file_metrics_provider.h:66: scoped_ptr<base::MemoryMappedFile> mapped; On 2016/02/15 15:42:39, grt wrote: > i think it would be useful to document the various valid states for these > members. it's a bit complicated. Done. https://codereview.chromium.org/1537743006/diff/500001/components/metrics/fil... File components/metrics/file_metrics_provider_unittest.cc (right): https://codereview.chromium.org/1537743006/diff/500001/components/metrics/fil... components/metrics/file_metrics_provider_unittest.cc:92: ThreadTaskRunnerHandle reply_runner_; On 2016/02/15 15:42:40, grt wrote: > nit: call this thread_task_runner_handle_. Done. https://codereview.chromium.org/1537743006/diff/500001/components/metrics/fil... components/metrics/file_metrics_provider_unittest.cc:119: EXPECT_TRUE(writer.IsValid()); On 2016/02/15 15:42:40, grt wrote: > should this be ASSERT_TRUE? is there any point in the test proceeding if this > fails? same for the Write call below. Done. https://codereview.chromium.org/1537743006/diff/500001/components/metrics/met... File components/metrics/metrics_pref_names.cc (right): https://codereview.chromium.org/1537743006/diff/500001/components/metrics/met... components/metrics/metrics_pref_names.cc:59: // The last-seen timestamp of setup metrics file. On 2016/02/15 15:42:40, grt wrote: > should this be something like > // The prefix of the last-seen timestamps for persistent histogram files. Values > are named for the files themselves. Done.
code lgtm. does it make sense to read the file into RAM from the get-go rather than keeping it mapped? https://codereview.chromium.org/1537743006/diff/500001/base/metrics/histogram... File base/metrics/histogram_snapshot_manager.cc (right): https://codereview.chromium.org/1537743006/diff/500001/base/metrics/histogram... base/metrics/histogram_snapshot_manager.cc:95: sample_info->histogram = nullptr; On 2016/02/15 19:22:10, bcwhite wrote: > On 2016/02/15 15:42:39, grt wrote: > > just curious: should |inconsistencies| also be cleared since the underlying > > HistogramBase object is destroyed on line 98? > > It needs to remain to prevent multiple reports of the inconsistency. The next > time a snapshot is created, the inconsistency will still be present but we don't > want to continuously report such to the server. groovy, thanks https://codereview.chromium.org/1537743006/diff/500001/base/metrics/histogram... File base/metrics/histogram_snapshot_manager.h (right): https://codereview.chromium.org/1537743006/diff/500001/base/metrics/histogram... base/metrics/histogram_snapshot_manager.h:100: int inconsistencies; On 2016/02/15 19:22:10, bcwhite wrote: > On 2016/02/15 15:42:39, grt wrote: > > use unsigned types for bitfields since some shifts are undefined for signed > > types. even if you're not doing any shifting, it's a simple rule of thumb to > > follow globally. uint32_t is the same size as |int|, so that seems like a good > > choice here. > > Okay but I'll do it elsewhere, either in the parent CL that defined this or 3rd > CL since it also goes into existing code. sgtm https://codereview.chromium.org/1537743006/diff/500001/components/metrics/fil... File components/metrics/file_metrics_provider.cc (right): https://codereview.chromium.org/1537743006/diff/500001/components/metrics/fil... components/metrics/file_metrics_provider.cc:115: // Move each processed file to either the "to-read" list (for processing) or On 2016/02/15 19:22:11, bcwhite wrote: > On 2016/02/15 15:42:39, grt wrote: > > DCHECK_CURRENTLY_ON(content::BrowserThread::UI); > > at the top of the function > > This is in "components". Can it access "content"? > > Also, this breaks the tests because neither TestSimpleTaskRunner nor the test > thread match this check. Ah, bummer. Another way to ensure that methods aren't called on the wrong threads is to add a ThreadChecker instance to the class, and sprinkle DCHECK(thread_checker_.CalledOnValidThread()); here and there. https://codereview.chromium.org/1537743006/diff/500001/components/metrics/fil... components/metrics/file_metrics_provider.cc:203: file->allocator_to_release.swap(file->allocator); On 2016/02/15 19:22:10, bcwhite wrote: > On 2016/02/15 15:42:39, grt wrote: > > since the allocator isn't released until the next pass through > > OnDidCreateMetricsLog, doesn't this mean that chrome keeps the installer's > > metrics file open pretty much all the time? this will prevent the installer > from > > providing new metrics at all while chrome is in use. i had thought this was > > going to change so that OnDidCreateMetricsLog would trigger an immediate read > of > > the metrics file into a buffer so that the file could be closed immediately. > > The call to OnDidCreateMetricsLog should be called every 30 minutes. As it > happens, the RecordHistogramSnapshots is called at the end of the period, just > before OnDidCreateMetricsLog is called again. This doesn't end up changing how > long the file is open. > > Also, the file is only open if it's been modified so most of the time it will > not be open. Ah, okay, so it'll generally be locked only during a single 30min window. That's better than I'd first imagined. > Yes, that is the plan. Or to move it to its own thread where it call all be > done in RecordHistogramSnapshots. Is there a reason not to do the "slurp into RAM" change now? https://codereview.chromium.org/1537743006/diff/520001/components/metrics/fil... File components/metrics/file_metrics_provider.h (right): https://codereview.chromium.org/1537743006/diff/520001/components/metrics/fil... components/metrics/file_metrics_provider.h:93: base::WeakPtrFactory<FileMetricsProvider> weak_factory_; move this down so it's the last data member as per the comment in weak_ptr.h.
https://codereview.chromium.org/1537743006/diff/500001/components/metrics/fil... File components/metrics/file_metrics_provider.cc (right): https://codereview.chromium.org/1537743006/diff/500001/components/metrics/fil... components/metrics/file_metrics_provider.cc:115: // Move each processed file to either the "to-read" list (for processing) or On 2016/02/15 19:53:55, grt wrote: > On 2016/02/15 19:22:11, bcwhite wrote: > > On 2016/02/15 15:42:39, grt wrote: > > > DCHECK_CURRENTLY_ON(content::BrowserThread::UI); > > > at the top of the function > > > > This is in "components". Can it access "content"? > > > > Also, this breaks the tests because neither TestSimpleTaskRunner nor the test > > thread match this check. > > Ah, bummer. Another way to ensure that methods aren't called on the wrong > threads is to add a ThreadChecker instance to the class, and sprinkle > DCHECK(thread_checker_.CalledOnValidThread()); here and there. Done. https://codereview.chromium.org/1537743006/diff/500001/components/metrics/fil... components/metrics/file_metrics_provider.cc:137: // Move finished metric files back to list of monitored files. On 2016/02/15 15:42:39, grt wrote: > DCHECK_CURRENTLY_ON(content::BrowserThread::UI); > at the top of the function Done. https://codereview.chromium.org/1537743006/diff/500001/components/metrics/fil... components/metrics/file_metrics_provider.cc:154: for (auto& file : files_to_read_) { On 2016/02/15 15:42:39, grt wrote: > DCHECK_CURRENTLY_ON(content::BrowserThread::UI); > at the top of the function Done. https://codereview.chromium.org/1537743006/diff/500001/components/metrics/fil... components/metrics/file_metrics_provider.cc:203: file->allocator_to_release.swap(file->allocator); On 2016/02/15 19:53:55, grt wrote: > On 2016/02/15 19:22:10, bcwhite wrote: > > On 2016/02/15 15:42:39, grt wrote: > > > since the allocator isn't released until the next pass through > > > OnDidCreateMetricsLog, doesn't this mean that chrome keeps the installer's > > > metrics file open pretty much all the time? this will prevent the installer > > from > > > providing new metrics at all while chrome is in use. i had thought this was > > > going to change so that OnDidCreateMetricsLog would trigger an immediate > read > > of > > > the metrics file into a buffer so that the file could be closed immediately. > > > > The call to OnDidCreateMetricsLog should be called every 30 minutes. As it > > happens, the RecordHistogramSnapshots is called at the end of the period, just > > before OnDidCreateMetricsLog is called again. This doesn't end up changing > how > > long the file is open. > > > > Also, the file is only open if it's been modified so most of the time it will > > not be open. > > Ah, okay, so it'll generally be locked only during a single 30min window. That's > better than I'd first imagined. > > > Yes, that is the plan. Or to move it to its own thread where it call all be > > done in RecordHistogramSnapshots. > > Is there a reason not to do the "slurp into RAM" change now? You know... That was more trouble than each little but but now it'll just be easier to rip out the hacks needed to manage multiple threads and do that. Done. https://codereview.chromium.org/1537743006/diff/520001/components/metrics/fil... File components/metrics/file_metrics_provider.h (right): https://codereview.chromium.org/1537743006/diff/520001/components/metrics/fil... components/metrics/file_metrics_provider.h:93: base::WeakPtrFactory<FileMetricsProvider> weak_factory_; On 2016/02/15 19:53:55, grt wrote: > move this down so it's the last data member as per the comment in weak_ptr.h. Done.
i'm finding it harder to understand the code since it tries to handle two cases, only one of which is really implemented. does it make sense for this to purely support ATOMIC files for the moment, and add support for ACTIVE files independently? i have the sense that the current code may need to change quite a bit for ACTIVE files. https://codereview.chromium.org/1537743006/diff/540001/components/metrics/fil... File components/metrics/file_metrics_provider.cc (right): https://codereview.chromium.org/1537743006/diff/540001/components/metrics/fil... components/metrics/file_metrics_provider.cc:95: bool FileMetricsProvider::CheckAndMapNewMetrics( this return val is ignored, so why not make it a void func? https://codereview.chromium.org/1537743006/diff/540001/components/metrics/fil... components/metrics/file_metrics_provider.cc:111: if (!file->mapped->Initialize(file->path)) { are ACTIVE files meant to be opened such that they're shared with another proc, whereas ATOMIC files aren't? https://codereview.chromium.org/1537743006/diff/540001/components/metrics/fil... components/metrics/file_metrics_provider.cc:120: file->size = file->mapped->length(); if you make the |data| member a std::vector<uint8_t> (or char, whichever is easier), these three lines can become: file->data.assign(file->mapped->data(), file->mapped->data() + file->mapped->length()); or possibly: file->data.reset(new std::vector<char>(file->mapped->data(), file->mapped->data() + file->mapped->length())); you also don't need the |size| param https://codereview.chromium.org/1537743006/diff/540001/components/metrics/fil... components/metrics/file_metrics_provider.cc:124: } for atomic files, stash the file's last modification time here since the file may change between now and when the snapshots are recorded. this value is the one that should be persisted in prefs. https://codereview.chromium.org/1537743006/diff/540001/components/metrics/fil... components/metrics/file_metrics_provider.cc:175: files_to_check_.splice(files_to_check_.end(), files_to_read_, temp); are only ATOMIC files meant to move back to the check list? https://codereview.chromium.org/1537743006/diff/540001/components/metrics/fil... components/metrics/file_metrics_provider.cc:211: LOG(ERROR) << "Metrics file \"" << file->path.value() i'm not sure that this meets the bar for needing a production log message. under what conditions would you get the message from a user, and what would they be asked to do about it? maybe it's better to bump an UMA metric when this happens so that we can see how widespread the problem of invalid files is.
>i'm finding it harder to understand the code since it tries to handle two cases, >only one of which is really implemented. does it make sense for this to purely >support ATOMIC files for the moment, and add support for ACTIVE files >independently? i have the sense that the current code may need to change quite a >bit for ACTIVE files. All the support for ACTIVE is there. Really it's just a matter of never unmapping it and calling PrepareDelta() instead of PrepareOnce(). However, it also needs read/write support in MemoryMappedFile which is currently limited to read-only. I can remove those few conditions if you'd prefer. https://codereview.chromium.org/1537743006/diff/540001/components/metrics/fil... File components/metrics/file_metrics_provider.cc (right): https://codereview.chromium.org/1537743006/diff/540001/components/metrics/fil... components/metrics/file_metrics_provider.cc:95: bool FileMetricsProvider::CheckAndMapNewMetrics( On 2016/02/17 20:32:43, grt wrote: > this return val is ignored, so why not make it a void func? It was originally for testing but those tests got removed to be more black-box oriented. I'll remove it. https://codereview.chromium.org/1537743006/diff/540001/components/metrics/fil... components/metrics/file_metrics_provider.cc:111: if (!file->mapped->Initialize(file->path)) { On 2016/02/17 20:32:43, grt wrote: > are ACTIVE files meant to be opened such that they're shared with another proc, > whereas ATOMIC files aren't? Correct. https://codereview.chromium.org/1537743006/diff/540001/components/metrics/fil... components/metrics/file_metrics_provider.cc:120: file->size = file->mapped->length(); On 2016/02/17 20:32:43, grt wrote: > if you make the |data| member a std::vector<uint8_t> (or char, whichever is > easier), these three lines can become: > file->data.assign(file->mapped->data(), file->mapped->data() + > file->mapped->length()); > or possibly: > file->data.reset(new std::vector<char>(file->mapped->data(), > file->mapped->data() + file->mapped->length())); > > you also don't need the |size| param Done. https://codereview.chromium.org/1537743006/diff/540001/components/metrics/fil... components/metrics/file_metrics_provider.cc:124: } On 2016/02/17 20:32:43, grt wrote: > for atomic files, stash the file's last modification time here since the file > may change between now and when the snapshots are recorded. this value is the > one that should be persisted in prefs. Done. https://codereview.chromium.org/1537743006/diff/540001/components/metrics/fil... components/metrics/file_metrics_provider.cc:175: files_to_check_.splice(files_to_check_.end(), files_to_read_, temp); On 2016/02/17 20:32:43, grt wrote: > are only ATOMIC files meant to move back to the check list? I don't expect that non-atomic files will ever move back, no, but it could without losing data or causing problems. https://codereview.chromium.org/1537743006/diff/540001/components/metrics/fil... components/metrics/file_metrics_provider.cc:211: LOG(ERROR) << "Metrics file \"" << file->path.value() On 2016/02/17 20:32:43, grt wrote: > i'm not sure that this meets the bar for needing a production log message. under > what conditions would you get the message from a user, and what would they be > asked to do about it? maybe it's better to bump an UMA metric when this happens > so that we can see how widespread the problem of invalid files is. Acknowledged.
https://codereview.chromium.org/1537743006/diff/160001/chrome/browser/metrics... File chrome/browser/metrics/chrome_metrics_service_client.cc (right): https://codereview.chromium.org/1537743006/diff/160001/chrome/browser/metrics... chrome/browser/metrics/chrome_metrics_service_client.cc:350: .AddExtension(L".pma"), On 2016/02/15 19:22:10, bcwhite wrote: > On 2016/02/09 19:48:15, Alexei Svitkine wrote: > > Can this logic be somewhere outside this file? Seems like an implementation > > detail of what we're trying to watch - maybe make a Create*() method for this > > provider somewhere? > > How about just a constant that has both the file name and extension together? > The alternative would be a function that takes a dir-name and adds both. Sorry, in this comment I meant all the stuff to initialize the FileMetricsProvider(). Can you add a Create*() method for the provider that does this initialization? Having a constant that includes the extension also SGTM but that's orthogonal. https://codereview.chromium.org/1537743006/diff/560001/components/metrics/fil... File components/metrics/file_metrics_provider.cc (right): https://codereview.chromium.org/1537743006/diff/560001/components/metrics/fil... components/metrics/file_metrics_provider.cc:30: base::FilePath path; // Where on disk the file is located. Nit: Put comments above members. https://codereview.chromium.org/1537743006/diff/560001/components/metrics/fil... components/metrics/file_metrics_provider.cc:65: FileInformation* file = new FileInformation(); Store it in a scoped ptr to begin. https://codereview.chromium.org/1537743006/diff/560001/components/metrics/fil... components/metrics/file_metrics_provider.cc:70: if (pref_service_ && !prefs_key.empty()) { Add a comment about why prefs_key would be empty. https://codereview.chromium.org/1537743006/diff/560001/components/metrics/fil... components/metrics/file_metrics_provider.cc:95: FileMetricsProvider::FileInformation* file) { Sounds like this should be a member function of FileMetricsProvider::FileInformation. https://codereview.chromium.org/1537743006/diff/560001/components/metrics/fil... components/metrics/file_metrics_provider.cc:125: return; Nit: Remove. https://codereview.chromium.org/1537743006/diff/560001/components/metrics/fil... components/metrics/file_metrics_provider.cc:188: for (auto& file : files_to_read_) { The loop body is pretty long - is it possible to make some helper functions to make it more concise and easier to follow at a high level what it's doing without reading all the details? https://codereview.chromium.org/1537743006/diff/560001/components/metrics/fil... File components/metrics/file_metrics_provider.h (right): https://codereview.chromium.org/1537743006/diff/560001/components/metrics/fil... components/metrics/file_metrics_provider.h:35: : public metrics::MetricsProvider { Nit: Fits on line above? https://codereview.chromium.org/1537743006/diff/560001/components/metrics/fil... components/metrics/file_metrics_provider.h:58: struct FileInformation; Nit: How about FileInfo and FileInfoList to make things a little bit less verbose? https://codereview.chromium.org/1537743006/diff/560001/components/metrics/fil... components/metrics/file_metrics_provider.h:61: using FileInformationList = std::list<scoped_ptr<FileInformation>>; Can this be moved to the private: section below? Along with the "struct FileInformation" above? https://codereview.chromium.org/1537743006/diff/560001/components/metrics/fil... components/metrics/file_metrics_provider.h:72: const base::StringPiece& prefs_key); Pass by value. Same below. https://codereview.chromium.org/1537743006/diff/560001/components/metrics/fil... components/metrics/file_metrics_provider.h:74: static void RegisterPrefs(PrefRegistrySimple* prefs, Add a comment. https://codereview.chromium.org/1537743006/diff/560001/components/metrics/fil... components/metrics/file_metrics_provider.h:81: static void CheckAndMapNewMetricFilesOnTaskRunner( Please add a short comment for each of these. https://codereview.chromium.org/1537743006/diff/560001/components/metrics/fil... components/metrics/file_metrics_provider.h:94: FileInformationList files_to_check_; Add comments about all these members except for obvious ones (like thread_checker_, weak_factory_). https://codereview.chromium.org/1537743006/diff/560001/components/metrics/fil... File components/metrics/file_metrics_provider_unittest.cc (right): https://codereview.chromium.org/1537743006/diff/560001/components/metrics/fil... components/metrics/file_metrics_provider_unittest.cc:23: using namespace base; Nit: I'm not a fan of using namespace in metrics code - especially since base is short. Just prefix things where needed. https://codereview.chromium.org/1537743006/diff/560001/components/metrics/met... File components/metrics/metrics_provider.cc (right): https://codereview.chromium.org/1537743006/diff/560001/components/metrics/met... components/metrics/metrics_provider.cc:48: base::HistogramSnapshotManager* hsm) { Nit: snapshot_manager https://codereview.chromium.org/1537743006/diff/560001/components/metrics/met... File components/metrics/metrics_provider.h (right): https://codereview.chromium.org/1537743006/diff/560001/components/metrics/met... components/metrics/metrics_provider.h:74: virtual void RecordHistogramSnapshots(base::HistogramSnapshotManager* hsm); Nit: snapshot_manager https://codereview.chromium.org/1537743006/diff/560001/components/metrics/met... File components/metrics/metrics_service.cc (right): https://codereview.chromium.org/1537743006/diff/560001/components/metrics/met... components/metrics/metrics_service.cc:1118: for (auto& provider : metrics_providers_) Nit: "for (MetricsProvider* provider : metrics_providers_)" Here and in other places where you're iterating over the providers.
Patchset #17 (id:580001) has been deleted
https://codereview.chromium.org/1537743006/diff/160001/chrome/browser/metrics... File chrome/browser/metrics/chrome_metrics_service_client.cc (right): https://codereview.chromium.org/1537743006/diff/160001/chrome/browser/metrics... chrome/browser/metrics/chrome_metrics_service_client.cc:350: .AddExtension(L".pma"), On 2016/02/17 21:55:35, Alexei Svitkine wrote: > On 2016/02/15 19:22:10, bcwhite wrote: > > On 2016/02/09 19:48:15, Alexei Svitkine wrote: > > > Can this logic be somewhere outside this file? Seems like an implementation > > > detail of what we're trying to watch - maybe make a Create*() method for > this > > > provider somewhere? > > > > How about just a constant that has both the file name and extension together? > > The alternative would be a function that takes a dir-name and adds both. > > Sorry, in this comment I meant all the stuff to initialize the > FileMetricsProvider(). Can you add a Create*() method for the provider that does > this initialization? Having a constant that includes the extension also SGTM but > that's orthogonal. As in class FileMetricsProvider { ... static scoped_ptr<FileMetricsProvider> CreateForFile(...) {...} ... } ??? This would effectively merge two calls into one but still have to include all the parameters of both. It also assumes only one file to be registered. Do I have that right? You sure? https://codereview.chromium.org/1537743006/diff/560001/components/metrics/fil... File components/metrics/file_metrics_provider.cc (right): https://codereview.chromium.org/1537743006/diff/560001/components/metrics/fil... components/metrics/file_metrics_provider.cc:30: base::FilePath path; // Where on disk the file is located. On 2016/02/17 21:55:35, Alexei Svitkine wrote: > Nit: Put comments above members. Done. https://codereview.chromium.org/1537743006/diff/560001/components/metrics/fil... components/metrics/file_metrics_provider.cc:65: FileInformation* file = new FileInformation(); On 2016/02/17 21:55:35, Alexei Svitkine wrote: > Store it in a scoped ptr to begin. Done. https://codereview.chromium.org/1537743006/diff/560001/components/metrics/fil... components/metrics/file_metrics_provider.cc:70: if (pref_service_ && !prefs_key.empty()) { On 2016/02/17 21:55:35, Alexei Svitkine wrote: > Add a comment about why prefs_key would be empty. Done. https://codereview.chromium.org/1537743006/diff/560001/components/metrics/fil... components/metrics/file_metrics_provider.cc:95: FileMetricsProvider::FileInformation* file) { On 2016/02/17 21:55:35, Alexei Svitkine wrote: > Sounds like this should be a member function of > FileMetricsProvider::FileInformation. I wanted FileInformation to be information storage only and keep the logic in FileMetricsProvider. https://codereview.chromium.org/1537743006/diff/560001/components/metrics/fil... components/metrics/file_metrics_provider.cc:125: return; On 2016/02/17 21:55:35, Alexei Svitkine wrote: > Nit: Remove. Done. https://codereview.chromium.org/1537743006/diff/560001/components/metrics/fil... components/metrics/file_metrics_provider.cc:188: for (auto& file : files_to_read_) { On 2016/02/17 21:55:35, Alexei Svitkine wrote: > The loop body is pretty long - is it possible to make some helper functions to > make it more concise and easier to follow at a high level what it's doing > without reading all the details? Done. https://codereview.chromium.org/1537743006/diff/560001/components/metrics/fil... File components/metrics/file_metrics_provider.h (right): https://codereview.chromium.org/1537743006/diff/560001/components/metrics/fil... components/metrics/file_metrics_provider.h:35: : public metrics::MetricsProvider { On 2016/02/17 21:55:35, Alexei Svitkine wrote: > Nit: Fits on line above? Done. https://codereview.chromium.org/1537743006/diff/560001/components/metrics/fil... components/metrics/file_metrics_provider.h:58: struct FileInformation; On 2016/02/17 21:55:35, Alexei Svitkine wrote: > Nit: How about FileInfo and FileInfoList to make things a little bit less > verbose? Heh. I think this is the first time I've been asked to _condense_ my code. :-) Done. https://codereview.chromium.org/1537743006/diff/560001/components/metrics/fil... components/metrics/file_metrics_provider.h:61: using FileInformationList = std::list<scoped_ptr<FileInformation>>; On 2016/02/17 21:55:35, Alexei Svitkine wrote: > Can this be moved to the private: section below? Along with the "struct > FileInformation" above? Done. https://codereview.chromium.org/1537743006/diff/560001/components/metrics/fil... components/metrics/file_metrics_provider.h:72: const base::StringPiece& prefs_key); On 2016/02/17 21:55:35, Alexei Svitkine wrote: > Pass by value. Same below. Done. https://codereview.chromium.org/1537743006/diff/560001/components/metrics/fil... components/metrics/file_metrics_provider.h:74: static void RegisterPrefs(PrefRegistrySimple* prefs, On 2016/02/17 21:55:35, Alexei Svitkine wrote: > Add a comment. Done. https://codereview.chromium.org/1537743006/diff/560001/components/metrics/fil... components/metrics/file_metrics_provider.h:81: static void CheckAndMapNewMetricFilesOnTaskRunner( On 2016/02/17 21:55:35, Alexei Svitkine wrote: > Please add a short comment for each of these. Done. https://codereview.chromium.org/1537743006/diff/560001/components/metrics/fil... components/metrics/file_metrics_provider.h:94: FileInformationList files_to_check_; On 2016/02/17 21:55:35, Alexei Svitkine wrote: > Add comments about all these members except for obvious ones (like > thread_checker_, weak_factory_). Done. https://codereview.chromium.org/1537743006/diff/560001/components/metrics/fil... File components/metrics/file_metrics_provider_unittest.cc (right): https://codereview.chromium.org/1537743006/diff/560001/components/metrics/fil... components/metrics/file_metrics_provider_unittest.cc:23: using namespace base; On 2016/02/17 21:55:36, Alexei Svitkine wrote: > Nit: I'm not a fan of using namespace in metrics code - especially since base is > short. Just prefix things where needed. Even in unittest code? Okay. Done. https://codereview.chromium.org/1537743006/diff/560001/components/metrics/met... File components/metrics/metrics_provider.cc (right): https://codereview.chromium.org/1537743006/diff/560001/components/metrics/met... components/metrics/metrics_provider.cc:48: base::HistogramSnapshotManager* hsm) { On 2016/02/17 21:55:36, Alexei Svitkine wrote: > Nit: snapshot_manager Done. https://codereview.chromium.org/1537743006/diff/560001/components/metrics/met... File components/metrics/metrics_provider.h (right): https://codereview.chromium.org/1537743006/diff/560001/components/metrics/met... components/metrics/metrics_provider.h:74: virtual void RecordHistogramSnapshots(base::HistogramSnapshotManager* hsm); On 2016/02/17 21:55:36, Alexei Svitkine wrote: > Nit: snapshot_manager Done. https://codereview.chromium.org/1537743006/diff/560001/components/metrics/met... File components/metrics/metrics_service.cc (right): https://codereview.chromium.org/1537743006/diff/560001/components/metrics/met... components/metrics/metrics_service.cc:1118: for (auto& provider : metrics_providers_) On 2016/02/17 21:55:36, Alexei Svitkine wrote: > Nit: "for (MetricsProvider* provider : metrics_providers_)" > > Here and in other places where you're iterating over the providers. Done.
https://codereview.chromium.org/1537743006/diff/160001/chrome/browser/metrics... File chrome/browser/metrics/chrome_metrics_service_client.cc (right): https://codereview.chromium.org/1537743006/diff/160001/chrome/browser/metrics... chrome/browser/metrics/chrome_metrics_service_client.cc:350: .AddExtension(L".pma"), On 2016/02/18 01:46:56, bcwhite wrote: > On 2016/02/17 21:55:35, Alexei Svitkine wrote: > > On 2016/02/15 19:22:10, bcwhite wrote: > > > On 2016/02/09 19:48:15, Alexei Svitkine wrote: > > > > Can this logic be somewhere outside this file? Seems like an > implementation > > > > detail of what we're trying to watch - maybe make a Create*() method for > > this > > > > provider somewhere? > > > > > > How about just a constant that has both the file name and extension > together? > > > The alternative would be a function that takes a dir-name and adds both. > > > > Sorry, in this comment I meant all the stuff to initialize the > > FileMetricsProvider(). Can you add a Create*() method for the provider that > does > > this initialization? Having a constant that includes the extension also SGTM > but > > that's orthogonal. > > As in > class FileMetricsProvider { > ... > static scoped_ptr<FileMetricsProvider> CreateForFile(...) {...} > ... > } > ??? > > This would effectively merge two calls into one but still have to include all > the parameters of both. It also assumes only one file to be registered. > > Do I have that right? You sure? Sorry, not exactly what I meant. I mean not having all this code inline here. You can still keep it in this file, but make a helper function in the anon namespace, so as not to have the specific logic be inside the function that adds all the providers (as this function is already long enough).
https://codereview.chromium.org/1537743006/diff/620001/base/metrics/histogram... File base/metrics/histogram_snapshot_manager.h (right): https://codereview.chromium.org/1537743006/diff/620001/base/metrics/histogram... base/metrics/histogram_snapshot_manager.h:97: unsigned inconsistencies = 0; this should explicitly be uint3_t or another sized type. "unsigned" is banned based on my read of https://google.github.io/styleguide/cppguide.html#Integer_Types. https://codereview.chromium.org/1537743006/diff/620001/components/metrics/fil... File components/metrics/file_metrics_provider.cc (right): https://codereview.chromium.org/1537743006/diff/620001/components/metrics/fil... components/metrics/file_metrics_provider.cc:251: if (file->mapped || !file->data.empty()) { nit: omit braces https://codereview.chromium.org/1537743006/diff/620001/components/metrics/fil... File components/metrics/file_metrics_provider.h (right): https://codereview.chromium.org/1537743006/diff/620001/components/metrics/fil... components/metrics/file_metrics_provider.h:54: private: get rid of this private block, moving FileInfo down to line 82 or thereabouts
Patchset #19 (id:640001) has been deleted
https://codereview.chromium.org/1537743006/diff/160001/chrome/browser/metrics... File chrome/browser/metrics/chrome_metrics_service_client.cc (right): https://codereview.chromium.org/1537743006/diff/160001/chrome/browser/metrics... chrome/browser/metrics/chrome_metrics_service_client.cc:350: .AddExtension(L".pma"), On 2016/02/18 15:50:32, Alexei Svitkine wrote: > On 2016/02/18 01:46:56, bcwhite wrote: > > On 2016/02/17 21:55:35, Alexei Svitkine wrote: > > > On 2016/02/15 19:22:10, bcwhite wrote: > > > > On 2016/02/09 19:48:15, Alexei Svitkine wrote: > > > > > Can this logic be somewhere outside this file? Seems like an > > implementation > > > > > detail of what we're trying to watch - maybe make a Create*() method for > > > this > > > > > provider somewhere? > > > > > > > > How about just a constant that has both the file name and extension > > together? > > > > The alternative would be a function that takes a dir-name and adds both. > > > > > > Sorry, in this comment I meant all the stuff to initialize the > > > FileMetricsProvider(). Can you add a Create*() method for the provider that > > does > > > this initialization? Having a constant that includes the extension also SGTM > > but > > > that's orthogonal. > > > > As in > > class FileMetricsProvider { > > ... > > static scoped_ptr<FileMetricsProvider> CreateForFile(...) {...} > > ... > > } > > ??? > > > > This would effectively merge two calls into one but still have to include all > > the parameters of both. It also assumes only one file to be registered. > > > > Do I have that right? You sure? > > Sorry, not exactly what I meant. I mean not having all this code inline here. > You can still keep it in this file, but make a helper function in the anon > namespace, so as not to have the specific logic be inside the function that adds > all the providers (as this function is already long enough). Done. https://codereview.chromium.org/1537743006/diff/620001/base/metrics/histogram... File base/metrics/histogram_snapshot_manager.h (right): https://codereview.chromium.org/1537743006/diff/620001/base/metrics/histogram... base/metrics/histogram_snapshot_manager.h:97: unsigned inconsistencies = 0; On 2016/02/18 16:44:07, grt wrote: > this should explicitly be uint3_t or another sized type. "unsigned" is banned > based on my read of > https://google.github.io/styleguide/cppguide.html#Integer_Types. Done. https://codereview.chromium.org/1537743006/diff/620001/components/metrics/fil... File components/metrics/file_metrics_provider.cc (right): https://codereview.chromium.org/1537743006/diff/620001/components/metrics/fil... components/metrics/file_metrics_provider.cc:251: if (file->mapped || !file->data.empty()) { On 2016/02/18 16:44:07, grt wrote: > nit: omit braces Done. https://codereview.chromium.org/1537743006/diff/620001/components/metrics/fil... File components/metrics/file_metrics_provider.h (right): https://codereview.chromium.org/1537743006/diff/620001/components/metrics/fil... components/metrics/file_metrics_provider.h:54: private: On 2016/02/18 16:44:07, grt wrote: > get rid of this private block, moving FileInfo down to line 82 or thereabouts Done.
https://codereview.chromium.org/1537743006/diff/660001/components/metrics/fil... File components/metrics/file_metrics_provider.h (right): https://codereview.chromium.org/1537743006/diff/660001/components/metrics/fil... components/metrics/file_metrics_provider.h:86: static int CheckAndMapNewMetrics(FileInfo* file); Document the return value in the comment. Actually, if it's returning an enum, declare the enum in the .h file and use that type as the return value. Alternatively, move the function entirely to the .cc file in the anon namespace. https://codereview.chromium.org/1537743006/diff/660001/components/metrics/fil... components/metrics/file_metrics_provider.h:97: base::HistogramSnapshotManager* snapshot_manager, FileInfo* file); Nit: 1 param per line if the first one wraps.
https://codereview.chromium.org/1537743006/diff/660001/components/metrics/fil... File components/metrics/file_metrics_provider.h (right): https://codereview.chromium.org/1537743006/diff/660001/components/metrics/fil... components/metrics/file_metrics_provider.h:86: static int CheckAndMapNewMetrics(FileInfo* file); On 2016/02/18 20:06:16, Alexei Svitkine wrote: > Document the return value in the comment. > > Actually, if it's returning an enum, declare the enum in the .h file and use > that type as the return value. > > Alternatively, move the function entirely to the .cc file in the anon namespace. I tried doing only a forward-declaration of the enum in the header file but that's not allowed. I'll have to fully define all the values in .h even though they're never used anywhere but the .cc file. Still what you want? The function can be anonymous because it needs access to FileInfo which is private to FileMetricsProvider.
https://codereview.chromium.org/1537743006/diff/660001/components/metrics/fil... File components/metrics/file_metrics_provider.h (right): https://codereview.chromium.org/1537743006/diff/660001/components/metrics/fil... components/metrics/file_metrics_provider.h:86: static int CheckAndMapNewMetrics(FileInfo* file); On 2016/02/18 20:13:31, bcwhite wrote: > On 2016/02/18 20:06:16, Alexei Svitkine wrote: > > Document the return value in the comment. > > > > Actually, if it's returning an enum, declare the enum in the .h file and use > > that type as the return value. > > > > Alternatively, move the function entirely to the .cc file in the anon > namespace. > > I tried doing only a forward-declaration of the enum in the header file but > that's not allowed. One nice perk to the new "enum class" over old "enum" is that they can be forward decl'd. > I'll have to fully define all the values in .h even though > they're never used anywhere but the .cc file. Still what you want? > > The function can be anonymous because it needs access to FileInfo which is > private to FileMetricsProvider.
Sure, just have the full enum be in the private section. On 18 Feb 2016 3:13 p.m., <bcwhite@chromium.org> wrote: > > https://codereview.chromium.org/1537743006/diff/660001/components/metrics/fil... > File components/metrics/file_metrics_provider.h (right): > https://codereview.chromium.org/1537743006/diff/660001/components/metrics/fil... > components/metrics/file_metrics_provider.h:86 <https://codereview.chromium.org/1537743006/diff/660001/components/metrics/fil...>: static int > CheckAndMapNewMetrics(FileInfo* file); > On 2016/02/18 20:06:16, Alexei Svitkine wrote: > > Document the return value in the comment. > > > > Actually, if it's returning an enum, declare the enum in the .h file > and use > > that type as the return value. > > > > Alternatively, move the function entirely to the .cc file in the anon > namespace. > > I tried doing only a forward-declaration of the enum in the header file > but that's not allowed. I'll have to fully define all the values in .h > even though they're never used anywhere but the .cc file. Still what > you want? > > The function can be anonymous because it needs access to FileInfo which > is private to FileMetricsProvider. > https://codereview.chromium.org/1537743006/ > > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
https://codereview.chromium.org/1537743006/diff/660001/components/metrics/fil... File components/metrics/file_metrics_provider.h (right): https://codereview.chromium.org/1537743006/diff/660001/components/metrics/fil... components/metrics/file_metrics_provider.h:86: static int CheckAndMapNewMetrics(FileInfo* file); On 2016/02/18 20:06:16, Alexei Svitkine wrote: > Document the return value in the comment. > > Actually, if it's returning an enum, declare the enum in the .h file and use > that type as the return value. > > Alternatively, move the function entirely to the .cc file in the anon namespace. Done. I tried Greg's solution using "enum class" but it doesn't play nice with the histogram macros that do an "MAX + 1" operation. https://codereview.chromium.org/1537743006/diff/660001/components/metrics/fil... components/metrics/file_metrics_provider.h:97: base::HistogramSnapshotManager* snapshot_manager, FileInfo* file); On 2016/02/18 20:06:16, Alexei Svitkine wrote: > Nit: 1 param per line if the first one wraps. Done.
https://codereview.chromium.org/1537743006/diff/660001/components/metrics/fil... File components/metrics/file_metrics_provider.h (right): https://codereview.chromium.org/1537743006/diff/660001/components/metrics/fil... components/metrics/file_metrics_provider.h:86: static int CheckAndMapNewMetrics(FileInfo* file); On 2016/02/18 22:23:05, bcwhite wrote: > On 2016/02/18 20:06:16, Alexei Svitkine wrote: > > Document the return value in the comment. > > > > Actually, if it's returning an enum, declare the enum in the .h file and use > > that type as the return value. > > > > Alternatively, move the function entirely to the .cc file in the anon > namespace. > > Done. > > I tried Greg's solution using "enum class" but it doesn't play nice with the > histogram macros that do an "MAX + 1" operation. Yeah, you need creative casting to make it work. This is unfortunate, since "enum class" is so great in all other respects.
Haven't finished completely reviewing this, but here are my comments so far. Plan to finish reviewing this afternoon. https://codereview.chromium.org/1537743006/diff/680001/base/metrics/histogram... File base/metrics/histogram_persistence.cc (right): https://codereview.chromium.org/1537743006/diff/680001/base/metrics/histogram... base/metrics/histogram_persistence.cc:216: // Fix by calling GetCreateHistogramResultHistogram() before setting I don't understand this comment. Is it meant to be a TODO or something? If not, can you rephrase to explain what you mean here? (i.e. What does "Fix" mean? Fixing what? Who will be doing the fixing?) https://codereview.chromium.org/1537743006/diff/680001/base/metrics/histogram... File base/metrics/histogram_samples.cc (right): https://codereview.chromium.org/1537743006/diff/680001/base/metrics/histogram... base/metrics/histogram_samples.cc:76: if (!meta_->id) Nit: == 0, since it's a number. However, I don't understand why this if is needed. If the above DCHECK is always true, shouldn't the if be a no-op? Or is |meta_| read-only or something? If it's something like the latter, needs a comment. https://codereview.chromium.org/1537743006/diff/680001/components/metrics/fil... File components/metrics/file_metrics_provider.cc (right): https://codereview.chromium.org/1537743006/diff/680001/components/metrics/fil... components/metrics/file_metrics_provider.cc:45: // memory as a file ("active") or local memory ("atomic") and is assigned an This comment should explicitly mention |data| and |mapped| and how they relate. i.e. if it's atomic, |data| will have its content and |mapped| will be cleared. https://codereview.chromium.org/1537743006/diff/680001/components/metrics/fil... components/metrics/file_metrics_provider.cc:78: prefs_key.as_string())); Nit: Please cache prefs_key.as_string() as a local since you use it in two places in this function. https://codereview.chromium.org/1537743006/diff/680001/components/metrics/fil... components/metrics/file_metrics_provider.cc:155: FileInfoList* check_list = new FileInfoList(); Nit: I'd add a comment about where this will be freed, since it took me a while to spot the base::Owned below. https://codereview.chromium.org/1537743006/diff/680001/components/metrics/fil... components/metrics/file_metrics_provider.cc:168: int histogram_count = 0; Nit: Move this to right above the while. https://codereview.chromium.org/1537743006/diff/680001/components/metrics/fil... components/metrics/file_metrics_provider.cc:208: DCHECK(thread_checker_.CalledOnValidThread()); Nit: Can you add this to all the other methods in this file? e.g. RegisterFile() https://codereview.chromium.org/1537743006/diff/680001/components/metrics/fil... components/metrics/file_metrics_provider.cc:235: auto temp = iter++; This loop structure is pretty messy. There's no cleaner way to do this because of splice()? https://codereview.chromium.org/1537743006/diff/680001/components/metrics/fil... components/metrics/file_metrics_provider.cc:252: for (auto& file : files_to_read_) { Nit: (FileInfo* file : files_to_read_) Same in other places where you do the iteration.
All done. Will post and upload after next set of comments is received.
https://codereview.chromium.org/1537743006/diff/680001/base/metrics/histogram... File base/metrics/histogram_snapshot_manager.cc (right): https://codereview.chromium.org/1537743006/diff/680001/base/metrics/histogram... base/metrics/histogram_snapshot_manager.cc:114: owned_histogram.reset(histogram); I don't think we need this extra complexity. Storing the histogram until Finished unconditionally should be fine in all cases. If so, you can simplify the code by reverting changes to this function and just have the *TakeOwnership() functions directly add to |owned_histograms_| after calling this. https://codereview.chromium.org/1537743006/diff/680001/chrome/browser/metrics... File chrome/browser/metrics/chrome_metrics_service_client.cc (right): https://codereview.chromium.org/1537743006/diff/680001/chrome/browser/metrics... chrome/browser/metrics/chrome_metrics_service_client.cc:141: void RegisterFileMetricsProvider(metrics::MetricsService* metrics_service) { Name this so it's obvious this is for the Installer metrics. e.g. RegisterInstallerFileMetricsProvider() https://codereview.chromium.org/1537743006/diff/680001/chrome/installer/setup... File chrome/installer/setup/installer_metrics.cc (right): https://codereview.chromium.org/1537743006/diff/680001/chrome/installer/setup... chrome/installer/setup/installer_metrics.cc:48: histograms.Flush()) { Can you use the utility function WriteFile() to simplify this logic? https://code.google.com/p/chromium/codesearch#chromium/src/base/files/file_ut... https://codereview.chromium.org/1537743006/diff/680001/chrome/installer/setup... File chrome/installer/setup/installer_metrics.h (right): https://codereview.chromium.org/1537743006/diff/680001/chrome/installer/setup... chrome/installer/setup/installer_metrics.h:19: // by the browser so it can be reported. Nit: Can you expand the comment to indicate whether it's an error to call this multiple times? Or is it valid if you have pairs of Begin() and End() calls? The naming scheme implies that the latter might be supported, but looking at the code, I'm not so sure. https://codereview.chromium.org/1537743006/diff/680001/chrome/installer/util/... File chrome/installer/util/util_constants.h (right): https://codereview.chromium.org/1537743006/diff/680001/chrome/installer/util/... chrome/installer/util/util_constants.h:256: // Name of allocator (and associated file) for storing histograms to be Nit: Name of the allocator https://codereview.chromium.org/1537743006/diff/680001/components/metrics/fil... File components/metrics/file_metrics_provider.h (right): https://codereview.chromium.org/1537743006/diff/680001/components/metrics/fil... components/metrics/file_metrics_provider.h:33: // cycle (at startup and about every 30 minutes thereafter) for data to send. The 30 minutes is a desktop-specific interval. Please rephrase the comment to either mention this as an example "e.g. 30 minutes on desktop" or just not mention the specific interval. https://codereview.chromium.org/1537743006/diff/680001/components/metrics/fil... components/metrics/file_metrics_provider.h:58: // Indicate a file to be monitored and how the file is used. Because some Nit: Indicates https://codereview.chromium.org/1537743006/diff/680001/components/metrics/fil... components/metrics/file_metrics_provider.h:63: void RegisterFile(const base::FilePath& path, FileType type, Nit: 1 param per line.
Patchset #21 (id:700001) has been deleted
https://codereview.chromium.org/1537743006/diff/680001/base/metrics/histogram... File base/metrics/histogram_persistence.cc (right): https://codereview.chromium.org/1537743006/diff/680001/base/metrics/histogram... base/metrics/histogram_persistence.cc:216: // Fix by calling GetCreateHistogramResultHistogram() before setting On 2016/02/19 16:57:00, Alexei Svitkine wrote: > I don't understand this comment. Is it meant to be a TODO or something? > > If not, can you rephrase to explain what you mean here? (i.e. What does "Fix" > mean? Fixing what? Who will be doing the fixing?) Done. https://codereview.chromium.org/1537743006/diff/680001/base/metrics/histogram... File base/metrics/histogram_samples.cc (right): https://codereview.chromium.org/1537743006/diff/680001/base/metrics/histogram... base/metrics/histogram_samples.cc:76: if (!meta_->id) On 2016/02/19 16:57:00, Alexei Svitkine wrote: > Nit: == 0, since it's a number. > > However, I don't understand why this if is needed. If the above DCHECK is always > true, shouldn't the if be a no-op? Or is |meta_| read-only or something? If it's > something like the latter, needs a comment. Done. https://codereview.chromium.org/1537743006/diff/680001/base/metrics/histogram... File base/metrics/histogram_snapshot_manager.cc (right): https://codereview.chromium.org/1537743006/diff/680001/base/metrics/histogram... base/metrics/histogram_snapshot_manager.cc:114: owned_histogram.reset(histogram); On 2016/02/19 18:16:40, Alexei Svitkine wrote: > I don't think we need this extra complexity. Storing the histogram until > Finished unconditionally should be fine in all cases. > > If so, you can simplify the code by reverting changes to this function and just > have the *TakeOwnership() functions directly add to |owned_histograms_| after > calling this. Done. https://codereview.chromium.org/1537743006/diff/680001/chrome/browser/metrics... File chrome/browser/metrics/chrome_metrics_service_client.cc (right): https://codereview.chromium.org/1537743006/diff/680001/chrome/browser/metrics... chrome/browser/metrics/chrome_metrics_service_client.cc:141: void RegisterFileMetricsProvider(metrics::MetricsService* metrics_service) { On 2016/02/19 18:16:40, Alexei Svitkine wrote: > Name this so it's obvious this is for the Installer metrics. > > e.g. > > RegisterInstallerFileMetricsProvider() Done. I suppose the name can be changed back should it ever monitor additional files. https://codereview.chromium.org/1537743006/diff/680001/chrome/installer/setup... File chrome/installer/setup/installer_metrics.cc (right): https://codereview.chromium.org/1537743006/diff/680001/chrome/installer/setup... chrome/installer/setup/installer_metrics.cc:48: histograms.Flush()) { On 2016/02/19 18:16:40, Alexei Svitkine wrote: > Can you use the utility function WriteFile() to simplify this logic? > > https://code.google.com/p/chromium/codesearch#chromium/src/base/files/file_ut... Done. https://codereview.chromium.org/1537743006/diff/680001/chrome/installer/setup... File chrome/installer/setup/installer_metrics.h (right): https://codereview.chromium.org/1537743006/diff/680001/chrome/installer/setup... chrome/installer/setup/installer_metrics.h:19: // by the browser so it can be reported. On 2016/02/19 18:16:40, Alexei Svitkine wrote: > Nit: Can you expand the comment to indicate whether it's an error to call this > multiple times? Or is it valid if you have pairs of Begin() and End() calls? > > The naming scheme implies that the latter might be supported, but looking at the > code, I'm not so sure. Done. https://codereview.chromium.org/1537743006/diff/680001/chrome/installer/util/... File chrome/installer/util/util_constants.h (right): https://codereview.chromium.org/1537743006/diff/680001/chrome/installer/util/... chrome/installer/util/util_constants.h:256: // Name of allocator (and associated file) for storing histograms to be On 2016/02/19 18:16:40, Alexei Svitkine wrote: > Nit: Name of the allocator Done. https://codereview.chromium.org/1537743006/diff/680001/components/metrics/fil... File components/metrics/file_metrics_provider.cc (right): https://codereview.chromium.org/1537743006/diff/680001/components/metrics/fil... components/metrics/file_metrics_provider.cc:45: // memory as a file ("active") or local memory ("atomic") and is assigned an On 2016/02/19 16:57:00, Alexei Svitkine wrote: > This comment should explicitly mention |data| and |mapped| and how they relate. > i.e. if it's atomic, |data| will have its content and |mapped| will be cleared. Done. https://codereview.chromium.org/1537743006/diff/680001/components/metrics/fil... components/metrics/file_metrics_provider.cc:78: prefs_key.as_string())); On 2016/02/19 16:57:00, Alexei Svitkine wrote: > Nit: Please cache prefs_key.as_string() as a local since you use it in two > places in this function. Even better, I can use file->prefs_key which is already a std::string. https://codereview.chromium.org/1537743006/diff/680001/components/metrics/fil... components/metrics/file_metrics_provider.cc:155: FileInfoList* check_list = new FileInfoList(); On 2016/02/19 16:57:00, Alexei Svitkine wrote: > Nit: I'd add a comment about where this will be freed, since it took me a while > to spot the base::Owned below. Done. https://codereview.chromium.org/1537743006/diff/680001/components/metrics/fil... components/metrics/file_metrics_provider.cc:168: int histogram_count = 0; On 2016/02/19 16:57:00, Alexei Svitkine wrote: > Nit: Move this to right above the while. Done. https://codereview.chromium.org/1537743006/diff/680001/components/metrics/fil... components/metrics/file_metrics_provider.cc:208: DCHECK(thread_checker_.CalledOnValidThread()); On 2016/02/19 16:57:00, Alexei Svitkine wrote: > Nit: Can you add this to all the other methods in this file? e.g. RegisterFile() Done. https://codereview.chromium.org/1537743006/diff/680001/components/metrics/fil... components/metrics/file_metrics_provider.cc:235: auto temp = iter++; On 2016/02/19 16:57:00, Alexei Svitkine wrote: > This loop structure is pretty messy. There's no cleaner way to do this because > of splice()? I can't say for sure how ":" iteration would deal with the underlying container being modified during iteration but traditional iterators become invalid if they are deleted. I assume the new style suffers from the same. https://codereview.chromium.org/1537743006/diff/680001/components/metrics/fil... components/metrics/file_metrics_provider.cc:252: for (auto& file : files_to_read_) { On 2016/02/19 16:57:00, Alexei Svitkine wrote: > Nit: (FileInfo* file : files_to_read_) > > Same in other places where you do the iteration. It'll have to be (scoped_ptr<FileInfo>& file : files_to_read_) Done. https://codereview.chromium.org/1537743006/diff/680001/components/metrics/fil... File components/metrics/file_metrics_provider.h (right): https://codereview.chromium.org/1537743006/diff/680001/components/metrics/fil... components/metrics/file_metrics_provider.h:33: // cycle (at startup and about every 30 minutes thereafter) for data to send. On 2016/02/19 18:16:40, Alexei Svitkine wrote: > The 30 minutes is a desktop-specific interval. Please rephrase the comment to > either mention this as an example "e.g. 30 minutes on desktop" or just not > mention the specific interval. Done. https://codereview.chromium.org/1537743006/diff/680001/components/metrics/fil... components/metrics/file_metrics_provider.h:58: // Indicate a file to be monitored and how the file is used. Because some On 2016/02/19 18:16:40, Alexei Svitkine wrote: > Nit: Indicates Done. https://codereview.chromium.org/1537743006/diff/680001/components/metrics/fil... components/metrics/file_metrics_provider.h:63: void RegisterFile(const base::FilePath& path, FileType type, On 2016/02/19 18:16:40, Alexei Svitkine wrote: > Nit: 1 param per line. Done.
lgtm % comments https://codereview.chromium.org/1537743006/diff/740001/components/metrics/fil... File components/metrics/file_metrics_provider.cc (right): https://codereview.chromium.org/1537743006/diff/740001/components/metrics/fil... components/metrics/file_metrics_provider.cc:62: DCHECK(thread_checker_.CalledOnValidThread()); Nit: No need to do it here since this will always be true since the thread checker is created in the ctor. https://codereview.chromium.org/1537743006/diff/740001/components/metrics/fil... components/metrics/file_metrics_provider.cc:96: // to run on a worker thread rather than the UI thread. Nit: The comment above the method should just be "// static" - the other part should either be in the body or in the header. https://codereview.chromium.org/1537743006/diff/740001/components/metrics/fil... components/metrics/file_metrics_provider.cc:115: FileMetricsProvider::FileInfo* file) { Add thread check here. https://codereview.chromium.org/1537743006/diff/740001/components/metrics/fil... components/metrics/file_metrics_provider.cc:155: void FileMetricsProvider::ScheduleFilesCheck() { Add thread check here. https://codereview.chromium.org/1537743006/diff/740001/components/metrics/fil... components/metrics/file_metrics_provider.cc:175: FileInfo* file) { Add thread check here.
https://codereview.chromium.org/1537743006/diff/740001/components/metrics/fil... File components/metrics/file_metrics_provider.cc (right): https://codereview.chromium.org/1537743006/diff/740001/components/metrics/fil... components/metrics/file_metrics_provider.cc:115: FileMetricsProvider::FileInfo* file) { On 2016/02/19 20:09:44, Alexei Svitkine wrote: > Add thread check here. Sorry, ignore this one. https://codereview.chromium.org/1537743006/diff/740001/components/metrics/fil... components/metrics/file_metrics_provider.cc:230: void FileMetricsProvider::RecordFileAsSeen(FileInfo* file) { Add thread check here.
https://codereview.chromium.org/1537743006/diff/740001/components/metrics/fil... File components/metrics/file_metrics_provider.cc (right): https://codereview.chromium.org/1537743006/diff/740001/components/metrics/fil... components/metrics/file_metrics_provider.cc:62: DCHECK(thread_checker_.CalledOnValidThread()); On 2016/02/19 20:09:44, Alexei Svitkine wrote: > Nit: No need to do it here since this will always be true since the thread > checker is created in the ctor. Done. https://codereview.chromium.org/1537743006/diff/740001/components/metrics/fil... components/metrics/file_metrics_provider.cc:96: // to run on a worker thread rather than the UI thread. On 2016/02/19 20:09:44, Alexei Svitkine wrote: > Nit: The comment above the method should just be "// static" - the other part > should either be in the body or in the header. Done. https://codereview.chromium.org/1537743006/diff/740001/components/metrics/fil... components/metrics/file_metrics_provider.cc:115: FileMetricsProvider::FileInfo* file) { On 2016/02/19 20:11:40, Alexei Svitkine wrote: > On 2016/02/19 20:09:44, Alexei Svitkine wrote: > > Add thread check here. > > Sorry, ignore this one. Acknowledged. https://codereview.chromium.org/1537743006/diff/740001/components/metrics/fil... components/metrics/file_metrics_provider.cc:155: void FileMetricsProvider::ScheduleFilesCheck() { On 2016/02/19 20:09:44, Alexei Svitkine wrote: > Add thread check here. Done. https://codereview.chromium.org/1537743006/diff/740001/components/metrics/fil... components/metrics/file_metrics_provider.cc:175: FileInfo* file) { On 2016/02/19 20:09:44, Alexei Svitkine wrote: > Add thread check here. Done. https://codereview.chromium.org/1537743006/diff/740001/components/metrics/fil... components/metrics/file_metrics_provider.cc:230: void FileMetricsProvider::RecordFileAsSeen(FileInfo* file) { On 2016/02/19 20:11:41, Alexei Svitkine wrote: > Add thread check here. Done.
The CQ bit was checked by bcwhite@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from grt@chromium.org, asvitkine@chromium.org Link to the patchset: https://codereview.chromium.org/1537743006/#ps760001 (title: "addressed final review comments by Alexei")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1537743006/760001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1537743006/760001
Message was sent while issue was closed.
Committed patchset #23 (id:760001)
Message was sent while issue was closed.
Description was changed from ========== Persist setup metrics and have Chrome report them during UMA upload. BUG=546019 ========== to ========== Persist setup metrics and have Chrome report them during UMA upload. BUG=546019 Committed: https://crrev.com/34c6bbf85c990ead28d334b3803093b614c958c6 Cr-Commit-Position: refs/heads/master@{#376549} ==========
Message was sent while issue was closed.
Patchset 23 (id:??) landed as https://crrev.com/34c6bbf85c990ead28d334b3803093b614c958c6 Cr-Commit-Position: refs/heads/master@{#376549} |
