Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2067633003:
Add UMA to measure NPEs coming from Android Wifi/Connectivity services (Closed)
|
Created:
4 years, 6 months ago by pauljensen Modified:
4 years, 5 months ago CC:
chromium-reviews, cbentzel+watch_chromium.org, asvitkine+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdd UMA to measure NPEs coming from Android Wifi/Connectivity services
The UMA being added measures how often we get NullPointerExceptions
when trying to query via WifiManager.getConnectionInfo() and
ConnectivityManager.getNetworkInfo(Network). If it fails the
first time, the call is repeated and the result is again recorded
with UMA.
BUG=592131
R=xunjieli@chromium.org,isherman@chromium.org
Committed: https://crrev.com/cff08ce07f12b978a4439dd59f83d76d82c3f9fe
Cr-Commit-Position: refs/heads/master@{#399888}
Patch Set 1 #
Total comments: 6
Patch Set 2 : address comments #
Total comments: 2
Patch Set 3 : address Helen's comments #
Total comments: 4
Patch Set 4 : address isherman comments #
Total comments: 2
Messages
Total messages: 16 (3 generated)
PTAL, thank you!
https://codereview.chromium.org/2067633003/diff/1/net/android/java/src/org/ch... File net/android/java/src/org/chromium/net/NetworkChangeNotifierAutoDetect.java (right): https://codereview.chromium.org/2067633003/diff/1/net/android/java/src/org/ch... net/android/java/src/org/chromium/net/NetworkChangeNotifierAutoDetect.java:91: // Fetch NetworkInfo and record UMA for NPEs. nit: s/Fetch/Fetches. Maybe also spell out NPE. https://codereview.chromium.org/2067633003/diff/1/net/android/java/src/org/ch... net/android/java/src/org/chromium/net/NetworkChangeNotifierAutoDetect.java:100: NetworkInfo networkInfo = mConnectivityManager.getNetworkInfo(network); Why do we do getNetworkInfo twice?
https://codereview.chromium.org/2067633003/diff/1/net/android/java/src/org/ch... File net/android/java/src/org/chromium/net/NetworkChangeNotifierAutoDetect.java (right): https://codereview.chromium.org/2067633003/diff/1/net/android/java/src/org/ch... net/android/java/src/org/chromium/net/NetworkChangeNotifierAutoDetect.java:91: // Fetch NetworkInfo and record UMA for NPEs. On 2016/06/14 13:55:06, xunjieli wrote: > nit: s/Fetch/Fetches. Maybe also spell out NPE. Done. https://codereview.chromium.org/2067633003/diff/1/net/android/java/src/org/ch... net/android/java/src/org/chromium/net/NetworkChangeNotifierAutoDetect.java:100: NetworkInfo networkInfo = mConnectivityManager.getNetworkInfo(network); On 2016/06/14 13:55:06, xunjieli wrote: > Why do we do getNetworkInfo twice? To see if trying again might be a viable solution. The idea is to test if the NPE is a random/transient/ephemeral failure.
net/ lgtm. https://codereview.chromium.org/2067633003/diff/1/net/android/java/src/org/ch... File net/android/java/src/org/chromium/net/NetworkChangeNotifierAutoDetect.java (right): https://codereview.chromium.org/2067633003/diff/1/net/android/java/src/org/ch... net/android/java/src/org/chromium/net/NetworkChangeNotifierAutoDetect.java:100: NetworkInfo networkInfo = mConnectivityManager.getNetworkInfo(network); On 2016/06/14 13:59:03, pauljensen wrote: > On 2016/06/14 13:55:06, xunjieli wrote: > > Why do we do getNetworkInfo twice? > > To see if trying again might be a viable solution. The idea is to test if the > NPE is a random/transient/ephemeral failure. I see. Could you make a quick comment here and below in getWifiInfo? https://codereview.chromium.org/2067633003/diff/20001/net/android/java/src/or... File net/android/java/src/org/chromium/net/NetworkChangeNotifierAutoDetect.java (right): https://codereview.chromium.org/2067633003/diff/20001/net/android/java/src/or... net/android/java/src/org/chromium/net/NetworkChangeNotifierAutoDetect.java:91: // Fetches NetworkInfo and record UMA for NullPointerExceptions. nit: s/record/records.
https://codereview.chromium.org/2067633003/diff/1/net/android/java/src/org/ch... File net/android/java/src/org/chromium/net/NetworkChangeNotifierAutoDetect.java (right): https://codereview.chromium.org/2067633003/diff/1/net/android/java/src/org/ch... net/android/java/src/org/chromium/net/NetworkChangeNotifierAutoDetect.java:100: NetworkInfo networkInfo = mConnectivityManager.getNetworkInfo(network); On 2016/06/14 14:10:50, xunjieli wrote: > On 2016/06/14 13:59:03, pauljensen wrote: > > On 2016/06/14 13:55:06, xunjieli wrote: > > > Why do we do getNetworkInfo twice? > > > > To see if trying again might be a viable solution. The idea is to test if the > > NPE is a random/transient/ephemeral failure. > > I see. Could you make a quick comment here and below in getWifiInfo? Done. https://codereview.chromium.org/2067633003/diff/20001/net/android/java/src/or... File net/android/java/src/org/chromium/net/NetworkChangeNotifierAutoDetect.java (right): https://codereview.chromium.org/2067633003/diff/20001/net/android/java/src/or... net/android/java/src/org/chromium/net/NetworkChangeNotifierAutoDetect.java:91: // Fetches NetworkInfo and record UMA for NullPointerExceptions. On 2016/06/14 14:10:50, xunjieli wrote: > nit: s/record/records. Done.
LGTM % nits: https://codereview.chromium.org/2067633003/diff/40001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2067633003/diff/40001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:25333: +<histogram name="NCN.getNetInfo1stSuccess" enum="BooleanSuccess"> (Just within histograms.xml), please use a custom enum with labels like "Had NullPointerException" and "No exception" https://codereview.chromium.org/2067633003/diff/40001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:25365: + NullPointerException, false if it did. For all of these histograms, I think it would be helpful to provide a bit of context -- perhaps just a bug link -- explaining *why* this is a useful thing to measure.
https://codereview.chromium.org/2067633003/diff/40001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2067633003/diff/40001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:25333: +<histogram name="NCN.getNetInfo1stSuccess" enum="BooleanSuccess"> On 2016/06/14 21:50:44, Ilya Sherman wrote: > (Just within histograms.xml), please use a custom enum with labels like "Had > NullPointerException" and "No exception" Done. https://codereview.chromium.org/2067633003/diff/40001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:25365: + NullPointerException, false if it did. On 2016/06/14 21:50:45, Ilya Sherman wrote: > For all of these histograms, I think it would be helpful to provide a bit of > context -- perhaps just a bug link -- explaining *why* this is a useful thing to > measure. Done.
The CQ bit was checked by pauljensen@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from xunjieli@chromium.org, isherman@chromium.org Link to the patchset: https://codereview.chromium.org/2067633003/#ps60001 (title: "address isherman comments")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2067633003/60001
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
CQ bit was unchecked
Message was sent while issue was closed.
Description was changed from ========== Add UMA to measure NPEs coming from Android Wifi/Connectivity services The UMA being added measures how often we get NullPointerExceptions when trying to query via WifiManager.getConnectionInfo() and ConnectivityManager.getNetworkInfo(Network). If it fails the first time, the call is repeated and the result is again recorded with UMA. BUG=592131 R=xunjieli@chromium.org,isherman@chromium.org ========== to ========== Add UMA to measure NPEs coming from Android Wifi/Connectivity services The UMA being added measures how often we get NullPointerExceptions when trying to query via WifiManager.getConnectionInfo() and ConnectivityManager.getNetworkInfo(Network). If it fails the first time, the call is repeated and the result is again recorded with UMA. BUG=592131 R=xunjieli@chromium.org,isherman@chromium.org Committed: https://crrev.com/cff08ce07f12b978a4439dd59f83d76d82c3f9fe Cr-Commit-Position: refs/heads/master@{#399888} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/cff08ce07f12b978a4439dd59f83d76d82c3f9fe Cr-Commit-Position: refs/heads/master@{#399888}
Message was sent while issue was closed.
https://codereview.chromium.org/2067633003/diff/60001/net/android/java/src/or... File net/android/java/src/org/chromium/net/NetworkChangeNotifierAutoDetect.java (right): https://codereview.chromium.org/2067633003/diff/60001/net/android/java/src/or... net/android/java/src/org/chromium/net/NetworkChangeNotifierAutoDetect.java:107: throw secondException; Helen, I think I may have messed up. If the second attempt always fails, we'll rethrow...and I imagine crash and never upload the UMA... Do you agree?
Message was sent while issue was closed.
https://codereview.chromium.org/2067633003/diff/60001/net/android/java/src/or... File net/android/java/src/org/chromium/net/NetworkChangeNotifierAutoDetect.java (right): https://codereview.chromium.org/2067633003/diff/60001/net/android/java/src/or... net/android/java/src/org/chromium/net/NetworkChangeNotifierAutoDetect.java:107: throw secondException; On 2016/07/18 17:17:22, pauljensen wrote: > Helen, I think I may have messed up. If the second attempt always fails, we'll > rethrow...and I imagine crash and never upload the UMA... Do you agree? Ah, you are right. I didn't realize that RecordHistogram.recordBooleanHistogram will not actually upload the histogram if the app dies. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
