[NTP::SectionOrder] Add category position metric for opened suggestions.

[NTP::SectionOrder] Add category position metric for opened suggestions.

For each opened suggestion record its category position (i.e. rank). The
same is done per each category as well. E.g. if a suggestion in category
A (which is located at position X) is clicked, X is emited into general
histogram and into A's historgam.

This is done in order to reflect dynamic category order and measure 
how well it is performing.

BUG=676264
Review-Url: https://codereview.chromium.org/2609413005
Cr-Commit-Position: refs/heads/master@{#444340}
Committed: https://chromium.googlesource.com/chromium/src/+/6edff76757001e14e1ea38c1e4b4e9f1cdf74d32

[vitaliii, 2017-01-05 14:09:23]

Description was changed from

==========
[NTP::SectionOrder] Add category position metric for opened suggestions.

For each opened suggestion record its category position (i.e. rank). The
same is done per each category as well. E.g. if a suggestion in category
A (which is located at position X) is clicked, X is emited into general
histogram and into A's historgam.

This is done in order to reflect dynamic category order and measuring
how well it is performing.

BUG=676264
==========

to

==========
[NTP::SectionOrder] Add category position metric for opened suggestions.

For each opened suggestion record its category position (i.e. rank). The
same is done per each category as well. E.g. if a suggestion in category
A (which is located at position X) is clicked, X is emited into general
histogram and into A's historgam.

This is done in order to reflect dynamic category order and measure 
how well it is performing.

BUG=676264
==========

[vitaliii, 2017-01-05 14:20:19]

Patchset #1 (id:1) has been deleted

[vitaliii, 2017-01-05 14:20:25]

Patchset #1 (id:20001) has been deleted

[vitaliii, 2017-01-05 14:21:12]

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

[vitaliii, 2017-01-05 14:21:13]

Hi jkrcal@ & tschumann@,

Could you have a look at my metrics CL?

[noyau (Ping after 24h), 2017-01-05 14:38:03]

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

[noyau (Ping after 24h), 2017-01-05 14:38:04]

https://codereview.chromium.org/2609413005/diff/40001/chrome/browser/android/...
File chrome/browser/android/ntp/ntp_snippets_bridge.cc (right):

https://codereview.chromium.org/2609413005/diff/40001/chrome/browser/android/...
chrome/browser/android/ntp/ntp_snippets_bridge.cc:120: }
Can this lookup can be done by the metrics code in component instead of here to
avoid code duplication with non android UI?

[vitaliii, 2017-01-05 15:01:35]

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

[vitaliii, 2017-01-05 15:01:35]

[histograms.xml change, adding new metrics]

Hi asvitkine@,

Could you have a look at my histograms.xml change?

[vitaliii, 2017-01-05 15:16:39]

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

[commit-bot: I haz the power, 2017-01-05 15:16:56]

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

[vitaliii, 2017-01-05 15:17:00]

Hi noyau@,

I addressed your comment, PTAL.

https://codereview.chromium.org/2609413005/diff/40001/chrome/browser/android/...
File chrome/browser/android/ntp/ntp_snippets_bridge.cc (right):

https://codereview.chromium.org/2609413005/diff/40001/chrome/browser/android/...
chrome/browser/android/ntp/ntp_snippets_bridge.cc:120: }
On 2017/01/05 14:38:04, noyau wrote:
> Can this lookup can be done by the metrics code in component instead of here
to
> avoid code duplication with non android UI?

Done.

[jkrcal, 2017-01-05 15:18:08]

lgtm

https://codereview.chromium.org/2609413005/diff/40001/chrome/browser/android/...
File chrome/browser/android/ntp/ntp_snippets_bridge.cc (right):

https://codereview.chromium.org/2609413005/diff/40001/chrome/browser/android/...
chrome/browser/android/ntp/ntp_snippets_bridge.cc:120: }
On 2017/01/05 14:38:04, noyau wrote:
> Can this lookup can be done by the metrics code in component instead of here
to
> avoid code duplication with non android UI?

I am not sure this is the right way, since metrics code would need a pointer to
ContentSuggestionsService in OnSuggestionOpened(). To me, this seems even more
ugly than code duplication.

Do you insist?

[jkrcal, 2017-01-05 15:20:46]

I see, already done :)

This also lgtm (unless you agree, Éric, that it was better before;).

[noyau (Ping after 24h), 2017-01-05 15:23:50]

On 2017/01/05 15:18:08, jkrcal wrote:
> lgtm
> 
>
https://codereview.chromium.org/2609413005/diff/40001/chrome/browser/android/...
> File chrome/browser/android/ntp/ntp_snippets_bridge.cc (right):
> 
>
https://codereview.chromium.org/2609413005/diff/40001/chrome/browser/android/...
> chrome/browser/android/ntp/ntp_snippets_bridge.cc:120: }
> On 2017/01/05 14:38:04, noyau wrote:
> > Can this lookup can be done by the metrics code in component instead of here
> to
> > avoid code duplication with non android UI?
> 
> I am not sure this is the right way, since metrics code would need a pointer
to
> ContentSuggestionsService in OnSuggestionOpened(). To me, this seems even more
> ugly than code duplication.
> 
> Do you insist?

I insist for less code in android/ and more in components/ as much as possible.
The way it is done is less relevant, maybe there is another possible approach.
In this case why is the category ID known by the UI but something as relevant
for the UI as a position not known and need to be recalculated by doing an
expensive search in a list? Can the UI keep track of the category position
instead maybe? I haven't looked at this code in detail, I'm just keeping an eye
for issues that will make using these API on iOS more difficult.

[vitaliii, 2017-01-05 15:47:37]

Hi noyau@,

> I insist for less code in android/ and more in components/ as much as
> possible. The way it is done is less relevant, maybe there is another
> possible approach. In this case why is the category ID known by the UI
> but something as relevant for the UI as a position not known and need to
> be recalculated by doing an expensive search in a list? Can the UI keep
> track of the category position instead maybe? I haven't looked at this
> code in detail, I'm just keeping an eye for issues that will make using
> these API on iOS more difficult.

I added a TODO to propagate from UI (crbug.com/678623). Unfortunately, the code
responsible for opening a suggestion does not seem to be aware where its
category is located on the NTP. Thus, it will take me some time to investigate
how to obtain this data there.

https://codereview.chromium.org/2609413005/diff/40001/chrome/browser/android/...
File chrome/browser/android/ntp/ntp_snippets_bridge.cc (right):

https://codereview.chromium.org/2609413005/diff/40001/chrome/browser/android/...
chrome/browser/android/ntp/ntp_snippets_bridge.cc:120: }
On 2017/01/05 15:18:08, jkrcal wrote:
> On 2017/01/05 14:38:04, noyau wrote:
> > Can this lookup can be done by the metrics code in component instead of here
> to
> > avoid code duplication with non android UI?
> 
> I am not sure this is the right way, since metrics code would need a pointer
to
> ContentSuggestionsService in OnSuggestionOpened(). To me, this seems even more
> ugly than code duplication.
> 
> Do you insist?

Acknowledged.

[Alexei Svitkine (slow), 2017-01-05 16:00:36]

lgtm

[finkm, 2017-01-05 16:31:29]

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

https://codereview.chromium.org/2609413005/diff/80001/components/ntp_snippets...
components/ntp_snippets/content_suggestions_metrics.cc:21: const int
kMaxSuggestionsPerCategory = 10;
Hmmm ... since we have the more button, a section can have more then 10
suggestions. How does UMA_HISTOGRAM_ENUMERATION(name, sample, enum_max) behave
if sample > enum_max?

https://codereview.chromium.org/2609413005/diff/80001/components/ntp_snippets...
components/ntp_snippets/content_suggestions_metrics.cc:22: const int
kMaxSuggestionsTotal = 50;
same as above

[vitaliii, 2017-01-05 17:02:23]

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

[commit-bot: I haz the power, 2017-01-05 17:02: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-01-05 17:02:46]

Hey finkm@,

I addressed your comments.

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

https://codereview.chromium.org/2609413005/diff/80001/components/ntp_snippets...
components/ntp_snippets/content_suggestions_metrics.cc:21: const int
kMaxSuggestionsPerCategory = 10;
On 2017/01/05 16:31:29, finkm wrote:
> Hmmm ... since we have the more button, a section can have more then 10
> suggestions. How does UMA_HISTOGRAM_ENUMERATION(name, sample, enum_max) behave
> if sample > enum_max?

I created a bug and added a TODO (as per our offline discussion).

https://codereview.chromium.org/2609413005/diff/80001/components/ntp_snippets...
components/ntp_snippets/content_suggestions_metrics.cc:22: const int
kMaxSuggestionsTotal = 50;
On 2017/01/05 16:31:29, finkm wrote:
> same as above

same as above.

[tschumann, 2017-01-05 17:17:26]

On 2017/01/05 15:23:50, noyau wrote:
> On 2017/01/05 15:18:08, jkrcal wrote:
> > lgtm
> > 
> >
>
https://codereview.chromium.org/2609413005/diff/40001/chrome/browser/android/...
> > File chrome/browser/android/ntp/ntp_snippets_bridge.cc (right):
> > 
> >
>
https://codereview.chromium.org/2609413005/diff/40001/chrome/browser/android/...
> > chrome/browser/android/ntp/ntp_snippets_bridge.cc:120: }
> > On 2017/01/05 14:38:04, noyau wrote:
> > > Can this lookup can be done by the metrics code in component instead of
here
> > to
> > > avoid code duplication with non android UI?
> > 
> > I am not sure this is the right way, since metrics code would need a pointer
> to
> > ContentSuggestionsService in OnSuggestionOpened(). To me, this seems even
more
> > ugly than code duplication.
> > 
> > Do you insist?
> 
> I insist for less code in android/ and more in components/ as much as
possible.
> The way it is done is less relevant, maybe there is another possible approach.
> In this case why is the category ID known by the UI but something as relevant
> for the UI as a position not known and need to be recalculated by doing an
> expensive search in a list? Can the UI keep track of the category position
> instead maybe? I haven't looked at this code in detail, I'm just keeping an
eye
> for issues that will make using these API on iOS more difficult.

IMO the extra lookup is not critical as it's super cheap to do. The actual
problem is that the current order might not be the one used in the NTP the user
interacted with. So yes, we would need to get this information from the UI...

[vitaliii, 2017-01-05 17:49:51]

On 2017/01/05 17:17:26, tschumann wrote:
> On 2017/01/05 15:23:50, noyau wrote:
> > On 2017/01/05 15:18:08, jkrcal wrote:
> > > lgtm
> > > 
> > >
> >
>
https://codereview.chromium.org/2609413005/diff/40001/chrome/browser/android/...
> > > File chrome/browser/android/ntp/ntp_snippets_bridge.cc (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/2609413005/diff/40001/chrome/browser/android/...
> > > chrome/browser/android/ntp/ntp_snippets_bridge.cc:120: }
> > > On 2017/01/05 14:38:04, noyau wrote:
> > > > Can this lookup can be done by the metrics code in component instead of
> here
> > > to
> > > > avoid code duplication with non android UI?
> > > 
> > > I am not sure this is the right way, since metrics code would need a
pointer
> > to
> > > ContentSuggestionsService in OnSuggestionOpened(). To me, this seems even
> more
> > > ugly than code duplication.
> > > 
> > > Do you insist?
> > 
> > I insist for less code in android/ and more in components/ as much as
> possible.
> > The way it is done is less relevant, maybe there is another possible
approach.
> > In this case why is the category ID known by the UI but something as
relevant
> > for the UI as a position not known and need to be recalculated by doing an
> > expensive search in a list? Can the UI keep track of the category position
> > instead maybe? I haven't looked at this code in detail, I'm just keeping an
> eye
> > for issues that will make using these API on iOS more difficult.
> 
> IMO the extra lookup is not critical as it's super cheap to do. The actual
> problem is that the current order might not be the one used in the NTP the
user
> interacted with. So yes, we would need to get this information from the UI...

As per our private discussion, we decided to wait with this CL and propagate the

category ID from UI (as noyau@ suggested).

[commit-bot: I haz the power, 2017-01-05 18:59:45]

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

[commit-bot: I haz the power, 2017-01-05 18:59:46]

Dry run: This issue passed the CQ dry run.

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

Patchset #6 (id:130001) has been deleted

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

Patchset #6 (id:150001) has been deleted

[vitaliii, 2017-01-16 07:36:38]

Patchset #6 (id:170001) has been deleted

[vitaliii, 2017-01-16 08:20:25]

Patchset #7 (id:210001) has been deleted

[vitaliii, 2017-01-16 08:32:40]

Patchset #7 (id:230001) has been deleted

[vitaliii, 2017-01-16 08:32:47]

Patchset #7 (id:250001) has been deleted

[vitaliii, 2017-01-16 08:41:18]

Patchset #7 (id:270001) has been deleted

[vitaliii, 2017-01-16 08:41:40]

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

[commit-bot: I haz the power, 2017-01-16 08:41:54]

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

[vitaliii, 2017-01-16 08:43:51]

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

[vitaliii, 2017-01-16 08:43:52]

Hi dgn@,

Could you have a look at SnippetsBridge.java?

Also what is your plan regarding updating categoryRank to store real position on
the NTP? Are you still planning to do this?
If you are going to do this before M57 BP, I could wait with this CL, otherwise
I will have to land it before M57 BP.

[vitaliii, 2017-01-16 08:44:55]

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

[vitaliii, 2017-01-16 08:44:56]

Hi treib@,

could you sanity check my first metric?

It would be a shame to make a mistake here.

[vitaliii, 2017-01-16 08:46:11]

treib@,

+ ntp_snippets_bridge.* review please

(I missed that components/ntp_snippets OWNERS does not apply there)

[vitaliii, 2017-01-16 08:47:37]

Hi noyau@,

Now the category rank is propagated from the UI (as you suggested), PTAL.

[commit-bot: I haz the power, 2017-01-16 09:51:38]

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

[commit-bot: I haz the power, 2017-01-16 09:51:39]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Marc Treib, 2017-01-16 10:45:42]

https://codereview.chromium.org/2609413005/diff/290001/components/ntp_snippet...
File components/ntp_snippets/content_suggestions_metrics.cc (right):

https://codereview.chromium.org/2609413005/diff/290001/components/ntp_snippet...
components/ntp_snippets/content_suggestions_metrics.cc:23: const int
kMaxCategories = 20;
This seems a bit excessive - I don't think we'll ever have more than 10
categories at any time?

https://codereview.chromium.org/2609413005/diff/290001/components/ntp_snippet...
File components/ntp_snippets/content_suggestions_metrics.h (right):

https://codereview.chromium.org/2609413005/diff/290001/components/ntp_snippet...
components/ntp_snippets/content_suggestions_metrics.h:27: float score);
Hm. I'm wondering whether we should pack most of these params into a common
struct (even if some of them would be unused in some cases).
As it is, it's super easy to mess up some parameter, since these all take some
subset of (global_position, category_rank, position_in_category), all of which
are ints.

Not for this CL, but WDYT about this in general?

https://codereview.chromium.org/2609413005/diff/290001/tools/metrics/histogra...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/2609413005/diff/290001/tools/metrics/histogra...
tools/metrics/histograms/histograms.xml:39011: +    are not shown (they are
empty or dismissed by the user).
I don't understand the part about empty categories. Are they counted or not? I
think if a category shows up (empty or not), then it should be counted.

[dgn, 2017-01-16 10:55:10]

On 2017/01/16 08:43:52, vitaliii wrote:
> Hi dgn@,
> 
> Could you have a look at SnippetsBridge.java?
> 
> Also what is your plan regarding updating categoryRank to store real position
on
> the NTP? Are you still planning to do this?
> If you are going to do this before M57 BP, I could wait with this CL,
otherwise
> I will have to land it before M57 BP.

SnippetsBridge.java lgtm

I'll land the CL about updating the ranks (the one about snippet local and
global position is https://codereview.chromium.org/2618893003/) when it will be
ready. We are still discussing the proper definition for these values atm.

FYI: I renamed the SnippetArticle's fooPosition to fooRank, as we care about
"position" in the UI in terms of index of items we are displaying, and we don't
want to mix it up there.

[tschumann, 2017-01-16 11:02:22]

https://codereview.chromium.org/2609413005/diff/290001/components/ntp_snippet...
File components/ntp_snippets/content_suggestions_metrics.h (right):

https://codereview.chromium.org/2609413005/diff/290001/components/ntp_snippet...
components/ntp_snippets/content_suggestions_metrics.h:27: float score);
On 2017/01/16 10:45:42, Marc Treib wrote:
> Hm. I'm wondering whether we should pack most of these params into a common
> struct (even if some of them would be unused in some cases).
> As it is, it's super easy to mess up some parameter, since these all take some
> subset of (global_position, category_rank, position_in_category), all of which
> are ints.
> 
> Not for this CL, but WDYT about this in general?
Seems like a good idea. However, I'd be in favor of identifying a set of
required properties and enforce they are always set -- otherwise, we get a
pretty loose contract and it gets really hard to say which field is set where.

[Marc Treib, 2017-01-16 11:18:54]

https://codereview.chromium.org/2609413005/diff/290001/components/ntp_snippet...
File components/ntp_snippets/content_suggestions_metrics.h (right):

https://codereview.chromium.org/2609413005/diff/290001/components/ntp_snippet...
components/ntp_snippets/content_suggestions_metrics.h:27: float score);
On 2017/01/16 11:02:22, tschumann wrote:
> On 2017/01/16 10:45:42, Marc Treib wrote:
> > Hm. I'm wondering whether we should pack most of these params into a common
> > struct (even if some of them would be unused in some cases).
> > As it is, it's super easy to mess up some parameter, since these all take
some
> > subset of (global_position, category_rank, position_in_category), all of
which
> > are ints.
> > 
> > Not for this CL, but WDYT about this in general?
> Seems like a good idea. However, I'd be in favor of identifying a set of
> required properties and enforce they are always set -- otherwise, we get a
> pretty loose contract and it gets really hard to say which field is set where.

Oh, absolutely! I was thinking global_position, category, category_rank,
position_in_category, publish_date, score. The contract would be that all those
must always be set (even if not all of the methods actually make use of all of
them (yet)).

[vitaliii, 2017-01-18 09:18:33]

Patchset #8 (id:310001) has been deleted

[vitaliii, 2017-01-18 09:18:53]

Patchset #9 (id:350001) has been deleted

[vitaliii, 2017-01-18 09:19:20]

Hi treib@,

PTAL?

https://codereview.chromium.org/2609413005/diff/290001/components/ntp_snippet...
File components/ntp_snippets/content_suggestions_metrics.cc (right):

https://codereview.chromium.org/2609413005/diff/290001/components/ntp_snippet...
components/ntp_snippets/content_suggestions_metrics.cc:23: const int
kMaxCategories = 20;
On 2017/01/16 10:45:42, Marc Treib wrote:
> This seems a bit excessive - I don't think we'll ever have more than 10
> categories at any time?

Is there any downside of overestimating it?
We may have per topic categories and then 20 won't be that many.

https://codereview.chromium.org/2609413005/diff/290001/components/ntp_snippet...
File components/ntp_snippets/content_suggestions_metrics.h (right):

https://codereview.chromium.org/2609413005/diff/290001/components/ntp_snippet...
components/ntp_snippets/content_suggestions_metrics.h:27: float score);
On 2017/01/16 11:18:54, Marc Treib wrote:
> On 2017/01/16 11:02:22, tschumann wrote:
> > On 2017/01/16 10:45:42, Marc Treib wrote:
> > > Hm. I'm wondering whether we should pack most of these params into a
common
> > > struct (even if some of them would be unused in some cases).
> > > As it is, it's super easy to mess up some parameter, since these all take
> some
> > > subset of (global_position, category_rank, position_in_category), all of
> which
> > > are ints.
> > > 
> > > Not for this CL, but WDYT about this in general?
> > Seems like a good idea. However, I'd be in favor of identifying a set of
> > required properties and enforce they are always set -- otherwise, we get a
> > pretty loose contract and it gets really hard to say which field is set
where.
> 
> Oh, absolutely! I was thinking global_position, category, category_rank,
> position_in_category, publish_date, score. The contract would be that all
those
> must always be set (even if not all of the methods actually make use of all of
> them (yet)).

I agree, I added a TODO and crbug.com/682160.

https://codereview.chromium.org/2609413005/diff/290001/tools/metrics/histogra...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/2609413005/diff/290001/tools/metrics/histogra...
tools/metrics/histograms/histograms.xml:39011: +    are not shown (they are
empty or dismissed by the user).
On 2017/01/16 10:45:42, Marc Treib wrote:
> I don't understand the part about empty categories. Are they counted or not? I
> think if a category shows up (empty or not), then it should be counted.

Done.

However, they won't be counted after https://codereview.chromium.org/2618893003.

[Marc Treib, 2017-01-18 09:30:04]

https://codereview.chromium.org/2609413005/diff/290001/components/ntp_snippet...
File components/ntp_snippets/content_suggestions_metrics.cc (right):

https://codereview.chromium.org/2609413005/diff/290001/components/ntp_snippet...
components/ntp_snippets/content_suggestions_metrics.cc:23: const int
kMaxCategories = 20;
On 2017/01/18 09:19:20, vitaliii (OOO until 23rd) wrote:
> On 2017/01/16 10:45:42, Marc Treib wrote:
> > This seems a bit excessive - I don't think we'll ever have more than 10
> > categories at any time?
> 
> Is there any downside of overestimating it?
> We may have per topic categories and then 20 won't be that many.

Well, the histogram will use more memory, probably more bandwidth when
transmitting it, etc. It's not a huge deal, but IMO in practice we'll just have
lots of empty buckets. With topic categories, there could easily *exist* more
than 10, but I don't think we'd ever show more than 10 *at the same time*.

https://codereview.chromium.org/2609413005/diff/290001/tools/metrics/histogra...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/2609413005/diff/290001/tools/metrics/histogra...
tools/metrics/histograms/histograms.xml:39011: +    are not shown (they are
empty or dismissed by the user).
On 2017/01/18 09:19:20, vitaliii (OOO until 23rd) wrote:
> On 2017/01/16 10:45:42, Marc Treib wrote:
> > I don't understand the part about empty categories. Are they counted or not?
I
> > think if a category shows up (empty or not), then it should be counted.
> 
> Done.
> 
> However, they won't be counted after
https://codereview.chromium.org/2618893003.

...so right now, we'll count empty categories, even if they're not shown to the
user at all?! Under what circumstances exactly? E.g. would we always count
"bookmarks" even if the user doesn't have any?

[vitaliii, 2017-01-18 09:36:30]

Hi treib@,

PTAL.

https://codereview.chromium.org/2609413005/diff/290001/components/ntp_snippet...
File components/ntp_snippets/content_suggestions_metrics.cc (right):

https://codereview.chromium.org/2609413005/diff/290001/components/ntp_snippet...
components/ntp_snippets/content_suggestions_metrics.cc:23: const int
kMaxCategories = 20;
On 2017/01/18 09:30:04, Marc Treib wrote:
> On 2017/01/18 09:19:20, vitaliii (OOO until 23rd) wrote:
> > On 2017/01/16 10:45:42, Marc Treib wrote:
> > > This seems a bit excessive - I don't think we'll ever have more than 10
> > > categories at any time?
> > 
> > Is there any downside of overestimating it?
> > We may have per topic categories and then 20 won't be that many.
> 
> Well, the histogram will use more memory, probably more bandwidth when
> transmitting it, etc. It's not a huge deal, but IMO in practice we'll just
have
> lots of empty buckets. With topic categories, there could easily *exist* more
> than 10, but I don't think we'd ever show more than 10 *at the same time*.

Done, 10 now.

https://codereview.chromium.org/2609413005/diff/290001/tools/metrics/histogra...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/2609413005/diff/290001/tools/metrics/histogra...
tools/metrics/histograms/histograms.xml:39011: +    are not shown (they are
empty or dismissed by the user).
On 2017/01/18 09:30:04, Marc Treib wrote:
> On 2017/01/18 09:19:20, vitaliii (OOO until 23rd) wrote:
> > On 2017/01/16 10:45:42, Marc Treib wrote:
> > > I don't understand the part about empty categories. Are they counted or
not?
> I
> > > think if a category shows up (empty or not), then it should be counted.
> > 
> > Done.
> > 
> > However, they won't be counted after
> https://codereview.chromium.org/2618893003.
> 
> ...so right now, we'll count empty categories, even if they're not shown to
the
> user at all?! Under what circumstances exactly? E.g. would we always count
> "bookmarks" even if the user doesn't have any?

Yes.

The idea was to evaluate how ranker performs. 
E.g. imagine there are 10 categories and only 1 of them is shown and it is
ranked last by the ranker. The metric above would show the ranker is doing bad
job, while ignoring empty ones wouldn't.

However, both approaches should become equivalent after some time (the situation
above is rare and if a category is empty, than it won't get any clicks), thus, I
do not mind either of them.

[Marc Treib, 2017-01-18 09:39:13]

lgtm

https://codereview.chromium.org/2609413005/diff/290001/tools/metrics/histogra...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/2609413005/diff/290001/tools/metrics/histogra...
tools/metrics/histograms/histograms.xml:39011: +    are not shown (they are
empty or dismissed by the user).
On 2017/01/18 09:36:30, vitaliii (OOO until 23rd) wrote:
> On 2017/01/18 09:30:04, Marc Treib wrote:
> > On 2017/01/18 09:19:20, vitaliii (OOO until 23rd) wrote:
> > > On 2017/01/16 10:45:42, Marc Treib wrote:
> > > > I don't understand the part about empty categories. Are they counted or
> not?
> > I
> > > > think if a category shows up (empty or not), then it should be counted.
> > > 
> > > Done.
> > > 
> > > However, they won't be counted after
> > https://codereview.chromium.org/2618893003.
> > 
> > ...so right now, we'll count empty categories, even if they're not shown to
> the
> > user at all?! Under what circumstances exactly? E.g. would we always count
> > "bookmarks" even if the user doesn't have any?
> 
> Yes.
> 
> The idea was to evaluate how ranker performs. 
> E.g. imagine there are 10 categories and only 1 of them is shown and it is
> ranked last by the ranker. The metric above would show the ranker is doing bad
> job, while ignoring empty ones wouldn't.
> 
> However, both approaches should become equivalent after some time (the
situation
> above is rare and if a category is empty, than it won't get any clicks), thus,
I
> do not mind either of them.

Okay, fair enough I guess. Thanks for the explanation!

[vitaliii, 2017-01-18 09:43:11]

The CQ bit was checked by vitaliii@chromium.org

[vitaliii, 2017-01-18 09:43:11]

The patchset sent to the CQ was uploaded after l-g-t-m from jkrcal@chromium.org,
asvitkine@chromium.org, dgn@chromium.org
Link to the patchset: https://codereview.chromium.org/2609413005/#ps390001
(title: "treib@ comments.")

[commit-bot: I haz the power, 2017-01-18 09:43:24]

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-01-18 10:40:11]

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

[commit-bot: I haz the power, 2017-01-18 10:40:13]

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_...)

[vitaliii, 2017-01-18 11:04:34]

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

[commit-bot: I haz the power, 2017-01-18 11:04:45]

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

[vitaliii, 2017-01-18 11:27:04]

The CQ bit was unchecked by vitaliii@chromium.org

[vitaliii, 2017-01-18 11:27:10]

The CQ bit was checked by vitaliii@chromium.org

[vitaliii, 2017-01-18 11:27:10]

The patchset sent to the CQ was uploaded after l-g-t-m from dgn@chromium.org,
jkrcal@chromium.org, asvitkine@chromium.org, treib@chromium.org
Link to the patchset: https://codereview.chromium.org/2609413005/#ps410001
(title: "rebase.")

[commit-bot: I haz the power, 2017-01-18 11:27:26]

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-01-18 13:00:01]

CQ is committing da patch.

Bot data: {"patchset_id": 410001, "attempt_start_ts": 1484738830081770,
"parent_rev": "6491c7fb7288d049950c030296a579683895483a", "commit_rev":
"6edff76757001e14e1ea38c1e4b4e9f1cdf74d32"}

[commit-bot: I haz the power, 2017-01-18 13:00:34]

Description was changed from

==========
[NTP::SectionOrder] Add category position metric for opened suggestions.

For each opened suggestion record its category position (i.e. rank). The
same is done per each category as well. E.g. if a suggestion in category
A (which is located at position X) is clicked, X is emited into general
histogram and into A's historgam.

This is done in order to reflect dynamic category order and measure 
how well it is performing.

BUG=676264
==========

to

==========
[NTP::SectionOrder] Add category position metric for opened suggestions.

For each opened suggestion record its category position (i.e. rank). The
same is done per each category as well. E.g. if a suggestion in category
A (which is located at position X) is clicked, X is emited into general
histogram and into A's historgam.

This is done in order to reflect dynamic category order and measure 
how well it is performing.

BUG=676264

Review-Url: https://codereview.chromium.org/2609413005
Cr-Commit-Position: refs/heads/master@{#444340}
Committed:
https://chromium.googlesource.com/chromium/src/+/6edff76757001e14e1ea38c1e4b4...
==========

[commit-bot: I haz the power, 2017-01-18 13:00:36]

Committed patchset #11 (id:410001) as
https://chromium.googlesource.com/chromium/src/+/6edff76757001e14e1ea38c1e4b4...