|
|
Created:
3 years, 9 months ago by Xi Han Modified:
3 years, 8 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. |
DescriptionAdd metrics to track Google Play install WebAPK failures.
We introduce new metric "WebApk.Install.GooglePlayInstallResult" to track
whether installing WebAPKs from Google Play succeeded, and if not the reason of
failures provided by Google Play.
The internal CL to record this metric is:
https://chrome-internal-review.googlesource.com/c/337268/
BUG=701497
Review-Url: https://codereview.chromium.org/2751913003
Cr-Commit-Position: refs/heads/master@{#460123}
Committed: https://chromium.googlesource.com/chromium/src/+/14d62f4baa9f21faf511b3f52d65a36d18fe3aad
Patch Set 1 #Patch Set 2 : pkotwicz@'s comments. #
Total comments: 12
Patch Set 3 : Nits. #
Total comments: 4
Patch Set 4 : dominickn@'s comments. #
Messages
Total messages: 28 (13 generated)
Patchset #1 (id:1) has been deleted
hanxi@chromium.org changed reviewers: + pkotwicz@chromium.org
Hi Peter, could you please take a look? Thanks!
Description was changed from ========== Add metrics to track Google Play install WebAPK failures. We introduce new metrics "WebApk.Install.GooglePlayInstallResult" to track whether installing WebAPKs from Google Play succeeded, and if not the reason of failures provided by Google Play. BUG=701497 ========== to ========== Add metrics to track Google Play install WebAPK failures. We introduce new metrics "WebApk.Install.GooglePlayInstallResult" to track whether installing WebAPKs from Google Play succeeded, and if not the reason of failures provided by Google Play. The internal CL is: https://chrome-internal-review.googlesource.com/c/337268/ BUG=701497 ==========
Added a comment to the internal CL which affects this one I think
Description was changed from ========== Add metrics to track Google Play install WebAPK failures. We introduce new metrics "WebApk.Install.GooglePlayInstallResult" to track whether installing WebAPKs from Google Play succeeded, and if not the reason of failures provided by Google Play. The internal CL is: https://chrome-internal-review.googlesource.com/c/337268/ BUG=701497 ========== to ========== Add metrics to track Google Play install WebAPK failures. We introduce new metric "WebApk.Install.GooglePlayInstallResult" to track whether installing WebAPKs from Google Play succeeded, and if not the reason of failures provided by Google Play. The internal CLs to records this metric is: https://chrome-internal-review.googlesource.com/c/337268/ BUG=701497 ==========
Description was changed from ========== Add metrics to track Google Play install WebAPK failures. We introduce new metric "WebApk.Install.GooglePlayInstallResult" to track whether installing WebAPKs from Google Play succeeded, and if not the reason of failures provided by Google Play. The internal CLs to records this metric is: https://chrome-internal-review.googlesource.com/c/337268/ BUG=701497 ========== to ========== Add metrics to track Google Play install WebAPK failures. We introduce new metric "WebApk.Install.GooglePlayInstallResult" to track whether installing WebAPKs from Google Play succeeded, and if not the reason of failures provided by Google Play. The internal CL to record this metric is: https://chrome-internal-review.googlesource.com/c/337268/ BUG=701497 ==========
PTAL, thanks!
LGTM with nits https://codereview.chromium.org/2751913003/diff/40001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/metrics/WebApkUma.java (right): https://codereview.chromium.org/2751913003/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/metrics/WebApkUma.java:86: * failures sent by Google Play. Nit: "the reason of failures" -> "the failure reason" "the reason that the install failed" is also OK https://codereview.chromium.org/2751913003/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/metrics/WebApkUma.java:91: HISTOGRAM_GOOGLE_PLAY_INSTALL_RESULT, result, GOOGLE_PLAY_INSTALL_RESULT_MAX); Nit: You can just inline the histogram name ("WebApk.Install.GooglePlayInstallResult") https://codereview.chromium.org/2751913003/diff/40001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/webapps/GooglePlayWebApkInstallDelegate.java (right): https://codereview.chromium.org/2751913003/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/webapps/GooglePlayWebApkInstallDelegate.java:56: Callback<Integer> callback); This is already in ToT :) https://codereview.chromium.org/2751913003/diff/40001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2751913003/diff/40001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:75984: + Records whether installing WebAPKs from Google Play succeeded. If not, "WebAPKs" -> "WebAPK" https://codereview.chromium.org/2751913003/diff/40001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:75985: + records the reason of failures sent by Google Play. "reason of failure" -> "failure reason" https://codereview.chromium.org/2751913003/diff/40001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:113297: + <int value="10" label="Install times out"/> Nit: "times out" -> "timed out"
hanxi@chromium.org changed reviewers: + dominickn@chromium.org
Hi Dom, could you please take a look? Thanks! https://codereview.chromium.org/2751913003/diff/40001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/metrics/WebApkUma.java (right): https://codereview.chromium.org/2751913003/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/metrics/WebApkUma.java:86: * failures sent by Google Play. On 2017/03/21 16:59:39, pkotwicz wrote: > Nit: "the reason of failures" -> "the failure reason" > > "the reason that the install failed" is also OK Done. https://codereview.chromium.org/2751913003/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/metrics/WebApkUma.java:91: HISTOGRAM_GOOGLE_PLAY_INSTALL_RESULT, result, GOOGLE_PLAY_INSTALL_RESULT_MAX); On 2017/03/21 16:59:39, pkotwicz wrote: > Nit: You can just inline the histogram name > ("WebApk.Install.GooglePlayInstallResult") Done. https://codereview.chromium.org/2751913003/diff/40001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/webapps/GooglePlayWebApkInstallDelegate.java (right): https://codereview.chromium.org/2751913003/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/webapps/GooglePlayWebApkInstallDelegate.java:56: Callback<Integer> callback); On 2017/03/21 16:59:39, pkotwicz wrote: > This is already in ToT :) Oh, yes, I didn't rebase it in time. It won't be in the new change patch. https://codereview.chromium.org/2751913003/diff/40001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2751913003/diff/40001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:75984: + Records whether installing WebAPKs from Google Play succeeded. If not, On 2017/03/21 16:59:39, pkotwicz wrote: > "WebAPKs" -> "WebAPK" "a WebAPK"? https://codereview.chromium.org/2751913003/diff/40001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:75985: + records the reason of failures sent by Google Play. On 2017/03/21 16:59:39, pkotwicz wrote: > "reason of failure" -> "failure reason" Done. https://codereview.chromium.org/2751913003/diff/40001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:113297: + <int value="10" label="Install times out"/> On 2017/03/21 16:59:39, pkotwicz wrote: > Nit: "times out" -> "timed out" Done.
https://codereview.chromium.org/2751913003/diff/60001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/metrics/WebApkUma.java (right): https://codereview.chromium.org/2751913003/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/metrics/WebApkUma.java:85: public static void recordGooglePlayIntallResult(int result) { "Install" (not "Intall") https://codereview.chromium.org/2751913003/diff/60001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkInstaller.java (right): https://codereview.chromium.org/2751913003/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkInstaller.java:113: WebApkUma.recordGooglePlayIntallResult( Should this conditional be moved after the isWebApkInstalled check? I'm thinking of cases where you've managed to get a WebAPK installed, but then you trigger another install and your Play Services isn't available. Seems like a rare case but we may as well account for it if there are no side effects.
PTAL, thanks! https://codereview.chromium.org/2751913003/diff/60001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/metrics/WebApkUma.java (right): https://codereview.chromium.org/2751913003/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/metrics/WebApkUma.java:85: public static void recordGooglePlayIntallResult(int result) { On 2017/03/22 00:15:22, dominickn wrote: > "Install" (not "Intall") Done. https://codereview.chromium.org/2751913003/diff/60001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkInstaller.java (right): https://codereview.chromium.org/2751913003/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkInstaller.java:113: WebApkUma.recordGooglePlayIntallResult( On 2017/03/22 00:15:22, dominickn wrote: > Should this conditional be moved after the isWebApkInstalled check? I'm thinking > of cases where you've managed to get a WebAPK installed, but then you trigger > another install and your Play Services isn't available. Seems like a rare case > but we may as well account for it if there are no side effects. It makes sense to me. Yes, we shouldn't records anything if the WebAPK has been installed. Moved.
lgtm, thanks
The CQ bit was checked by hanxi@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from pkotwicz@chromium.org Link to the patchset: https://codereview.chromium.org/2751913003/#ps80001 (title: "dominickn@'s comments.")
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...)
hanxi@chromium.org changed reviewers: + dfalcantara@chromium.org, isherman@chromium.org
+dfalcantara@: Please review: - chrome/android/java/src/org/chromium/chrome/browser/metrics/WebApkUma.java +isherman@: Please review: - tools/metrics/histograms/histograms.xml Thank you both!
Metrics LGTM, thanks.
lgtm
The CQ bit was checked by hanxi@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": 80001, "attempt_start_ts": 1490707713638770, "parent_rev": "71b662e14e6d11cb0584a8102fcb6006afac8698", "commit_rev": "14d62f4baa9f21faf511b3f52d65a36d18fe3aad"}
Message was sent while issue was closed.
Description was changed from ========== Add metrics to track Google Play install WebAPK failures. We introduce new metric "WebApk.Install.GooglePlayInstallResult" to track whether installing WebAPKs from Google Play succeeded, and if not the reason of failures provided by Google Play. The internal CL to record this metric is: https://chrome-internal-review.googlesource.com/c/337268/ BUG=701497 ========== to ========== Add metrics to track Google Play install WebAPK failures. We introduce new metric "WebApk.Install.GooglePlayInstallResult" to track whether installing WebAPKs from Google Play succeeded, and if not the reason of failures provided by Google Play. The internal CL to record this metric is: https://chrome-internal-review.googlesource.com/c/337268/ BUG=701497 Review-Url: https://codereview.chromium.org/2751913003 Cr-Commit-Position: refs/heads/master@{#460123} Committed: https://chromium.googlesource.com/chromium/src/+/14d62f4baa9f21faf511b3f52d65... ==========
Message was sent while issue was closed.
Committed patchset #4 (id:80001) as https://chromium.googlesource.com/chromium/src/+/14d62f4baa9f21faf511b3f52d65... |