Primary histograms for InBrowserCleanerUI experiment

Primary histograms for InBrowserCleanerUI experiment

BUG=690020
Review-Url: https://codereview.chromium.org/2973873002
Cr-Commit-Position: refs/heads/master@{#485728}
Committed: https://chromium.googlesource.com/chromium/src/+/3d931394f6ac24c64c200d3f8f98914316a0d195

[ftirelo, 2017-07-07 02:02:15]

Description was changed from

==========
Primary histograms for InBrowserCleanerUI experiment

WIP

BUG=
==========

to

==========
Primary histograms for InBrowserCleanerUI experiment

BUG=690020
==========

[ftirelo, 2017-07-07 02:02:15]

ftirelo@chromium.org changed reviewers:
+ csharp@chromium.org

[csharp, 2017-07-07 17:32:51]

https://codereview.chromium.org/2973873002/diff/40001/chrome/browser/safe_bro...
File
chrome/browser/safe_browsing/chrome_cleaner/chrome_cleaner_controller_win.cc
(right):

https://codereview.chromium.org/2973873002/diff/40001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/chrome_cleaner/chrome_cleaner_controller_win.cc:105:
void RecordCleanerLogsAcceptanceHistogram(bool value) {
what about logs_uploaded instead of value?

https://codereview.chromium.org/2973873002/diff/40001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/chrome_cleaner/chrome_cleaner_controller_win.cc:415:
files_to_delete->size());
Is there any problem if files_to_delete->size() > 1000? I vaguely remember
hearing about problems on the back end if we gave values a lot higher than what
the count should have been

https://codereview.chromium.org/2973873002/diff/40001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/chrome_cleaner/chrome_cleaner_controller_win.cc:430:
NO_PROMPT_REASON_IPC_CONNECTION_BROKEN);
Is it possible for the connect to be closed after the user has seen the prompt?

https://codereview.chromium.org/2973873002/diff/40001/chrome/browser/safe_bro...
File
chrome/browser/safe_browsing/chrome_cleaner/chrome_cleaner_dialog_controller_impl_win.cc
(right):

https://codereview.chromium.org/2973873002/diff/40001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/chrome_cleaner/chrome_cleaner_dialog_controller_impl_win.cc:45:
enum PromptDialogResponseHistogramValue {
Please add a comment saying this much match the enum in the histogram file

https://codereview.chromium.org/2973873002/diff/40001/chrome/browser/safe_bro...
File chrome/browser/safe_browsing/chrome_cleaner/chrome_cleaner_fetcher_win.cc
(right):

https://codereview.chromium.org/2973873002/diff/40001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/chrome_cleaner/chrome_cleaner_fetcher_win.cc:56:
enum CleanerDownloadStatusHistogramValue {
Please add a comment saying this must say in sync with the enum in the histogram
file

https://codereview.chromium.org/2973873002/diff/40001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/chrome_cleaner/chrome_cleaner_fetcher_win.cc:145:
RecordCleanerDownloadStatusHistogram(
This should probably be move up a line so it is done before this object is
delete.

https://codereview.chromium.org/2973873002/diff/40001/chrome/browser/safe_bro...
File chrome/browser/safe_browsing/chrome_cleaner/settings_resetter_win.cc
(right):

https://codereview.chromium.org/2973873002/diff/40001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/chrome_cleaner/settings_resetter_win.cc:169: if
(!num_pending_resets_) {
nit: No need for the braces since this is a single line

https://codereview.chromium.org/2973873002/diff/40001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/settings/chrome_cleanup_handler.cc (right):

https://codereview.chromium.org/2973873002/diff/40001/chrome/browser/ui/webui...
chrome/browser/ui/webui/settings/chrome_cleanup_handler.cc:137:
UMA_HISTOGRAM_BOOLEAN("SoftwareReporter.CleanupCardShown", true);
It seems odd to have a boolean histogram that is only every marked as true.

Looking at some uma comments, there is UMA_HISTOGRAM_COUNT_*, but the comment
there says if you just want to capture the number of some event, an enum should
be used, ideally with an appropriate baseline.

What about changing this enum to SoftwareReporter.CleanupCard and recording
SHOWN here, and also recording "SHOULD_SHOW" or something like that when users
click more detailed in the prompt.

[1]
https://cs.chromium.org/chromium/src/base/metrics/histogram_macros.h?type=cs&...

https://codereview.chromium.org/2973873002/diff/40001/chrome/browser/ui/webui...
chrome/browser/ui/webui/settings/chrome_cleanup_handler.cc:152:
safe_browsing::RecordCleanupStartedHistogram(
Maybe also worth recording this in the suggested SoftwareReporter.CleanupCard,
to make it easy to see what percentage of people that go to details actually run
the cleanup

https://codereview.chromium.org/2973873002/diff/40001/tools/metrics/histogram...
File tools/metrics/histograms/enums.xml (left):

https://codereview.chromium.org/2973873002/diff/40001/tools/metrics/histogram...
tools/metrics/histograms/enums.xml:34155: 
What do you think about change SoftwareReporter -> ChromeCleaner for the cases
where it is the cleaner and not the reporter?

Like SoftwareReporterCleanerDownloadStatus -> ChromeCleanerDownloadStatus

Or Maybe still prefix it with SoftwareReporter to group with the others

https://codereview.chromium.org/2973873002/diff/40001/tools/metrics/histogram...
File tools/metrics/histograms/enums.xml (right):

https://codereview.chromium.org/2973873002/diff/40001/tools/metrics/histogram...
tools/metrics/histograms/enums.xml:34201: +  <int value="4" label="Recently
prompted"/>
Where is recently prompted used and how it is different from already prompted?

https://codereview.chromium.org/2973873002/diff/40001/tools/metrics/histogram...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/2973873002/diff/40001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:73099: +<histogram
name="SoftwareReporter.RebootInitiated" units="boolean">
These last 3 new histograms only seem to ever get the value true set. If this is
just for counting, they should probably be converted to enums as previous
suggested.

Although I think RebootRequired and TaggedProfile* both make sense to stay as
bools, but they should get their false values recorded.

[ftirelo, 2017-07-07 20:27:53]

PTAL

https://codereview.chromium.org/2973873002/diff/40001/chrome/browser/safe_bro...
File
chrome/browser/safe_browsing/chrome_cleaner/chrome_cleaner_controller_win.cc
(right):

https://codereview.chromium.org/2973873002/diff/40001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/chrome_cleaner/chrome_cleaner_controller_win.cc:105:
void RecordCleanerLogsAcceptanceHistogram(bool value) {
On 2017/07/07 17:32:51, csharp wrote:
> what about logs_uploaded instead of value?

Used logs_accepted instead.

https://codereview.chromium.org/2973873002/diff/40001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/chrome_cleaner/chrome_cleaner_controller_win.cc:415:
files_to_delete->size());
On 2017/07/07 17:32:51, csharp wrote:
> Is there any problem if files_to_delete->size() > 1000? I vaguely remember
> hearing about problems on the back end if we gave values a lot higher than
what
> the count should have been

There is an overflow buffer for counts > 1000. Since less than 0.5% of file
lists are larger than that, we should be fine.

https://codereview.chromium.org/2973873002/diff/40001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/chrome_cleaner/chrome_cleaner_controller_win.cc:430:
NO_PROMPT_REASON_IPC_CONNECTION_BROKEN);
On 2017/07/07 17:32:51, csharp wrote:
> Is it possible for the connect to be closed after the user has seen the
prompt?

Yes, if the cleaner binary crashes.

https://codereview.chromium.org/2973873002/diff/40001/chrome/browser/safe_bro...
File
chrome/browser/safe_browsing/chrome_cleaner/chrome_cleaner_dialog_controller_impl_win.cc
(right):

https://codereview.chromium.org/2973873002/diff/40001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/chrome_cleaner/chrome_cleaner_dialog_controller_impl_win.cc:45:
enum PromptDialogResponseHistogramValue {
On 2017/07/07 17:32:51, csharp wrote:
> Please add a comment saying this much match the enum in the histogram file

Done.

https://codereview.chromium.org/2973873002/diff/40001/chrome/browser/safe_bro...
File chrome/browser/safe_browsing/chrome_cleaner/chrome_cleaner_fetcher_win.cc
(right):

https://codereview.chromium.org/2973873002/diff/40001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/chrome_cleaner/chrome_cleaner_fetcher_win.cc:56:
enum CleanerDownloadStatusHistogramValue {
On 2017/07/07 17:32:51, csharp wrote:
> Please add a comment saying this must say in sync with the enum in the
histogram
> file

Done.

https://codereview.chromium.org/2973873002/diff/40001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/chrome_cleaner/chrome_cleaner_fetcher_win.cc:145:
RecordCleanerDownloadStatusHistogram(
On 2017/07/07 17:32:51, csharp wrote:
> This should probably be move up a line so it is done before this object is
> delete.

Done. Also did for the other calls.

https://codereview.chromium.org/2973873002/diff/40001/chrome/browser/safe_bro...
File chrome/browser/safe_browsing/chrome_cleaner/settings_resetter_win.cc
(right):

https://codereview.chromium.org/2973873002/diff/40001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/chrome_cleaner/settings_resetter_win.cc:169: if
(!num_pending_resets_) {
On 2017/07/07 17:32:51, csharp wrote:
> nit: No need for the braces since this is a single line

Done.

https://codereview.chromium.org/2973873002/diff/40001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/settings/chrome_cleanup_handler.cc (right):

https://codereview.chromium.org/2973873002/diff/40001/chrome/browser/ui/webui...
chrome/browser/ui/webui/settings/chrome_cleanup_handler.cc:137:
UMA_HISTOGRAM_BOOLEAN("SoftwareReporter.CleanupCardShown", true);
On 2017/07/07 17:32:51, csharp wrote:
> It seems odd to have a boolean histogram that is only every marked as true.
> 
> Looking at some uma comments, there is UMA_HISTOGRAM_COUNT_*, but the comment
> there says if you just want to capture the number of some event, an enum
should
> be used, ideally with an appropriate baseline.
> 
> What about changing this enum to SoftwareReporter.CleanupCard and recording
> SHOWN here, and also recording "SHOULD_SHOW" or something like that when users
> click more detailed in the prompt.
> 
> [1]
>
https://cs.chromium.org/chromium/src/base/metrics/histogram_macros.h?type=cs&...

Used a single-value enum instead.

https://codereview.chromium.org/2973873002/diff/40001/chrome/browser/ui/webui...
chrome/browser/ui/webui/settings/chrome_cleanup_handler.cc:152:
safe_browsing::RecordCleanupStartedHistogram(
On 2017/07/07 17:32:51, csharp wrote:
> Maybe also worth recording this in the suggested SoftwareReporter.CleanupCard,
> to make it easy to see what percentage of people that go to details actually
run
> the cleanup

As discussed offline, it will be tricky to do that unless we add a parameter to
the settings page, which is not ideal. We should be able to see the results in a
custom query.

https://codereview.chromium.org/2973873002/diff/40001/tools/metrics/histogram...
File tools/metrics/histograms/enums.xml (left):

https://codereview.chromium.org/2973873002/diff/40001/tools/metrics/histogram...
tools/metrics/histograms/enums.xml:34155: 
On 2017/07/07 17:32:51, csharp wrote:
> What do you think about change SoftwareReporter -> ChromeCleaner for the cases
> where it is the cleaner and not the reporter?
> 
> Like SoftwareReporterCleanerDownloadStatus -> ChromeCleanerDownloadStatus
> 
> Or Maybe still prefix it with SoftwareReporter to group with the others

As discussed, we have several histograms prefixed with SoftwareReporter.Cleaner.
Even though it's not ideal, we can't get rid of all legacy analysis, so I'll
keep naming consistent.

https://codereview.chromium.org/2973873002/diff/40001/tools/metrics/histogram...
File tools/metrics/histograms/enums.xml (right):

https://codereview.chromium.org/2973873002/diff/40001/tools/metrics/histogram...
tools/metrics/histograms/enums.xml:34201: +  <int value="4" label="Recently
prompted"/>
On 2017/07/07 17:32:51, csharp wrote:
> Where is recently prompted used and how it is different from already prompted?

Good catch! Removed.

https://codereview.chromium.org/2973873002/diff/40001/tools/metrics/histogram...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/2973873002/diff/40001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:73099: +<histogram
name="SoftwareReporter.RebootInitiated" units="boolean">
On 2017/07/07 17:32:51, csharp wrote:
> These last 3 new histograms only seem to ever get the value true set. If this
is
> just for counting, they should probably be converted to enums as previous
> suggested.
> 
> Although I think RebootRequired and TaggedProfile* both make sense to stay as
> bools, but they should get their false values recorded.

Replaced RebootRequired with CleanupStatus, which is more general.
TaggedProfile* was replaced with an enum since we always tag profiles for
resetting.

[csharp, 2017-07-07 20:53:34]

lgtm

https://codereview.chromium.org/2973873002/diff/40001/chrome/browser/safe_bro...
File
chrome/browser/safe_browsing/chrome_cleaner/chrome_cleaner_controller_win.cc
(right):

https://codereview.chromium.org/2973873002/diff/40001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/chrome_cleaner/chrome_cleaner_controller_win.cc:430:
NO_PROMPT_REASON_IPC_CONNECTION_BROKEN);
On 2017/07/07 20:27:52, ftirelo wrote:
> On 2017/07/07 17:32:51, csharp wrote:
> > Is it possible for the connect to be closed after the user has seen the
> prompt?
> 
> Yes, if the cleaner binary crashes.

Should this be counted then, since in that case wouldn't we have already
recorded the prompt as being shown?

https://codereview.chromium.org/2973873002/diff/40002/chrome/browser/safe_bro...
File chrome/browser/safe_browsing/chrome_cleaner/settings_resetter_win.cc
(right):

https://codereview.chromium.org/2973873002/diff/40002/chrome/browser/safe_bro...
chrome/browser/safe_browsing/chrome_cleaner/settings_resetter_win.cc:39: enum
TaggedProfileForResetting {
nit: Add a comment about matching uma

https://codereview.chromium.org/2973873002/diff/40002/chrome/browser/ui/webui...
File chrome/browser/ui/webui/settings/chrome_cleanup_handler.cc (right):

https://codereview.chromium.org/2973873002/diff/40002/chrome/browser/ui/webui...
chrome/browser/ui/webui/settings/chrome_cleanup_handler.cc:57: enum
CleanupCardHistogramValue {
nit: Add a comment about how this needs to match uma

[ftirelo, 2017-07-07 21:22:08]

Thanks for the excellent review!

https://codereview.chromium.org/2973873002/diff/40001/chrome/browser/safe_bro...
File
chrome/browser/safe_browsing/chrome_cleaner/chrome_cleaner_controller_win.cc
(right):

https://codereview.chromium.org/2973873002/diff/40001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/chrome_cleaner/chrome_cleaner_controller_win.cc:430:
NO_PROMPT_REASON_IPC_CONNECTION_BROKEN);
On 2017/07/07 20:53:33, csharp wrote:
> On 2017/07/07 20:27:52, ftirelo wrote:
> > On 2017/07/07 17:32:51, csharp wrote:
> > > Is it possible for the connect to be closed after the user has seen the
> > prompt?
> > 
> > Yes, if the cleaner binary crashes.
> 
> Should this be counted then, since in that case wouldn't we have already
> recorded the prompt as being shown?

Added a new histogram for moment when IPC connection was lost and changed this
to only register non-prompt if state is kScanning.

https://codereview.chromium.org/2973873002/diff/40002/chrome/browser/safe_bro...
File chrome/browser/safe_browsing/chrome_cleaner/settings_resetter_win.cc
(right):

https://codereview.chromium.org/2973873002/diff/40002/chrome/browser/safe_bro...
chrome/browser/safe_browsing/chrome_cleaner/settings_resetter_win.cc:39: enum
TaggedProfileForResetting {
On 2017/07/07 20:53:33, csharp wrote:
> nit: Add a comment about matching uma

Done.

https://codereview.chromium.org/2973873002/diff/40002/chrome/browser/ui/webui...
File chrome/browser/ui/webui/settings/chrome_cleanup_handler.cc (right):

https://codereview.chromium.org/2973873002/diff/40002/chrome/browser/ui/webui...
chrome/browser/ui/webui/settings/chrome_cleanup_handler.cc:57: enum
CleanupCardHistogramValue {
On 2017/07/07 20:53:33, csharp wrote:
> nit: Add a comment about how this needs to match uma

Done.

[ftirelo, 2017-07-07 21:23:45]

ftirelo@chromium.org changed reviewers:
+ rkaplow@chromium.org, tommycli@chromium.org

[ftirelo, 2017-07-07 21:23:46]

+rkaplow for OWNERS approval for tools\metrics\histograms

+tommycli for OWNERS approval for chrome\browser\ui\webui\settings

[tommycli, 2017-07-07 21:32:26]

webui lgtm

https://codereview.chromium.org/2973873002/diff/90001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/settings/chrome_cleanup_handler.cc (right):

https://codereview.chromium.org/2973873002/diff/90001/chrome/browser/ui/webui...
chrome/browser/ui/webui/settings/chrome_cleanup_handler.cc:30: 
nit: no \n here

[rkaplow, 2017-07-10 14:23:06]

https://codereview.chromium.org/2973873002/diff/90001/tools/metrics/histogram...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/2973873002/diff/90001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:73296: +    Indicates that the user
accepted to initiate a reboot to complete a run
similar to comment below - can you track when they did not initiate as well?

Can you also mention when this is logged

https://codereview.chromium.org/2973873002/diff/90001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:73313: +  <summary>
mention when this is logged

https://codereview.chromium.org/2973873002/diff/90001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:73356: +    Indicates IPC connection
errors between Chrome and the Chrome Cleanup Tool.
mention when it is logged. Should you also have a bucket when there was no IPC
error or is that not useful?

https://codereview.chromium.org/2973873002/diff/90001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:73429: +<histogram
name="SoftwareReporter.NumberOfFilesToDelete">
add unit

https://codereview.chromium.org/2973873002/diff/90001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:73436: +<histogram
name="SoftwareReporter.PostCleanupSettingsReset">
unit

https://codereview.chromium.org/2973873002/diff/90001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:73498: +<histogram
name="SoftwareReporter.TaggedProfileForResetting"
it's not disallowed, but it's best when there is multiple enum buckets - for
example to do some sort of baselining. Wondering it if makes sense to track a
negative case here - i.e. when a profile wasn't tagged. Just a suggestion if
that sort of baselining can make sense for your case

[rkaplow, 2017-07-10 14:34:53]

https://codereview.chromium.org/2973873002/diff/90001/tools/metrics/histogram...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/2973873002/diff/90001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:73311: +<histogram
name="SoftwareReporter.CleanerLogsAcceptance" units="boolean">
also this should be enum=Boolean

https://codereview.chromium.org/2973873002/diff/90001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:73453: +<histogram
name="SoftwareReporter.PromptShown" units="boolean">
also enum=Boolean

[ftirelo, 2017-07-10 19:08:51]

https://codereview.chromium.org/2973873002/diff/90001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/settings/chrome_cleanup_handler.cc (right):

https://codereview.chromium.org/2973873002/diff/90001/chrome/browser/ui/webui...
chrome/browser/ui/webui/settings/chrome_cleanup_handler.cc:30: 
On 2017/07/07 21:32:26, tommycli wrote:
> nit: no \n here

Done.

https://codereview.chromium.org/2973873002/diff/90001/tools/metrics/histogram...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/2973873002/diff/90001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:73296: +    Indicates that the user
accepted to initiate a reboot to complete a run
On 2017/07/10 14:23:06, rkaplow wrote:
> similar to comment below - can you track when they did not initiate as well?
> 
> Can you also mention when this is logged

The reboot can happen at any moment in the future, so we can't track when it's
not initiated. Added this to the description along with when it's logged.

https://codereview.chromium.org/2973873002/diff/90001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:73311: +<histogram
name="SoftwareReporter.CleanerLogsAcceptance" units="boolean">
On 2017/07/10 14:34:53, rkaplow wrote:
> also this should be enum=Boolean

Done.

https://codereview.chromium.org/2973873002/diff/90001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:73313: +  <summary>
On 2017/07/10 14:23:05, rkaplow wrote:
> mention when this is logged

Done.

https://codereview.chromium.org/2973873002/diff/90001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:73356: +    Indicates IPC connection
errors between Chrome and the Chrome Cleanup Tool.
On 2017/07/10 14:23:06, rkaplow wrote:
> mention when it is logged. Should you also have a bucket when there was no IPC
> error or is that not useful?

It might be, and it's not hard to log. Changed to include success.

https://codereview.chromium.org/2973873002/diff/90001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:73429: +<histogram
name="SoftwareReporter.NumberOfFilesToDelete">
On 2017/07/10 14:23:05, rkaplow wrote:
> add unit

Done.

https://codereview.chromium.org/2973873002/diff/90001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:73436: +<histogram
name="SoftwareReporter.PostCleanupSettingsReset">
On 2017/07/10 14:23:06, rkaplow wrote:
> unit

Done.

https://codereview.chromium.org/2973873002/diff/90001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:73453: +<histogram
name="SoftwareReporter.PromptShown" units="boolean">
On 2017/07/10 14:34:53, rkaplow wrote:
> also enum=Boolean

Done.

https://codereview.chromium.org/2973873002/diff/90001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:73498: +<histogram
name="SoftwareReporter.TaggedProfileForResetting"
On 2017/07/10 14:23:06, rkaplow wrote:
> it's not disallowed, but it's best when there is multiple enum buckets - for
> example to do some sort of baselining. Wondering it if makes sense to track a
> negative case here - i.e. when a profile wasn't tagged. Just a suggestion if
> that sort of baselining can make sense for your case

Could you help us figure out the best approach here?

Profiles are always tagged. Our goal is to compare this with
PostCleanupSettingsReset, in order to know the ratio of tagged profiles that are
actually reset. Using a single histogram for that doesn't seem right, because we
can't log the negative case - a reset can happen several hours after a profile
is tagged (e.g. a cleanup may require a reboot and reset will only happen next
time the user opens the browser post-reboot).

[rkaplow, 2017-07-11 14:10:52]

lgtm

https://codereview.chromium.org/2973873002/diff/90001/tools/metrics/histogram...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/2973873002/diff/90001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:73498: +<histogram
name="SoftwareReporter.TaggedProfileForResetting"
On 2017/07/10 19:08:51, ftirelo wrote:
> On 2017/07/10 14:23:06, rkaplow wrote:
> > it's not disallowed, but it's best when there is multiple enum buckets - for
> > example to do some sort of baselining. Wondering it if makes sense to track
a
> > negative case here - i.e. when a profile wasn't tagged. Just a suggestion if
> > that sort of baselining can make sense for your case
> 
> Could you help us figure out the best approach here?
> 
> Profiles are always tagged. Our goal is to compare this with
> PostCleanupSettingsReset, in order to know the ratio of tagged profiles that
are
> actually reset. Using a single histogram for that doesn't seem right, because
we
> can't log the negative case - a reset can happen several hours after a profile
> is tagged (e.g. a cleanup may require a reboot and reset will only happen next
> time the user opens the browser post-reboot).

I actually don't see why that would be a problem - in that case the aggregate
metric would still make sense right? (although you wouldn't be able to look at a
single user with high accuracy).

[ftirelo, 2017-07-11 14:26:01]

Thanks!

[ftirelo, 2017-07-11 14:26:07]

The CQ bit was checked by ftirelo@chromium.org

[ftirelo, 2017-07-11 14:26:08]

The patchset sent to the CQ was uploaded after l-g-t-m from csharp@chromium.org
Link to the patchset: https://codereview.chromium.org/2973873002/#ps90001
(title: "Next round of reviews")

[commit-bot: I haz the power, 2017-07-11 14:26:15]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-07-11 15:54:30]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-07-11 15:54:31]

Try jobs failed on following builders:
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[ftirelo, 2017-07-11 20:08:01]

Enum histograms must have at least two values. Replaced all unitary enums with
booleans in the last patches.

[ftirelo, 2017-07-11 20:08:09]

The CQ bit was checked by ftirelo@chromium.org

[ftirelo, 2017-07-11 20:08:10]

The patchset sent to the CQ was uploaded after l-g-t-m from
tommycli@chromium.org, csharp@chromium.org, rkaplow@chromium.org
Link to the patchset: https://codereview.chromium.org/2973873002/#ps170001
(title: "Revert changes to histogram.cc")

[commit-bot: I haz the power, 2017-07-11 20:08:38]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-07-11 23:37:00]

CQ is committing da patch.

Bot data: {"patchset_id": 170001, "attempt_start_ts": 1499803689672210,
"parent_rev": "0cb76e866966b82ddb4a201a629295696cb59d34", "commit_rev":
"8d8300d532dab427ba6b748d6ebb8901969d668f"}

[commit-bot: I haz the power, 2017-07-11 23:37:19]

CQ is committing da patch.

Bot data: {"patchset_id": 170001, "attempt_start_ts": 1499803689672210,
"parent_rev": "b206022ca21c62a7a0a9262a6a0e67811f38a4d3", "commit_rev":
"3d931394f6ac24c64c200d3f8f98914316a0d195"}

[commit-bot: I haz the power, 2017-07-11 23:37:41]

Description was changed from

==========
Primary histograms for InBrowserCleanerUI experiment

BUG=690020
==========

to

==========
Primary histograms for InBrowserCleanerUI experiment

BUG=690020

Review-Url: https://codereview.chromium.org/2973873002
Cr-Commit-Position: refs/heads/master@{#485728}
Committed:
https://chromium.googlesource.com/chromium/src/+/3d931394f6ac24c64c200d3f8f98...
==========

[commit-bot: I haz the power, 2017-07-11 23:37:44]

Committed patchset #10 (id:170001) as
https://chromium.googlesource.com/chromium/src/+/3d931394f6ac24c64c200d3f8f98...