Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
312653003:
Adding bluetooth histograms for NCN.CM histograms (Closed)
|
Created:
6 years, 6 months ago by jkarlin Modified:
6 years, 6 months ago CC:
chromium-reviews, asvitkine+watch_chromium.org, jar+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@bluetooth_histograms Visibility:
Public. |
DescriptionAdding bluetooth histograms for NCN.CM histograms.
This is part of the addition of bluetooth to the NetworkChangeNotifier.
BUG=378785
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=276648
Patch Set 1 #
Total comments: 10
Patch Set 2 : Added clarification to summary of FastestRTT histograms #Patch Set 3 : Updated comments to other histograms #
Messages
Total messages: 19 (0 generated)
lgtm
It seems like you might only be recording these metrics when the connection changes. If so, it seems likely that you'll be missing data from the most active users, who don't tend to change away from this connection type. Is that intentional? https://codereview.chromium.org/312653003/diff/1/tools/metrics/histograms/his... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/312653003/diff/1/tools/metrics/histograms/his... tools/metrics/histograms/histograms.xml:11333: + connection, before the NetworkChangeNotifier detected a connectivity change. Please document when this metric is recorded. Is it only when the network changes? Once per session, for some definition of session? When a new fastest RTT is observed? Something else? https://codereview.chromium.org/312653003/diff/1/tools/metrics/histograms/his... tools/metrics/histograms/histograms.xml:11461: + before the NetworkChangeNotifier detected a connectivity change. Ditto. https://codereview.chromium.org/312653003/diff/1/tools/metrics/histograms/his... tools/metrics/histograms/histograms.xml:11526: + NetworkChangeNotifier detected a connectivity change. Ditto. https://codereview.chromium.org/312653003/diff/1/tools/metrics/histograms/his... tools/metrics/histograms/histograms.xml:11591: + NetworkChangeNotifier detected a connectivity change. Ditto.
We are missing data from users whose connection type never changes. We are also biasing towards users whose connection type changes frequently. We were mostly interested in mobile users, whose connection type tends to change very frequently. I think we accepted all these inadequacies in reporting as the stats themselves are generally rough estimates. https://codereview.chromium.org/312653003/diff/1/tools/metrics/histograms/his... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/312653003/diff/1/tools/metrics/histograms/his... tools/metrics/histograms/histograms.xml:11333: + connection, before the NetworkChangeNotifier detected a connectivity change. On 2014/06/03 19:19:16, Ilya Sherman wrote: > Please document when this metric is recorded. Is it only when the network > changes? Once per session, for some definition of session? When a new fastest > RTT is observed? Something else? I thought the "...fastest...seen on a XYZ connection, before the NetworkChangeNotifier detected a connectivity change." implied that it was recorded when the NetworkChangeNotifier detected a connectivity change. If it were not recorded at the end of the specified time period there would be no way to know it's the fastest during that period. Would you like me to make it more explicit like: "...seen on a XYZ connection, before the NetworkChangeNotifier detected a connectivity change at which point the metric is recorded." Or maybe make it a separate sentence: "This metric is recorded when the NetworkChangeNotifier detects a connectivity change. This will miss data from users whose connection type never changes and will be biased to users whose connection type changes frequently."
LGTM with clarifications added. Thanks. https://codereview.chromium.org/312653003/diff/1/tools/metrics/histograms/his... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/312653003/diff/1/tools/metrics/histograms/his... tools/metrics/histograms/histograms.xml:11333: + connection, before the NetworkChangeNotifier detected a connectivity change. On 2014/06/09 16:17:54, pauljensen wrote: > On 2014/06/03 19:19:16, Ilya Sherman wrote: > > Please document when this metric is recorded. Is it only when the network > > changes? Once per session, for some definition of session? When a new > fastest > > RTT is observed? Something else? > > I thought the "...fastest...seen on a XYZ connection, before the > NetworkChangeNotifier detected a connectivity change." implied that it was > recorded when the NetworkChangeNotifier detected a connectivity change. If it > were not recorded at the end of the specified time period there would be no way > to know it's the fastest during that period. Would you like me to make it more > explicit like: > "...seen on a XYZ connection, before the NetworkChangeNotifier detected a > connectivity change at which point the metric is recorded." > Or maybe make it a separate sentence: > "This metric is recorded when the NetworkChangeNotifier detects a connectivity > change. This will miss data from users whose connection type never changes and > will be biased to users whose connection type changes frequently." I'd prefer that you add it as an extra sentence. It's nice to specifically call out potential sources of bias in histogram descriptions, so that anyone viewing the histogram will have a better chance of remembering to watch out for this bias.
Added comment to all of the affected histograms. https://codereview.chromium.org/312653003/diff/1/tools/metrics/histograms/his... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/312653003/diff/1/tools/metrics/histograms/his... tools/metrics/histograms/histograms.xml:11333: + connection, before the NetworkChangeNotifier detected a connectivity change. On 2014/06/09 22:14:48, Ilya Sherman wrote: > On 2014/06/09 16:17:54, pauljensen wrote: > > On 2014/06/03 19:19:16, Ilya Sherman wrote: > > > Please document when this metric is recorded. Is it only when the network > > > changes? Once per session, for some definition of session? When a new > > fastest > > > RTT is observed? Something else? > > > > I thought the "...fastest...seen on a XYZ connection, before the > > NetworkChangeNotifier detected a connectivity change." implied that it was > > recorded when the NetworkChangeNotifier detected a connectivity change. If it > > were not recorded at the end of the specified time period there would be no > way > > to know it's the fastest during that period. Would you like me to make it > more > > explicit like: > > "...seen on a XYZ connection, before the NetworkChangeNotifier detected a > > connectivity change at which point the metric is recorded." > > Or maybe make it a separate sentence: > > "This metric is recorded when the NetworkChangeNotifier detects a connectivity > > change. This will miss data from users whose connection type never changes > and > > will be biased to users whose connection type changes frequently." > > I'd prefer that you add it as an extra sentence. It's nice to specifically call > out potential sources of bias in histogram descriptions, so that anyone viewing > the histogram will have a better chance of remembering to watch out for this > bias. Done. https://codereview.chromium.org/312653003/diff/1/tools/metrics/histograms/his... tools/metrics/histograms/histograms.xml:11461: + before the NetworkChangeNotifier detected a connectivity change. On 2014/06/03 19:19:16, Ilya Sherman wrote: > Ditto. Done. https://codereview.chromium.org/312653003/diff/1/tools/metrics/histograms/his... tools/metrics/histograms/histograms.xml:11526: + NetworkChangeNotifier detected a connectivity change. On 2014/06/03 19:19:16, Ilya Sherman wrote: > Ditto. Done. https://codereview.chromium.org/312653003/diff/1/tools/metrics/histograms/his... tools/metrics/histograms/histograms.xml:11591: + NetworkChangeNotifier detected a connectivity change. On 2014/06/03 19:19:16, Ilya Sherman wrote: > Ditto. Done.
The CQ bit was checked by jkarlin@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/jkarlin@chromium.org/312653003/30001
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: android_dbg_triggered_tests on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/android_dbg_triggered...) win_chromium_x64_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_x64_rel/...)
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_chromium_x64_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_x64_rel/...)
The CQ bit was checked by jkarlin@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/jkarlin@chromium.org/312653003/30001
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: win_chromium_x64_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_x64_rel/...)
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_chromium_x64_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_x64_rel/...)
The CQ bit was checked by jkarlin@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/jkarlin@chromium.org/312653003/30001
Message was sent while issue was closed.
Change committed as 276648 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
