[Media Router] Add UMA histograms tracking component extension version/wakeups

[Media Router] Add UMA histograms tracking component extension version & wakeup outcome.

This patch adds two histograms:

- MediaRouter.Provider.Version compares the component version with the browser version and records the difference, the first time that the component wakes up.

- MediaRouter.Provider.Wakeup records the overall success/failure outcome for waking the event page.

Note that Mojo only provides a boolean to indicate a failure to establish the message pipe.  If we can get better errors from Mojo (or the extensions system) we can add more error categories.

BUG=588230
Committed: https://crrev.com/0dc8e6b8fb72e8d204c76ffc43be6516f114c91b
Cr-Commit-Position: refs/heads/master@{#382735}

[mark a. foltz, 2016-03-12 00:01:01]

Description was changed from

==========
[Media Router] Add UMA histogram tracking component extension status.

THis is a draft patch: DO_NOT_COMMIT

BUG=588230
==========

to

==========
[Media Router] Add UMA histogram tracking component extension status.

Note to self:  Not happy with current design; would prefer not to have MRMI
require an ExtensionPrefs* in its ctor.

DO_NOT_COMMIT

BUG=588230
==========

[mark a. foltz, 2016-03-16 21:49:32]

Description was changed from

==========
[Media Router] Add UMA histogram tracking component extension status.

Note to self:  Not happy with current design; would prefer not to have MRMI
require an ExtensionPrefs* in its ctor.

DO_NOT_COMMIT

BUG=588230
==========

to

==========
[Media Router] Add UMA histogram tracking component extension version & wakeup
outcome.

This patch adds two histograms:

- MediaRouter.Provider.Version compares the component version with the browser
version and records the difference, the first time that the component wakes up.

- MediaRouter.Provider.Wakeup records the overall success/failure outcome for
waking the event page.

Note that Mojo only provides a boolean to indicate a failure to establish the
message pipe.  If we can get better errors from Mojo (or the extensions system)
we can add more error categories.

BUG=588230
==========

[mark a. foltz, 2016-03-16 21:54:22]

mfoltz@chromium.org changed reviewers:
+ benwells@chromium.org, isherman@chromium.org, wez@chromium.org

[mark a. foltz, 2016-03-16 21:54:23]

wez: PTAL at everything
isherman: PTAL at histograms.xml
benwells: PTAL c/b/extensions/chrome_mojo_service_registration.cc

Thanks!

[mark a. foltz, 2016-03-16 21:55:02]

Description was changed from

==========
[Media Router] Add UMA histogram tracking component extension version & wakeup
outcome.

This patch adds two histograms:

- MediaRouter.Provider.Version compares the component version with the browser
version and records the difference, the first time that the component wakes up.

- MediaRouter.Provider.Wakeup records the overall success/failure outcome for
waking the event page.

Note that Mojo only provides a boolean to indicate a failure to establish the
message pipe.  If we can get better errors from Mojo (or the extensions system)
we can add more error categories.

BUG=588230
==========

to

==========
[Media Router] Add UMA histograms tracking component extension version & wakeup
outcome.

This patch adds two histograms:

- MediaRouter.Provider.Version compares the component version with the browser
version and records the difference, the first time that the component wakes up.

- MediaRouter.Provider.Wakeup records the overall success/failure outcome for
waking the event page.

Note that Mojo only provides a boolean to indicate a failure to establish the
message pipe.  If we can get better errors from Mojo (or the extensions system)
we can add more error categories.

BUG=588230
==========

[Ilya Sherman, 2016-03-16 22:23:06]

https://codereview.chromium.org/1765143002/diff/100001/chrome/browser/media/r...
File chrome/browser/media/router/media_router_metrics.cc (right):

https://codereview.chromium.org/1765143002/diff/100001/chrome/browser/media/r...
chrome/browser/media/router/media_router_metrics.cc:104:
DCHECK_NE(static_cast<int>(version),
Optional nit: Maybe DCHECK_LT?  Ditto below.

https://codereview.chromium.org/1765143002/diff/100001/tools/metrics/histogra...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/1765143002/diff/100001/tools/metrics/histogra...
tools/metrics/histograms/histograms.xml:73996: +  <int value="3"
label="OutOfDateVersion"/>
It's not really clear to me what these labels mean.  Same as what?  How does
"previous" differ from "out of date" -- does "out of date" mean "older than the
immediately previous version"?  Please try to rephrase these labels to clarify,
or maybe just clarify in the histogram's summary what this is measuring.

[benwells, 2016-03-16 23:07:34]

c/b/e lgtm

[mark a. foltz, 2016-03-17 23:35:40]

On 2016/03/16 at 22:23:06, isherman wrote:
>
https://codereview.chromium.org/1765143002/diff/100001/chrome/browser/media/r...
> File chrome/browser/media/router/media_router_metrics.cc (right):
> 
>
https://codereview.chromium.org/1765143002/diff/100001/chrome/browser/media/r...
> chrome/browser/media/router/media_router_metrics.cc:104:
DCHECK_NE(static_cast<int>(version),
> Optional nit: Maybe DCHECK_LT?  Ditto below.

Done here and elsewhere.

> 
>
https://codereview.chromium.org/1765143002/diff/100001/tools/metrics/histogra...
> File tools/metrics/histograms/histograms.xml (right):
> 
>
https://codereview.chromium.org/1765143002/diff/100001/tools/metrics/histogra...
> tools/metrics/histograms/histograms.xml:73996: +  <int value="3"
label="OutOfDateVersion"/>
> It's not really clear to me what these labels mean.  Same as what?  How does
"previous" differ from "out of date" -- does "out of date" mean "older than the
immediately previous version"?  Please try to rephrase these labels to clarify,
or maybe just clarify in the histogram's summary what this is measuring.

Revised the enum names and the summary to clarify.

[Ilya Sherman, 2016-03-18 03:51:51]

Metrics LGTM, thanks.

https://codereview.chromium.org/1765143002/diff/120001/tools/metrics/histogra...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/1765143002/diff/120001/tools/metrics/histogra...
tools/metrics/histograms/histograms.xml:74310: +  <int value="3"
label="MultipleVersionsBehindChrome"/>
nit: It's fine to use spaces in labels if you like, e.g. "Multiple versions
behind Chrome"

[Wez, 2016-03-18 18:35:43]

https://codereview.chromium.org/1765143002/diff/120001/chrome/browser/media/r...
File chrome/browser/media/router/media_router_metrics.cc (right):

https://codereview.chromium.org/1765143002/diff/120001/chrome/browser/media/r...
chrome/browser/media/router/media_router_metrics.cc:122: const base::Version&
second) {
nit: Since we don't need the actual version difference, you could move the
difference->enum logic in to this version and avoid the nasty limits::min() hack
for UNKNOWN. Your test would then be more accurately testing what will actually
get recorded, too.

https://codereview.chromium.org/1765143002/diff/120001/chrome/browser/media/r...
File chrome/browser/media/router/media_router_metrics_unittest.cc (right):

https://codereview.chromium.org/1765143002/diff/120001/chrome/browser/media/r...
chrome/browser/media/router/media_router_metrics_unittest.cc:15: base::Version
second("50.0.2396.71");
nit: const base::Version kVersionForTest, say? - |second| doesn't seem a very
useful name here.

https://codereview.chromium.org/1765143002/diff/120001/chrome/browser/media/r...
File chrome/browser/media/router/media_router_mojo_impl.h (right):

https://codereview.chromium.org/1765143002/diff/120001/chrome/browser/media/r...
chrome/browser/media/router/media_router_mojo_impl.h:379: bool
provider_version_recorded_;
nit: Suggest was_provider_version_recorded_ or have_recorded_provider_version_

nit: Note that this, wakeup_event_count_, event_page_tracker_ could be inline
initialized if you want.

https://codereview.chromium.org/1765143002/diff/120001/tools/metrics/histogra...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/1765143002/diff/120001/tools/metrics/histogra...
tools/metrics/histograms/histograms.xml:74310: +  <int value="3"
label="MultipleVersionsBehindChrome"/>
On 2016/03/18 03:51:51, Ilya Sherman (Away Mar 19-27) wrote:
> nit: It's fine to use spaces in labels if you like, e.g. "Multiple versions
> behind Chrome"

FWIW I like the space-less version; slightly easier to grep correctly and
matches the style of e.g. WakeReason etc below.

[mark a. foltz, 2016-03-18 20:11:01]

PTAL

https://codereview.chromium.org/1765143002/diff/120001/chrome/browser/media/r...
File chrome/browser/media/router/media_router_metrics.cc (right):

https://codereview.chromium.org/1765143002/diff/120001/chrome/browser/media/r...
chrome/browser/media/router/media_router_metrics.cc:122: const base::Version&
second) {
On 2016/03/18 at 18:35:43, Wez wrote:
> nit: Since we don't need the actual version difference, you could move the
difference->enum logic in to this version and avoid the nasty limits::min() hack
for UNKNOWN. Your test would then be more accurately testing what will actually
get recorded, too.

I don't think base::Version should know about enums specific to Media Router.

https://codereview.chromium.org/1765143002/diff/120001/chrome/browser/media/r...
chrome/browser/media/router/media_router_metrics.cc:122: const base::Version&
second) {
On 2016/03/18 at 18:35:43, Wez wrote:
> nit: Since we don't need the actual version difference, you could move the
difference->enum logic in to this version and avoid the nasty limits::min() hack
for UNKNOWN. Your test would then be more accurately testing what will actually
get recorded, too.

I'm not sure it's a good idea to have base::Version know about Media Router
specific enums.

https://codereview.chromium.org/1765143002/diff/120001/chrome/browser/media/r...
File chrome/browser/media/router/media_router_metrics_unittest.cc (right):

https://codereview.chromium.org/1765143002/diff/120001/chrome/browser/media/r...
chrome/browser/media/router/media_router_metrics_unittest.cc:15: base::Version
second("50.0.2396.71");
On 2016/03/18 at 18:35:43, Wez wrote:
> nit: const base::Version kVersionForTest, say? - |second| doesn't seem a very
useful name here.

Done.

https://codereview.chromium.org/1765143002/diff/120001/chrome/browser/media/r...
File chrome/browser/media/router/media_router_mojo_impl.h (right):

https://codereview.chromium.org/1765143002/diff/120001/chrome/browser/media/r...
chrome/browser/media/router/media_router_mojo_impl.h:379: bool
provider_version_recorded_;
On 2016/03/18 at 18:35:43, Wez wrote:
> nit: Suggest was_provider_version_recorded_ or have_recorded_provider_version_
> 
> nit: Note that this, wakeup_event_count_, event_page_tracker_ could be inline
initialized if you want.

Done.

https://codereview.chromium.org/1765143002/diff/120001/tools/metrics/histogra...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/1765143002/diff/120001/tools/metrics/histogra...
tools/metrics/histograms/histograms.xml:74310: +  <int value="3"
label="MultipleVersionsBehindChrome"/>
On 2016/03/18 at 18:35:43, Wez wrote:
> On 2016/03/18 03:51:51, Ilya Sherman (Away Mar 19-27) wrote:
> > nit: It's fine to use spaces in labels if you like, e.g. "Multiple versions
> > behind Chrome"
> 
> FWIW I like the space-less version; slightly easier to grep correctly and
matches the style of e.g. WakeReason etc below.

I think I am going to leave them as camel case.

[Wez, 2016-03-18 20:12:31]

https://codereview.chromium.org/1765143002/diff/120001/chrome/browser/media/r...
File chrome/browser/media/router/media_router_metrics.cc (right):

https://codereview.chromium.org/1765143002/diff/120001/chrome/browser/media/r...
chrome/browser/media/router/media_router_metrics.cc:122: const base::Version&
second) {
On 2016/03/18 20:11:00, mark a. foltz wrote:
> On 2016/03/18 at 18:35:43, Wez wrote:
> > nit: Since we don't need the actual version difference, you could move the
> difference->enum logic in to this version and avoid the nasty limits::min()
hack
> for UNKNOWN. Your test would then be more accurately testing what will
actually
> get recorded, too.
> 
> I don't think base::Version should know about enums specific to Media Router.

Sorry, s/version/function in my original comment.

[mark a. foltz, 2016-03-18 21:03:15]

On 2016/03/18 at 20:12:31, wez wrote:
>
https://codereview.chromium.org/1765143002/diff/120001/chrome/browser/media/r...
> File chrome/browser/media/router/media_router_metrics.cc (right):
> 
>
https://codereview.chromium.org/1765143002/diff/120001/chrome/browser/media/r...
> chrome/browser/media/router/media_router_metrics.cc:122: const base::Version&
second) {
> On 2016/03/18 20:11:00, mark a. foltz wrote:
> > On 2016/03/18 at 18:35:43, Wez wrote:
> > > nit: Since we don't need the actual version difference, you could move the
> > difference->enum logic in to this version and avoid the nasty limits::min()
hack
> > for UNKNOWN. Your test would then be more accurately testing what will
actually
> > get recorded, too.
> > 
> > I don't think base::Version should know about enums specific to Media
Router.
> 
> Sorry, s/version/function in my original comment.

That makes more sense :) Done.

[Wez, 2016-03-18 23:32:00]

Great! LGTM

https://codereview.chromium.org/1765143002/diff/180001/chrome/browser/media/r...
File chrome/browser/media/router/media_router_metrics.cc (right):

https://codereview.chromium.org/1765143002/diff/180001/chrome/browser/media/r...
chrome/browser/media/router/media_router_metrics.cc:100: if
(!extension_version.IsValid() || !extension_version.components().size() ||
nit: Use empty() instead of !size().

https://codereview.chromium.org/1765143002/diff/180001/chrome/browser/media/r...
chrome/browser/media/router/media_router_metrics.cc:108: if (extension_major ==
0 || browser_major == 0) {
nit: Hard to imagine when |browser_major| could be zero, since this code will
only run in official Chrome builds; want to DCHECK it here as well?

[mark a. foltz, 2016-03-19 00:15:03]

https://codereview.chromium.org/1765143002/diff/180001/chrome/browser/media/r...
File chrome/browser/media/router/media_router_metrics.cc (right):

https://codereview.chromium.org/1765143002/diff/180001/chrome/browser/media/r...
chrome/browser/media/router/media_router_metrics.cc:100: if
(!extension_version.IsValid() || !extension_version.components().size() ||
On 2016/03/18 at 23:32:00, Wez wrote:
> nit: Use empty() instead of !size().

Done.

https://codereview.chromium.org/1765143002/diff/180001/chrome/browser/media/r...
chrome/browser/media/router/media_router_metrics.cc:108: if (extension_major ==
0 || browser_major == 0) {
On 2016/03/18 at 23:32:00, Wez wrote:
> nit: Hard to imagine when |browser_major| could be zero, since this code will
only run in official Chrome builds; want to DCHECK it here as well?

That seems a little drastic for this since it's just metrics.  A better place
for the check would behind an #ifdef in version_info.cc which is stamped with
the version string.

[mark a. foltz, 2016-03-19 00:15:06]

The CQ bit was checked by mfoltz@chromium.org

[mark a. foltz, 2016-03-19 00:15:07]

The patchset sent to the CQ was uploaded after l-g-t-m from
benwells@chromium.org, isherman@chromium.org, wez@chromium.org
Link to the patchset: https://codereview.chromium.org/1765143002/#ps200001
(title: "Fix size/empty")

[commit-bot: I haz the power, 2016-03-19 00:15:56]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1765143002/200001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1765143002/200001

[commit-bot: I haz the power, 2016-03-19 00:32:55]

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

[commit-bot: I haz the power, 2016-03-19 00:32:57]

Try jobs failed on following builders:
  android_compile_dbg on tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)

[mark a. foltz, 2016-03-22 21:17:52]

wez, PTAL

I had to #ifdef out some of the new code since it won't compile on Android.  I
filed crbug.com/596999 to do a cleaner job of separating out our Mojo-specific
code from the rest of the MR so that hopefully we can get rid of them.

It's not clear why some of the Android bots refuse to apply patch; I'll bug the
troopers if I can't figure it out.

[Wez, 2016-03-22 21:50:06]

LGTM

Presumably things are failing because Chromium has no extension support on
Android; is there a more specific macro we could be conditional on that
indicates that, rather than tying directly to OS_ANDROID?

https://codereview.chromium.org/1765143002/diff/260001/chrome/browser/media/r...
File chrome/browser/media/router/media_router_metrics_unittest.cc (right):

https://codereview.chromium.org/1765143002/diff/260001/chrome/browser/media/r...
chrome/browser/media/router/media_router_metrics_unittest.cc:12: #if
!defined(OS_ANDROID)
nit: Would be cleaner to make these unittests.cc conditional in the GN+GYP
files, rather than #ifdefing here.

[mark a. foltz, 2016-03-22 22:21:06]

On 2016/03/22 at 21:50:06, wez wrote:
> LGTM
> 
> Presumably things are failing because Chromium has no extension support on
Android; is there a more specific macro we could be conditional on that
indicates that, rather than tying directly to OS_ANDROID?

Not sure.  For now, staying consistent with the rest of the MR code, to make it
easier to remove OS_ANDROID later :)

>
https://codereview.chromium.org/1765143002/diff/260001/chrome/browser/media/r...
> File chrome/browser/media/router/media_router_metrics_unittest.cc (right):
> 
>
https://codereview.chromium.org/1765143002/diff/260001/chrome/browser/media/r...
> chrome/browser/media/router/media_router_metrics_unittest.cc:12: #if
!defined(OS_ANDROID)
> nit: Would be cleaner to make these unittests.cc conditional in the GN+GYP
files, rather than #ifdefing here.

Done.

[mark a. foltz, 2016-03-22 22:21:10]

The CQ bit was checked by mfoltz@chromium.org

[mark a. foltz, 2016-03-22 22:21:11]

The patchset sent to the CQ was uploaded after l-g-t-m from
benwells@chromium.org, isherman@chromium.org, wez@chromium.org
Link to the patchset: https://codereview.chromium.org/1765143002/#ps280001
(title: "Move media_router_metrics_unittest to non_android")

[commit-bot: I haz the power, 2016-03-22 22:21:36]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1765143002/280001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1765143002/280001

[commit-bot: I haz the power, 2016-03-22 23:46:34]

Committed patchset #15 (id:280001)

[commit-bot: I haz the power, 2016-03-22 23:49:07]

Description was changed from

==========
[Media Router] Add UMA histograms tracking component extension version & wakeup
outcome.

This patch adds two histograms:

- MediaRouter.Provider.Version compares the component version with the browser
version and records the difference, the first time that the component wakes up.

- MediaRouter.Provider.Wakeup records the overall success/failure outcome for
waking the event page.

Note that Mojo only provides a boolean to indicate a failure to establish the
message pipe.  If we can get better errors from Mojo (or the extensions system)
we can add more error categories.

BUG=588230
==========

to

==========
[Media Router] Add UMA histograms tracking component extension version & wakeup
outcome.

This patch adds two histograms:

- MediaRouter.Provider.Version compares the component version with the browser
version and records the difference, the first time that the component wakes up.

- MediaRouter.Provider.Wakeup records the overall success/failure outcome for
waking the event page.

Note that Mojo only provides a boolean to indicate a failure to establish the
message pipe.  If we can get better errors from Mojo (or the extensions system)
we can add more error categories.

BUG=588230

Committed: https://crrev.com/0dc8e6b8fb72e8d204c76ffc43be6516f114c91b
Cr-Commit-Position: refs/heads/master@{#382735}
==========

[commit-bot: I haz the power, 2016-03-22 23:49:09]

Patchset 15 (id:??) landed as
https://crrev.com/0dc8e6b8fb72e8d204c76ffc43be6516f114c91b
Cr-Commit-Position: refs/heads/master@{#382735}