Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2910313002:
[Android WebAPK] Add UMA histogram to record error code when install fails pt 2/3 (Closed)
|
Created:
3 years, 6 months ago by pkotwicz Modified:
3 years, 6 months ago CC:
chromium-reviews, dominickn+watch_chromium.org, pkotwicz+watch_chromium.org, zpeng+watch_chromium.org, asvitkine+watch_chromium.org, agrieve+watch_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
Description[Android WebAPK] Add UMA histogram to record error code when install fails pt 2/3
This CL adds a new UMA histogram WebApk.Install.GooglePlayInstallErrorCode to
record the error code when a WebAPK install fails.
The metric is temporary. Once we know which error codes are of interest, we will
add new enum entries to WebApkGooglePlayInstallResult.
BUG=727893
R=hanxi,dominickn,isherman
TBR=yfriedman for WebApkUma.java
Review-Url: https://codereview.chromium.org/2910313002
Cr-Commit-Position: refs/heads/master@{#478054}
Committed: https://chromium.googlesource.com/chromium/src/+/0a73bd712e140cc2307ec379d364533b653962e6
Patch Set 1 #
Total comments: 1
Patch Set 2 : Merge branch 'master' into more_uma #
Total comments: 3
Patch Set 3 : Merge branch 'master' into more_uma #
Total comments: 7
Patch Set 4 : Merge branch 'master' into more_uma #
Messages
Total messages: 34 (13 generated)
pkotwicz@chromium.org changed reviewers: + hanxi@chromium.org
Xi can you please take a look?
https://codereview.chromium.org/2910313002/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/webapps/GooglePlayWebApkInstallDelegate.java (left): https://codereview.chromium.org/2910313002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/webapps/GooglePlayWebApkInstallDelegate.java:62: void onGotInstallEvent(String packageName, @InstallerPackageEvent int event); It is better to move the definition of @InstallerPackageEvent to PlayInstallWebApkClient too.
Patchset #2 (id:20001) has been deleted
Xi, can you please take another look?
Xi: Ping!
lgtm. Sorry for the late reply, since I thought I already replied:(
pkotwicz@chromium.org changed reviewers: + dominickn@chromium.org
Dominick for OWNERS
On 2017/06/05 21:00:05, pkotwicz wrote: > Dominick for OWNERS There's an internal counterpart for this right?
Yes, I have CCed you on the internal CL
https://codereview.chromium.org/2910313002/diff/40001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/metrics/WebApkUma.java (right): https://codereview.chromium.org/2910313002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/metrics/WebApkUma.java:101: "WebApk.Install.GooglePlayErrorCode", Math.min(errorCode, 1000)); Why is the Math.min needed? Do we not know what codes are going to be sent? I worry that this metric won't really be useful because we'll have a distribution over a bunch of numbers, and then have no idea what any of those numbers mean. And if we do know what they mean, it would be much better to have a metric that just records the enum.
https://codereview.chromium.org/2910313002/diff/40001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/metrics/WebApkUma.java (right): https://codereview.chromium.org/2910313002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/metrics/WebApkUma.java:101: "WebApk.Install.GooglePlayErrorCode", Math.min(errorCode, 1000)); I think that recordSparseSlowlyHistogram is designed for this situation. I wanted to avoid using an enum because there are lots of error codes. There are 61 that I know about.
lgtm % nit
Oops missed the comment. https://codereview.chromium.org/2910313002/diff/40001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/metrics/WebApkUma.java (right): https://codereview.chromium.org/2910313002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/metrics/WebApkUma.java:101: "WebApk.Install.GooglePlayErrorCode", Math.min(errorCode, 1000)); That's a lot of codes! Can you add a small comment here to say that: "Don't use an enumerated histogram since there are >60 codes that could be sent here"
Patchset #3 (id:60001) has been deleted
pkotwicz@chromium.org changed reviewers: + isherman@chromium.org
isherman@ for tools/metrics/histograms OWNERS
https://codereview.chromium.org/2910313002/diff/80001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/metrics/WebApkUma.java (right): https://codereview.chromium.org/2910313002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/metrics/WebApkUma.java:103: "WebApk.Install.GooglePlayErrorCode", Math.min(errorCode, 1000)); It's probably ok not to cap the error code, since I assume there's a reasonably finite and limited set of possibilities. Doesn't hurt to have the extra safety check, though, if all of the error codes are below 1000. https://codereview.chromium.org/2910313002/diff/80001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2910313002/diff/80001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:81162: +<histogram name="WebApk.Install.GooglePlayInstallErrorCode" units="code"> Would it make sense to instead specify an enum here, with translations for the known error codes? https://codereview.chromium.org/2910313002/diff/80001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:81167: + Records the error code when installing a WebAPK from Google Play fails. Do you also record success when the installation succeeds? If not, you might want to -- it can be nice to have a baseline.
isherman@ can you please take another look? https://codereview.chromium.org/2910313002/diff/80001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/metrics/WebApkUma.java (right): https://codereview.chromium.org/2910313002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/metrics/WebApkUma.java:103: "WebApk.Install.GooglePlayErrorCode", Math.min(errorCode, 1000)); The error codes have whacky values. I think that all of the values are between 901 and 981 https://codereview.chromium.org/2910313002/diff/80001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2910313002/diff/80001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:81162: +<histogram name="WebApk.Install.GooglePlayInstallErrorCode" units="code"> - The error codes are defined in the internal code for Google Play (com.google.android.finsky.installqueue.InstallerListener). I am unsure if I am allowed to open source their meanings. - I can add this in the future once we know which error codes are common. (Hopefully we can eradicate error codes which are common) https://codereview.chromium.org/2910313002/diff/80001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:81167: + Records the error code when installing a WebAPK from Google Play fails. The WebApk.Install.GooglePlayInstallResult histogram will tell us when the installation succeeds. The WebApk.Install.GooglePlayInstallResult histogram is recorded at the same time as WebApk.Install.GooglePlayInstallErrorCode
Metrics LGTM, thanks. https://codereview.chromium.org/2910313002/diff/80001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2910313002/diff/80001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:81162: +<histogram name="WebApk.Install.GooglePlayInstallErrorCode" units="code"> On 2017/06/08 02:23:21, pkotwicz wrote: > - The error codes are defined in the internal code for Google Play > (com.google.android.finsky.installqueue.InstallerListener). I am unsure if I am > allowed to open source their meanings. > - I can add this in the future once we know which error codes are common. > (Hopefully we can eradicate error codes which are common) Okay. Could you please add this com.google pointer to the histogram summary, if it's fine to include in an open source project?
I have added a reference to the values in the internal code which records the histogram
Patchset #4 (id:100001) has been deleted
The CQ bit was checked by pkotwicz@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from dominickn@chromium.org, hanxi@chromium.org, isherman@chromium.org Link to the patchset: https://codereview.chromium.org/2910313002/#ps120001 (title: "Merge branch 'master' into more_uma")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
Description was changed from ========== [Android WebAPK] Add UMA histogram to record error code when install fails pt 2/3 This CL adds a new UMA histogram WebApk.Install.GooglePlayInstallErrorCode to record the error code when a WebAPK install fails. The metric is temporary. Once we know which error codes are of interest, we will add new enum entries to WebApkGooglePlayInstallResult. BUG=727893 ========== to ========== [Android WebAPK] Add UMA histogram to record error code when install fails pt 2/3 This CL adds a new UMA histogram WebApk.Install.GooglePlayInstallErrorCode to record the error code when a WebAPK install fails. The metric is temporary. Once we know which error codes are of interest, we will add new enum entries to WebApkGooglePlayInstallResult. BUG=727893 R=hanxi,dominickn,isherman TBR=yfriedman for WebApkUma.java ==========
The CQ bit was checked by pkotwicz@chromium.org
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 120001, "attempt_start_ts": 1496946238144080,
"parent_rev": "242c8082ae66fee2667469a64c3e4b91205fe75a", "commit_rev":
"0a73bd712e140cc2307ec379d364533b653962e6"}
Message was sent while issue was closed.
Description was changed from ========== [Android WebAPK] Add UMA histogram to record error code when install fails pt 2/3 This CL adds a new UMA histogram WebApk.Install.GooglePlayInstallErrorCode to record the error code when a WebAPK install fails. The metric is temporary. Once we know which error codes are of interest, we will add new enum entries to WebApkGooglePlayInstallResult. BUG=727893 R=hanxi,dominickn,isherman TBR=yfriedman for WebApkUma.java ========== to ========== [Android WebAPK] Add UMA histogram to record error code when install fails pt 2/3 This CL adds a new UMA histogram WebApk.Install.GooglePlayInstallErrorCode to record the error code when a WebAPK install fails. The metric is temporary. Once we know which error codes are of interest, we will add new enum entries to WebApkGooglePlayInstallResult. BUG=727893 R=hanxi,dominickn,isherman TBR=yfriedman for WebApkUma.java Review-Url: https://codereview.chromium.org/2910313002 Cr-Commit-Position: refs/heads/master@{#478054} Committed: https://chromium.googlesource.com/chromium/src/+/0a73bd712e140cc2307ec379d364... ==========
Message was sent while issue was closed.
Committed patchset #4 (id:120001) as https://chromium.googlesource.com/chromium/src/+/0a73bd712e140cc2307ec379d364... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
