Add Media.AudioRendererEvents histogram to measure how often OnRenderError() is called.

Add Media.AudioRendererEvents histogram to measure how often OnRenderError() is called.

BUG=234708
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=196442

[scherkus (not reviewing), 2013-04-24 20:27:41]

WDYT?

alternatively we could go the CHECK(false) path and look for crashes ... but
that's a bit heavy handed

https://codereview.chromium.org/14054019/diff/1/media/filters/audio_renderer_...
File media/filters/audio_renderer_impl.cc (right):

https://codereview.chromium.org/14054019/diff/1/media/filters/audio_renderer_...
media/filters/audio_renderer_impl.cc:635:
HISTOGRAM_BOOLEAN("Media.AudioRendererSinkErrors", true);
we can also record audio_parameters_ when hitting an error, but I'm inclined to
wait to see how often this gets hit (I long for way to record histograms for
parameters without having a histogram for each individual parameter!)

[scherkus (not reviewing), 2013-04-24 20:27:55]

+dalecurtis for real this time

[DaleCurtis, 2013-04-24 20:56:39]

I prefer just the stat for now.

https://codereview.chromium.org/14054019/diff/1/media/filters/audio_renderer_...
File media/filters/audio_renderer_impl.cc (right):

https://codereview.chromium.org/14054019/diff/1/media/filters/audio_renderer_...
media/filters/audio_renderer_impl.cc:288:
HISTOGRAM_BOOLEAN("Media.AudioRendererSinkErrors", false);
I believe you want UMA_*, otherwise this won't get uploaded?  Or is w/o UMA_ the
new public pattern?

I'm also not sure histograms work this way.  Won't this end up recording a false
and true value if an error occurs?  Is it smart enough to only record one value?

[jar (doing other things), 2013-04-24 21:51:07]

A few nits below. Mostly looks good.

https://codereview.chromium.org/14054019/diff/1/media/filters/audio_renderer_...
File media/filters/audio_renderer_impl.cc (right):

https://codereview.chromium.org/14054019/diff/1/media/filters/audio_renderer_...
media/filters/audio_renderer_impl.cc:288:
HISTOGRAM_BOOLEAN("Media.AudioRendererSinkErrors", false);
+1 for needing the prefix UMA_HISTOGRAM_BOOLEAN

The code, as written, will work, producing counts in two distinct buckets.  I
always hate the chance of (for example) misspelling the histogram in one place
or the other, so I'd suggest factoring out:

void histogram_renderer_status(bool error) {
  UMA_HISTOGRAM_BOOLEAN("Media.AudioRendererSinkErrors", error);
}

You can then call this from line 288 and 635.


On 2013/04/24 20:56:39, DaleCurtis wrote:
> I believe you want UMA_*, otherwise this won't get uploaded?  Or is w/o UMA_
the
> new public pattern?
> 
> I'm also not sure histograms work this way.  Won't this end up recording a
false
> and true value if an error occurs?  Is it smart enough to only record one
value?

https://codereview.chromium.org/14054019/diff/1/media/filters/audio_renderer_...
media/filters/audio_renderer_impl.cc:635:
HISTOGRAM_BOOLEAN("Media.AudioRendererSinkErrors", true);
Some of this stuff *might* be pulled from dremel logs, looking for correlation
between failure reports, and parameters seen in other histograms.  I agree that
first you should see how significant the issue is. ;-)

On 2013/04/24 20:27:41, scherkus wrote:
> we can also record audio_parameters_ when hitting an error, but I'm inclined
to
> wait to see how often this gets hit (I long for way to record histograms for
> parameters without having a histogram for each individual parameter!)

https://codereview.chromium.org/14054019/diff/1/tools/metrics/histograms/hist...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/14054019/diff/1/tools/metrics/histograms/hist...
tools/metrics/histograms/histograms.xml:519: False is recorded whenever
AudioRendererImpl successfully initializes an
You should add an enum to make this more readable:

0   Initialized
1   Terminated with Error

https://codereview.chromium.org/14054019/diff/1/tools/metrics/histograms/hist...
tools/metrics/histograms/histograms.xml:521: follows with the sink reporting
OnRenderError().
nit: "follows with" --> "is followed by the"

[scherkus (not reviewing), 2013-04-25 00:04:46]

[subject changed]

PTAL

I decided to rename this to AudioRendererEvents seeing as I have a few other
ideas for events we can be logging after this lands.

https://codereview.chromium.org/14054019/diff/1/media/filters/audio_renderer_...
File media/filters/audio_renderer_impl.cc (right):

https://codereview.chromium.org/14054019/diff/1/media/filters/audio_renderer_...
media/filters/audio_renderer_impl.cc:288:
HISTOGRAM_BOOLEAN("Media.AudioRendererSinkErrors", false);
On 2013/04/24 21:51:07, jar wrote:
> +1 for needing the prefix UMA_HISTOGRAM_BOOLEAN
> 
> The code, as written, will work, producing counts in two distinct buckets.  I
> always hate the chance of (for example) misspelling the histogram in one place
> or the other, so I'd suggest factoring out:
> 
> void histogram_renderer_status(bool error) {
>   UMA_HISTOGRAM_BOOLEAN("Media.AudioRendererSinkErrors", error);
> }
> 
> You can then call this from line 288 and 635.
> 
> 
> On 2013/04/24 20:56:39, DaleCurtis wrote:
> > I believe you want UMA_*, otherwise this won't get uploaded?  Or is w/o UMA_
> the
> > new public pattern?
> > 
> > I'm also not sure histograms work this way.  Won't this end up recording a
> false
> > and true value if an error occurs?  Is it smart enough to only record one
> value?
> 

Done.

https://codereview.chromium.org/14054019/diff/1/tools/metrics/histograms/hist...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/14054019/diff/1/tools/metrics/histograms/hist...
tools/metrics/histograms/histograms.xml:519: False is recorded whenever
AudioRendererImpl successfully initializes an
On 2013/04/24 21:51:07, jar wrote:
> You should add an enum to make this more readable:
> 
> 0   Initialized
> 1   Terminated with Error
> 

Done.

https://codereview.chromium.org/14054019/diff/1/tools/metrics/histograms/hist...
tools/metrics/histograms/histograms.xml:521: follows with the sink reporting
OnRenderError().
On 2013/04/24 21:51:07, jar wrote:
> nit: "follows with" --> "is followed by the"

Reworded a bit. Mind taking another scan?

[jar (doing other things), 2013-04-25 00:09:39]

lgtm

[DaleCurtis, 2013-04-25 17:33:06]

lgtm % nit(s).

https://codereview.chromium.org/14054019/diff/12001/media/filters/audio_rende...
File media/filters/audio_renderer_impl.cc (right):

https://codereview.chromium.org/14054019/diff/12001/media/filters/audio_rende...
media/filters/audio_renderer_impl.cc:37: void
HistogramRendererEvent(AudioRendererEvent event) {
Unnamed namespace in media?! :)

https://codereview.chromium.org/14054019/diff/12001/media/filters/audio_rende...
media/filters/audio_renderer_impl.cc:42: 
extra line.

[scherkus (not reviewing), 2013-04-25 18:28:49]

https://codereview.chromium.org/14054019/diff/12001/media/filters/audio_rende...
File media/filters/audio_renderer_impl.cc (right):

https://codereview.chromium.org/14054019/diff/12001/media/filters/audio_rende...
media/filters/audio_renderer_impl.cc:37: void
HistogramRendererEvent(AudioRendererEvent event) {
On 2013/04/25 17:33:06, DaleCurtis wrote:
> Unnamed namespace in media?! :)

The enum declaration made me do it! (prevents possible collisions ... especially
with a value like "INITIALIZED")

https://codereview.chromium.org/14054019/diff/12001/media/filters/audio_rende...
media/filters/audio_renderer_impl.cc:42: 
On 2013/04/25 17:33:06, DaleCurtis wrote:
> extra line.

Done.

[scherkus (not reviewing), 2013-04-25 18:29:37]

Committed patchset #4 manually as r196442 (presubmit successful).