[LargeIconService] Report size of favicon for fallback style to UMA

[LargeIconService] Report size of favicon for fallback style to UMA

This CL adds UMA metrics for sizes of favicons from which fallback style
was generated (because large enough icon is missing). This is important
for evaluating if changing the minimum size limit / using more advanced
method for upscaling would help.

BUG=709498
Review-Url: https://codereview.chromium.org/2809783003
Cr-Commit-Position: refs/heads/master@{#464363}
Committed: https://chromium.googlesource.com/chromium/src/+/5f0f424e645a9b8209244256f9a7d8bdb1b4e035

[jkrcal, 2017-04-11 16:36:02]

jkrcal@chromium.org changed reviewers:
+ isherman@chromium.org, pkotwicz@chromium.org

[jkrcal, 2017-04-11 16:36:03]

Peter, could you PTAL?

Ilya, could you PTAL at histograms.xml?

[jkrcal, 2017-04-11 16:36:10]

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

[commit-bot: I haz the power, 2017-04-11 16:36:38]

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, 2017-04-11 17:43:03]

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

[commit-bot: I haz the power, 2017-04-11 17:43:04]

Dry run: This issue passed the CQ dry run.

[Ilya Sherman, 2017-04-11 21:36:10]

https://codereview.chromium.org/2809783003/diff/1/components/favicon/core/lar...
File components/favicon/core/large_icon_service.cc (right):

https://codereview.chromium.org/2809783003/diff/1/components/favicon/core/lar...
components/favicon/core/large_icon_service.cc:127: fallback_icon_size);
Perhaps UMA_HISTOGRAM_SPARSE_SLOWLY, with some custom sanity-checks on the
logged value, would be more appropriate?

[pkotwicz, 2017-04-12 02:54:07]

lgtm

[jkrcal, 2017-04-12 05:40:52]

Thanks, PTAL, again!

https://codereview.chromium.org/2809783003/diff/1/components/favicon/core/lar...
File components/favicon/core/large_icon_service.cc (right):

https://codereview.chromium.org/2809783003/diff/1/components/favicon/core/lar...
components/favicon/core/large_icon_service.cc:127: fallback_icon_size);
On 2017/04/11 21:36:09, Ilya Sherman wrote:
> Perhaps UMA_HISTOGRAM_SPARSE_SLOWLY, with some custom sanity-checks on the
> logged value, would be more appropriate?

Done. 
No sanity checks are needed as this must be non-negative and relatively small
number (<100) by definition. If not, there are some bugs that we would like to
see :)

[Ilya Sherman, 2017-04-12 20:45:34]

LGTM % comment:

https://codereview.chromium.org/2809783003/diff/1/components/favicon/core/lar...
File components/favicon/core/large_icon_service.cc (right):

https://codereview.chromium.org/2809783003/diff/1/components/favicon/core/lar...
components/favicon/core/large_icon_service.cc:127: fallback_icon_size);
On 2017/04/12 05:40:51, jkrcal wrote:
> On 2017/04/11 21:36:09, Ilya Sherman wrote:
> > Perhaps UMA_HISTOGRAM_SPARSE_SLOWLY, with some custom sanity-checks on the
> > logged value, would be more appropriate?
> 
> Done. 
> No sanity checks are needed as this must be non-negative and relatively small
> number (<100) by definition. If not, there are some bugs that we would like to
> see :)

Okay.  Could you please add a DCHECK at least?  =)

[jkrcal, 2017-04-13 07:16:05]

Thanks!

https://codereview.chromium.org/2809783003/diff/1/components/favicon/core/lar...
File components/favicon/core/large_icon_service.cc (right):

https://codereview.chromium.org/2809783003/diff/1/components/favicon/core/lar...
components/favicon/core/large_icon_service.cc:127: fallback_icon_size);
On 2017/04/12 20:45:33, Ilya Sherman wrote:
> On 2017/04/12 05:40:51, jkrcal wrote:
> > On 2017/04/11 21:36:09, Ilya Sherman wrote:
> > > Perhaps UMA_HISTOGRAM_SPARSE_SLOWLY, with some custom sanity-checks on the
> > > logged value, would be more appropriate?
> > 
> > Done. 
> > No sanity checks are needed as this must be non-negative and relatively
small
> > number (<100) by definition. If not, there are some bugs that we would like
to
> > see :)
> 
> Okay.  Could you please add a DCHECK at least?  =)

Done (I check only the lower limit, the upper limit depends on what the client
passes as |min_source_size| and I do not want to restrict something that the API
allows).

[jkrcal, 2017-04-13 07:16:31]

The CQ bit was checked by jkrcal@chromium.org

[jkrcal, 2017-04-13 07:16:31]

The patchset sent to the CQ was uploaded after l-g-t-m from
pkotwicz@chromium.org, isherman@chromium.org
Link to the patchset: https://codereview.chromium.org/2809783003/#ps40001
(title: "Ilya's comment #2")

[commit-bot: I haz the power, 2017-04-13 07:16:48]

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

[Ilya Sherman, 2017-04-13 07:46:29]

https://codereview.chromium.org/2809783003/diff/40001/components/favicon/core...
File components/favicon/core/large_icon_service.cc (right):

https://codereview.chromium.org/2809783003/diff/40001/components/favicon/core...
components/favicon/core/large_icon_service.cc:125: DCHECK_GT(fallback_icon_size,
0);
Hmm, if the API allows setting an arbitrary upper bound, then what guarantees
that the histogram will not explode?

[jkrcal, 2017-04-13 07:55:34]

The CQ bit was unchecked by jkrcal@chromium.org

[jkrcal, 2017-04-13 08:05:07]

On 2017/04/13 07:46:29, Ilya Sherman wrote:
>
https://codereview.chromium.org/2809783003/diff/40001/components/favicon/core...
> File components/favicon/core/large_icon_service.cc (right):
> 
>
https://codereview.chromium.org/2809783003/diff/40001/components/favicon/core...
> components/favicon/core/large_icon_service.cc:125:
DCHECK_GT(fallback_icon_size,
> 0);
> Hmm, if the API allows setting an arbitrary upper bound, then what guarantees
> that the histogram will not explode?

I switched off the commit bit, for the time being.

What do you mean by the histogram exploding? I guess I do not fully understand
the implications of using UMA_HISTOGRAM_SPARSE_SLOWLY and the comments in
histogram_macros.h do not help me either to understand that.

I do not want to DCHECK because this way, I would (debug) crash for an
absolutely legal value.
I am totally willing to cap the values logged by, e.g.
 fallback_icon_size = std::min(fallback_icon_size, 128)
What is the advantage of logging SPARSE? Is it that I get always the precise
number and not just one of the buckets?

I would also be fine with the original
UMA_HISTOGRAM_COUNTS_100("Favicons.LargeIconService.FallbackSize",
fallback_icon_size);
 (+ the DCHECK_GT(fallback_icon_size, 0))

[Ilya Sherman, 2017-04-13 08:10:50]

On 2017/04/13 08:05:07, jkrcal wrote:
> On 2017/04/13 07:46:29, Ilya Sherman wrote:
> >
>
https://codereview.chromium.org/2809783003/diff/40001/components/favicon/core...
> > File components/favicon/core/large_icon_service.cc (right):
> > 
> >
>
https://codereview.chromium.org/2809783003/diff/40001/components/favicon/core...
> > components/favicon/core/large_icon_service.cc:125:
> DCHECK_GT(fallback_icon_size,
> > 0);
> > Hmm, if the API allows setting an arbitrary upper bound, then what
guarantees
> > that the histogram will not explode?
> 
> I switched off the commit bit, for the time being.
> 
> What do you mean by the histogram exploding? I guess I do not fully understand
> the implications of using UMA_HISTOGRAM_SPARSE_SLOWLY and the comments in
> histogram_macros.h do not help me either to understand that.
> 
> I do not want to DCHECK because this way, I would (debug) crash for an
> absolutely legal value.
> I am totally willing to cap the values logged by, e.g.
>  fallback_icon_size = std::min(fallback_icon_size, 128)
> What is the advantage of logging SPARSE? Is it that I get always the precise
> number and not just one of the buckets?
> 
> I would also be fine with the original
> UMA_HISTOGRAM_COUNTS_100("Favicons.LargeIconService.FallbackSize",
> fallback_icon_size);
>  (+ the DCHECK_GT(fallback_icon_size, 0))

By the histogram exploding, I mean seeing many many distinct values uploaded to
the server.  Sparse histograms have the advantage of using less memory
client-side for sparse data, because they allocate buckets on demand rather than
preallocating.  (They do also give you precise numbers rather than bucketed
ones.)  However, server-side, we still need to load all buckets, across all
users, at once.  We can blacklist histograms that generate too many buckets, but
it's easier to just not generate too many buckets in the first place =)

Capping to 128 sounds like a good solution.

[jkrcal, 2017-04-13 08:17:54]

On 2017/04/13 08:10:50, Ilya Sherman wrote:
> On 2017/04/13 08:05:07, jkrcal wrote:
> > On 2017/04/13 07:46:29, Ilya Sherman wrote:
> > >
> >
>
https://codereview.chromium.org/2809783003/diff/40001/components/favicon/core...
> > > File components/favicon/core/large_icon_service.cc (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/2809783003/diff/40001/components/favicon/core...
> > > components/favicon/core/large_icon_service.cc:125:
> > DCHECK_GT(fallback_icon_size,
> > > 0);
> > > Hmm, if the API allows setting an arbitrary upper bound, then what
> guarantees
> > > that the histogram will not explode?
> > 
> > I switched off the commit bit, for the time being.
> > 
> > What do you mean by the histogram exploding? I guess I do not fully
understand
> > the implications of using UMA_HISTOGRAM_SPARSE_SLOWLY and the comments in
> > histogram_macros.h do not help me either to understand that.
> > 
> > I do not want to DCHECK because this way, I would (debug) crash for an
> > absolutely legal value.
> > I am totally willing to cap the values logged by, e.g.
> >  fallback_icon_size = std::min(fallback_icon_size, 128)
> > What is the advantage of logging SPARSE? Is it that I get always the precise
> > number and not just one of the buckets?
> > 
> > I would also be fine with the original
> > UMA_HISTOGRAM_COUNTS_100("Favicons.LargeIconService.FallbackSize",
> > fallback_icon_size);
> >  (+ the DCHECK_GT(fallback_icon_size, 0))
> 
> By the histogram exploding, I mean seeing many many distinct values uploaded
to
> the server.  Sparse histograms have the advantage of using less memory
> client-side for sparse data, because they allocate buckets on demand rather
than
> preallocating.  (They do also give you precise numbers rather than bucketed
> ones.)  However, server-side, we still need to load all buckets, across all
> users, at once.  We can blacklist histograms that generate too many buckets,
but
> it's easier to just not generate too many buckets in the first place =)
> 
> Capping to 128 sounds like a good solution.

Thanks for the explanation! Done.

[jkrcal, 2017-04-13 08:19:17]

The CQ bit was checked by jkrcal@chromium.org

[jkrcal, 2017-04-13 08:19:18]

The patchset sent to the CQ was uploaded after l-g-t-m from
pkotwicz@chromium.org, isherman@chromium.org
Link to the patchset: https://codereview.chromium.org/2809783003/#ps60001
(title: "Ilya's comment #3")

[commit-bot: I haz the power, 2017-04-13 08:19:46]

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-13 09:49:48]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1492071557813160,
"parent_rev": "89c72983c8a205ed7318ad2bd299b8d8aca77ea1", "commit_rev":
"5f0f424e645a9b8209244256f9a7d8bdb1b4e035"}

[commit-bot: I haz the power, 2017-04-13 09:50:43]

Description was changed from

==========
[LargeIconService] Report size of favicon for fallback style to UMA

This CL adds UMA metrics for sizes of favicons from which fallback style
was generated (because large enough icon is missing). This is important
for evaluating if changing the minimum size limit / using more advanced
method for upscaling would help.

BUG=709498
==========

to

==========
[LargeIconService] Report size of favicon for fallback style to UMA

This CL adds UMA metrics for sizes of favicons from which fallback style
was generated (because large enough icon is missing). This is important
for evaluating if changing the minimum size limit / using more advanced
method for upscaling would help.

BUG=709498

Review-Url: https://codereview.chromium.org/2809783003
Cr-Commit-Position: refs/heads/master@{#464363}
Committed:
https://chromium.googlesource.com/chromium/src/+/5f0f424e645a9b8209244256f9a7...
==========

[commit-bot: I haz the power, 2017-04-13 09:50:44]

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