NTP: Record TileType metrics also on desktop

NTP: Record TileType metrics also on desktop

BUG=703165
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation

Review-Url: https://codereview.chromium.org/2796643002
Cr-Commit-Position: refs/heads/master@{#462804}
Committed: https://chromium.googlesource.com/chromium/src/+/80f5122d11253e8c76379813a5352feb024b9ea2

[Marc Treib, 2017-04-03 14:48:35]

Description was changed from

==========
NTP: Record TileType metrics also on desktop

BUG=703165
==========

to

==========
NTP: Record TileType metrics also on desktop

BUG=703165
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

[Marc Treib, 2017-04-03 14:53:01]

treib@chromium.org changed reviewers:
+ mastiz@chromium.org, sfiera@chromium.org

[Marc Treib, 2017-04-03 14:53:01]

This is not yet ready for submission (no tests etc), but I'm looking for some
advice. With how things are currently set up, we can't track which thumbnails
are local vs. remote. If either of you have any ideas on how to do that (without
changing the remote NTP), I'd appreciate it.

Alternatively, mastiz@: How much do we need the local vs remote distinction in
UMA?

[sfiera, 2017-04-03 15:00:45]

On 2017/04/03 14:53:01, Marc Treib wrote:
> This is not yet ready for submission (no tests etc), but I'm looking for some
> advice. With how things are currently set up, we can't track which thumbnails
> are local vs. remote. If either of you have any ideas on how to do that
(without
> changing the remote NTP), I'd appreciate it.

I'm not sure I understand. We already have CTR metrics for client vs. server
tiles on Desktop, and all tiles are thumbnail tiles, right? Now, we'll have CTR
metrics for loaded vs. broken thumbnails, but it's fine for the latter to be
independent of the former.

> Alternatively, mastiz@: How much do we need the local vs remote distinction in
> UMA?

[Marc Treib, 2017-04-03 15:08:29]

On 2017/04/03 15:00:45, sfiera wrote:
> On 2017/04/03 14:53:01, Marc Treib wrote:
> > This is not yet ready for submission (no tests etc), but I'm looking for
some
> > advice. With how things are currently set up, we can't track which
thumbnails
> > are local vs. remote. If either of you have any ideas on how to do that
> (without
> > changing the remote NTP), I'd appreciate it.
> 
> I'm not sure I understand. We already have CTR metrics for client vs. server
> tiles on Desktop, and all tiles are thumbnail tiles, right? Now, we'll have
CTR
> metrics for loaded vs. broken thumbnails, but it's fine for the latter to be
> independent of the former.

Sorry, I wasn't very clear :-/

We do track just fine whether the tile itself came from the client or the
server. But for tiles from the server, we can't easily track whether the
thumbnail we show was a local one, or a remote one from Kodachrome, i.e. from
that "?fb=..." param in the thumbnail URL. As far as I know, the JS can't tell
if it got the preferred local thumbnail or the fallback, since that's decided in
C++ (ThumbnailSource).

[mastiz, 2017-04-03 15:26:31]

On 2017/04/03 14:53:01, Marc Treib wrote:
> This is not yet ready for submission (no tests etc), but I'm looking for some
> advice. With how things are currently set up, we can't track which thumbnails
> are local vs. remote. If either of you have any ideas on how to do that
(without
> changing the remote NTP), I'd appreciate it.
> 
> Alternatively, mastiz@: How much do we need the local vs remote distinction in
> UMA?

Not much. I would only implement this distinction if it's straightforward, since
we have server-side metrics that should cover most analysis needs.

[sfiera, 2017-04-03 15:27:44]

On 2017/04/03 15:08:29, Marc Treib wrote:
> On 2017/04/03 15:00:45, sfiera wrote:
> > On 2017/04/03 14:53:01, Marc Treib wrote:
> > > This is not yet ready for submission (no tests etc), but I'm looking for
> some
> > > advice. With how things are currently set up, we can't track which
> thumbnails
> > > are local vs. remote. If either of you have any ideas on how to do that
> > (without
> > > changing the remote NTP), I'd appreciate it.
> > 
> > I'm not sure I understand. We already have CTR metrics for client vs. server
> > tiles on Desktop, and all tiles are thumbnail tiles, right? Now, we'll have
> CTR
> > metrics for loaded vs. broken thumbnails, but it's fine for the latter to be
> > independent of the former.
> 
> Sorry, I wasn't very clear :-/
> 
> We do track just fine whether the tile itself came from the client or the
> server. But for tiles from the server, we can't easily track whether the
> thumbnail we show was a local one, or a remote one from Kodachrome, i.e. from
> that "?fb=..." param in the thumbnail URL. As far as I know, the JS can't tell
> if it got the preferred local thumbnail or the fallback, since that's decided
in
> C++ (ThumbnailSource).

I don't imagine there's any way to get that information from JS, or without
otherwise breaking the encapsulation of using the thumbnail source scheme.
(maybe, if the ?fb= fallback is a 302 redirect, but I assume not).

We should have metrics for how often we fall back, irrespective of CTR for
fallback tiles, right? It seems that the user experience should be pretty much
the same as non-?fb= tiles, so assuming independence of fallback rate and CTR
rate would be reasonable.

[Marc Treib, 2017-04-04 08:05:36]

On 2017/04/03 15:27:44, sfiera wrote:
> On 2017/04/03 15:08:29, Marc Treib wrote:
> > On 2017/04/03 15:00:45, sfiera wrote:
> > > On 2017/04/03 14:53:01, Marc Treib wrote:
> > > > This is not yet ready for submission (no tests etc), but I'm looking for
> > some
> > > > advice. With how things are currently set up, we can't track which
> > thumbnails
> > > > are local vs. remote. If either of you have any ideas on how to do that
> > > (without
> > > > changing the remote NTP), I'd appreciate it.
> > > 
> > > I'm not sure I understand. We already have CTR metrics for client vs.
server
> > > tiles on Desktop, and all tiles are thumbnail tiles, right? Now, we'll
have
> > CTR
> > > metrics for loaded vs. broken thumbnails, but it's fine for the latter to
be
> > > independent of the former.
> > 
> > Sorry, I wasn't very clear :-/
> > 
> > We do track just fine whether the tile itself came from the client or the
> > server. But for tiles from the server, we can't easily track whether the
> > thumbnail we show was a local one, or a remote one from Kodachrome, i.e.
from
> > that "?fb=..." param in the thumbnail URL. As far as I know, the JS can't
tell
> > if it got the preferred local thumbnail or the fallback, since that's
decided
> in
> > C++ (ThumbnailSource).
> 
> I don't imagine there's any way to get that information from JS, or without
> otherwise breaking the encapsulation of using the thumbnail source scheme.
> (maybe, if the ?fb= fallback is a 302 redirect, but I assume not).
> 
> We should have metrics for how often we fall back, irrespective of CTR for
> fallback tiles, right? It seems that the user experience should be pretty much
> the same as non-?fb= tiles, so assuming independence of fallback rate and CTR
> rate would be reasonable.

Thanks both!
Then I guess we won't record local vs server thumbnails. I think it would've
been useful to have that, e.g. to detect problem with the local thumbnail
taking, or to measure improvements. But I guess we can always look at TopSites
only for that.

No, the ?fb= is not a redirect; ThumbnailSource just handles that transparently.
No, I don't think we have metrics for how often we fall back to remote
thumbnails, though that should be easy to add.
The user experience isn't quite the same: E.g. for any sites that require
sign-in (Facebook, many Google properties etc), the server thumbnail will just
show a login page.

I'll ping the CL when it's ready for a full review.

[Marc Treib, 2017-04-04 08:52:17]

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

[commit-bot: I haz the power, 2017-04-04 08:52:34]

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

[Marc Treib, 2017-04-04 08:56:06]

Tests are here now. sfiera@, please review! :)

[Marc Treib, 2017-04-04 08:56:56]

treib@chromium.org changed reviewers:
- mastiz@chromium.org

[Marc Treib, 2017-04-04 08:56:56]

(mastiz to CC)

[commit-bot: I haz the power, 2017-04-04 10:46:54]

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

[commit-bot: I haz the power, 2017-04-04 10:46:55]

Dry run: This issue passed the CQ dry run.

[Marc Treib, 2017-04-05 14:22:35]

Ping!

[sfiera, 2017-04-06 08:53:28]

https://codereview.chromium.org/2796643002/diff/40001/chrome/renderer/searchb...
File chrome/renderer/searchbox/searchbox_extension.cc (right):

https://codereview.chromium.org/2796643002/diff/40001/chrome/renderer/searchb...
chrome/renderer/searchbox/searchbox_extension.cc:1007: args[2]->Uint32Value() <=
ntp_tiles::MAX_RECORDED_TILE_TYPE) {
Can we instead check here only that args[2] is a valid enum value, and let
ntp_tiles::metrics decide whether that value is recorded or not? As written,
this would change (0, TileSource::TOP_SITES, TileVisualType::UNKNOWN) to stop
logging the CLIENT impression because UNKNOWN isn't recorded. (even if that
tuple of arguments shouldn't actually happen)

https://codereview.chromium.org/2796643002/diff/40001/chrome/renderer/searchb...
chrome/renderer/searchbox/searchbox_extension.cc:1010: ntp_tiles::TileVisualType
tile_type =
Optional: with the static_cast<>, seems like a good place for auto.

https://codereview.chromium.org/2796643002/diff/40001/components/ntp_tiles/me...
File components/ntp_tiles/metrics_unittest.cc (right):

https://codereview.chromium.org/2796643002/diff/40001/components/ntp_tiles/me...
components/ntp_tiles/metrics_unittest.cc:174: {TileSource::TOP_SITES,
UNKNOWN_TILE_TYPE, GURL("http://www.s4.com/")}},
Can this be its own test, instead of hiding it in the Rappor test?

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

https://codereview.chromium.org/2796643002/diff/40001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:104462: +    FIXME he item displays a
thumbnail of the page.
FIXIT!

[Marc Treib, 2017-04-06 09:16:44]

Comments addressed, PTAL again!

https://codereview.chromium.org/2796643002/diff/40001/chrome/renderer/searchb...
File chrome/renderer/searchbox/searchbox_extension.cc (right):

https://codereview.chromium.org/2796643002/diff/40001/chrome/renderer/searchb...
chrome/renderer/searchbox/searchbox_extension.cc:1007: args[2]->Uint32Value() <=
ntp_tiles::MAX_RECORDED_TILE_TYPE) {
On 2017/04/06 08:53:28, sfiera wrote:
> Can we instead check here only that args[2] is a valid enum value, and let
> ntp_tiles::metrics decide whether that value is recorded or not? As written,
> this would change (0, TileSource::TOP_SITES, TileVisualType::UNKNOWN) to stop
> logging the CLIENT impression because UNKNOWN isn't recorded. (even if that
> tuple of arguments shouldn't actually happen)

Done.

https://codereview.chromium.org/2796643002/diff/40001/chrome/renderer/searchb...
chrome/renderer/searchbox/searchbox_extension.cc:1010: ntp_tiles::TileVisualType
tile_type =
On 2017/04/06 08:53:28, sfiera wrote:
> Optional: with the static_cast<>, seems like a good place for auto.

Done.

https://codereview.chromium.org/2796643002/diff/40001/components/ntp_tiles/me...
File components/ntp_tiles/metrics_unittest.cc (right):

https://codereview.chromium.org/2796643002/diff/40001/components/ntp_tiles/me...
components/ntp_tiles/metrics_unittest.cc:174: {TileSource::TOP_SITES,
UNKNOWN_TILE_TYPE, GURL("http://www.s4.com/")}},
On 2017/04/06 08:53:28, sfiera wrote:
> Can this be its own test, instead of hiding it in the Rappor test?

Sure, done. I've also updated/expanded the tests above a bit.

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

https://codereview.chromium.org/2796643002/diff/40001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:104462: +    FIXME he item displays a
thumbnail of the page.
On 2017/04/06 08:53:28, sfiera wrote:
> FIXIT!

Not sure what happened here... sorry, fixed.

[Marc Treib, 2017-04-06 09:16:50]

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

[commit-bot: I haz the power, 2017-04-06 09:17:10]

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

[sfiera, 2017-04-06 09:24:12]

LGTM

[Marc Treib, 2017-04-06 09:35:34]

treib@chromium.org changed reviewers:
+ isherman@chromium.org, rsesek@chromium.org

[Marc Treib, 2017-04-06 09:35:35]

+rsesek for IPC (instant.mojom, instant.typemap, instant_struct_traits.h)

+isherman for histograms.xml

PTAL!

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

https://codereview.chromium.org/2796643002/diff/60001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:42408: + 
<owner>mastiz@chromium.org</owner>
newt@ doesn't work on Chromium anymore. mastiz@ is currently the main person
looking at these metrics.

[commit-bot: I haz the power, 2017-04-06 10:17:29]

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

[commit-bot: I haz the power, 2017-04-06 10:17:30]

Dry run: This issue passed the CQ dry run.

[Robert Sesek, 2017-04-06 15:06:21]

IPC LGTM

[Ilya Sherman, 2017-04-06 19:39:15]

histograms.xml lgtm

[Marc Treib, 2017-04-07 07:54:31]

The CQ bit was checked by treib@chromium.org

[commit-bot: I haz the power, 2017-04-07 07:54:53]

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-04-07 07:59:43]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1491551671668270,
"parent_rev": "ea3bd60b2f5f5b394bde861c15d3c865e47caf87", "commit_rev":
"80f5122d11253e8c76379813a5352feb024b9ea2"}

[commit-bot: I haz the power, 2017-04-07 08:00:38]

Description was changed from

==========
NTP: Record TileType metrics also on desktop

BUG=703165
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

to

==========
NTP: Record TileType metrics also on desktop

BUG=703165
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation

Review-Url: https://codereview.chromium.org/2796643002
Cr-Commit-Position: refs/heads/master@{#462804}
Committed:
https://chromium.googlesource.com/chromium/src/+/80f5122d11253e8c76379813a535...
==========

[commit-bot: I haz the power, 2017-04-07 08:00:40]

Committed patchset #4 (id:60001) as
https://chromium.googlesource.com/chromium/src/+/80f5122d11253e8c76379813a535...