Resetting metrics ids on clump detection.

Resetting metrics ids on clump detection.

When a cloned install is detected the metrics client id and low entropy source are reset on the next start of Chrome. This is done by setting a pref in local state. The next time either the client id or low entropy source are loaded from the local state, they are first reset.

BUG=343273
NOTRY=true

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=260348

[jwd, 2014-03-26 21:20:27]

Hey Alexei, can you have a look at the strategy I'm using to reset the client id
and low entropy source, and let me know what you think.

Please ignore the DVlOG and stack trace stuff.

[Alexei Svitkine (slow), 2014-03-26 21:34:50]

https://codereview.chromium.org/213363004/diff/1/chrome/browser/chrome_browse...
File chrome/browser/chrome_browser_main.cc (right):

https://codereview.chromium.org/213363004/diff/1/chrome/browser/chrome_browse...
chrome/browser/chrome_browser_main.cc:583: metrics->ResetVariationsState();
Hmm, I'd actually prefer that the logic be entirely in MetricsService.

Maybe we can make the new function do the pref check itself and then call it
from both ForceClientIdCreation() and GetLowEntropySource().

https://codereview.chromium.org/213363004/diff/1/chrome/browser/metrics/clone...
File chrome/browser/metrics/cloned_install_detector.cc (right):

https://codereview.chromium.org/213363004/diff/1/chrome/browser/metrics/clone...
chrome/browser/metrics/cloned_install_detector.cc:80:
local_state->SetBoolean(prefs::kMetricsCloneDetected, true);
Is there a way we can make the querying and setting of this pref happen in the
same class? i.e. either both in MetricsService or both in ClonedInstallDetector?

Maybe make ClonedInstallDetector::CheckForClonedInstall() take a callback that's
posted if we detect a cloned install and then have the logic be in
MetricsService related to the pref.

https://codereview.chromium.org/213363004/diff/1/chrome/browser/metrics/metri...
File chrome/browser/metrics/metrics_service.cc (right):

https://codereview.chromium.org/213363004/diff/1/chrome/browser/metrics/metri...
chrome/browser/metrics/metrics_service.cc:1899:
local_state->SetBoolean(prefs::kMetricsCloneDetected, false);
Why not ClearPref()?

https://codereview.chromium.org/213363004/diff/1/chrome/common/pref_names.cc
File chrome/common/pref_names.cc (right):

https://codereview.chromium.org/213363004/diff/1/chrome/common/pref_names.cc#...
chrome/common/pref_names.cc:1388: const char kMetricsCloneDetected[] =
"user_experience_metrics.clone_detected";
Hmm, I think it would be better to use a name to describe its function, rather
than the cause.

e.g. kMetricsResetIds.

[Alexei Svitkine (slow), 2014-03-28 15:00:17]

https://codereview.chromium.org/213363004/diff/20001/chrome/browser/metrics/m...
File chrome/browser/metrics/metrics_service.cc (right):

https://codereview.chromium.org/213363004/diff/20001/chrome/browser/metrics/m...
chrome/browser/metrics/metrics_service.cc:573: if (recording_enabled)
Hmm, this requires the caller to know this, which we don't from
GetLowEntropySource()...

Let's go with ResetMetricsIDsIfNecessary() instead and call it from both places
and a have a bool to guard if we did the check already.

https://codereview.chromium.org/213363004/diff/20001/chrome/browser/metrics/m...
chrome/browser/metrics/metrics_service.cc:1212: ResetVariationsState();
Why is this called from here? Did you mean Initialize instead?

https://codereview.chromium.org/213363004/diff/20001/chrome/browser/metrics/m...
File chrome/browser/metrics/metrics_service.h (right):

https://codereview.chromium.org/213363004/diff/20001/chrome/browser/metrics/m...
chrome/browser/metrics/metrics_service.h:146: void
InitializeVariationsState(bool recording_enabled);
It's not really variations state, since UMA client id isn't just for variations.

Maybe InitializeMetricsIDs()?

https://codereview.chromium.org/213363004/diff/20001/chrome/browser/metrics/m...
chrome/browser/metrics/metrics_service.h:299: void ResetVariationsState();
Private?

[jwd, 2014-03-28 16:23:49]

https://codereview.chromium.org/213363004/diff/1/chrome/browser/chrome_browse...
File chrome/browser/chrome_browser_main.cc (right):

https://codereview.chromium.org/213363004/diff/1/chrome/browser/chrome_browse...
chrome/browser/chrome_browser_main.cc:583: metrics->ResetVariationsState();
On 2014/03/26 21:34:50, Alexei Svitkine wrote:
> Hmm, I'd actually prefer that the logic be entirely in MetricsService.
> 
> Maybe we can make the new function do the pref check itself and then call it
> from both ForceClientIdCreation() and GetLowEntropySource().

Done.

https://codereview.chromium.org/213363004/diff/1/chrome/browser/metrics/metri...
File chrome/browser/metrics/metrics_service.cc (right):

https://codereview.chromium.org/213363004/diff/1/chrome/browser/metrics/metri...
chrome/browser/metrics/metrics_service.cc:1899:
local_state->SetBoolean(prefs::kMetricsCloneDetected, false);
On 2014/03/26 21:34:50, Alexei Svitkine wrote:
> Why not ClearPref()?

Done.

https://codereview.chromium.org/213363004/diff/1/chrome/common/pref_names.cc
File chrome/common/pref_names.cc (right):

https://codereview.chromium.org/213363004/diff/1/chrome/common/pref_names.cc#...
chrome/common/pref_names.cc:1388: const char kMetricsCloneDetected[] =
"user_experience_metrics.clone_detected";
On 2014/03/26 21:34:50, Alexei Svitkine wrote:
> Hmm, I think it would be better to use a name to describe its function, rather
> than the cause.
> 
> e.g. kMetricsResetIds.

Done.

https://codereview.chromium.org/213363004/diff/20001/chrome/browser/metrics/m...
File chrome/browser/metrics/metrics_service.cc (right):

https://codereview.chromium.org/213363004/diff/20001/chrome/browser/metrics/m...
chrome/browser/metrics/metrics_service.cc:573: if (recording_enabled)
On 2014/03/28 15:00:17, Alexei Svitkine wrote:
> Hmm, this requires the caller to know this, which we don't from
> GetLowEntropySource()...
> 
> Let's go with ResetMetricsIDsIfNecessary() instead and call it from both
places
> and a have a bool to guard if we did the check already.

Done.

https://codereview.chromium.org/213363004/diff/20001/chrome/browser/metrics/m...
chrome/browser/metrics/metrics_service.cc:1212: ResetVariationsState();
On 2014/03/28 15:00:17, Alexei Svitkine wrote:
> Why is this called from here? Did you mean Initialize instead?

Done.

https://codereview.chromium.org/213363004/diff/20001/chrome/browser/metrics/m...
File chrome/browser/metrics/metrics_service.h (right):

https://codereview.chromium.org/213363004/diff/20001/chrome/browser/metrics/m...
chrome/browser/metrics/metrics_service.h:146: void
InitializeVariationsState(bool recording_enabled);
On 2014/03/28 15:00:17, Alexei Svitkine wrote:
> It's not really variations state, since UMA client id isn't just for
variations.
> 
> Maybe InitializeMetricsIDs()?

Done.

https://codereview.chromium.org/213363004/diff/20001/chrome/browser/metrics/m...
chrome/browser/metrics/metrics_service.h:299: void ResetVariationsState();
On 2014/03/28 15:00:17, Alexei Svitkine wrote:
> Private?

Done.

[Alexei Svitkine (slow), 2014-03-28 16:30:47]

https://codereview.chromium.org/213363004/diff/60001/chrome/browser/metrics/c...
File chrome/browser/metrics/cloned_install_detector.cc (right):

https://codereview.chromium.org/213363004/diff/60001/chrome/browser/metrics/c...
chrome/browser/metrics/cloned_install_detector.cc:80:
local_state->SetBoolean(prefs::kMetricsResetIds, true);
Can this be a callback passed to this class at construction time? That way,
MetricsService can be the one to reset the pref.

(I'm okay with making this a TODO for this CL, though.)

[Alexei Svitkine (slow), 2014-03-28 17:20:26]

Looks good modulo a few suggestions below.

Once you've verified this is working as expected,  please remove the log message
so I can approved it for real.

Also, make it point to a crbug.

https://codereview.chromium.org/213363004/diff/60001/chrome/browser/chrome_br...
File chrome/browser/chrome_browser_main.cc (right):

https://codereview.chromium.org/213363004/diff/60001/chrome/browser/chrome_br...
chrome/browser/chrome_browser_main.cc:581: 
Nit: Revert the change to this file.

https://codereview.chromium.org/213363004/diff/60001/chrome/browser/metrics/m...
File chrome/browser/metrics/metrics_service.cc (right):

https://codereview.chromium.org/213363004/diff/60001/chrome/browser/metrics/m...
chrome/browser/metrics/metrics_service.cc:1204: DCHECK(low_entropy_source_ ==
kLowEntropySourceNotSet);
Nit: DCHECK_EQ and put kLowEntropySourceNotSet on the left side.

https://codereview.chromium.org/213363004/diff/60001/chrome/browser/metrics/m...
File chrome/browser/metrics/metrics_service.h (right):

https://codereview.chromium.org/213363004/diff/60001/chrome/browser/metrics/m...
chrome/browser/metrics/metrics_service.h:530: bool metrics_ids_reset_;
Nit: I'd name it metrics_ids_reset_check_performed_

[jwd, 2014-03-28 18:59:20]

Still adding tests to metrics service.

https://codereview.chromium.org/213363004/diff/60001/chrome/browser/chrome_br...
File chrome/browser/chrome_browser_main.cc (right):

https://codereview.chromium.org/213363004/diff/60001/chrome/browser/chrome_br...
chrome/browser/chrome_browser_main.cc:581: 
On 2014/03/28 17:20:26, Alexei Svitkine wrote:
> Nit: Revert the change to this file.

Done.

https://codereview.chromium.org/213363004/diff/60001/chrome/browser/metrics/c...
File chrome/browser/metrics/cloned_install_detector.cc (right):

https://codereview.chromium.org/213363004/diff/60001/chrome/browser/metrics/c...
chrome/browser/metrics/cloned_install_detector.cc:80:
local_state->SetBoolean(prefs::kMetricsResetIds, true);
On 2014/03/28 16:30:48, Alexei Svitkine wrote:
> Can this be a callback passed to this class at construction time? That way,
> MetricsService can be the one to reset the pref.
> 
> (I'm okay with making this a TODO for this CL, though.)

Done.

https://codereview.chromium.org/213363004/diff/60001/chrome/browser/metrics/m...
File chrome/browser/metrics/metrics_service.cc (right):

https://codereview.chromium.org/213363004/diff/60001/chrome/browser/metrics/m...
chrome/browser/metrics/metrics_service.cc:1204: DCHECK(low_entropy_source_ ==
kLowEntropySourceNotSet);
On 2014/03/28 17:20:26, Alexei Svitkine wrote:
> Nit: DCHECK_EQ and put kLowEntropySourceNotSet on the left side.

Done.

https://codereview.chromium.org/213363004/diff/60001/chrome/browser/metrics/m...
File chrome/browser/metrics/metrics_service.h (right):

https://codereview.chromium.org/213363004/diff/60001/chrome/browser/metrics/m...
chrome/browser/metrics/metrics_service.h:530: bool metrics_ids_reset_;
On 2014/03/28 17:20:26, Alexei Svitkine wrote:
> Nit: I'd name it metrics_ids_reset_check_performed_

Done.

[Alexei Svitkine (slow), 2014-03-28 19:02:49]

https://codereview.chromium.org/213363004/diff/100001/chrome/browser/metrics/...
File chrome/browser/metrics/cloned_install_detector_unittest.cc (right):

https://codereview.chromium.org/213363004/diff/100001/chrome/browser/metrics/...
chrome/browser/metrics/cloned_install_detector_unittest.cc:51:
EXPECT_TRUE(prefs::kMetricsResetIds);
This needs to actually fetch the pref and check it's value...

https://codereview.chromium.org/213363004/diff/100001/chrome/browser/metrics/...
File chrome/browser/metrics/machine_id_provider_win.cc (right):

https://codereview.chromium.org/213363004/diff/100001/chrome/browser/metrics/...
chrome/browser/metrics/machine_id_provider_win.cc:24: DVLOG(2) << "\n\nGetting
machine id\n\n";
Nit: Remove.

https://codereview.chromium.org/213363004/diff/100001/chrome/browser/metrics/...
File chrome/browser/metrics/metrics_service.cc (right):

https://codereview.chromium.org/213363004/diff/100001/chrome/browser/metrics/...
chrome/browser/metrics/metrics_service.cc:442:
registry->RegisterBooleanPref(prefs::kMetricsResetIds, false);
Nit: Maybe move this at the top of the list, so there won't be a merge conflict
between our two CLs.

https://codereview.chromium.org/213363004/diff/100001/chrome/browser/metrics/...
File chrome/browser/metrics/metrics_service.h (right):

https://codereview.chromium.org/213363004/diff/100001/chrome/browser/metrics/...
chrome/browser/metrics/metrics_service.h:530: bool
metrics_ids_reset_check_performed_;
Nit: Add a comment.

https://codereview.chromium.org/213363004/diff/100001/chrome/browser/metrics/...
File chrome/browser/metrics/metrics_service_unittest.cc (right):

https://codereview.chromium.org/213363004/diff/100001/chrome/browser/metrics/...
chrome/browser/metrics/metrics_service_unittest.cc:380: service.ForceC
What's going on here?

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

https://codereview.chromium.org/213363004/diff/100001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:27632: +  <summary>
Nit: Add owner tag as well.

[jwd, 2014-03-28 19:43:14]

https://codereview.chromium.org/213363004/diff/100001/chrome/browser/metrics/...
File chrome/browser/metrics/cloned_install_detector_unittest.cc (right):

https://codereview.chromium.org/213363004/diff/100001/chrome/browser/metrics/...
chrome/browser/metrics/cloned_install_detector_unittest.cc:51:
EXPECT_TRUE(prefs::kMetricsResetIds);
On 2014/03/28 19:02:50, Alexei Svitkine wrote:
> This needs to actually fetch the pref and check it's value...

Heh, yes, good catch.
Done

https://codereview.chromium.org/213363004/diff/100001/chrome/browser/metrics/...
File chrome/browser/metrics/machine_id_provider_win.cc (right):

https://codereview.chromium.org/213363004/diff/100001/chrome/browser/metrics/...
chrome/browser/metrics/machine_id_provider_win.cc:24: DVLOG(2) << "\n\nGetting
machine id\n\n";
On 2014/03/28 19:02:50, Alexei Svitkine wrote:
> Nit: Remove.

Done.

https://codereview.chromium.org/213363004/diff/100001/chrome/browser/metrics/...
File chrome/browser/metrics/metrics_service.cc (right):

https://codereview.chromium.org/213363004/diff/100001/chrome/browser/metrics/...
chrome/browser/metrics/metrics_service.cc:442:
registry->RegisterBooleanPref(prefs::kMetricsResetIds, false);
On 2014/03/28 19:02:50, Alexei Svitkine wrote:
> Nit: Maybe move this at the top of the list, so there won't be a merge
conflict
> between our two CLs.

Done.

https://codereview.chromium.org/213363004/diff/100001/chrome/browser/metrics/...
File chrome/browser/metrics/metrics_service.h (right):

https://codereview.chromium.org/213363004/diff/100001/chrome/browser/metrics/...
chrome/browser/metrics/metrics_service.h:530: bool
metrics_ids_reset_check_performed_;
On 2014/03/28 19:02:50, Alexei Svitkine wrote:
> Nit: Add a comment.

Done.

https://codereview.chromium.org/213363004/diff/100001/chrome/browser/metrics/...
File chrome/browser/metrics/metrics_service_unittest.cc (right):

https://codereview.chromium.org/213363004/diff/100001/chrome/browser/metrics/...
chrome/browser/metrics/metrics_service_unittest.cc:380: service.ForceC
On 2014/03/28 19:02:50, Alexei Svitkine wrote:
> What's going on here?

Wasn't done yet. Next patch has actual content.

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

https://codereview.chromium.org/213363004/diff/100001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:27632: +  <summary>
On 2014/03/28 19:02:50, Alexei Svitkine wrote:
> Nit: Add owner tag as well.

Done.

[Alexei Svitkine (slow), 2014-03-28 19:49:02]

Almost there, I think.

https://codereview.chromium.org/213363004/diff/120001/chrome/browser/metrics/...
File chrome/browser/metrics/metrics_service.h (right):

https://codereview.chromium.org/213363004/diff/120001/chrome/browser/metrics/...
chrome/browser/metrics/metrics_service.h:531: // to the kMetricsResetIds pref
being true. This prevents multiple resets
This comment isn't correct. It's also set to true if the check was performed and
nothing was reset.

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

https://codereview.chromium.org/213363004/diff/120001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:27641: +<histogram
name="UMA.MetricsIDsReset" enum="Boolean">
Hmm, this will likely not have a clean merge with my CL. I wonder if there's
another good name that can be used that would avoid that...

Also, change Boolean to BooleanHit.

[jwd, 2014-03-28 19:58:58]

https://codereview.chromium.org/213363004/diff/120001/chrome/browser/metrics/...
File chrome/browser/metrics/metrics_service.h (right):

https://codereview.chromium.org/213363004/diff/120001/chrome/browser/metrics/...
chrome/browser/metrics/metrics_service.h:531: // to the kMetricsResetIds pref
being true. This prevents multiple resets
On 2014/03/28 19:49:02, Alexei Svitkine wrote:
> This comment isn't correct. It's also set to true if the check was performed
and
> nothing was reset.

Done.

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

https://codereview.chromium.org/213363004/diff/120001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:27641: +<histogram
name="UMA.MetricsIDsReset" enum="Boolean">
On 2014/03/28 19:49:02, Alexei Svitkine wrote:
> Hmm, this will likely not have a clean merge with my CL. I wonder if there's
> another good name that can be used that would avoid that...
> 
> Also, change Boolean to BooleanHit.

Done.

[Alexei Svitkine (slow), 2014-03-28 20:05:47]

https://codereview.chromium.org/213363004/diff/140001/chrome/browser/metrics/...
File chrome/browser/metrics/metrics_service.h (right):

https://codereview.chromium.org/213363004/diff/140001/chrome/browser/metrics/...
chrome/browser/metrics/metrics_service.h:530: // Set to true when
|ResetMetricsIDsIfNecessary is called for the first time.
Nit: Either add another | or remove the single one.

https://codereview.chromium.org/213363004/diff/140001/chrome/browser/metrics/...
chrome/browser/metrics/metrics_service.h:531: // This prevents multiple resets
within the same chrome session.
Nit: chrome -> Chrome

https://codereview.chromium.org/213363004/diff/140001/chrome/browser/metrics/...
File chrome/browser/metrics/metrics_service_unittest.cc (right):

https://codereview.chromium.org/213363004/diff/140001/chrome/browser/metrics/...
chrome/browser/metrics/metrics_service_unittest.cc:379: // Ensuere that the low
entropy source and client id have been generated.
Nit: Ensure

https://codereview.chromium.org/213363004/diff/140001/chrome/browser/metrics/...
chrome/browser/metrics/metrics_service_unittest.cc:402:
EXPECT_NE(old_low_entropy_value, service.low_entropy_source_);
This will be flaky - since it could happen that we get the same low entropy
source value. Let's just do the check for the client id.

[jwd, 2014-03-28 20:18:13]

https://codereview.chromium.org/213363004/diff/140001/chrome/browser/metrics/...
File chrome/browser/metrics/metrics_service_unittest.cc (right):

https://codereview.chromium.org/213363004/diff/140001/chrome/browser/metrics/...
chrome/browser/metrics/metrics_service_unittest.cc:379: // Ensuere that the low
entropy source and client id have been generated.
On 2014/03/28 20:05:47, Alexei Svitkine wrote:
> Nit: Ensure

Done.

https://codereview.chromium.org/213363004/diff/140001/chrome/browser/metrics/...
chrome/browser/metrics/metrics_service_unittest.cc:402:
EXPECT_NE(old_low_entropy_value, service.low_entropy_source_);
On 2014/03/28 20:05:47, Alexei Svitkine wrote:
> This will be flaky - since it could happen that we get the same low entropy
> source value. Let's just do the check for the client id.

Done.

[Alexei Svitkine (slow), 2014-03-28 20:21:29]

LGTM % the metrics_service.h comment from a previous patchset

[jwd, 2014-03-28 20:27:17]

https://codereview.chromium.org/213363004/diff/140001/chrome/browser/metrics/...
File chrome/browser/metrics/metrics_service.h (right):

https://codereview.chromium.org/213363004/diff/140001/chrome/browser/metrics/...
chrome/browser/metrics/metrics_service.h:530: // Set to true when
|ResetMetricsIDsIfNecessary is called for the first time.
On 2014/03/28 20:05:47, Alexei Svitkine wrote:
> Nit: Either add another | or remove the single one.

Done.

https://codereview.chromium.org/213363004/diff/140001/chrome/browser/metrics/...
chrome/browser/metrics/metrics_service.h:531: // This prevents multiple resets
within the same chrome session.
On 2014/03/28 20:05:47, Alexei Svitkine wrote:
> Nit: chrome -> Chrome

Done.

[jwd, 2014-03-28 20:28:00]

The CQ bit was checked by jwd@chromium.org

[commit-bot: I haz the power, 2014-03-28 20:29:34]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jwd@chromium.org/213363004/170001

[commit-bot: I haz the power, 2014-03-28 23:45:08]

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

[commit-bot: I haz the power, 2014-03-28 23:45:09]

Commit queue rejected this change because the description was changed
between the time the change entered the commit queue and the time it
was ready to commit. You can safely check the commit box again.

[Ilya Sherman, 2014-03-28 23:57:49]

The CQ bit was checked by isherman@chromium.org

[commit-bot: I haz the power, 2014-03-29 00:00:50]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jwd@chromium.org/213363004/170001

[commit-bot: I haz the power, 2014-03-29 01:43:38]

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

[commit-bot: I haz the power, 2014-03-29 01:43:38]

Try jobs failed on following builders:
  tryserver.chromium on linux_chromium_chromeos_rel

[Alexei Svitkine (slow), 2014-03-29 02:08:36]

The CQ bit was checked by asvitkine@chromium.org

[commit-bot: I haz the power, 2014-03-29 02:10:19]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jwd@chromium.org/213363004/170001

[commit-bot: I haz the power, 2014-03-29 03:07:43]

Change committed as 260348