Add detailed histograms for navigator.share.

Add detailed histograms for navigator.share.

WebShare.ApiCount is needed (in addition to the existing use counter) to
get the total number of calls to the API, not de-duplicated per page
view.

BUG=636288
Committed: https://crrev.com/c39517b04b69a9eb356f913b6b5da3a98f328fcc
Cr-Commit-Position: refs/heads/master@{#418760}

[Matt Giuca, 2016-09-08 07:52:38]

mgiuca@chromium.org changed reviewers:
+ asvitkine@chromium.org, dtrainor@chromium.org

[Matt Giuca, 2016-09-08 07:52:38]

Hi.

Note that I am adding the WebShare.ApiCount enum (with just a single value)
because a) I already have a use counter, but that de-duplicates multiple calls
within a single page view. I want the total count. And b) I need a way to count
many calls to just 1 thing; a single-value enum seems appropriate (this way, if
we ever add more WebShare related methods, we can just add new buckets to the
enum, instead of adding new histograms).

Note that in doing so, I ran into https://crbug.com/645032. Not sure if we
should fix that or just document. For now, I worked around it.

[Matt Giuca, 2016-09-08 07:53:34]

Description was changed from

==========
Add detailed histograms for navigator.share.

DO NOT SUBMIT: This currently crashes and I don't know why.

BUG=636288
==========

to

==========
Add detailed histograms for navigator.share.

WebShare.ApiCount is needed (in addition to the existing use counter) to get the
total number of calls to the API, not de-duplicated per page view.

BUG=636288
==========

[Matt Giuca, 2016-09-08 07:53:46]

Description was changed from

==========
Add detailed histograms for navigator.share.

WebShare.ApiCount is needed (in addition to the existing use counter) to get the
total number of calls to the API, not de-duplicated per page view.

BUG=636288
==========

to

==========
Add detailed histograms for navigator.share.

WebShare.ApiCount is needed (in addition to the existing use counter) to
get the total number of calls to the API, not de-duplicated per page
view.

BUG=636288
==========

[Alexei Svitkine (slow), 2016-09-08 17:08:01]

lgtm % suggestion

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

https://codereview.chromium.org/2228403002/diff/20001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:68221: +<histogram
name="WebShare.ShareOutcome" enum="WebShareOutcome">
These metrics are Android specific, right?

How about prefixing them with the Android. prefix? e.g.
Android.WebShare.ShareOutcome.

[David Trainor- moved to gerrit, 2016-09-08 20:39:59]

lgtm % Alexei's suggestion!

[Matt Giuca, 2016-09-09 00:31:50]

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

https://codereview.chromium.org/2228403002/diff/20001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:68221: +<histogram
name="WebShare.ShareOutcome" enum="WebShareOutcome">
On 2016/09/08 17:08:01, Alexei Svitkine (very slow) wrote:
> These metrics are Android specific, right?
> 
> How about prefixing them with the Android. prefix? e.g.
> Android.WebShare.ShareOutcome.

They are only on Android right now, but they won't always be. While we'd still
likely have separate platform-specific code logging to this metric on each
platform, there's no point having a separate histogram on each. If we want to
break it down by platform, we can do that in the UMA dashboard.

[Mark P, 2016-09-09 18:34:30]

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

[Mark P, 2016-09-09 18:34:31]

drive-by comments

https://codereview.chromium.org/2228403002/diff/20001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/webshare/ShareServiceImpl.java
(right):

https://codereview.chromium.org/2228403002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/webshare/ShareServiceImpl.java:49:
RecordHistogram.recordEnumeratedHistogram("WebShare.ApiCount",
WEBSHARE_METHOD_SHARE,
For the record, an alternate approach that doesn't require comparing counts
across histograms is:
a boolean histogram like WebShare.APICalledCorrectly reflecting true if the
parameters are suitable to be passed to the ShareHelper.  I.e., contains true if
mActivity != null.
a histogram with two values, one emitted to when share helper is called and one
emitted to when it returns (successfully).

The current approach in this changelist can be slightly misleading in cases when
the callback doesn't get called (dunno if / how often that can happen in your
situation).  To interpret the success / failure rate, you'd naturally want to
look at WebShare.ShareOutcome and see the percent breakdown.  But this can be
wrong if some callbacks don't return; a user will need to consult
WebShare.ApiCount to see how often this happens.  The proposed histograms above
don't have this problem.

[Matt Giuca, 2016-09-12 05:41:00]

https://codereview.chromium.org/2228403002/diff/20001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/webshare/ShareServiceImpl.java
(right):

https://codereview.chromium.org/2228403002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/webshare/ShareServiceImpl.java:49:
RecordHistogram.recordEnumeratedHistogram("WebShare.ApiCount",
WEBSHARE_METHOD_SHARE,
On 2016/09/09 18:34:30, Mark P wrote:
> For the record, an alternate approach that doesn't require comparing counts
> across histograms is:
> a boolean histogram like WebShare.APICalledCorrectly reflecting true if the
> parameters are suitable to be passed to the ShareHelper.  I.e., contains true
if
> mActivity != null.

I'm not sure about CalledCorrectly... this API doesn't just fail if mActivity is
null (which probably should never happen; maybe it should in fact just be a
DCHECK). Principally, it fails if the user cancels the dialog. So that's why I
called it ShareOutcome and I want to log all the different failure modes. (Note:
I need to rebase over a CL that adds the user cancelled failure mode.)

> a histogram with two values, one emitted to when share helper is called and
one
> emitted to when it returns (successfully).

Is this an alternative suggestion to APICalledCorrectly, or you suggest having
both? I don't really want a histogram whose total is meaningless. I think it's
valuable being able to look at PDF/CDF and percentages in the dashboard.

I'm not sure whether comparing across histograms is actually meaningless, or
just discouraged because it makes reading more complicated? Either way, I don't
plan to really rely on it. With the two metrics I am proposing:
- ApiCount would just be used to get an idea of the total count of calls the API
(to establish the basic usage metric, alongside the UseCounter).
- ShareOutcome would be used to get an idea of the percentage of shares that
succeed, and what the other failure modes are (by comparing each bucket to the
total count).

There's a slight problem right now that there's a significant delay before
cancellations show up, and there's a high probability that the user will close
the page before it's recorded. Therefore, judging the success ratio in the short
term may require comparing WebShare.ShareOutcome.SUCCESS with
WebShare.ApiCount.SHARE. Are you saying that isn't accurate? Unless that's
statistically invalid, I'd rather compare across metrics than have one histogram
whose total is meaningless.

> 
> The current approach in this changelist can be slightly misleading in cases
when
> the callback doesn't get called (dunno if / how often that can happen in your
> situation).  To interpret the success / failure rate, you'd naturally want to
> look at WebShare.ShareOutcome and see the percent breakdown.  But this can be
> wrong if some callbacks don't return; a user will need to consult
> WebShare.ApiCount to see how often this happens.  The proposed histograms
above
> don't have this problem.
>

[Mark P, 2016-09-13 17:53:38]

https://codereview.chromium.org/2228403002/diff/20001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/webshare/ShareServiceImpl.java
(right):

https://codereview.chromium.org/2228403002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/webshare/ShareServiceImpl.java:49:
RecordHistogram.recordEnumeratedHistogram("WebShare.ApiCount",
WEBSHARE_METHOD_SHARE,
On 2016/09/12 05:41:00, Matt Giuca wrote:
> On 2016/09/09 18:34:30, Mark P wrote:
> > For the record, an alternate approach that doesn't require comparing counts
> > across histograms is:
> > a boolean histogram like WebShare.APICalledCorrectly reflecting true if the
> > parameters are suitable to be passed to the ShareHelper.  I.e., contains
true
> if
> > mActivity != null.
> 
> I'm not sure about CalledCorrectly... this API doesn't just fail if mActivity
is
> null (which probably should never happen; maybe it should in fact just be a
> DCHECK). Principally, it fails if the user cancels the dialog. So that's why I
> called it ShareOutcome and I want to log all the different failure modes.
(Note:
> I need to rebase over a CL that adds the user cancelled failure mode.)
> 
> > a histogram with two values, one emitted to when share helper is called and
> one
> > emitted to when it returns (successfully).
> 
> Is this an alternative suggestion to APICalledCorrectly, or you suggest having
> both?  I don't really want a histogram whose total is meaningless. I think
it's
> valuable being able to look at PDF/CDF and percentages in the dashboard.

I was suggesting having both.

> I'm not sure whether comparing across histograms is actually meaningless, or
> just discouraged because it makes reading more complicated?

The latter: more complicated and error-prone.

> Either way, I don't
> plan to really rely on it. With the two metrics I am proposing:
> - ApiCount would just be used to get an idea of the total count of calls the
API
> (to establish the basic usage metric, alongside the UseCounter).
> - ShareOutcome would be used to get an idea of the percentage of shares that
> succeed, and what the other failure modes are (by comparing each bucket to the
> total count).
> 
> There's a slight problem right now that there's a significant delay before
> cancellations show up, and there's a high probability that the user will close
> the page before it's recorded. Therefore, judging the success ratio in the
short
> term may require comparing WebShare.ShareOutcome.SUCCESS with
> WebShare.ApiCount.SHARE. Are you saying that isn't accurate? Unless that's
> statistically invalid, I'd rather compare across metrics than have one
histogram
> whose total is meaningless.

You're correct that comparing across those two histograms is accurate.
But if that's what you need to do to make sense of the ApiCount histogram,
the histograms description should clearly say something like "Do NOT look at the
success or failure rate of this histogram as a percent of the total.  You must
compare
these numbers to the ApiCount histogram."

This makes it obvious what someone looking at this histogram should do.

I see that you complain that my proposed second histogram has a total count over
all buckets that doesn't make sense ("is meaningless").  But the total count
over the
ShareOutcome histogram also doesn't make sense because it doesn't represent
the true total.

> 
> > 
> > The current approach in this changelist can be slightly misleading in cases
> when
> > the callback doesn't get called (dunno if / how often that can happen in
your
> > situation).  To interpret the success / failure rate, you'd naturally want
to
> > look at WebShare.ShareOutcome and see the percent breakdown.  But this can
be
> > wrong if some callbacks don't return; a user will need to consult
> > WebShare.ApiCount to see how often this happens.  The proposed histograms
> above
> > don't have this problem.
> > 
>

[Matt Giuca, 2016-09-14 00:55:36]

https://codereview.chromium.org/2228403002/diff/20001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/webshare/ShareServiceImpl.java
(right):

https://codereview.chromium.org/2228403002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/webshare/ShareServiceImpl.java:49:
RecordHistogram.recordEnumeratedHistogram("WebShare.ApiCount",
WEBSHARE_METHOD_SHARE,
On 2016/09/13 17:53:38, Mark P wrote:
> You're correct that comparing across those two histograms is accurate.
> But if that's what you need to do to make sense of the ApiCount histogram,
> the histograms description should clearly say something like "Do NOT look at
the
> success or failure rate of this histogram as a percent of the total.  You must
> compare
> these numbers to the ApiCount histogram."
> 
> This makes it obvious what someone looking at this histogram should do.

I had some wording to that effect but it wasn't quite clear. Updated with
stronger wording.

> I see that you complain that my proposed second histogram has a total count
over
> all buckets that doesn't make sense ("is meaningless").  But the total count
> over the
> ShareOutcome histogram also doesn't make sense because it doesn't represent
> the true total.

True, but I think this way is more meaningful (at least the total represents
"total number of shares that completed one way or another", with a caveat that
this total is missing any shares that were unresolved when the page closed.
Under your suggestion, the total represents "total number of shares started +
total number of shares completed". The only advantage as I see it is that you
can compare the success to the total from the buckets of a single histogram, but
IMO that does not justify doing approx. two counts to one histogram for each
share.

[Mark P, 2016-09-14 05:19:28]

lgtm with comments below

--mark

https://codereview.chromium.org/2228403002/diff/20001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/webshare/ShareServiceImpl.java
(right):

https://codereview.chromium.org/2228403002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/webshare/ShareServiceImpl.java:49:
RecordHistogram.recordEnumeratedHistogram("WebShare.ApiCount",
WEBSHARE_METHOD_SHARE,
On 2016/09/14 00:55:36, Matt Giuca wrote:
> On 2016/09/13 17:53:38, Mark P wrote:
> > You're correct that comparing across those two histograms is accurate.
> > But if that's what you need to do to make sense of the ApiCount histogram,
> > the histograms description should clearly say something like "Do NOT look at
> the
> > success or failure rate of this histogram as a percent of the total.  You
must
> > compare
> > these numbers to the ApiCount histogram."
> > 
> > This makes it obvious what someone looking at this histogram should do.
> 
> I had some wording to that effect but it wasn't quite clear. Updated with
> stronger wording.
> 
> > I see that you complain that my proposed second histogram has a total count
> over
> > all buckets that doesn't make sense ("is meaningless").  But the total count
> > over the
> > ShareOutcome histogram also doesn't make sense because it doesn't represent
> > the true total.
> 
> True, but I think this way is more meaningful (at least the total represents
> "total number of shares that completed one way or another", with a caveat that
> this total is missing any shares that were unresolved when the page closed.
> Under your suggestion, the total represents "total number of shares started +
> total number of shares completed". The only advantage as I see it is that you
> can compare the success to the total from the buckets of a single histogram,
but
> IMO that does not justify doing approx. two counts to one histogram for each
> share.

Okay.  Thanks for the clearer wording.

https://codereview.chromium.org/2228403002/diff/70001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/webshare/ShareServiceImpl.java
(right):

https://codereview.chromium.org/2228403002/diff/70001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/webshare/ShareServiceImpl.java:27:
// Keep in sync with WebShareMethod enum in histograms.xml.
"Keep in sync" is not a strong enough statement.  You cannot change these values
(even if you change histograms.xml simultaneously) (except the _COUNT value)
because they are written to logs.

ditto below

[Matt Giuca, 2016-09-14 05:45:24]

https://codereview.chromium.org/2228403002/diff/70001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/webshare/ShareServiceImpl.java
(right):

https://codereview.chromium.org/2228403002/diff/70001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/webshare/ShareServiceImpl.java:27:
// Keep in sync with WebShareMethod enum in histograms.xml.
On 2016/09/14 05:19:28, Mark P wrote:
> "Keep in sync" is not a strong enough statement.  You cannot change these
values
> (even if you change histograms.xml simultaneously) (except the _COUNT value)
> because they are written to logs.
> 
> ditto below

I think I copied this from
chrome/android/java/src/org/chromium/chrome/browser/media/remote/RecordCastAction.java

I don't really know what you're asking for. "Keep in sync" neither implies that
you can or cannot change these values; it just says that it should be the same
as in histograms.xml (i.e., if new entries are added there, they should be added
here). What language would you prefer: "This should be the same as ..."?

(It seems redundant to explicitly state here that the values cannot be changed;
that is implied for any histograms and whether you can change the histograms is
irrelevant in this Java client.)

[Mark P, 2016-09-14 19:39:11]

https://codereview.chromium.org/2228403002/diff/70001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/webshare/ShareServiceImpl.java
(right):

https://codereview.chromium.org/2228403002/diff/70001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/webshare/ShareServiceImpl.java:27:
// Keep in sync with WebShareMethod enum in histograms.xml.
On 2016/09/14 05:45:24, Matt Giuca wrote:
> On 2016/09/14 05:19:28, Mark P wrote:
> > "Keep in sync" is not a strong enough statement.  You cannot change these
> values
> > (even if you change histograms.xml simultaneously) (except the _COUNT value)
> > because they are written to logs.
> > 
> > ditto below
> 
> I think I copied this from
>
chrome/android/java/src/org/chromium/chrome/browser/media/remote/RecordCastAction.java
> 
> I don't really know what you're asking for. "Keep in sync" neither implies
that
> you can or cannot change these values; it just says that it should be the same
> as in histograms.xml (i.e., if new entries are added there, they should be
added
> here). What language would you prefer: "This should be the same as ..."?
> 
> (It seems redundant to explicitly state here that the values cannot be
changed;
> that is implied for any histograms and whether you can change the histograms
is
> irrelevant in this Java client.)

I usually prefer something like
// These numbers are written to logs. Don't reuse or renumber entries (except
for the final entry _COUNT).

[Matt Giuca, 2016-09-15 01:33:59]

https://codereview.chromium.org/2228403002/diff/70001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/webshare/ShareServiceImpl.java
(right):

https://codereview.chromium.org/2228403002/diff/70001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/webshare/ShareServiceImpl.java:27:
// Keep in sync with WebShareMethod enum in histograms.xml.
On 2016/09/14 19:39:11, Mark P wrote:
> On 2016/09/14 05:45:24, Matt Giuca wrote:
> > On 2016/09/14 05:19:28, Mark P wrote:
> > > "Keep in sync" is not a strong enough statement.  You cannot change these
> > values
> > > (even if you change histograms.xml simultaneously) (except the _COUNT
value)
> > > because they are written to logs.
> > > 
> > > ditto below
> > 
> > I think I copied this from
> >
>
chrome/android/java/src/org/chromium/chrome/browser/media/remote/RecordCastAction.java
> > 
> > I don't really know what you're asking for. "Keep in sync" neither implies
> that
> > you can or cannot change these values; it just says that it should be the
same
> > as in histograms.xml (i.e., if new entries are added there, they should be
> added
> > here). What language would you prefer: "This should be the same as ..."?
> > 
> > (It seems redundant to explicitly state here that the values cannot be
> changed;
> > that is implied for any histograms and whether you can change the histograms
> is
> > irrelevant in this Java client.)
> 
> I usually prefer something like
> // These numbers are written to logs. Don't reuse or renumber entries (except
> for the final entry _COUNT).

OK. I added that text, but also kept the bit about keeping in sync with
histograms.xml (because I think that's the most important part).

[Matt Giuca, 2016-09-15 01:34:28]

The CQ bit was checked by mgiuca@chromium.org

[Matt Giuca, 2016-09-15 01:34:29]

The patchset sent to the CQ was uploaded after l-g-t-m from
asvitkine@chromium.org, dtrainor@chromium.org, mpearson@chromium.org
Link to the patchset: https://codereview.chromium.org/2228403002/#ps90001
(title: "Update comment for enum constants.")

[commit-bot: I haz the power, 2016-09-15 01:34:48]

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-15 02:25:56]

Committed patchset #6 (id:90001)

[commit-bot: I haz the power, 2016-09-15 02:27:58]

Description was changed from

==========
Add detailed histograms for navigator.share.

WebShare.ApiCount is needed (in addition to the existing use counter) to
get the total number of calls to the API, not de-duplicated per page
view.

BUG=636288
==========

to

==========
Add detailed histograms for navigator.share.

WebShare.ApiCount is needed (in addition to the existing use counter) to
get the total number of calls to the API, not de-duplicated per page
view.

BUG=636288

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

[commit-bot: I haz the power, 2016-09-15 02:27:59]

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