Measure usage metrics to prepare for adaptive fetching rates in M55

Measure usage metrics to prepare for adaptive fetching rates in M55

In order to scale to 100% of users in M55, we need adaptive fetching
rates for server-side suggestions on the new tab page. This CL
implements reporting of usage metrics to UMA. We need to collect these
metrics already in M54 so that we have enough data to define default
parameters for the adaptive fetching rates in M55.

Design doc: https://docs.google.com/a/google.com/document/d/1tDl2Dx_ZPnSyoe7hAe55mT2uTeXDaFVphqsWZEjp3EQ/edit?usp=sharing

BUG=644716
Committed: https://crrev.com/e13510e5938155d4eb7b8a742919a11aa7f8556c
Cr-Commit-Position: refs/heads/master@{#417336}

[jkrcal, 2016-09-07 13:09:00]

jkrcal@chromium.org changed reviewers:
+ bauerb@chromium.org, noyau@chromium.org, rkaplow@chromium.org

[jkrcal, 2016-09-07 13:09:01]

Bernhard, PTAL!

Éric, PTAL at the iOS factory.
Robert, PTAL at histograms.xml.

[Marc Treib, 2016-09-07 13:23:47]

treib@chromium.org changed reviewers:
+ treib@chromium.org

[Marc Treib, 2016-09-07 13:23:48]

Some drive-by comments :)

https://codereview.chromium.org/2315273002/diff/1/chrome/browser/prefs/browse...
File chrome/browser/prefs/browser_prefs.cc (right):

https://codereview.chromium.org/2315273002/diff/1/chrome/browser/prefs/browse...
chrome/browser/prefs/browser_prefs.cc:535: #if BUILDFLAG(ANDROID_JAVA_UI)
Is this the correct flag, or should it be OS_ANDROID? I always get confused.

Also, at least some parts of the code technically exist on non-Android too (even
if they're not active yet). E.g. it might be safer/more consistent to register
the BookmarkProvider prefs on all platforms?

https://codereview.chromium.org/2315273002/diff/1/components/ntp_snippets/con...
File components/ntp_snippets/content_suggestions_service.h (right):

https://codereview.chromium.org/2315273002/diff/1/components/ntp_snippets/con...
components/ntp_snippets/content_suggestions_service.h:10: #include <set>
Is this required?

https://codereview.chromium.org/2315273002/diff/1/components/ntp_snippets/con...
components/ntp_snippets/content_suggestions_service.h:87: PrefService*
pref_service);
Forward-declare PrefService

https://codereview.chromium.org/2315273002/diff/1/components/ntp_snippets/use...
File components/ntp_snippets/user_classifier.cc (right):

https://codereview.chromium.org/2315273002/diff/1/components/ntp_snippets/use...
components/ntp_snippets/user_classifier.cc:26: // as loosing your phone or going
for a long offline vacation do not skew the
nit: s/loosing/losing/

https://codereview.chromium.org/2315273002/diff/1/components/ntp_snippets/use...
File components/ntp_snippets/user_classifier.h (right):

https://codereview.chromium.org/2315273002/diff/1/components/ntp_snippets/use...
components/ntp_snippets/user_classifier.h:17: // TODO(jkrcal): Add the
classification and the getters.
nit: IMO the comment above should say only what the class actually does right
now; the TODO should mention what you plan on adding.

[rkaplow, 2016-09-07 13:33:10]

https://codereview.chromium.org/2315273002/diff/1/components/ntp_snippets/use...
File components/ntp_snippets/user_classifier.cc (right):

https://codereview.chromium.org/2315273002/diff/1/components/ntp_snippets/use...
components/ntp_snippets/user_classifier.cc:18: // TODO(jkrcal): Make all of this
Finch configurable.
nit, variations_service is the external chromium name

https://codereview.chromium.org/2315273002/diff/1/components/ntp_snippets/use...
components/ntp_snippets/user_classifier.cc:90: double default_metric_value) {
align

https://codereview.chromium.org/2315273002/diff/1/components/ntp_snippets/use...
components/ntp_snippets/user_classifier.cc:101: avg = hours_since_last_time *
kDiscountFactor + avg * (1 - kDiscountFactor);
is it best to discount after rounding down to the nearest hour?

[noyau (Ping after 24h), 2016-09-07 13:44:05]

lgtm

ios lgtm.

[Bernhard Bauer, 2016-09-07 13:46:46]

Could you add a link to the design doc to this CL?

https://codereview.chromium.org/2315273002/diff/1/components/ntp_snippets/con...
File components/ntp_snippets/content_suggestions_service_unittest.cc (right):

https://codereview.chromium.org/2315273002/diff/1/components/ntp_snippets/con...
components/ntp_snippets/content_suggestions_service_unittest.cc:209:
/*history_service=*/nullptr,
Could you add spaces between the comment delimiters? This all looks a bit
cramped.

https://codereview.chromium.org/2315273002/diff/1/components/ntp_snippets/use...
File components/ntp_snippets/user_classifier.cc (right):

https://codereview.chromium.org/2315273002/diff/1/components/ntp_snippets/use...
components/ntp_snippets/user_classifier.cc:26: // as loosing your phone or going
for a long offline vacation do not skew the
Nit: "losing"

https://codereview.chromium.org/2315273002/diff/1/components/ntp_snippets/use...
components/ntp_snippets/user_classifier.cc:89: const char* last_time_pref_name,
Nit: Align arguments.

https://codereview.chromium.org/2315273002/diff/1/components/ntp_snippets/use...
File components/ntp_snippets/user_classifier.h (right):

https://codereview.chromium.org/2315273002/diff/1/components/ntp_snippets/use...
components/ntp_snippets/user_classifier.h:30: // When the content suggestions
are shown to the user - in the current
Add empty lines before comment blocks?

https://codereview.chromium.org/2315273002/diff/1/components/ntp_snippets/use...
components/ntp_snippets/user_classifier.h:45: PrefService* pref_service_;
DISALLOW_COPY_AND_ASSIGN

[jkrcal, 2016-09-07 14:18:42]

Description was changed from

==========
Measure usage metrics to prepare for adaptive fetching rates in M55

In order to scale to 100% of users in M55, we need adaptive fetching
rates for server-side suggestions on the new tab page. This CL
implements reporting of usage metrics to UMA. We need to collect these
metrics already in M54 so that we have enough data to define default
parameters for the adaptive fetching rates in M55.

BUG=644716
==========

to

==========
Measure usage metrics to prepare for adaptive fetching rates in M55

In order to scale to 100% of users in M55, we need adaptive fetching
rates for server-side suggestions on the new tab page. This CL
implements reporting of usage metrics to UMA. We need to collect these
metrics already in M54 so that we have enough data to define default
parameters for the adaptive fetching rates in M55.

Design doc:
https://docs.google.com/a/google.com/document/d/1tDl2Dx_ZPnSyoe7hAe55mT2uTeXD...

BUG=644716
==========

[jkrcal, 2016-09-07 14:19:08]

Thanks! PTAL, again.

https://codereview.chromium.org/2315273002/diff/1/chrome/browser/prefs/browse...
File chrome/browser/prefs/browser_prefs.cc (right):

https://codereview.chromium.org/2315273002/diff/1/chrome/browser/prefs/browse...
chrome/browser/prefs/browser_prefs.cc:535: #if BUILDFLAG(ANDROID_JAVA_UI)
On 2016/09/07 13:23:48, Marc Treib wrote:
> Is this the correct flag, or should it be OS_ANDROID? I always get confused.
> 
> Also, at least some parts of the code technically exist on non-Android too
(even
> if they're not active yet). E.g. it might be safer/more consistent to register
> the BookmarkProvider prefs on all platforms?

I thought this is the right way. I just tried to organize the scattered parts in
one place. Probably too risky to merge into M54 in the last minute. I will undo
the changes. and register the new stuff always. Still I think we should clarify
where all this should go.

https://codereview.chromium.org/2315273002/diff/1/components/ntp_snippets/con...
File components/ntp_snippets/content_suggestions_service.h (right):

https://codereview.chromium.org/2315273002/diff/1/components/ntp_snippets/con...
components/ntp_snippets/content_suggestions_service.h:10: #include <set>
On 2016/09/07 13:23:48, Marc Treib wrote:
> Is this required?

Not for the changes I did. Still, std::set is used in this file.

https://codereview.chromium.org/2315273002/diff/1/components/ntp_snippets/con...
components/ntp_snippets/content_suggestions_service.h:87: PrefService*
pref_service);
On 2016/09/07 13:23:48, Marc Treib wrote:
> Forward-declare PrefService

Done.

https://codereview.chromium.org/2315273002/diff/1/components/ntp_snippets/con...
File components/ntp_snippets/content_suggestions_service_unittest.cc (right):

https://codereview.chromium.org/2315273002/diff/1/components/ntp_snippets/con...
components/ntp_snippets/content_suggestions_service_unittest.cc:209:
/*history_service=*/nullptr,
On 2016/09/07 13:46:45, Bernhard Bauer wrote:
> Could you add spaces between the comment delimiters? This all looks a bit
> cramped.

By chromium code search, this seems the way more popular way to format it. Do
you insist?

https://codereview.chromium.org/2315273002/diff/1/components/ntp_snippets/use...
File components/ntp_snippets/user_classifier.cc (right):

https://codereview.chromium.org/2315273002/diff/1/components/ntp_snippets/use...
components/ntp_snippets/user_classifier.cc:18: // TODO(jkrcal): Make all of this
Finch configurable.
On 2016/09/07 13:33:10, rkaplow (ooo) wrote:
> nit, variations_service is the external chromium name

Done.

https://codereview.chromium.org/2315273002/diff/1/components/ntp_snippets/use...
components/ntp_snippets/user_classifier.cc:26: // as loosing your phone or going
for a long offline vacation do not skew the
On 2016/09/07 13:23:48, Marc Treib wrote:
> nit: s/loosing/losing/

Done.

https://codereview.chromium.org/2315273002/diff/1/components/ntp_snippets/use...
components/ntp_snippets/user_classifier.cc:89: const char* last_time_pref_name,
On 2016/09/07 13:46:45, Bernhard Bauer wrote:
> Nit: Align arguments.

Done.

https://codereview.chromium.org/2315273002/diff/1/components/ntp_snippets/use...
components/ntp_snippets/user_classifier.cc:90: double default_metric_value) {
On 2016/09/07 13:33:10, rkaplow (ooo) wrote:
> align

Done.

https://codereview.chromium.org/2315273002/diff/1/components/ntp_snippets/use...
components/ntp_snippets/user_classifier.cc:101: avg = hours_since_last_time *
kDiscountFactor + avg * (1 - kDiscountFactor);
On 2016/09/07 13:33:10, rkaplow (ooo) wrote:
> is it best to discount after rounding down to the nearest hour?

I do not understand. I treat the number always as doubles, never round down in
the computation. I round down only when reporting to UMA. Do you suggest me to
round it down here? If so, why?

https://codereview.chromium.org/2315273002/diff/1/components/ntp_snippets/use...
File components/ntp_snippets/user_classifier.h (right):

https://codereview.chromium.org/2315273002/diff/1/components/ntp_snippets/use...
components/ntp_snippets/user_classifier.h:17: // TODO(jkrcal): Add the
classification and the getters.
On 2016/09/07 13:23:48, Marc Treib wrote:
> nit: IMO the comment above should say only what the class actually does right
> now; the TODO should mention what you plan on adding.

Done.

https://codereview.chromium.org/2315273002/diff/1/components/ntp_snippets/use...
components/ntp_snippets/user_classifier.h:30: // When the content suggestions
are shown to the user - in the current
On 2016/09/07 13:46:45, Bernhard Bauer wrote:
> Add empty lines before comment blocks?

Done.

https://codereview.chromium.org/2315273002/diff/1/components/ntp_snippets/use...
components/ntp_snippets/user_classifier.h:45: PrefService* pref_service_;
On 2016/09/07 13:46:45, Bernhard Bauer wrote:
> DISALLOW_COPY_AND_ASSIGN

Done.

[Bernhard Bauer, 2016-09-07 17:21:13]

https://codereview.chromium.org/2315273002/diff/1/components/ntp_snippets/con...
File components/ntp_snippets/content_suggestions_service_unittest.cc (right):

https://codereview.chromium.org/2315273002/diff/1/components/ntp_snippets/con...
components/ntp_snippets/content_suggestions_service_unittest.cc:209:
/*history_service=*/nullptr,
On 2016/09/07 14:19:07, jkrcal wrote:
> On 2016/09/07 13:46:45, Bernhard Bauer wrote:
> > Could you add spaces between the comment delimiters? This all looks a bit
> > cramped.
> 
> By chromium code search, this seems the way more popular way to format it. Do
> you insist?

Actually, what seems to be the most common style is `value /* variable_name */`,
which I would not be averse to :)

[jkrcal, 2016-09-07 19:09:35]

PTAL, again.

https://codereview.chromium.org/2315273002/diff/1/components/ntp_snippets/con...
File components/ntp_snippets/content_suggestions_service_unittest.cc (right):

https://codereview.chromium.org/2315273002/diff/1/components/ntp_snippets/con...
components/ntp_snippets/content_suggestions_service_unittest.cc:209:
/*history_service=*/nullptr,
On 2016/09/07 17:21:13, Bernhard Bauer wrote:
> On 2016/09/07 14:19:07, jkrcal wrote:
> > On 2016/09/07 13:46:45, Bernhard Bauer wrote:
> > > Could you add spaces between the comment delimiters? This all looks a bit
> > > cramped.
> > 
> > By chromium code search, this seems the way more popular way to format it.
Do
> > you insist?
> 
> Actually, what seems to be the most common style is `value /* variable_name
*/`,
> which I would not be averse to :)

Done.

[jkrcal, 2016-09-07 19:11:19]

The CQ bit was checked by jkrcal@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-07 19:12:02]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[jkrcal, 2016-09-07 19:51:14]

The CQ bit was checked by jkrcal@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-07 19:51:36]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[jkrcal, 2016-09-07 20:33:02]

The CQ bit was checked by jkrcal@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-07 20:33:35]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[jkrcal, 2016-09-07 20:34:09]

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

[jkrcal, 2016-09-07 20:34:10]

Alexei, could you PTAL at histograms.xml instead of Robert (who is ooo)?

I would still like to land (and merge to M54) today :)

[Bernhard Bauer, 2016-09-07 21:31:00]

LGTM w/ nits:

https://codereview.chromium.org/2315273002/diff/80001/components/ntp_snippets...
File components/ntp_snippets/user_classifier.cc (right):

https://codereview.chromium.org/2315273002/diff/80001/components/ntp_snippets...
components/ntp_snippets/user_classifier.cc:29: const double kMaxHours = 7*24;
Nit: Space before and after multiplication operator.

https://codereview.chromium.org/2315273002/diff/80001/components/ntp_snippets...
components/ntp_snippets/user_classifier.cc:117: avg_events_per_hour =
Maybe use a new variable |new_avg_events_per_hour| for this?

https://codereview.chromium.org/2315273002/diff/80001/components/ntp_snippets...
components/ntp_snippets/user_classifier.cc:128: if (avg_events_per_hour > 1) {
Flip the condition and return kMaxHours in the other case?

Or actually, is the other case even supposed to happen if this is called right
after an update?

https://codereview.chromium.org/2315273002/diff/80001/components/ntp_snippets...
File components/ntp_snippets/user_classifier.h (right):

https://codereview.chromium.org/2315273002/diff/80001/components/ntp_snippets...
components/ntp_snippets/user_classifier.h:54: double discount_rate_per_hour_;
const

[commit-bot: I haz the power, 2016-09-07 21:36:36]

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

[commit-bot: I haz the power, 2016-09-07 21:36:36]

Dry run: This issue passed the CQ dry run.

[jkrcal, 2016-09-08 05:44:58]

The CQ bit was checked by jkrcal@chromium.org to run a CQ dry run

[jkrcal, 2016-09-08 05:45:25]

Thanks, Berhnard!

Robert / Alexei, PTAL at histograms.xml.

https://codereview.chromium.org/2315273002/diff/80001/components/ntp_snippets...
File components/ntp_snippets/user_classifier.cc (right):

https://codereview.chromium.org/2315273002/diff/80001/components/ntp_snippets...
components/ntp_snippets/user_classifier.cc:29: const double kMaxHours = 7*24;
On 2016/09/07 21:31:00, Bernhard Bauer wrote:
> Nit: Space before and after multiplication operator.

Done.

https://codereview.chromium.org/2315273002/diff/80001/components/ntp_snippets...
components/ntp_snippets/user_classifier.cc:117: avg_events_per_hour =
On 2016/09/07 21:31:00, Bernhard Bauer wrote:
> Maybe use a new variable |new_avg_events_per_hour| for this?

Done.

https://codereview.chromium.org/2315273002/diff/80001/components/ntp_snippets...
components/ntp_snippets/user_classifier.cc:128: if (avg_events_per_hour > 1) {
On 2016/09/07 21:31:00, Bernhard Bauer wrote:
> Flip the condition and return kMaxHours in the other case?
> 
> Or actually, is the other case even supposed to happen if this is called right
> after an update?

Done. Yes, it can be equal to 1.

https://codereview.chromium.org/2315273002/diff/80001/components/ntp_snippets...
File components/ntp_snippets/user_classifier.h (right):

https://codereview.chromium.org/2315273002/diff/80001/components/ntp_snippets...
components/ntp_snippets/user_classifier.h:54: double discount_rate_per_hour_;
On 2016/09/07 21:31:00, Bernhard Bauer wrote:
> const

Done.

[commit-bot: I haz the power, 2016-09-08 05:45:34]

Dry run: 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, 2016-09-08 07:55:26]

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

[commit-bot: I haz the power, 2016-09-08 07:55:28]

Dry run: This issue passed the CQ dry run.

[Alexei Svitkine (slow), 2016-09-08 16:51:41]

lgtm

[jkrcal, 2016-09-08 17:49:30]

The CQ bit was checked by jkrcal@chromium.org

[jkrcal, 2016-09-08 17:49:30]

The patchset sent to the CQ was uploaded after l-g-t-m from noyau@chromium.org,
bauerb@chromium.org
Link to the patchset: https://codereview.chromium.org/2315273002/#ps100001
(title: "Bernhard's comments")

[commit-bot: I haz the power, 2016-09-08 17:50:23]

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, 2016-09-08 17:56:38]

Description was changed from

==========
Measure usage metrics to prepare for adaptive fetching rates in M55

In order to scale to 100% of users in M55, we need adaptive fetching
rates for server-side suggestions on the new tab page. This CL
implements reporting of usage metrics to UMA. We need to collect these
metrics already in M54 so that we have enough data to define default
parameters for the adaptive fetching rates in M55.

Design doc:
https://docs.google.com/a/google.com/document/d/1tDl2Dx_ZPnSyoe7hAe55mT2uTeXD...

BUG=644716
==========

to

==========
Measure usage metrics to prepare for adaptive fetching rates in M55

In order to scale to 100% of users in M55, we need adaptive fetching
rates for server-side suggestions on the new tab page. This CL
implements reporting of usage metrics to UMA. We need to collect these
metrics already in M54 so that we have enough data to define default
parameters for the adaptive fetching rates in M55.

Design doc:
https://docs.google.com/a/google.com/document/d/1tDl2Dx_ZPnSyoe7hAe55mT2uTeXD...

BUG=644716
==========

[commit-bot: I haz the power, 2016-09-08 17:56:39]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2016-09-08 17:59:43]

Description was changed from

==========
Measure usage metrics to prepare for adaptive fetching rates in M55

In order to scale to 100% of users in M55, we need adaptive fetching
rates for server-side suggestions on the new tab page. This CL
implements reporting of usage metrics to UMA. We need to collect these
metrics already in M54 so that we have enough data to define default
parameters for the adaptive fetching rates in M55.

Design doc:
https://docs.google.com/a/google.com/document/d/1tDl2Dx_ZPnSyoe7hAe55mT2uTeXD...

BUG=644716
==========

to

==========
Measure usage metrics to prepare for adaptive fetching rates in M55

In order to scale to 100% of users in M55, we need adaptive fetching
rates for server-side suggestions on the new tab page. This CL
implements reporting of usage metrics to UMA. We need to collect these
metrics already in M54 so that we have enough data to define default
parameters for the adaptive fetching rates in M55.

Design doc:
https://docs.google.com/a/google.com/document/d/1tDl2Dx_ZPnSyoe7hAe55mT2uTeXD...

BUG=644716

Committed: https://crrev.com/e13510e5938155d4eb7b8a742919a11aa7f8556c
Cr-Commit-Position: refs/heads/master@{#417336}
==========

[commit-bot: I haz the power, 2016-09-08 17:59:44]

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