[NTP::SectionOrder] Track category index after it has been moved up.

[NTP::SectionOrder] Track category index after it has been moved up.

Add a new metric to track category index after it has been moved up due
to a click. This is done in order to check how stable to order is and
whether it is becoming more stable.

BUG=687505
Review-Url: https://codereview.chromium.org/2668063004
Cr-Commit-Position: refs/heads/master@{#447968}
Committed: https://chromium.googlesource.com/chromium/src/+/7496904fab33e2c38f8c6ec6cfad27ebd158c1f4

[vitaliii, 2017-02-01 11:56:22]

vitaliii@chromium.org changed reviewers:
+ tschumann@chromium.org

[vitaliii, 2017-02-01 11:56:23]

Would you like to have a look?

[vitaliii, 2017-02-01 12:46:40]

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

[vitaliii, 2017-02-01 12:46:40]

Hi treib@,

Would you mind to review a new metric?

[Marc Treib, 2017-02-01 13:04:24]

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

https://codereview.chromium.org/2668063004/diff/1/components/ntp_snippets/con...
components/ntp_snippets/content_suggestions_metrics.cc:59:
"NewTabPage.ContentSuggestions.MovedUpCategoryNewIndex";
".CategoryMovedUpNewIndex"?

https://codereview.chromium.org/2668063004/diff/1/components/ntp_snippets/con...
components/ntp_snippets/content_suggestions_metrics.cc:337: kMaxCategories);
Hm. I guess you'd mostly look at the number of samples in the histogram over a
given period of time? Or do you see any use for the actual index values?

I'm also a bit worried that this will be hard to interpret: You'd have to
"normalize" over number of users of course, but also over total usage - if there
are more clicks overall, then there's probably also be more movement in the
categories. Or whenever we add a new category, there'll be more movement...

But I also can't immediately think of a better way to track this. Any ideas?

[vitaliii, 2017-02-01 13:30:19]

Hi treib@,

I replied to your comments, PTAL.

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

https://codereview.chromium.org/2668063004/diff/1/components/ntp_snippets/con...
components/ntp_snippets/content_suggestions_metrics.cc:59:
"NewTabPage.ContentSuggestions.MovedUpCategoryNewIndex";
On 2017/02/01 13:04:23, Marc Treib wrote:
> ".CategoryMovedUpNewIndex"?

I like previous one more.

Which category? Moved up.

The new one feels like an action, "category moved up".

Also old one seems more consistent with "OpenedCategoryIndex".

Do you feel other way?

https://codereview.chromium.org/2668063004/diff/1/components/ntp_snippets/con...
components/ntp_snippets/content_suggestions_metrics.cc:337: kMaxCategories);
On 2017/02/01 13:04:23, Marc Treib wrote:
> Hm. I guess you'd mostly look at the number of samples in the histogram over a
> given period of time? Or do you see any use for the actual index values?

Partially true. We are worried that top most categories will wiggle around.
Thus, we could use data for the first 3 positions and ignore lower positions.

> I'm also a bit worried that this will be hard to interpret: You'd have to
> "normalize" over number of users of course, but also over total usage - if
there
> are more clicks overall, then there's probably also be more movement in the
> categories. Or whenever we add a new category, there'll be more movement...

Number of users - probably, number of clicks - likely not. The order should
become more stable with time, so number of clicks should not matter. Ideally new
users will introduce some values here, but only before their order settles down.
If this highly depends on clicks, we set the thresholds too low. 

> 
> But I also can't immediately think of a better way to track this. Any ideas?

tschumann@ and I cannot find a better way as well, but this seemed good enough
to see how stable the order becomes with time.

[Marc Treib, 2017-02-01 13:40:46]

lgtm

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

https://codereview.chromium.org/2668063004/diff/1/components/ntp_snippets/con...
components/ntp_snippets/content_suggestions_metrics.cc:59:
"NewTabPage.ContentSuggestions.MovedUpCategoryNewIndex";
On 2017/02/01 13:30:18, vitaliii wrote:
> On 2017/02/01 13:04:23, Marc Treib wrote:
> > ".CategoryMovedUpNewIndex"?
> 
> I like previous one more.
> 
> Which category? Moved up.
> 
> The new one feels like an action, "category moved up".
> 
> Also old one seems more consistent with "OpenedCategoryIndex".
> 
> Do you feel other way?

Ah, I wasn't aware of OpenedCategoryIndex. I guess I'd have preferred
CategoryOpenedIndex there as well (or actually just CategoryOpened; the "Index"
is specified by the "units" field in the .xml), so that all the category-related
things start with "Category". But since it's too late for that, I guess it
doesn't matter anymore.

https://codereview.chromium.org/2668063004/diff/1/components/ntp_snippets/con...
components/ntp_snippets/content_suggestions_metrics.cc:337: kMaxCategories);
On 2017/02/01 13:30:18, vitaliii wrote:
> On 2017/02/01 13:04:23, Marc Treib wrote:
> > Hm. I guess you'd mostly look at the number of samples in the histogram over
a
> > given period of time? Or do you see any use for the actual index values?
> 
> Partially true. We are worried that top most categories will wiggle around.
> Thus, we could use data for the first 3 positions and ignore lower positions.
> 
> > I'm also a bit worried that this will be hard to interpret: You'd have to
> > "normalize" over number of users of course, but also over total usage - if
> there
> > are more clicks overall, then there's probably also be more movement in the
> > categories. Or whenever we add a new category, there'll be more movement...
> 
> Number of users - probably, number of clicks - likely not. The order should
> become more stable with time, so number of clicks should not matter. Ideally
new
> users will introduce some values here, but only before their order settles
down.
> If this highly depends on clicks, we set the thresholds too low. 
> 
> > 
> > But I also can't immediately think of a better way to track this. Any ideas?
> 
> tschumann@ and I cannot find a better way as well, but this seemed good enough
> to see how stable the order becomes with time.

Alright, fair enough.

[Marc Treib, 2017-02-01 13:42:37]

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

https://codereview.chromium.org/2668063004/diff/1/components/ntp_snippets/con...
components/ntp_snippets/content_suggestions_metrics.cc:337: kMaxCategories);
On 2017/02/01 13:40:46, Marc Treib wrote:
> On 2017/02/01 13:30:18, vitaliii wrote:
> > On 2017/02/01 13:04:23, Marc Treib wrote:
> > > Hm. I guess you'd mostly look at the number of samples in the histogram
over
> a
> > > given period of time? Or do you see any use for the actual index values?
> > 
> > Partially true. We are worried that top most categories will wiggle around.
> > Thus, we could use data for the first 3 positions and ignore lower
positions.
> > 
> > > I'm also a bit worried that this will be hard to interpret: You'd have to
> > > "normalize" over number of users of course, but also over total usage - if
> > there
> > > are more clicks overall, then there's probably also be more movement in
the
> > > categories. Or whenever we add a new category, there'll be more
movement...
> > 
> > Number of users - probably, number of clicks - likely not. The order should
> > become more stable with time, so number of clicks should not matter. Ideally
> new
> > users will introduce some values here, but only before their order settles
> down.
> > If this highly depends on clicks, we set the thresholds too low. 
> > 
> > > 
> > > But I also can't immediately think of a better way to track this. Any
ideas?
> > 
> > tschumann@ and I cannot find a better way as well, but this seemed good
enough
> > to see how stable the order becomes with time.
> 
> Alright, fair enough.

(About tracking only the top 3 or something: No, if we do care about the index
(at least in some cases), then let's just track the index.)

[vitaliii, 2017-02-01 14:18:29]

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

[commit-bot: I haz the power, 2017-02-01 14:18:41]

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

[vitaliii, 2017-02-01 14:19:37]

Hi treib@,

I addressed your comments, no need to look.

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

https://codereview.chromium.org/2668063004/diff/1/components/ntp_snippets/con...
components/ntp_snippets/content_suggestions_metrics.cc:59:
"NewTabPage.ContentSuggestions.MovedUpCategoryNewIndex";
On 2017/02/01 13:40:46, Marc Treib wrote:
> On 2017/02/01 13:30:18, vitaliii wrote:
> > On 2017/02/01 13:04:23, Marc Treib wrote:
> > > ".CategoryMovedUpNewIndex"?
> > 
> > I like previous one more.
> > 
> > Which category? Moved up.
> > 
> > The new one feels like an action, "category moved up".
> > 
> > Also old one seems more consistent with "OpenedCategoryIndex".
> > 
> > Do you feel other way?
> 
> Ah, I wasn't aware of OpenedCategoryIndex. I guess I'd have preferred
> CategoryOpenedIndex there as well (or actually just CategoryOpened; the
"Index"
> is specified by the "units" field in the .xml), so that all the
category-related
> things start with "Category". But since it's too late for that, I guess it
> doesn't matter anymore.

Acknowledged.

https://codereview.chromium.org/2668063004/diff/1/components/ntp_snippets/con...
components/ntp_snippets/content_suggestions_metrics.cc:337: kMaxCategories);
On 2017/02/01 13:42:37, Marc Treib wrote:
> On 2017/02/01 13:40:46, Marc Treib wrote:
> > On 2017/02/01 13:30:18, vitaliii wrote:
> > > On 2017/02/01 13:04:23, Marc Treib wrote:
> > > > Hm. I guess you'd mostly look at the number of samples in the histogram
> over
> > a
> > > > given period of time? Or do you see any use for the actual index values?
> > > 
> > > Partially true. We are worried that top most categories will wiggle
around.
> > > Thus, we could use data for the first 3 positions and ignore lower
> positions.
> > > 
> > > > I'm also a bit worried that this will be hard to interpret: You'd have
to
> > > > "normalize" over number of users of course, but also over total usage -
if
> > > there
> > > > are more clicks overall, then there's probably also be more movement in
> the
> > > > categories. Or whenever we add a new category, there'll be more
> movement...
> > > 
> > > Number of users - probably, number of clicks - likely not. The order
should
> > > become more stable with time, so number of clicks should not matter.
Ideally
> > new
> > > users will introduce some values here, but only before their order settles
> > down.
> > > If this highly depends on clicks, we set the thresholds too low. 
> > > 
> > > > 
> > > > But I also can't immediately think of a better way to track this. Any
> ideas?
> > > 
> > > tschumann@ and I cannot find a better way as well, but this seemed good
> enough
> > > to see how stable the order becomes with time.
> > 
> > Alright, fair enough.
> 
> (About tracking only the top 3 or something: No, if we do care about the index
> (at least in some cases), then let's just track the index.)

Acknowledged.

The mention of top 3 was to explain why we need indices.

[vitaliii, 2017-02-01 14:21:31]

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

[vitaliii, 2017-02-01 14:21:32]

Hi asvitkine@,

Could you have a look at my histograms.xml change?
I am adding a new metric.

[commit-bot: I haz the power, 2017-02-01 15:13:06]

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

[commit-bot: I haz the power, 2017-02-01 15:13:07]

Dry run: This issue passed the CQ dry run.

[Alexei Svitkine (slow), 2017-02-01 18:40:35]

lgtm

[vitaliii, 2017-02-02 10:55:12]

Patchset #3 (id:40001) has been deleted

[vitaliii, 2017-02-02 10:55:35]

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

[commit-bot: I haz the power, 2017-02-02 10:55:41]

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

[vitaliii, 2017-02-02 10:55:55]

Hi treib@,

could you have another look? I have added tests.

[Marc Treib, 2017-02-02 11:04:16]

lgtm

Nice! Some small suggestions below.

https://codereview.chromium.org/2668063004/diff/60001/components/ntp_snippets...
File
components/ntp_snippets/category_rankers/click_based_category_ranker_unittest.cc
(right):

https://codereview.chromium.org/2668063004/diff/60001/components/ntp_snippets...
components/ntp_snippets/category_rankers/click_based_category_ranker_unittest.cc:695:
EXPECT_THAT(histogram_tester.GetAllSamples(kHistogramMovedUpCategoryNewIndex),
nit: ASSERT_THAT since this is a precondition (right?)

https://codereview.chromium.org/2668063004/diff/60001/components/ntp_snippets...
components/ntp_snippets/category_rankers/click_based_category_ranker_unittest.cc:718:
ranker()->ClearHistory(/*begin=*/base::Time(),
This should reset the category order, right? Maybe ASSERT that? (IMO that'd make
the test a bit easier to read, since it's not immediately clear what clearing
history would have to do with anything.)

https://codereview.chromium.org/2668063004/diff/60001/components/ntp_snippets...
components/ntp_snippets/category_rankers/click_based_category_ranker_unittest.cc:736:
EXPECT_THAT(histogram_tester.GetAllSamples(kHistogramMovedUpCategoryNewIndex),
Also here: ASSERT_THAT?

[vitaliii, 2017-02-02 11:25:00]

Hi treib@,

I addressed your comments, no need to look.

https://codereview.chromium.org/2668063004/diff/60001/components/ntp_snippets...
File
components/ntp_snippets/category_rankers/click_based_category_ranker_unittest.cc
(right):

https://codereview.chromium.org/2668063004/diff/60001/components/ntp_snippets...
components/ntp_snippets/category_rankers/click_based_category_ranker_unittest.cc:695:
EXPECT_THAT(histogram_tester.GetAllSamples(kHistogramMovedUpCategoryNewIndex),
On 2017/02/02 11:04:15, Marc Treib wrote:
> nit: ASSERT_THAT since this is a precondition (right?)

Done.

https://codereview.chromium.org/2668063004/diff/60001/components/ntp_snippets...
components/ntp_snippets/category_rankers/click_based_category_ranker_unittest.cc:718:
ranker()->ClearHistory(/*begin=*/base::Time(),
On 2017/02/02 11:04:15, Marc Treib wrote:
> This should reset the category order, right? Maybe ASSERT that? (IMO that'd
make
> the test a bit easier to read, since it's not immediately clear what clearing
> history would have to do with anything.)

Done.

Right, I added ASSERTs for the order.

https://codereview.chromium.org/2668063004/diff/60001/components/ntp_snippets...
components/ntp_snippets/category_rankers/click_based_category_ranker_unittest.cc:736:
EXPECT_THAT(histogram_tester.GetAllSamples(kHistogramMovedUpCategoryNewIndex),
On 2017/02/02 11:04:15, Marc Treib wrote:
> Also here: ASSERT_THAT?

Done.

[vitaliii, 2017-02-02 11:25:12]

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

[commit-bot: I haz the power, 2017-02-02 11:25:25]

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

[tschumann, 2017-02-02 11:45:09]

lgtm

https://codereview.chromium.org/2668063004/diff/80001/components/ntp_snippets...
File
components/ntp_snippets/category_rankers/click_based_category_ranker_unittest.cc
(right):

https://codereview.chromium.org/2668063004/diff/80001/components/ntp_snippets...
components/ntp_snippets/category_rankers/click_based_category_ranker_unittest.cc:675:
while (CompareCategories(first, second)) {
i had a hard time understanding the loop. 
The biggest issue was NotifySuggestionOpened().
why not write
ranker()->OnSuggestionOpened(second); 

which is much easier to read?

Maybe also extend the comment to:
// Increase score of |second| until the order changes.

same in line 715.

That's a nice example how convenience methods can make the test harder to read.

[commit-bot: I haz the power, 2017-02-02 12:38:01]

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

[commit-bot: I haz the power, 2017-02-02 12:38:02]

Dry run: This issue passed the CQ dry run.

[vitaliii, 2017-02-03 08:09:30]

Patchset #6 (id:120001) has been deleted

[vitaliii, 2017-02-03 08:09:38]

Patchset #6 (id:140001) has been deleted

[vitaliii, 2017-02-03 08:10:51]

Hi tschumann@,

I addressed your comment, no need to look.

https://codereview.chromium.org/2668063004/diff/80001/components/ntp_snippets...
File
components/ntp_snippets/category_rankers/click_based_category_ranker_unittest.cc
(right):

https://codereview.chromium.org/2668063004/diff/80001/components/ntp_snippets...
components/ntp_snippets/category_rankers/click_based_category_ranker_unittest.cc:675:
while (CompareCategories(first, second)) {
On 2017/02/02 11:45:09, tschumann wrote:
> i had a hard time understanding the loop. 
> The biggest issue was NotifySuggestionOpened().
> why not write
> ranker()->OnSuggestionOpened(second); 
> 
> which is much easier to read?
> 
> Maybe also extend the comment to:
> // Increase score of |second| until the order changes.
> 
> same in line 715.
> 
> That's a nice example how convenience methods can make the test harder to
read. 

Done. 

I replaced NotifySuggestionOpened(/*times=*/1, category) with
ranker()->OnSuggestionOpened(category) in tests where all instances have |times
= 1|.

In my view for the case |times > 1| this helper shortens the code and makes it
easier to read.

[vitaliii, 2017-02-03 08:12:01]

The CQ bit was checked by vitaliii@chromium.org

[vitaliii, 2017-02-03 08:12:01]

The patchset sent to the CQ was uploaded after l-g-t-m from
asvitkine@chromium.org, treib@chromium.org, tschumann@chromium.org
Link to the patchset: https://codereview.chromium.org/2668063004/#ps160001
(title: "tschumann@ comment.")

[commit-bot: I haz the power, 2017-02-03 08:14:02]

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-02-03 09:27:45]

CQ is committing da patch.

Bot data: {"patchset_id": 160001, "attempt_start_ts": 1486109521567520,
"parent_rev": "ca0b0f3cb32cc26808f0979e8a9a0082740c5eac", "commit_rev":
"7496904fab33e2c38f8c6ec6cfad27ebd158c1f4"}

[commit-bot: I haz the power, 2017-02-03 09:28:22]

Description was changed from

==========
[NTP::SectionOrder] Track category index after it has been moved up.

Add a new metric to track category index after it has been moved up due
to a click. This is done in order to check how stable to order is and
whether it is becoming more stable.

BUG=687505
==========

to

==========
[NTP::SectionOrder] Track category index after it has been moved up.

Add a new metric to track category index after it has been moved up due
to a click. This is done in order to check how stable to order is and
whether it is becoming more stable.

BUG=687505

Review-Url: https://codereview.chromium.org/2668063004
Cr-Commit-Position: refs/heads/master@{#447968}
Committed:
https://chromium.googlesource.com/chromium/src/+/7496904fab33e2c38f8c6ec6cfad...
==========

[commit-bot: I haz the power, 2017-02-03 09:28:23]

Committed patchset #6 (id:160001) as
https://chromium.googlesource.com/chromium/src/+/7496904fab33e2c38f8c6ec6cfad...