Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
429353002:
Network performance related histograms. (Closed)
DescriptionNetwork performance related histograms.
Added a histogram "Network.Shill.DeviceConnectionStatus" for tracking the
connection status of the device. A sample is emitted once every 3
minutes.
Added a histogram "Network.Shill.Wifi.NetworkProblemDetected" for tracking
the connection problem encountered by TrafficMonitor after the wifi
connection is established. This event is reported whenever a network
problem is encountered while connected to a wifi network.
Added a histogram "NetworkShill.WiFi.UserInitiatedConnectionFailureReason"
for tracking the reasons for failed wifi connection attempts that are
manually initiated by the user. This event is reported everytime
user-initiated wifi connection attempts failed.
BUG=chromium:374274, chromium:392965, chromium:392990
TEST=Start chrome, and browse to chrome://histograms to verify the
histograms mentioned above exist.
NOTRY=true
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=287826
Patch Set 1 #
Total comments: 5
Patch Set 2 : #
Total comments: 4
Patch Set 3 : #
Messages
Total messages: 18 (0 generated)
+Jesse for histograms.xml review. Where are these metrics defined?
On 2014/07/30 21:25:16, Ilya Sherman wrote: > +Jesse for histograms.xml review. > > Where are these metrics defined? These metrics are defined in shill (chromeos network manager).
https://codereview.chromium.org/429353002/diff/1/tools/metrics/histograms/his... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/429353002/diff/1/tools/metrics/histograms/his... tools/metrics/histograms/histograms.xml:18017: +<histogram name="Network.Shill.Wifi.NetworkProblemDetected" Are there other technologies that can report this metric? https://codereview.chromium.org/429353002/diff/1/tools/metrics/histograms/his... tools/metrics/histograms/histograms.xml:18203: + user-initiated wifi connection attempts. nit: "...the reasons for...", or if you want some less ambiguity, "...the reason for each failed..." maybe.
https://codereview.chromium.org/429353002/diff/1/tools/metrics/histograms/his... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/429353002/diff/1/tools/metrics/histograms/his... tools/metrics/histograms/histograms.xml:18017: +<histogram name="Network.Shill.Wifi.NetworkProblemDetected" On 2014/07/31 15:45:07, Jesse Doherty wrote: > Are there other technologies that can report this metric? Yes, but we only track this for wifi for now. https://codereview.chromium.org/429353002/diff/1/tools/metrics/histograms/his... tools/metrics/histograms/histograms.xml:18203: + user-initiated wifi connection attempts. On 2014/07/31 15:45:07, Jesse Doherty wrote: > nit: "...the reasons for...", or if you want some less ambiguity, "...the reason > for each failed..." maybe. Done.
PTAL.
lgtm https://codereview.chromium.org/429353002/diff/1/tools/metrics/histograms/his... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/429353002/diff/1/tools/metrics/histograms/his... tools/metrics/histograms/histograms.xml:18017: +<histogram name="Network.Shill.Wifi.NetworkProblemDetected" On 2014/07/31 17:48:03, zqiu1 wrote: > On 2014/07/31 15:45:07, Jesse Doherty wrote: > > Are there other technologies that can report this metric? > Yes, but we only track this for wifi for now. To clarify, does this mean that we log a bunch of similar histograms that aren't specified in this file?
On 2014/07/31 18:34:31, Jesse Doherty wrote: > lgtm > > https://codereview.chromium.org/429353002/diff/1/tools/metrics/histograms/his... > File tools/metrics/histograms/histograms.xml (right): > > https://codereview.chromium.org/429353002/diff/1/tools/metrics/histograms/his... > tools/metrics/histograms/histograms.xml:18017: +<histogram > name="Network.Shill.Wifi.NetworkProblemDetected" > On 2014/07/31 17:48:03, zqiu1 wrote: > > On 2014/07/31 15:45:07, Jesse Doherty wrote: > > > Are there other technologies that can report this metric? > > Yes, but we only track this for wifi for now. > > To clarify, does this mean that we log a bunch of similar histograms that aren't > specified in this file? @Jesse, Thanks. @isherman, looks like Jesse is not in the owner group of this file, but you're. So I would need LGTM from you in order to be able to commit.
On 2014/07/31 18:39:15, zqiu1 wrote: > On 2014/07/31 18:34:31, Jesse Doherty wrote: > > lgtm > > > > > https://codereview.chromium.org/429353002/diff/1/tools/metrics/histograms/his... > > File tools/metrics/histograms/histograms.xml (right): > > > > > https://codereview.chromium.org/429353002/diff/1/tools/metrics/histograms/his... > > tools/metrics/histograms/histograms.xml:18017: +<histogram > > name="Network.Shill.Wifi.NetworkProblemDetected" > > On 2014/07/31 17:48:03, zqiu1 wrote: > > > On 2014/07/31 15:45:07, Jesse Doherty wrote: > > > > Are there other technologies that can report this metric? > > > Yes, but we only track this for wifi for now. > > > > To clarify, does this mean that we log a bunch of similar histograms that > aren't > > specified in this file? > > @Jesse, Thanks. Is that a yes to the clarification? > > @isherman, looks like Jesse is not in the owner group of this file, but you're. > So I would need LGTM from you in order to be able to commit.
On 2014/07/31 20:14:45, Jesse Doherty wrote: > On 2014/07/31 18:39:15, zqiu1 wrote: > > On 2014/07/31 18:34:31, Jesse Doherty wrote: > > > lgtm > > > > > > > > > https://codereview.chromium.org/429353002/diff/1/tools/metrics/histograms/his... > > > File tools/metrics/histograms/histograms.xml (right): > > > > > > > > > https://codereview.chromium.org/429353002/diff/1/tools/metrics/histograms/his... > > > tools/metrics/histograms/histograms.xml:18017: +<histogram > > > name="Network.Shill.Wifi.NetworkProblemDetected" > > > On 2014/07/31 17:48:03, zqiu1 wrote: > > > > On 2014/07/31 15:45:07, Jesse Doherty wrote: > > > > > Are there other technologies that can report this metric? > > > > Yes, but we only track this for wifi for now. > > > > > > To clarify, does this mean that we log a bunch of similar histograms that > > aren't > > > specified in this file? > > > > @Jesse, Thanks. > > Is that a yes to the clarification? > > > > > @isherman, looks like Jesse is not in the owner group of this file, but > you're. > > So I would need LGTM from you in order to be able to commit. Sorry I didn't see your previous comment. Yes, there are histograms that aren't specified in this file.
https://codereview.chromium.org/429353002/diff/20001/tools/metrics/histograms... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/429353002/diff/20001/tools/metrics/histograms... tools/metrics/histograms/histograms.xml:18017: +<histogram name="Network.Shill.Wifi.NetworkProblemDetected" Is "Wifi" spelled correctly here? It's odd that for some of the histograms you use "Wifi" and others you use "WiFi". https://codereview.chromium.org/429353002/diff/20001/tools/metrics/histograms... tools/metrics/histograms/histograms.xml:18022: + encountered by TrafficMonitor after wifi connection is established. nit: "wifi" -> "WiFi" https://codereview.chromium.org/429353002/diff/20001/tools/metrics/histograms... tools/metrics/histograms/histograms.xml:18203: + user-initiated wifi connection attempts. DItto. https://codereview.chromium.org/429353002/diff/20001/tools/metrics/histograms... tools/metrics/histograms/histograms.xml:18203: + user-initiated wifi connection attempts. When is this logged -- just on an error? What is the "success" baseline that the error rate is relative to?
On 2014/07/31 22:08:14, Ilya Sherman wrote: > https://codereview.chromium.org/429353002/diff/20001/tools/metrics/histograms... > File tools/metrics/histograms/histograms.xml (right): > > https://codereview.chromium.org/429353002/diff/20001/tools/metrics/histograms... > tools/metrics/histograms/histograms.xml:18017: +<histogram > name="Network.Shill.Wifi.NetworkProblemDetected" > Is "Wifi" spelled correctly here? It's odd that for some of the histograms you > use "Wifi" and others you use "WiFi". It is spelled correctly. Just the way is been done, sorry for the confusion. > > https://codereview.chromium.org/429353002/diff/20001/tools/metrics/histograms... > tools/metrics/histograms/histograms.xml:18022: + encountered by > TrafficMonitor after wifi connection is established. > nit: "wifi" -> "WiFi" Will Update. > > https://codereview.chromium.org/429353002/diff/20001/tools/metrics/histograms... > tools/metrics/histograms/histograms.xml:18203: + user-initiated wifi > connection attempts. > DItto. Will update. > > https://codereview.chromium.org/429353002/diff/20001/tools/metrics/histograms... > tools/metrics/histograms/histograms.xml:18203: + user-initiated wifi > connection attempts. > When is this logged -- just on an error? What is the "success" baseline that > the error rate is relative to? This is logged just on an error, the connection result is already logged as Network.Shill.WiFi.UserInitiatedConnectionResult.
On 2014/08/04 17:22:26, zqiu1 wrote: > On 2014/07/31 22:08:14, Ilya Sherman wrote: > https://codereview.chromium.org/429353002/diff/20001/tools/metrics/histograms... > > tools/metrics/histograms/histograms.xml:18203: + user-initiated wifi > > connection attempts. > > When is this logged -- just on an error? What is the "success" baseline that > > the error rate is relative to? > This is logged just on an error, the connection result is already logged as > Network.Shill.WiFi.UserInitiatedConnectionResult. Ok. Please at least include a reference to the baseline in the histogram <summary>, so that people viewing this histogram can tell what they should be comparing it against.
PTAL.
LGTM, thanks.
The CQ bit was checked by isherman@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/zqiu@chromium.org/429353002/40001
Message was sent while issue was closed.
Change committed as 287826 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
