Add 4 Variations.SeedDateSkew.* histograms to analyze whether the system clock or the build time co…

Add 4 Variations.SeedDateSkew.* histograms to analyze whether the system clock or the build time could be used as a long term expiry clock for very old variations seeds.

BUG=419622
Committed: https://crrev.com/ca7e80742478074d6f96190c14f9b97e8eeba2ba
Cr-Commit-Position: refs/heads/master@{#297841}

[gab, 2014-10-02 02:32:15]

Patchset #1 (id:1) has been deleted

[gab, 2014-10-02 02:34:23]

gab@chromium.org changed reviewers:
+ asvitkine@chromium.org

[gab, 2014-10-02 02:34:23]

Alexei PTAL, as discussed long ago, see bug for details.

Thanks!
Gab

[Alexei Svitkine (slow), 2014-10-02 13:00:33]

https://codereview.chromium.org/617263007/diff/20001/chrome/browser/metrics/v...
File chrome/browser/metrics/variations/variations_service.cc (right):

https://codereview.chromium.org/617263007/diff/20001/chrome/browser/metrics/v...
chrome/browser/metrics/variations/variations_service.cc:297:
local_state_->GetInt64(prefs::kVariationsSeedDate);
Note that if we do start using this to make decisions, we'll need to protect
this pref in some way (probably instead of storing it separately, make it a
field in the seed that's received). But we can do that.

https://codereview.chromium.org/617263007/diff/20001/chrome/browser/metrics/v...
chrome/browser/metrics/variations/variations_service.cc:325:
UMA_HISTOGRAM_CUSTOM_COUNTS("Variations.SeedDateSkew.BuildTimeAheadBy",
I suggest using UMA_HISTOGRAM_COUNTS_100() for all of these, instead of a custom
macro that's not very different.

https://codereview.chromium.org/617263007/diff/20001/tools/metrics/histograms...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/617263007/diff/20001/tools/metrics/histograms...
tools/metrics/histograms/histograms.xml:36511: +    could be used to discard
very old seeds.
Mention when this is logged (Logged on start up when creating field trials from
the variations seed.) - do so for all of these.

[gab, 2014-10-02 14:03:23]

Thanks, PTAL.

https://codereview.chromium.org/617263007/diff/20001/chrome/browser/metrics/v...
File chrome/browser/metrics/variations/variations_service.cc (right):

https://codereview.chromium.org/617263007/diff/20001/chrome/browser/metrics/v...
chrome/browser/metrics/variations/variations_service.cc:297:
local_state_->GetInt64(prefs::kVariationsSeedDate);
On 2014/10/02 13:00:32, Alexei Svitkine wrote:
> Note that if we do start using this to make decisions, we'll need to protect
> this pref in some way (probably instead of storing it separately, make it a
> field in the seed that's received). But we can do that.

Right that makes sense, we can discuss implementation details if this proves to
be worthwhile avenue.

https://codereview.chromium.org/617263007/diff/20001/chrome/browser/metrics/v...
chrome/browser/metrics/variations/variations_service.cc:325:
UMA_HISTOGRAM_CUSTOM_COUNTS("Variations.SeedDateSkew.BuildTimeAheadBy",
On 2014/10/02 13:00:32, Alexei Svitkine wrote:
> I suggest using UMA_HISTOGRAM_COUNTS_100() for all of these, instead of a
custom
> macro that's not very different.

UMA_HISTOGRAM_COUNTS_100 is from 1 to 100; I assume that includes 0 despite the
range suggesting it doesn't?

https://codereview.chromium.org/617263007/diff/20001/tools/metrics/histograms...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/617263007/diff/20001/tools/metrics/histograms...
tools/metrics/histograms/histograms.xml:36511: +    could be used to discard
very old seeds.
On 2014/10/02 13:00:33, Alexei Svitkine wrote:
> Mention when this is logged (Logged on start up when creating field trials
from
> the variations seed.) - do so for all of these.

Done.

[Alexei Svitkine (slow), 2014-10-02 14:05:01]

https://codereview.chromium.org/617263007/diff/20001/chrome/browser/metrics/v...
File chrome/browser/metrics/variations/variations_service.cc (right):

https://codereview.chromium.org/617263007/diff/20001/chrome/browser/metrics/v...
chrome/browser/metrics/variations/variations_service.cc:325:
UMA_HISTOGRAM_CUSTOM_COUNTS("Variations.SeedDateSkew.BuildTimeAheadBy",
On 2014/10/02 14:03:23, gab wrote:
> On 2014/10/02 13:00:32, Alexei Svitkine wrote:
> > I suggest using UMA_HISTOGRAM_COUNTS_100() for all of these, instead of a
> custom
> > macro that's not very different.
> 
> UMA_HISTOGRAM_COUNTS_100 is from 1 to 100; I assume that includes 0 despite
the
> range suggesting it doesn't?

Yes, there's always an underflow bucket - in this case 0 to 1.

[Alexei Svitkine (slow), 2014-10-02 14:05:37]

LGTM

[gab, 2014-10-02 14:05:51]

The CQ bit was checked by gab@chromium.org

[commit-bot: I haz the power, 2014-10-02 14:06:46]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/617263007/40001

[commit-bot: I haz the power, 2014-10-02 15:45:38]

Committed patchset #2 (id:40001) as b538c2ab90e6307d7a145c576271889d1c4614ab

[commit-bot: I haz the power, 2014-10-02 15:46:24]

Patchset 2 (id:??) landed as
https://crrev.com/ca7e80742478074d6f96190c14f9b97e8eeba2ba
Cr-Commit-Position: refs/heads/master@{#297841}