[MediaRouter] Record DIAL device counts in DialMediaSinkServiceImpl

[MediaRouter] Record DIAL device counts in DialMediaSinkServiceImpl

For browser side DIAL discovery, record device count at browser side. Keep in sync with recordDeviceCounts_() function in dial_sink_discovery_service.js (https://cs.corp.google.com/piper///depot/google3/chrome_platform/media_router/extension/providers/dial/dial_sink_discovery_service.js?rcl=157541007&l=209)

BUG=687375
Review-Url: https://codereview.chromium.org/2916163002
Cr-Commit-Position: refs/heads/master@{#477877}
Committed: https://chromium.googlesource.com/chromium/src/+/cfa975069fd5856fdc4c494276a3a9af6be3da79

[zhaobin, 2017-06-01 21:20:40]

zhaobin@chromium.org changed reviewers:
+ imcheng@chromium.org, mfoltz@chromium.org

[imcheng, 2017-06-02 21:34:51]

lgtm % comments

https://codereview.chromium.org/2916163002/diff/1/chrome/browser/media/router...
File chrome/browser/media/router/discovery/dial/dial_media_sink_service_impl.cc
(right):

https://codereview.chromium.org/2916163002/diff/1/chrome/browser/media/router...
chrome/browser/media/router/discovery/dial/dial_media_sink_service_impl.cc:17:
const int kDeviceCountMetricThresholdMins = 60 * 60;
I think this should be just 60?

https://codereview.chromium.org/2916163002/diff/1/chrome/browser/media/router...
chrome/browser/media/router/discovery/dial/dial_media_sink_service_impl.cc:139:
if (base::Time::Now() - device_count_metrics_record_time_ <
Can the time check logic be part of MediaSinkServiceBase? That way, it can be
shared with other implementations.

[mark a. foltz, 2017-06-02 22:06:41]

https://codereview.chromium.org/2916163002/diff/1/chrome/browser/media/router...
File chrome/browser/media/router/discovery/dial/dial_media_sink_service_impl.cc
(right):

https://codereview.chromium.org/2916163002/diff/1/chrome/browser/media/router...
chrome/browser/media/router/discovery/dial/dial_media_sink_service_impl.cc:139:
if (base::Time::Now() - device_count_metrics_record_time_ <
On 2017/06/02 at 21:34:51, imcheng wrote:
> Can the time check logic be part of MediaSinkServiceBase? That way, it can be
shared with other implementations.

I would rather bookkeeping for recording UMA live in MediaRouterMetrics, unless
there is some state that is tied to a specific instance of an object.  I think
DMSS is a singleton so maybe we can move this.

[zhaobin, 2017-06-05 23:20:55]

https://codereview.chromium.org/2916163002/diff/1/chrome/browser/media/router...
File chrome/browser/media/router/discovery/dial/dial_media_sink_service_impl.cc
(right):

https://codereview.chromium.org/2916163002/diff/1/chrome/browser/media/router...
chrome/browser/media/router/discovery/dial/dial_media_sink_service_impl.cc:17:
const int kDeviceCountMetricThresholdMins = 60 * 60;
On 2017/06/02 21:34:51, imcheng wrote:
> I think this should be just 60?

Fixed in media_router_metrics.cc

https://codereview.chromium.org/2916163002/diff/1/chrome/browser/media/router...
chrome/browser/media/router/discovery/dial/dial_media_sink_service_impl.cc:139:
if (base::Time::Now() - device_count_metrics_record_time_ <
On 2017/06/02 22:06:40, mark a. foltz wrote:
> On 2017/06/02 at 21:34:51, imcheng wrote:
> > Can the time check logic be part of MediaSinkServiceBase? That way, it can
be
> shared with other implementations.
> 
> I would rather bookkeeping for recording UMA live in MediaRouterMetrics,
unless
> there is some state that is tied to a specific instance of an object.  I think
> DMSS is a singleton so maybe we can move this.

Done.

[mark a. foltz, 2017-06-06 23:45:00]

LGTM.

I suppose the device count specific metrics and state could be put into its own
class eventually, but let's wait until there's a need for it.

Maybe add a // comment to that effect in media_router_metrics.h.

https://codereview.chromium.org/2916163002/diff/40001/chrome/browser/media/ro...
File chrome/browser/media/router/media_router_metrics.h (right):

https://codereview.chromium.org/2916163002/diff/40001/chrome/browser/media/ro...
chrome/browser/media/router/media_router_metrics.h:64: static const char
kDialAvailableDeviceCount[];
kHistogramDialAvailableDeviceCount (and similarly below)

[zhaobin, 2017-06-07 02:02:15]

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

[commit-bot: I haz the power, 2017-06-07 02:02:37]

Dry run: CQ is trying da patch.

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

[zhaobin, 2017-06-07 02:05:59]

zhaobin@chromium.org changed reviewers:
+ isherman@chromium.org

[zhaobin, 2017-06-07 02:05:59]

+isherman@ to review: tools/metrics/histograms/histograms.xml

https://codereview.chromium.org/2916163002/diff/40001/chrome/browser/media/ro...
File chrome/browser/media/router/media_router_metrics.h (right):

https://codereview.chromium.org/2916163002/diff/40001/chrome/browser/media/ro...
chrome/browser/media/router/media_router_metrics.h:64: static const char
kDialAvailableDeviceCount[];
On 2017/06/06 23:45:00, mark a. foltz wrote:
> kHistogramDialAvailableDeviceCount (and similarly below)

Done.

[commit-bot: I haz the power, 2017-06-07 03:45:04]

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

[commit-bot: I haz the power, 2017-06-07 03:45:05]

Dry run: This issue passed the CQ dry run.

[Ilya Sherman, 2017-06-07 04:57:03]

https://codereview.chromium.org/2916163002/diff/60001/chrome/browser/media/ro...
File chrome/browser/media/router/media_router_metrics.cc (right):

https://codereview.chromium.org/2916163002/diff/60001/chrome/browser/media/ro...
chrome/browser/media/router/media_router_metrics.cc:13: const int
kDeviceCountMetricThresholdMins = 60;
What's the reason for this?  What's the motivation for recording the metric
repeatedly, roughly once per hour?

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

https://codereview.chromium.org/2916163002/diff/60001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:30119: +<histogram
name="MediaRouter.Dial.AvailableDevicesCount">
nit: Please add units="devices"

https://codereview.chromium.org/2916163002/diff/60001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:30121: +  <summary>The number of
available DIAL devices.</summary>
Please document when this is recorded.

https://codereview.chromium.org/2916163002/diff/60001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:30124: +<histogram
name="MediaRouter.Dial.KnownDevicesCount">
nit: Please add units="devices"

https://codereview.chromium.org/2916163002/diff/60001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:30126: +  <summary>The number of known
DIAL devices.</summary>
Ditto

[zhaobin, 2017-06-07 20:45:56]

https://codereview.chromium.org/2916163002/diff/60001/chrome/browser/media/ro...
File chrome/browser/media/router/media_router_metrics.cc (right):

https://codereview.chromium.org/2916163002/diff/60001/chrome/browser/media/ro...
chrome/browser/media/router/media_router_metrics.cc:13: const int
kDeviceCountMetricThresholdMins = 60;
On 2017/06/07 04:57:03, Ilya Sherman wrote:
> What's the reason for this?  What's the motivation for recording the metric
> repeatedly, roughly once per hour?

To keep device count recording mechanism in browser the same as that in
extensions
(https://cs.corp.google.com/piper///depot/google3/chrome_platform/media_router...)

We are moving device discovery from extension (old code) to browser (new code).
We want to compare device count found by new code and old code, and need to keep
recording mechanism the same.

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

https://codereview.chromium.org/2916163002/diff/60001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:30119: +<histogram
name="MediaRouter.Dial.AvailableDevicesCount">
On 2017/06/07 04:57:03, Ilya Sherman wrote:
> nit: Please add units="devices"

Done.

https://codereview.chromium.org/2916163002/diff/60001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:30121: +  <summary>The number of
available DIAL devices.</summary>
On 2017/06/07 04:57:03, Ilya Sherman wrote:
> Please document when this is recorded.

Done.

https://codereview.chromium.org/2916163002/diff/60001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:30124: +<histogram
name="MediaRouter.Dial.KnownDevicesCount">
On 2017/06/07 04:57:03, Ilya Sherman wrote:
> nit: Please add units="devices"

Done.

https://codereview.chromium.org/2916163002/diff/60001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:30126: +  <summary>The number of known
DIAL devices.</summary>
On 2017/06/07 04:57:03, Ilya Sherman wrote:
> Ditto

Done.

[Ilya Sherman, 2017-06-07 21:08:45]

Metrics LGTM

[zhaobin, 2017-06-07 21:10:00]

The CQ bit was checked by zhaobin@chromium.org

[zhaobin, 2017-06-07 21:10:01]

The patchset sent to the CQ was uploaded after l-g-t-m from
imcheng@chromium.org, mfoltz@chromium.org
Link to the patchset: https://codereview.chromium.org/2916163002/#ps80001
(title: "resolve code review comments from isherman")

[commit-bot: I haz the power, 2017-06-07 21:11:51]

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-06-07 21:26:09]

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

[commit-bot: I haz the power, 2017-06-07 21:26:11]

Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[zhaobin, 2017-06-07 23:36:23]

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

[commit-bot: I haz the power, 2017-06-07 23:36:58]

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-06-08 01:23:15]

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

[commit-bot: I haz the power, 2017-06-08 01:23:16]

Dry run: This issue passed the CQ dry run.

[zhaobin, 2017-06-08 03:55:43]

The CQ bit was checked by zhaobin@chromium.org

[commit-bot: I haz the power, 2017-06-08 03:55:57]

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-06-08 03:59:42]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1496894143395550,
"parent_rev": "54dd1f94d5fe8bbf9ae0df468a4e8224d970ddeb", "commit_rev":
"cfa975069fd5856fdc4c494276a3a9af6be3da79"}

[commit-bot: I haz the power, 2017-06-08 03:59:54]

Description was changed from

==========
[MediaRouter] Record DIAL device counts in DialMediaSinkServiceImpl

For browser side DIAL discovery, record device count at browser side. Keep in
sync with recordDeviceCounts_() function in dial_sink_discovery_service.js
(https://cs.corp.google.com/piper///depot/google3/chrome_platform/media_router...)

BUG=687375
==========

to

==========
[MediaRouter] Record DIAL device counts in DialMediaSinkServiceImpl

For browser side DIAL discovery, record device count at browser side. Keep in
sync with recordDeviceCounts_() function in dial_sink_discovery_service.js
(https://cs.corp.google.com/piper///depot/google3/chrome_platform/media_router...)

BUG=687375

Review-Url: https://codereview.chromium.org/2916163002
Cr-Commit-Position: refs/heads/master@{#477877}
Committed:
https://chromium.googlesource.com/chromium/src/+/cfa975069fd5856fdc4c494276a3...
==========

[commit-bot: I haz the power, 2017-06-08 03:59:56]

Committed patchset #5 (id:80001) as
https://chromium.googlesource.com/chromium/src/+/cfa975069fd5856fdc4c494276a3...