New issue
Advanced search Search tips
Note: Color blocks (like █ or █) mean that a user may not be available. Tooltip shows the reason.

Issue 687380 link

Starred by 2 users
Status: Available
Owner: ----
Cc:
█ abdulsyed@chromium.org
mfo...@chromium.org
█ manoranj...@chromium.org
powerb@chromium.org
btolsch@chromium.org
Components:
EstimatedDays: ----
NextAction: ----
OS: Linux , Windows , Chrome , Mac
Pri: 3
Type: Feature



Sign in to add a comment

[Media Router] Implement logging mechanism for in-browser components

Project Member Reported by imch...@chromium.org, Jan 31 2017 
Logging related to device discovery happens in the MR extension and is attached to feedback reports. When we move device discovery to the browser, it would be very useful to continue supplying device discovery logs in feedback for troubleshooting purposes.


Some things to consider when designing a logging system for device discovery:
- Capping the max number of log entries
- Whether log upload browser-side or extension-side. Note that either way Media Router will need to inform the extension about the upload.
- Whether reconciliation with extension logs should be done prior to upload or after (e.g., done with an offline script).

Comment 1 by imch...@chromium.org, Jan 31 2017

Blocking: 687383

Comment 2 by sko...@chromium.org, Feb 9 2017

Status: Available (was: Untriaged)

Comment 3 by mfo...@chromium.org, Oct 26 2017

Blocking: -687383

Comment 4 by mfo...@chromium.org, Oct 26 2017

Blocking: 752604

Comment 5 by imch...@chromium.org, Jan 10 2018

Owner: zhaobin@chromium.org
Summary: [Media Router] Implement logging mechanism for in-browser components (was: [Media Router] Implement logging mechanism for MediaSinkService)
Assigning to Bin for design work and also updated title to reflect that we want extend this to other components that we are migrating to the browser.

Comment 6 by mfo...@chromium.org, Mar 2 2018

Status: Started (was: Available)

Comment 7 by mfo...@chromium.org, Mar 2 2018

Blocking: -752604
Project Member

Comment 8 by bugdroid1@chromium.org, Mar 6 2018 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/89f721984216a7a45c88325a723ae533e7fa9f18

commit 89f721984216a7a45c88325a723ae533e7fa9f18
Author: Bin Zhao <zhaobin@chromium.org>
Date: Tue Mar 06 19:56:53 2018

[Media Router] Added GetMediaSinkServiceStatus() to MediaRouter interface

We would like to collect media sink service status and include it in
feedback report. Sending feedback is currently done at extension side.
Extension will fetch media sink service status via
GetMediaSinkServiceStatus() mojo call.

- Added GetMediaSinkServiceStatus() to media_router.mojom
- Added empty implementation to media_router_mojo_impl
- Added MediaSinkServiceStatus class
- Implemented GetMediaSinkServiceStatus() in media_router_desktop

Bug: 687380
Change-Id: I2cbbf997912b6fc1af4897e929250235e70d7837
Reviewed-on: https://chromium-review.googlesource.com/884743
Commit-Queue: Bin Zhao <zhaobin@chromium.org>
Reviewed-by: Daniel Cheng <dcheng@chromium.org>
Reviewed-by: Derek Cheng <imcheng@chromium.org>
Cr-Commit-Position: refs/heads/master@{#541177}
[modify] https://crrev.com/89f721984216a7a45c88325a723ae533e7fa9f18/chrome/browser/media/router/BUILD.gn
[modify] https://crrev.com/89f721984216a7a45c88325a723ae533e7fa9f18/chrome/browser/media/router/mojo/media_router_desktop.cc
[modify] https://crrev.com/89f721984216a7a45c88325a723ae533e7fa9f18/chrome/browser/media/router/mojo/media_router_desktop.h
[modify] https://crrev.com/89f721984216a7a45c88325a723ae533e7fa9f18/chrome/browser/media/router/mojo/media_router_mojo_impl.cc
[modify] https://crrev.com/89f721984216a7a45c88325a723ae533e7fa9f18/chrome/browser/media/router/mojo/media_router_mojo_impl.h
[add] https://crrev.com/89f721984216a7a45c88325a723ae533e7fa9f18/chrome/browser/media/router/mojo/media_sink_service_status.cc
[add] https://crrev.com/89f721984216a7a45c88325a723ae533e7fa9f18/chrome/browser/media/router/mojo/media_sink_service_status.h
[add] https://crrev.com/89f721984216a7a45c88325a723ae533e7fa9f18/chrome/browser/media/router/mojo/media_sink_service_status_unittest.cc
[modify] https://crrev.com/89f721984216a7a45c88325a723ae533e7fa9f18/chrome/browser/media/router/test/mock_mojo_media_router.h
[modify] https://crrev.com/89f721984216a7a45c88325a723ae533e7fa9f18/chrome/common/media_router/BUILD.gn
[add] https://crrev.com/89f721984216a7a45c88325a723ae533e7fa9f18/chrome/common/media_router/media_route_provider_helper.cc
[modify] https://crrev.com/89f721984216a7a45c88325a723ae533e7fa9f18/chrome/common/media_router/media_route_provider_helper.h
[modify] https://crrev.com/89f721984216a7a45c88325a723ae533e7fa9f18/chrome/common/media_router/mojo/media_router.mojom
[modify] https://crrev.com/89f721984216a7a45c88325a723ae533e7fa9f18/chrome/renderer/resources/extensions/media_router_bindings.js
[modify] https://crrev.com/89f721984216a7a45c88325a723ae533e7fa9f18/chrome/test/BUILD.gn

Comment 9 by zhaobin@chromium.org, Mar 6 2018

Labels: Merge-Request-66

Comment 10 by candr...@chromium.org, Mar 6 2018 

Please add affected OSs.

Comment 11 by zhaobin@chromium.org, Mar 6 2018

Labels: OS-Chrome OS-Linux OS-Mac OS-Windows
Project Member

Comment 12 by bugdroid1@chromium.org, Mar 7 2018 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/59c6647d213a63c63a5671048c08577ca961dfae

commit 59c6647d213a63c63a5671048c08577ca961dfae
Author: Bin Zhao <zhaobin@chromium.org>
Date: Wed Mar 07 20:30:45 2018

[Media Router] Added chrome://media-router-internals page

Added chrome://media-router-internals page to show Media Router debug info.

Bug: 687380
Cq-Include-Trybots: master.tryserver.chromium.linux:closure_compilation
Change-Id: I28f2e996b9643acf92a07bc0e213c075882a8a6c
Reviewed-on: https://chromium-review.googlesource.com/929997
Commit-Queue: Bin Zhao <zhaobin@chromium.org>
Reviewed-by: Bernhard Bauer <bauerb@chromium.org>
Reviewed-by: Derek Cheng <imcheng@chromium.org>
Cr-Commit-Position: refs/heads/master@{#541557}
[modify] https://crrev.com/59c6647d213a63c63a5671048c08577ca961dfae/chrome/browser/media/router/media_router.h
[modify] https://crrev.com/59c6647d213a63c63a5671048c08577ca961dfae/chrome/browser/media/router/media_router_base.cc
[modify] https://crrev.com/59c6647d213a63c63a5671048c08577ca961dfae/chrome/browser/media/router/media_router_base.h
[modify] https://crrev.com/59c6647d213a63c63a5671048c08577ca961dfae/chrome/browser/media/router/mojo/media_router_desktop.cc
[modify] https://crrev.com/59c6647d213a63c63a5671048c08577ca961dfae/chrome/browser/media/router/mojo/media_router_desktop.h
[modify] https://crrev.com/59c6647d213a63c63a5671048c08577ca961dfae/chrome/browser/media/router/mojo/media_sink_service_status.cc
[modify] https://crrev.com/59c6647d213a63c63a5671048c08577ca961dfae/chrome/browser/media/router/mojo/media_sink_service_status.h
[modify] https://crrev.com/59c6647d213a63c63a5671048c08577ca961dfae/chrome/browser/media/router/test/mock_media_router.h
[modify] https://crrev.com/59c6647d213a63c63a5671048c08577ca961dfae/chrome/browser/media/router/test/mock_mojo_media_router.h
[modify] https://crrev.com/59c6647d213a63c63a5671048c08577ca961dfae/chrome/browser/media_router_resources.grdp
[add] https://crrev.com/59c6647d213a63c63a5671048c08577ca961dfae/chrome/browser/resources/media_router/media_router_internals.css
[add] https://crrev.com/59c6647d213a63c63a5671048c08577ca961dfae/chrome/browser/resources/media_router/media_router_internals.html
[add] https://crrev.com/59c6647d213a63c63a5671048c08577ca961dfae/chrome/browser/resources/media_router/media_router_internals.js
[modify] https://crrev.com/59c6647d213a63c63a5671048c08577ca961dfae/chrome/browser/ui/BUILD.gn
[modify] https://crrev.com/59c6647d213a63c63a5671048c08577ca961dfae/chrome/browser/ui/webui/chrome_web_ui_controller_factory.cc
[add] https://crrev.com/59c6647d213a63c63a5671048c08577ca961dfae/chrome/browser/ui/webui/media_router/media_router_internals_ui.cc
[add] https://crrev.com/59c6647d213a63c63a5671048c08577ca961dfae/chrome/browser/ui/webui/media_router/media_router_internals_ui.h
[add] https://crrev.com/59c6647d213a63c63a5671048c08577ca961dfae/chrome/browser/ui/webui/media_router/media_router_internals_webui_message_handler.cc
[add] https://crrev.com/59c6647d213a63c63a5671048c08577ca961dfae/chrome/browser/ui/webui/media_router/media_router_internals_webui_message_handler.h
[modify] https://crrev.com/59c6647d213a63c63a5671048c08577ca961dfae/chrome/common/webui_url_constants.cc
[modify] https://crrev.com/59c6647d213a63c63a5671048c08577ca961dfae/chrome/common/webui_url_constants.h
Project Member

Comment 13 by sheriffbot@chromium.org, Mar 8 2018

Labels: -Merge-Request-66 Merge-Review-66 Hotlist-Merge-Review
This bug requires manual review: There is .grd file changes and we are only 39 days from stable.
Please contact the milestone owner if you have questions.
Owners: cmasso@(Android), cmasso@(iOS), josafat@(ChromeOS), abdulsyed@(Desktop)

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot

Comment 14 by imch...@chromium.org, Mar 8 2018 

Note we are only requesting merge for the patch in comment 8 which does not contain grd file changes. This change is needed to include additional logs in the Media Router feedback which would be useful to help debug in-browser device discovery. Thanks.

Comment 15 by gov...@chromium.org, Mar 9 2018 

How is the change listed at #8 looking in canary? will it be a safe merge to M66?

Comment 16 by zhaobin@chromium.org, Mar 9 2018 

It looks fine and safe to me.

#8 went in 67.0.3364.0. Manually tested with Mac Canary 67.0.3364.0, no crash.

UMA data suggests that browser crash rate does not increase for 67.0.3364.0 comparing to previous version. https://uma.googleplex.com/p/chrome/stability_continuous?sid=aa445a0d5bbf97d163b94ddcdb18727b

Comment 17 by gov...@chromium.org, Mar 9 2018 

Thank you zhaobin@. Is this feature behind Finch?

Comment 18 by zhaobin@chromium.org, Mar 9 2018 

No, it's not. 

We would like to merge this patch because  crbug.com/698940  depends on this. To merge  crbug.com/698940 , it seems easier and safer to merge this instead of resolving merge conflicts.

Comment 19 by gov...@chromium.org, Mar 9 2018

Labels: -Merge-Review-66 Merge-Approved-66
Approving merge to M66 branch 3359 based on comment #14, #16, #18. Please merge. Thank you.

Comment 20 by gov...@chromium.org, Mar 9 2018 

Pls merge your change to M66 branch ASAP so we can pick it up for next M66 Dev/Beta release. Thank you.
Project Member

Comment 21 by bugdroid1@chromium.org, Mar 9 2018

Labels: -merge-approved-66 merge-merged-3359
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/317e0e7b3afeffa6da6ab896cd5584f301dc095a

commit 317e0e7b3afeffa6da6ab896cd5584f301dc095a
Author: Bin Zhao <zhaobin@chromium.org>
Date: Fri Mar 09 23:41:10 2018

[Media Router] Added GetMediaSinkServiceStatus() to MediaRouter interface

We would like to collect media sink service status and include it in
feedback report. Sending feedback is currently done at extension side.
Extension will fetch media sink service status via
GetMediaSinkServiceStatus() mojo call.

- Added GetMediaSinkServiceStatus() to media_router.mojom
- Added empty implementation to media_router_mojo_impl
- Added MediaSinkServiceStatus class
- Implemented GetMediaSinkServiceStatus() in media_router_desktop

Bug: 687380
Change-Id: I2cbbf997912b6fc1af4897e929250235e70d7837
Reviewed-on: https://chromium-review.googlesource.com/884743
Commit-Queue: Bin Zhao <zhaobin@chromium.org>
Reviewed-by: Daniel Cheng <dcheng@chromium.org>
Reviewed-by: Derek Cheng <imcheng@chromium.org>
Cr-Original-Commit-Position: refs/heads/master@{#541177}(cherry picked from commit 89f721984216a7a45c88325a723ae533e7fa9f18)
Reviewed-on: https://chromium-review.googlesource.com/957782
Reviewed-by: Bin Zhao <zhaobin@chromium.org>
Cr-Commit-Position: refs/branch-heads/3359@{#140}
Cr-Branched-From: 66afc5e5d10127546cc4b98b9117aff588b5e66b-refs/heads/master@{#540276}
[modify] https://crrev.com/317e0e7b3afeffa6da6ab896cd5584f301dc095a/chrome/browser/media/router/BUILD.gn
[modify] https://crrev.com/317e0e7b3afeffa6da6ab896cd5584f301dc095a/chrome/browser/media/router/mojo/media_router_desktop.cc
[modify] https://crrev.com/317e0e7b3afeffa6da6ab896cd5584f301dc095a/chrome/browser/media/router/mojo/media_router_desktop.h
[modify] https://crrev.com/317e0e7b3afeffa6da6ab896cd5584f301dc095a/chrome/browser/media/router/mojo/media_router_mojo_impl.cc
[modify] https://crrev.com/317e0e7b3afeffa6da6ab896cd5584f301dc095a/chrome/browser/media/router/mojo/media_router_mojo_impl.h
[add] https://crrev.com/317e0e7b3afeffa6da6ab896cd5584f301dc095a/chrome/browser/media/router/mojo/media_sink_service_status.cc
[add] https://crrev.com/317e0e7b3afeffa6da6ab896cd5584f301dc095a/chrome/browser/media/router/mojo/media_sink_service_status.h
[add] https://crrev.com/317e0e7b3afeffa6da6ab896cd5584f301dc095a/chrome/browser/media/router/mojo/media_sink_service_status_unittest.cc
[modify] https://crrev.com/317e0e7b3afeffa6da6ab896cd5584f301dc095a/chrome/browser/media/router/test/mock_mojo_media_router.h
[modify] https://crrev.com/317e0e7b3afeffa6da6ab896cd5584f301dc095a/chrome/common/media_router/BUILD.gn
[add] https://crrev.com/317e0e7b3afeffa6da6ab896cd5584f301dc095a/chrome/common/media_router/media_route_provider_helper.cc
[modify] https://crrev.com/317e0e7b3afeffa6da6ab896cd5584f301dc095a/chrome/common/media_router/media_route_provider_helper.h
[modify] https://crrev.com/317e0e7b3afeffa6da6ab896cd5584f301dc095a/chrome/common/media_router/mojo/media_router.mojom
[modify] https://crrev.com/317e0e7b3afeffa6da6ab896cd5584f301dc095a/chrome/renderer/resources/extensions/media_router_bindings.js
[modify] https://crrev.com/317e0e7b3afeffa6da6ab896cd5584f301dc095a/chrome/test/BUILD.gn

Comment 22 by gov...@chromium.org, Mar 10 2018

Cc: abdulsyed@chromium.org manoranj...@chromium.org
There is a Top #1 browser crash (90% crashes) on latest Dev 66.0.3359.22 (https://bugs.chromium.org/p/chromium/issues/detail?id=816628#c23). Could you pls double confirm merge listed at #21 is safe for M66? If not, pls revert the change ASAP. Thank you.

Comment 23 by zhaobin@chromium.org, Mar 10 2018 

Per discussion with govind@, it seems safe at this point. We will leave it in M66.

Comment 24 by mfo...@chromium.org, Apr 5 2018

Owner: ----
Status: Untriaged (was: Started)
-zhaobin@

I believe we've landed a V1 API that extracts information from the MR to attach to feedback reports.  We should evaluate its usefulness and use that to plan future improvements.

Assigning to triage to evaluate next steps.

Comment 25 by mfo...@chromium.org, Aug 2

Cc: powerb@chromium.org

Comment 26 by mfo...@chromium.org, Aug 2

Cc: -zhaobin@chromium.org

Comment 27 by powerb@chromium.org, Aug 14

Owner: imch...@chromium.org
Status: Assigned (was: Untriaged)

Comment 28 by imch...@chromium.org, Sep 24

Owner: ----
Status: Available (was: Assigned)
Design doc drafted (see team drive). Recommendation is to implement logging for critical code paths first that would allow us to diagnose outstanding discovery issues, and implement plumbing of browser-side logs to the extension feedback logic.

Comment 29 by mfo...@chromium.org, Nov 7

Cc: -imch...@chromium.org
►

Sign in to add a comment