Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1974723002:
[Chromecast] Add metric reporting to ConnectivityCheckerImpl (Closed)
|
Created:
4 years, 7 months ago by bcf Modified:
4 years, 6 months ago CC:
chromium-reviews, cbentzel+watch_chromium.org, lcwu+watch_chromium.org, halliwell+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Description[Chromecast] Add metric reporting to ConnectivityCheckerImpl
BUG=internal b/28294837
TEST=Manual testing
Committed: https://crrev.com/aedc70831b5008e78e036265d60a739ee3393b62
Cr-Commit-Position: refs/heads/master@{#393640}
Patch Set 1 #
Total comments: 3
Patch Set 2 : Add CastMetricsHelper::RecordEventWithValue #
Total comments: 4
Patch Set 3 : Move event reporting to location where connectivity change is reported #Patch Set 4 : Remove unused include #Patch Set 5 : Fix GN dependency #Patch Set 6 : Initial PS #Messages
Total messages: 23 (9 generated)
Description was changed from ========== [Chromecast] Add metric reporting to ConnectivityCheckerImpl BUG=internal b/28294837 TEST=Manual testing ========== to ========== [Chromecast] Add metric reporting to ConnectivityCheckerImpl BUG=internal b/28294837 TEST=Manual testing ==========
bcf@chromium.org changed reviewers: + derekjchow@chromium.org, slan@chromium.org, wzhong@chromium.org
https://codereview.chromium.org/1974723002/diff/1/chromecast/net/connectivity... File chromecast/net/connectivity_checker_impl.cc (right): https://codereview.chromium.org/1974723002/diff/1/chromecast/net/connectivity... chromecast/net/connectivity_checker_impl.cc:234: std::unique_ptr<base::DictionaryValue> cast_event( Is there a better way of constructing metrics in chromium/src/chromecast? It seems like most of the easy ways are in internal.
halliwell@chromium.org changed reviewers: + halliwell@chromium.org
https://codereview.chromium.org/1974723002/diff/1/chromecast/net/connectivity... File chromecast/net/connectivity_checker_impl.cc (right): https://codereview.chromium.org/1974723002/diff/1/chromecast/net/connectivity... chromecast/net/connectivity_checker_impl.cc:234: std::unique_ptr<base::DictionaryValue> cast_event( On 2016/05/12 07:35:56, bcf wrote: > Is there a better way of constructing metrics in chromium/src/chromecast? It > seems like most of the easy ways are in internal. CastMetricsHelper::RecordApplicationEventWIthValue is a helper that essentially does what you've done here.
https://codereview.chromium.org/1974723002/diff/1/chromecast/net/connectivity... File chromecast/net/connectivity_checker_impl.cc (right): https://codereview.chromium.org/1974723002/diff/1/chromecast/net/connectivity... chromecast/net/connectivity_checker_impl.cc:234: std::unique_ptr<base::DictionaryValue> cast_event( On 2016/05/12 13:46:19, halliwell wrote: > On 2016/05/12 07:35:56, bcf wrote: > > Is there a better way of constructing metrics in chromium/src/chromecast? It > > seems like most of the easy ways are in internal. > > CastMetricsHelper::RecordApplicationEventWIthValue is a helper that essentially > does what you've done here. I saw this, but I wasn't sure we wanted to add the other parts (app_id, session_id, sdk_version). I guess I can make a new CastMetricsHelper::RecordEventWIthValue
On 2016/05/12 17:04:10, bcf wrote: > https://codereview.chromium.org/1974723002/diff/1/chromecast/net/connectivity... > File chromecast/net/connectivity_checker_impl.cc (right): > > https://codereview.chromium.org/1974723002/diff/1/chromecast/net/connectivity... > chromecast/net/connectivity_checker_impl.cc:234: > std::unique_ptr<base::DictionaryValue> cast_event( > On 2016/05/12 13:46:19, halliwell wrote: > > On 2016/05/12 07:35:56, bcf wrote: > > > Is there a better way of constructing metrics in chromium/src/chromecast? It > > > seems like most of the easy ways are in internal. > > > > CastMetricsHelper::RecordApplicationEventWIthValue is a helper that > essentially > > does what you've done here. > > I saw this, but I wasn't sure we wanted to add the other parts (app_id, > session_id, sdk_version). I guess I can make a new > CastMetricsHelper::RecordEventWIthValue Yep, if we want something independent of app, such a helper would make sense.
On 2016/05/12 17:05:00, halliwell wrote: > On 2016/05/12 17:04:10, bcf wrote: > > > https://codereview.chromium.org/1974723002/diff/1/chromecast/net/connectivity... > > File chromecast/net/connectivity_checker_impl.cc (right): > > > > > https://codereview.chromium.org/1974723002/diff/1/chromecast/net/connectivity... > > chromecast/net/connectivity_checker_impl.cc:234: > > std::unique_ptr<base::DictionaryValue> cast_event( > > On 2016/05/12 13:46:19, halliwell wrote: > > > On 2016/05/12 07:35:56, bcf wrote: > > > > Is there a better way of constructing metrics in chromium/src/chromecast? > It > > > > seems like most of the easy ways are in internal. > > > > > > CastMetricsHelper::RecordApplicationEventWIthValue is a helper that > > essentially > > > does what you've done here. > > > > I saw this, but I wasn't sure we wanted to add the other parts (app_id, > > session_id, sdk_version). I guess I can make a new > > CastMetricsHelper::RecordEventWIthValue > > Yep, if we want something independent of app, such a helper would make sense. How does PS2 look?
https://codereview.chromium.org/1974723002/diff/20001/chromecast/net/connecti... File chromecast/net/connectivity_checker_impl.cc (right): https://codereview.chromium.org/1974723002/diff/20001/chromecast/net/connecti... chromecast/net/connectivity_checker_impl.cc:49: "Network.Connectivity.ErrorType"; Network.ConnectivityChecking.ErrorType https://codereview.chromium.org/1974723002/diff/20001/chromecast/net/connecti... chromecast/net/connectivity_checker_impl.cc:203: // Only record a metric for the initial disconnect event I think we should record the event when we declare losing connectivity, in line 212.
https://codereview.chromium.org/1974723002/diff/20001/chromecast/net/connecti... File chromecast/net/connectivity_checker_impl.cc (right): https://codereview.chromium.org/1974723002/diff/20001/chromecast/net/connecti... chromecast/net/connectivity_checker_impl.cc:49: "Network.Connectivity.ErrorType"; On 2016/05/12 22:48:52, wzhong wrote: > Network.ConnectivityChecking.ErrorType Done. https://codereview.chromium.org/1974723002/diff/20001/chromecast/net/connecti... chromecast/net/connectivity_checker_impl.cc:203: // Only record a metric for the initial disconnect event On 2016/05/12 22:48:52, wzhong wrote: > I think we should record the event when we declare losing connectivity, in line > 212. Done.
lgtm
On 2016/05/13 03:29:30, wzhong wrote: > lgtm lgtm
The CQ bit was checked by bcf@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1974723002/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1974723002/60001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: cast_shell_android on tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)
The CQ bit was checked by bcf@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from wzhong@chromium.org, halliwell@chromium.org Link to the patchset: https://codereview.chromium.org/1974723002/#ps80001 (title: "Fix GN dependency")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1974723002/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1974723002/80001
Message was sent while issue was closed.
Description was changed from ========== [Chromecast] Add metric reporting to ConnectivityCheckerImpl BUG=internal b/28294837 TEST=Manual testing ========== to ========== [Chromecast] Add metric reporting to ConnectivityCheckerImpl BUG=internal b/28294837 TEST=Manual testing ==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001)
Message was sent while issue was closed.
Description was changed from ========== [Chromecast] Add metric reporting to ConnectivityCheckerImpl BUG=internal b/28294837 TEST=Manual testing ========== to ========== [Chromecast] Add metric reporting to ConnectivityCheckerImpl BUG=internal b/28294837 TEST=Manual testing Committed: https://crrev.com/aedc70831b5008e78e036265d60a739ee3393b62 Cr-Commit-Position: refs/heads/master@{#393640} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/aedc70831b5008e78e036265d60a739ee3393b62 Cr-Commit-Position: refs/heads/master@{#393640} |
