Add UMA to hotword trigger extension.

Add UMA to hotword trigger extension.

BUG=397019
Committed: https://crrev.com/72720a1af9ed5b8e1a6a88f4f9aee28c11408ced
Cr-Commit-Position: refs/heads/master@{#301003}

[Anand Mistry (off Chromium), 2014-10-20 23:47:31]

amistry@chromium.org changed reviewers:
+ benwells@chromium.org, dbeam@chromium.org, mpearson@chromium.org,
rlp@chromium.org

[Dan Beam, 2014-10-21 01:57:29]

rs lgtm

https://codereview.chromium.org/667873002/diff/20001/chrome/browser/resources...
File chrome/browser/resources/hotword/metrics.js (right):

https://codereview.chromium.org/667873002/diff/20001/chrome/browser/resources...
chrome/browser/resources/hotword/metrics.js:14: function recordEnum(name, value,
max_value) {
maxValue

[benwells, 2014-10-21 05:02:31]

whitelist lgtm

[Anand Mistry (off Chromium), 2014-10-21 05:26:41]

https://codereview.chromium.org/667873002/diff/20001/chrome/browser/resources...
File chrome/browser/resources/hotword/metrics.js (right):

https://codereview.chromium.org/667873002/diff/20001/chrome/browser/resources...
chrome/browser/resources/hotword/metrics.js:14: function recordEnum(name, value,
max_value) {
On 2014/10/21 01:57:29, Dan Beam wrote:
> maxValue

Done.

[Mark P, 2014-10-21 17:33:05]

I also see a bunch of recordEnum calls in your code.  Are these recording to a
UMA histogram?  If so, why aren't you updating histograms.xml?

--mark

https://codereview.chromium.org/667873002/diff/40001/tools/metrics/actions/ac...
File tools/metrics/actions/actions.xml (right):

https://codereview.chromium.org/667873002/diff/40001/tools/metrics/actions/ac...
tools/metrics/actions/actions.xml:3138: Counts the number of times the hotword
was triggered by the user saying 'Ok
Actions are not count.  Actions are events with timestamps.  If you only want a
count, use a histogram instead.  If you want an action, try a description like:
User triggered the hotword by saying 'Ok Google'.

[rpetterson, 2014-10-21 17:47:56]

lgtm

This is exciting! Looking forward to starting to see these come in!

[Anand Mistry (off Chromium), 2014-10-21 23:19:26]

Updated histograms.xml as well.

https://codereview.chromium.org/667873002/diff/40001/tools/metrics/actions/ac...
File tools/metrics/actions/actions.xml (right):

https://codereview.chromium.org/667873002/diff/40001/tools/metrics/actions/ac...
tools/metrics/actions/actions.xml:3138: Counts the number of times the hotword
was triggered by the user saying 'Ok
On 2014/10/21 17:33:05, Mark P wrote:
> Actions are not count.  Actions are events with timestamps.  If you only want
a
> count, use a histogram instead.  If you want an action, try a description
like:
> User triggered the hotword by saying 'Ok Google'.

Done.

[Mark P, 2014-10-22 21:20:17]

I'm not a fluent reader of javascript so I only looked at tools/metrics/...

--mark

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

https://codereview.chromium.org/667873002/diff/60001/tools/metrics/histograms...
tools/metrics/histograms/histograms.xml:10012: +    Errors when attempting to
open a MediaStream for the microphone.
Is it possible the same stream causes multiple errors (e.g., invalid origin
certificate, permission dismissed)?

https://codereview.chromium.org/667873002/diff/60001/tools/metrics/histograms...
tools/metrics/histograms/histograms.xml:10012: +    Errors when attempting to
open a MediaStream for the microphone.
Do you have a histogram or something that allows normalizing this one to tell
how often errors are a whole happen?

https://codereview.chromium.org/667873002/diff/60001/tools/metrics/histograms...
tools/metrics/histograms/histograms.xml:10024: +    plugin.
For my information, if the NaCl plugin failed to load (as recorded in the below
histogram), nothing will end up being recorded here, right?

[Anand Mistry (off Chromium), 2014-10-22 22:14:15]

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

https://codereview.chromium.org/667873002/diff/60001/tools/metrics/histograms...
tools/metrics/histograms/histograms.xml:10012: +    Errors when attempting to
open a MediaStream for the microphone.
On 2014/10/22 21:20:17, Mark P wrote:
> Do you have a histogram or something that allows normalizing this one to tell
> how often errors are a whole happen?

Are you suggesting to have a success/fail histogram for this? I don't currently
have one, but it's trivial for me to add. Would adding a "success" enum value to
this (and other) histogram be appropriate instead?

https://codereview.chromium.org/667873002/diff/60001/tools/metrics/histograms...
tools/metrics/histograms/histograms.xml:10012: +    Errors when attempting to
open a MediaStream for the microphone.
On 2014/10/22 21:20:17, Mark P wrote:
> Is it possible the same stream causes multiple errors (e.g., invalid origin
> certificate, permission dismissed)?

No. This error occurs on the attempt to open the stream, which only returns one
error. If it fails, we don't have a valid stream to produce more errors.

https://codereview.chromium.org/667873002/diff/60001/tools/metrics/histograms...
tools/metrics/histograms/histograms.xml:10024: +    plugin.
On 2014/10/22 21:20:17, Mark P wrote:
> For my information, if the NaCl plugin failed to load (as recorded in the
below
> histogram), nothing will end up being recorded here, right?

Correct.

[Mark P, 2014-10-22 22:32:17]

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

https://codereview.chromium.org/667873002/diff/60001/tools/metrics/histograms...
tools/metrics/histograms/histograms.xml:10012: +    Errors when attempting to
open a MediaStream for the microphone.
On 2014/10/22 22:14:15, Anand Mistry wrote:
> On 2014/10/22 21:20:17, Mark P wrote:
> > Do you have a histogram or something that allows normalizing this one to
tell
> > how often errors are a whole happen?
> 
> Are you suggesting to have a success/fail histogram for this? I don't
currently
> have one, but it's trivial for me to add. Would adding a "success" enum value
to
> this (and other) histogram be appropriate instead?

Either method (success/fail histogram or adding success enum) is fine.  Or
neither.  I was mostly pointing out that many people find having the denominator
useful.  If you think you might too, record it in whichever way you want. :-)

https://codereview.chromium.org/667873002/diff/60001/tools/metrics/histograms...
tools/metrics/histograms/histograms.xml:10012: +    Errors when attempting to
open a MediaStream for the microphone.
On 2014/10/22 22:14:14, Anand Mistry wrote:
> On 2014/10/22 21:20:17, Mark P wrote:
> > Is it possible the same stream causes multiple errors (e.g., invalid origin
> > certificate, permission dismissed)?
> 
> No. This error occurs on the attempt to open the stream, which only returns
one
> error. If it fails, we don't have a valid stream to produce more errors.

optional suggestion: Say explicitly something like
// Errors are detected serially, meaning that at most one error will be emitted
for an attempt to open a stream.

[Anand Mistry (off Chromium), 2014-10-23 00:20:37]

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

https://codereview.chromium.org/667873002/diff/60001/tools/metrics/histograms...
tools/metrics/histograms/histograms.xml:10012: +    Errors when attempting to
open a MediaStream for the microphone.
On 2014/10/22 22:32:16, Mark P wrote:
> On 2014/10/22 22:14:15, Anand Mistry wrote:
> > On 2014/10/22 21:20:17, Mark P wrote:
> > > Do you have a histogram or something that allows normalizing this one to
> tell
> > > how often errors are a whole happen?
> > 
> > Are you suggesting to have a success/fail histogram for this? I don't
> currently
> > have one, but it's trivial for me to add. Would adding a "success" enum
value
> to
> > this (and other) histogram be appropriate instead?
> 
> Either method (success/fail histogram or adding success enum) is fine.  Or
> neither.  I was mostly pointing out that many people find having the
denominator
> useful.  If you think you might too, record it in whichever way you want. :-)

Done. I think it's useful.

https://codereview.chromium.org/667873002/diff/60001/tools/metrics/histograms...
tools/metrics/histograms/histograms.xml:10012: +    Errors when attempting to
open a MediaStream for the microphone.
On 2014/10/22 22:32:17, Mark P wrote:
> On 2014/10/22 22:14:14, Anand Mistry wrote:
> > On 2014/10/22 21:20:17, Mark P wrote:
> > > Is it possible the same stream causes multiple errors (e.g., invalid
origin
> > > certificate, permission dismissed)?
> > 
> > No. This error occurs on the attempt to open the stream, which only returns
> one
> > error. If it fails, we don't have a valid stream to produce more errors.
> 
> optional suggestion: Say explicitly something like
> // Errors are detected serially, meaning that at most one error will be
emitted
> for an attempt to open a stream.

Done, although not so wordy.

[Mark P, 2014-10-23 18:54:00]

lgtm, though you may want a primary reviewer to double-check that your
newly-added emits of SUCCESS happen in the correct places and don't omit any.

--mark

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

https://codereview.chromium.org/667873002/diff/60001/tools/metrics/histograms...
tools/metrics/histograms/histograms.xml:10012: +    Errors when attempting to
open a MediaStream for the microphone.
On 2014/10/23 00:20:37, Anand Mistry wrote:
> On 2014/10/22 22:32:17, Mark P wrote:
> > On 2014/10/22 22:14:14, Anand Mistry wrote:
> > > On 2014/10/22 21:20:17, Mark P wrote:
> > > > Is it possible the same stream causes multiple errors (e.g., invalid
> origin
> > > > certificate, permission dismissed)?
> > > 
> > > No. This error occurs on the attempt to open the stream, which only
returns
> > one
> > > error. If it fails, we don't have a valid stream to produce more errors.
> > 
> > optional suggestion: Say explicitly something like
> > // Errors are detected serially, meaning that at most one error will be
> emitted
> > for an attempt to open a stream.
> 
> Done, although not so wordy.

Yeah, I tend to be wordy. :-)

[rpetterson, 2014-10-23 19:03:36]

https://codereview.chromium.org/667873002/diff/80001/chrome/browser/resources...
File chrome/browser/resources/hotword/nacl_manager.js (right):

https://codereview.chromium.org/667873002/diff/80001/chrome/browser/resources...
chrome/browser/resources/hotword/nacl_manager.js:342: metricValue,
Perhaps success should simply be one of the cases here?

https://codereview.chromium.org/667873002/diff/80001/chrome/browser/resources...
File chrome/browser/resources/hotword/state_manager.js (right):

https://codereview.chromium.org/667873002/diff/80001/chrome/browser/resources...
chrome/browser/resources/hotword/state_manager.js:230:
hotword.metrics.recordEnum(
I don't fully understand when function(stream) vs function(error) gets called.
Is it possible both could be called?

[Anand Mistry (off Chromium), 2014-10-23 21:38:16]

https://codereview.chromium.org/667873002/diff/80001/chrome/browser/resources...
File chrome/browser/resources/hotword/nacl_manager.js (right):

https://codereview.chromium.org/667873002/diff/80001/chrome/browser/resources...
chrome/browser/resources/hotword/nacl_manager.js:342: metricValue,
On 2014/10/23 19:03:36, rpetterson wrote:
> Perhaps success should simply be one of the cases here?

I don't understand. I don't think we want to track successes for this metric
since I don't think it would be a useful denominator.

https://codereview.chromium.org/667873002/diff/80001/chrome/browser/resources...
File chrome/browser/resources/hotword/state_manager.js (right):

https://codereview.chromium.org/667873002/diff/80001/chrome/browser/resources...
chrome/browser/resources/hotword/state_manager.js:230:
hotword.metrics.recordEnum(
On 2014/10/23 19:03:36, rpetterson wrote:
> I don't fully understand when function(stream) vs function(error) gets called.
> Is it possible both could be called?

It's one or the other, never both.
function(stream) is called when the requested stream (audio in our case) is
successfully opened.
function(error) is called on any fatal error condition, such as resource busy,
or no device available.

[Anand Mistry (off Chromium), 2014-10-23 22:06:54]

The CQ bit was checked by amistry@chromium.org

[commit-bot: I haz the power, 2014-10-23 22:09:10]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/667873002/100001

[Anand Mistry (off Chromium), 2014-10-23 22:22:00]

The CQ bit was unchecked by amistry@chromium.org

[Anand Mistry (off Chromium), 2014-10-23 22:36:59]

On 2014/10/23 21:38:16, Anand Mistry wrote:
>
https://codereview.chromium.org/667873002/diff/80001/chrome/browser/resources...
> File chrome/browser/resources/hotword/nacl_manager.js (right):
> 
>
https://codereview.chromium.org/667873002/diff/80001/chrome/browser/resources...
> chrome/browser/resources/hotword/nacl_manager.js:342: metricValue,
> On 2014/10/23 19:03:36, rpetterson wrote:
> > Perhaps success should simply be one of the cases here?
> 
> I don't understand. I don't think we want to track successes for this metric
> since I don't think it would be a useful denominator.

I've removed the extra, incorrect call to logPluginLoadResult_ here.

[rpetterson, 2014-10-23 22:52:16]

On 2014/10/23 22:36:59, Anand Mistry wrote:
> On 2014/10/23 21:38:16, Anand Mistry wrote:
> >
>
https://codereview.chromium.org/667873002/diff/80001/chrome/browser/resources...
> > File chrome/browser/resources/hotword/nacl_manager.js (right):
> > 
> >
>
https://codereview.chromium.org/667873002/diff/80001/chrome/browser/resources...
> > chrome/browser/resources/hotword/nacl_manager.js:342: metricValue,
> > On 2014/10/23 19:03:36, rpetterson wrote:
> > > Perhaps success should simply be one of the cases here?
> > 
> > I don't understand. I don't think we want to track successes for this metric
> > since I don't think it would be a useful denominator.
> 
> I've removed the extra, incorrect call to logPluginLoadResult_ here.

cool. lgtm again.

[Anand Mistry (off Chromium), 2014-10-23 23:06:22]

The CQ bit was checked by amistry@chromium.org

[commit-bot: I haz the power, 2014-10-23 23:09:51]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/667873002/120001

[commit-bot: I haz the power, 2014-10-24 00:28:41]

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2014-10-24 00:29:16]

Patchset 7 (id:??) landed as
https://crrev.com/72720a1af9ed5b8e1a6a88f4f9aee28c11408ced
Cr-Commit-Position: refs/heads/master@{#301003}