Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2678903002:
[Payments] Only record abort metrics if the Payment Request was shown. (Closed)
|
Created:
3 years, 10 months ago by sebsg Modified:
3 years, 10 months ago CC:
chromium-reviews, rouslan+payments_chromium.org, sebsg+paymentswatch_chromium.org, gogerald+paymentswatch_chromium.org, agrieve+watch_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Description[Payments] Only record abort metrics if the Payment Request was shown.
Also move the metrics of event that prevent showing the payment request to their separate bucket.
BUG=689111, 689232
Review-Url: https://codereview.chromium.org/2678903002
Cr-Commit-Position: refs/heads/master@{#449325}
Committed: https://chromium.googlesource.com/chromium/src/+/752b06a473dd6f9a4072c9412431a90aa70f6f81
Patch Set 1 #
Total comments: 10
Patch Set 2 : Addressed comments #
Total comments: 4
Patch Set 3 : Nit in histograms.xml #
Total comments: 2
Patch Set 4 : Changed histogram label #Patch Set 5 : Rebase #
Messages
Total messages: 30 (17 generated)
Patchset #1 (id:1) has been deleted
Description was changed from ========== [Payments] Only record abort metrics if the Payment Request was shown. BUG= ========== to ========== [Payments] Only record abort metrics if the Payment Request was shown. BUG=689111 ==========
sebsg@chromium.org changed reviewers: + rouslan@chromium.org
Hi Rouslan, PTAL?
https://codereview.chromium.org/2678903002/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentRequestImpl.java (right): https://codereview.chromium.org/2678903002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentRequestImpl.java:530: mShouldRecordAbortReason = true; Should not be here, because the user may not be seeing the payment UI at this point yet. At this point the user might be looking at "Loading..." screen, while Chrome is talking to the payment apps. https://codereview.chromium.org/2678903002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentRequestImpl.java:540: assert !mPaymentMethodsSection.isEmpty(); "mShouldRecordAbortReason = true;" should also be here for the "ui-skip" case. This is the time when the user is able to interact with the UI and decide to proceed with transaction or cancel it. https://codereview.chromium.org/2678903002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentRequestImpl.java:809: private void providePaymentInformation() { "mShouldRecordAbortReason = true;" should be here, because this is when the UI shows users information (address, cards, contact info...) and lets user user cancel out of the UI.
https://codereview.chromium.org/2678903002/diff/20001/chrome/android/javatest... File chrome/android/javatests/src/org/chromium/chrome/browser/payments/PaymentRequestMetricsTest.java (right): https://codereview.chromium.org/2678903002/diff/20001/chrome/android/javatest... chrome/android/javatests/src/org/chromium/chrome/browser/payments/PaymentRequestMetricsTest.java:204: * Expect only the ABORT_REASON_NO_SUPPORTED_PAYMENT_METHOD enum value gets logged when a Update the comment. https://codereview.chromium.org/2678903002/diff/20001/chrome/android/javatest... chrome/android/javatests/src/org/chromium/chrome/browser/payments/PaymentRequestMetricsTest.java:204: * Expect only the ABORT_REASON_NO_SUPPORTED_PAYMENT_METHOD enum value gets logged when a Would still be good to measure the number of users without supported payment methods, even though that's outside of the "user abort" bucket.
Patchset #2 (id:40001) has been deleted
Patchset #2 (id:60001) has been deleted
Thanks for the comments, I ended up adding the other histogram in this CL as it makes the purpose clearer. Could you take another look? Thanks! https://codereview.chromium.org/2678903002/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentRequestImpl.java (right): https://codereview.chromium.org/2678903002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentRequestImpl.java:530: mShouldRecordAbortReason = true; On 2017/02/06 20:55:32, rouslan wrote: > Should not be here, because the user may not be seeing the payment UI at this > point yet. At this point the user might be looking at "Loading..." screen, while > Chrome is talking to the payment apps. Done. https://codereview.chromium.org/2678903002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentRequestImpl.java:540: assert !mPaymentMethodsSection.isEmpty(); On 2017/02/06 20:55:32, rouslan wrote: > "mShouldRecordAbortReason = true;" should also be here for the "ui-skip" case. > This is the time when the user is able to interact with the UI and decide to > proceed with transaction or cancel it. Done. https://codereview.chromium.org/2678903002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentRequestImpl.java:809: private void providePaymentInformation() { On 2017/02/06 20:55:32, rouslan wrote: > "mShouldRecordAbortReason = true;" should be here, because this is when the UI > shows users information (address, cards, contact info...) and lets user user > cancel out of the UI. Done. https://codereview.chromium.org/2678903002/diff/20001/chrome/android/javatest... File chrome/android/javatests/src/org/chromium/chrome/browser/payments/PaymentRequestMetricsTest.java (right): https://codereview.chromium.org/2678903002/diff/20001/chrome/android/javatest... chrome/android/javatests/src/org/chromium/chrome/browser/payments/PaymentRequestMetricsTest.java:204: * Expect only the ABORT_REASON_NO_SUPPORTED_PAYMENT_METHOD enum value gets logged when a On 2017/02/06 20:57:26, rouslan wrote: > Update the comment. Done. https://codereview.chromium.org/2678903002/diff/20001/chrome/android/javatest... chrome/android/javatests/src/org/chromium/chrome/browser/payments/PaymentRequestMetricsTest.java:204: * Expect only the ABORT_REASON_NO_SUPPORTED_PAYMENT_METHOD enum value gets logged when a On 2017/02/06 20:57:26, rouslan wrote: > Would still be good to measure the number of users without supported payment > methods, even though that's outside of the "user abort" bucket. Yep I want to move both to another bucket. I was thinking of doing it in the next CL but I'll add it in this one for clarity.
Description was changed from ========== [Payments] Only record abort metrics if the Payment Request was shown. BUG=689111 ========== to ========== [Payments] Only record abort metrics if the Payment Request was shown. BUG=689111,689232 ==========
Description was changed from ========== [Payments] Only record abort metrics if the Payment Request was shown. BUG=689111,689232 ========== to ========== [Payments] Only record abort metrics if the Payment Request was shown. Also move the metrics of event that prevent showing the payment request to their separate bucket. BUG=689111,689232 ==========
lgtm https://codereview.chromium.org/2678903002/diff/80001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentRequestMetrics.java (right): https://codereview.chromium.org/2678903002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentRequestMetrics.java:47: public static final int ABORT_REASON_OTHER = 8; I have not seen this used anywhere. What is its purpose? I plan to use it soon for inexplicable errors, if that's OK ;-) http://crrev.com/2666953005 https://codereview.chromium.org/2678903002/diff/80001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2678903002/diff/80001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:45743: + The reason that lead to the Payment Request not being shown to the user. leads
Thanks! https://codereview.chromium.org/2678903002/diff/80001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentRequestMetrics.java (right): https://codereview.chromium.org/2678903002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentRequestMetrics.java:47: public static final int ABORT_REASON_OTHER = 8; On 2017/02/06 22:42:14, rouslan wrote: > I have not seen this used anywhere. What is its purpose? I plan to use it soon > for inexplicable errors, if that's OK ;-) http://crrev.com/2666953005 That's exactly why it's there :) To be used for errors that should never happen. If at some point we see that this happens in the wild, we can spit it to be more explicit of the error. https://codereview.chromium.org/2678903002/diff/80001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2678903002/diff/80001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:45743: + The reason that lead to the Payment Request not being shown to the user. On 2017/02/06 22:42:14, rouslan wrote: > leads Done.
sebsg@chromium.org changed reviewers: + jwd@chromium.org
+ jwd@ Could you please review histograms.xml? Thanks!
LGTM with nit https://codereview.chromium.org/2678903002/diff/100001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2678903002/diff/100001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:101588: + <int value="6" label="Deprecated"/> Keep the original label, just add Deprecated.
Thanks sending to CQ https://codereview.chromium.org/2678903002/diff/100001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2678903002/diff/100001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:101588: + <int value="6" label="Deprecated"/> On 2017/02/08 21:55:23, jwd wrote: > Keep the original label, just add Deprecated. Done.
The CQ bit was checked by sebsg@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from rouslan@chromium.org, jwd@chromium.org Link to the patchset: https://codereview.chromium.org/2678903002/#ps120001 (title: "Changed histogram label")
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
Failed to apply patch for
chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentRequestImpl.java:
While running git apply --index -p1;
error: patch failed:
chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentRequestImpl.java:290
error:
chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentRequestImpl.java:
patch does not apply
Patch:
chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentRequestImpl.java
Index:
chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentRequestImpl.java
diff --git
a/chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentRequestImpl.java
b/chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentRequestImpl.java
index
a9ecebacfe4c5b09ad0ee5ac40fef86e9731d573..859a46a7401153ce814bd6c60c33c3367f7b06c9
100644
---
a/chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentRequestImpl.java
+++
b/chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentRequestImpl.java
@@ -290,6 +290,9 @@ public class PaymentRequestImpl
private boolean mQueriedCanMakePayment;
private CurrencyFormatter mCurrencyFormatter;
+ /** Aborts should only be recorded if the Payment Request was shown to the
user. */
+ private boolean mShouldRecordAbortReason;
+
/** True if any of the requested payment methods are supported. */
private boolean mArePaymentMethodsSupported;
@@ -523,8 +526,7 @@ public class PaymentRequestImpl
mContext.getCurrentTabModel().addObserver(mTabModelObserver);
if (!mShouldSkipShowingPaymentRequestUi) mUI.show();
- recordSuccessFunnelHistograms("Shown");
- mJourneyLogger.setShowCalled();
+
triggerPaymentAppUiSkipIfApplicable();
}
@@ -534,6 +536,11 @@ public class PaymentRequestImpl
if (mShouldSkipShowingPaymentRequestUi &&
isFinishedQueryingPaymentApps()
&& getIsShowing()) {
assert !mPaymentMethodsSection.isEmpty();
+
+ recordSuccessFunnelHistograms("Shown");
+ mShouldRecordAbortReason = true;
+ mJourneyLogger.setShowCalled();
+
onPayClicked(null /* selectedShippingAddress */, null /*
selectedShippingOption */,
mPaymentMethodsSection.getItem(0));
}
@@ -807,6 +814,10 @@ public class PaymentRequestImpl
new PaymentInformation(mUiShoppingCart,
mShippingAddressesSection,
mUiShippingOptions, mContactSection,
mPaymentMethodsSection));
mPaymentInformationCallback = null;
+
+ recordSuccessFunnelHistograms("Shown");
+ mShouldRecordAbortReason = true;
+ mJourneyLogger.setShowCalled();
}
@Override
@@ -1380,9 +1391,9 @@ public class PaymentRequestImpl
// must be rejected.
disconnectFromClientWithDebugMessage("Requested payment methods
have no instruments",
PaymentErrorReason.NOT_SUPPORTED);
- recordAbortReasonHistogram(mArePaymentMethodsSupported
- ?
PaymentRequestMetrics.ABORT_REASON_NO_MATCHING_PAYMENT_METHOD
- :
PaymentRequestMetrics.ABORT_REASON_NO_SUPPORTED_PAYMENT_METHOD);
+ recordNoShowReasonHistogram(mArePaymentMethodsSupported
+ ?
PaymentRequestMetrics.NO_SHOW_NO_MATCHING_PAYMENT_METHOD
+ :
PaymentRequestMetrics.NO_SHOW_NO_SUPPORTED_PAYMENT_METHOD);
if (sObserverForTest != null)
sObserverForTest.onPaymentRequestServiceShowFailed();
return true;
}
@@ -1545,7 +1556,7 @@ public class PaymentRequestImpl
*/
private void recordAbortReasonHistogram(int abortReason) {
assert abortReason < PaymentRequestMetrics.ABORT_REASON_MAX;
- if (mHasRecordedAbortReason) return;
+ if (mHasRecordedAbortReason || !mShouldRecordAbortReason) return;
mHasRecordedAbortReason = true;
RecordHistogram.recordEnumeratedHistogram(
@@ -1560,6 +1571,17 @@ public class PaymentRequestImpl
}
/**
+ * Adds an entry to the NoShow Payment Request histogram in the bucket
corresponding to the
+ * reason for not showing the Payment Request.
+ */
+ private void recordNoShowReasonHistogram(int reason) {
+ assert reason < PaymentRequestMetrics.NO_SHOW_REASON_MAX;
+
+
RecordHistogram.recordEnumeratedHistogram("PaymentRequest.CheckoutFunnel.NoShow",
reason,
+ PaymentRequestMetrics.NO_SHOW_REASON_MAX);
+ }
+
+ /**
* Compares two payment instruments by frecency.
* Return negative value if a has strictly lower frecency score than b.
* Return zero if a and b have the same frecency score.
The CQ bit was checked by sebsg@chromium.org
The CQ bit was unchecked by sebsg@chromium.org
The CQ bit was checked by sebsg@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from rouslan@chromium.org, jwd@chromium.org Link to the patchset: https://codereview.chromium.org/2678903002/#ps140001 (title: "Rebase")
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": 140001, "attempt_start_ts": 1486655590018200,
"parent_rev": "fdf3c8882b164d5bc70bc52776bdade079dfefe5", "commit_rev":
"752b06a473dd6f9a4072c9412431a90aa70f6f81"}
Message was sent while issue was closed.
Description was changed from ========== [Payments] Only record abort metrics if the Payment Request was shown. Also move the metrics of event that prevent showing the payment request to their separate bucket. BUG=689111,689232 ========== to ========== [Payments] Only record abort metrics if the Payment Request was shown. Also move the metrics of event that prevent showing the payment request to their separate bucket. BUG=689111,689232 Review-Url: https://codereview.chromium.org/2678903002 Cr-Commit-Position: refs/heads/master@{#449325} Committed: https://chromium.googlesource.com/chromium/src/+/752b06a473dd6f9a4072c9412431... ==========
Message was sent while issue was closed.
Committed patchset #5 (id:140001) as https://chromium.googlesource.com/chromium/src/+/752b06a473dd6f9a4072c9412431... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
