|
|
Created:
5 years, 10 months ago by Walter Cacau Modified:
5 years, 9 months ago CC:
chromium-reviews, estade+watch_chromium.org, browser-components-watch_chromium.org, rouslan+autofillwatch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionCollecting UMA metrics for the UnmaskPrompt.
BUG=454140
Committed: https://crrev.com/1b447a9e6add5a07a3f2357388aeedf08ef53f27
Cr-Commit-Position: refs/heads/master@{#321319}
Patch Set 1 #Patch Set 2 : Moving collection to CardUnmaskPromptControllerImpl. #
Total comments: 2
Patch Set 3 : Adding first test #Patch Set 4 : #
Total comments: 16
Patch Set 5 : review comments #
Total comments: 8
Patch Set 6 : review comments 2 #
Total comments: 3
Patch Set 7 : isherman comments #
Total comments: 7
Patch Set 8 : estade comments #
Total comments: 4
Patch Set 9 : #Patch Set 10 : rebasing #Patch Set 11 : fixing build #Patch Set 12 : fix android build #
Messages
Total messages: 30 (10 generated)
waltercacau@chromium.org changed reviewers: + estade@chromium.org
approach looks fine. ping me when ready for full review https://codereview.chromium.org/949323002/diff/20001/components/autofill/core... File components/autofill/core/browser/autofill_metrics.h (right): https://codereview.chromium.org/949323002/diff/20001/components/autofill/core... components/autofill/core/browser/autofill_metrics.h:305: UNMASK_PROMPT_LOCAL_SAVE_KEPT_OPT_IN, this name doesn't quite make sense to me. You can't keep opted in. It's more like "didn't opt out".
waltercacau@chromium.org changed reviewers: + isherman@chromium.org
Ready for review https://codereview.chromium.org/949323002/diff/20001/components/autofill/core... File components/autofill/core/browser/autofill_metrics.h (right): https://codereview.chromium.org/949323002/diff/20001/components/autofill/core... components/autofill/core/browser/autofill_metrics.h:305: UNMASK_PROMPT_LOCAL_SAVE_KEPT_OPT_IN, On 2015/03/17 01:49:15, Evan Stade wrote: > this name doesn't quite make sense to me. You can't keep opted in. It's more > like "didn't opt out". Done.
good stuff. I don't understand why histograms.xml appears to have so many unrelated changes. https://codereview.chromium.org/949323002/diff/60001/chrome/browser/ui/autofi... File chrome/browser/ui/autofill/card_unmask_prompt_controller_impl.cc (right): https://codereview.chromium.org/949323002/diff/60001/chrome/browser/ui/autofi... chrome/browser/ui/autofill/card_unmask_prompt_controller_impl.cc:109: if (unmasking_number_of_attempts_ > 0) { nit: use ternary operator https://codereview.chromium.org/949323002/diff/60001/chrome/browser/ui/autofi... chrome/browser/ui/autofill/card_unmask_prompt_controller_impl.cc:120: ->GetBoolean(prefs::kAutofillWalletImportStorageCheckboxState); final_should_store_pan needs to come from the param in OnUnmaskResponse. (We don't really care if they uncheck the box and press cancel.) https://codereview.chromium.org/949323002/diff/60001/chrome/browser/ui/autofi... chrome/browser/ui/autofill/card_unmask_prompt_controller_impl.cc:122: AutofillMetrics::LogUnmaskPromptEvent( nit: pull out LogUnmaskPromptEvent so it looks like if (foo && bar) event = AutofillMetrics::UNMASK_PROMPT_LOCAL_SAVE_DID_NOT_OPT_OUT; else if... https://codereview.chromium.org/949323002/diff/60001/chrome/browser/ui/autofi... File chrome/browser/ui/autofill/card_unmask_prompt_controller_impl_unittest.cc (right): https://codereview.chromium.org/949323002/diff/60001/chrome/browser/ui/autofi... chrome/browser/ui/autofill/card_unmask_prompt_controller_impl_unittest.cc:147: controller()->OnUnmaskResponse(base::UTF8ToUTF16("123"), nit: ASCIIToUTF16 https://codereview.chromium.org/949323002/diff/60001/components/autofill/core... File components/autofill/core/browser/autofill_metrics.cc (right): https://codereview.chromium.org/949323002/diff/60001/components/autofill/core... components/autofill/core/browser/autofill_metrics.cc:346: NUM_UNMASK_PROMT_EVENTS); PROMPT https://codereview.chromium.org/949323002/diff/60001/components/autofill/core... File components/autofill/core/browser/autofill_metrics.h (right): https://codereview.chromium.org/949323002/diff/60001/components/autofill/core... components/autofill/core/browser/autofill_metrics.h:295: UNMASK_PROMPT_CLOSED_FAILED_TO_UNMASK, I think it would be nice to have: - closed without trying - closed after trying, with an allow_retry failure present - closed after trying, with a !allow_retry failure present - saved successfully on the first try - saved successfully, after an allow_retry failure present (there are basically two kinds of error right now: ones that permit retry and ones that don't)
I think I was able to fix histograms.xml. Apparently running pretty_print.py caused the unrelated changes. https://codereview.chromium.org/949323002/diff/60001/chrome/browser/ui/autofi... File chrome/browser/ui/autofill/card_unmask_prompt_controller_impl.cc (right): https://codereview.chromium.org/949323002/diff/60001/chrome/browser/ui/autofi... chrome/browser/ui/autofill/card_unmask_prompt_controller_impl.cc:109: if (unmasking_number_of_attempts_ > 0) { On 2015/03/18 01:47:59, Evan Stade wrote: > nit: use ternary operator Done. https://codereview.chromium.org/949323002/diff/60001/chrome/browser/ui/autofi... chrome/browser/ui/autofill/card_unmask_prompt_controller_impl.cc:120: ->GetBoolean(prefs::kAutofillWalletImportStorageCheckboxState); On 2015/03/18 01:47:59, Evan Stade wrote: > final_should_store_pan needs to come from the param in OnUnmaskResponse. (We > don't really care if they uncheck the box and press cancel.) Shouldn't we care? IMHO, it is changing chrome state and it would be nice to track. note: just opening the popup and closing will not trigger these events as there was no attempt, but after the first attempt we will start recording because we had a chance to save the pref. https://codereview.chromium.org/949323002/diff/60001/chrome/browser/ui/autofi... chrome/browser/ui/autofill/card_unmask_prompt_controller_impl.cc:122: AutofillMetrics::LogUnmaskPromptEvent( On 2015/03/18 01:47:59, Evan Stade wrote: > nit: pull out LogUnmaskPromptEvent so it looks like > > if (foo && bar) > event = AutofillMetrics::UNMASK_PROMPT_LOCAL_SAVE_DID_NOT_OPT_OUT; > else if... Done. https://codereview.chromium.org/949323002/diff/60001/chrome/browser/ui/autofi... File chrome/browser/ui/autofill/card_unmask_prompt_controller_impl_unittest.cc (right): https://codereview.chromium.org/949323002/diff/60001/chrome/browser/ui/autofi... chrome/browser/ui/autofill/card_unmask_prompt_controller_impl_unittest.cc:147: controller()->OnUnmaskResponse(base::UTF8ToUTF16("123"), On 2015/03/18 01:47:59, Evan Stade wrote: > nit: ASCIIToUTF16 Done. https://codereview.chromium.org/949323002/diff/60001/components/autofill/core... File components/autofill/core/browser/autofill_metrics.cc (right): https://codereview.chromium.org/949323002/diff/60001/components/autofill/core... components/autofill/core/browser/autofill_metrics.cc:346: NUM_UNMASK_PROMT_EVENTS); On 2015/03/18 01:47:59, Evan Stade wrote: > PROMPT Done. https://codereview.chromium.org/949323002/diff/60001/components/autofill/core... components/autofill/core/browser/autofill_metrics.cc:346: NUM_UNMASK_PROMT_EVENTS); On 2015/03/18 01:47:59, Evan Stade wrote: > PROMPT Done. https://codereview.chromium.org/949323002/diff/60001/components/autofill/core... File components/autofill/core/browser/autofill_metrics.h (right): https://codereview.chromium.org/949323002/diff/60001/components/autofill/core... components/autofill/core/browser/autofill_metrics.h:295: UNMASK_PROMPT_CLOSED_FAILED_TO_UNMASK, On 2015/03/18 01:47:59, Evan Stade wrote: > I think it would be nice to have: > > - closed without trying > - closed after trying, with an allow_retry failure present > - closed after trying, with a !allow_retry failure present > - saved successfully on the first try > - saved successfully, after an allow_retry failure present > > (there are basically two kinds of error right now: ones that permit retry and > ones that don't) nice suggestion :) Done
https://codereview.chromium.org/949323002/diff/60001/chrome/browser/ui/autofi... File chrome/browser/ui/autofill/card_unmask_prompt_controller_impl.cc (right): https://codereview.chromium.org/949323002/diff/60001/chrome/browser/ui/autofi... chrome/browser/ui/autofill/card_unmask_prompt_controller_impl.cc:70: AutofillMetrics::LogUnmaskPromptEvent( you can move this logging down to OnUnmaskDialogClosed so that (almost) all logging is concentrated in one location https://codereview.chromium.org/949323002/diff/60001/chrome/browser/ui/autofi... chrome/browser/ui/autofill/card_unmask_prompt_controller_impl.cc:120: ->GetBoolean(prefs::kAutofillWalletImportStorageCheckboxState); On 2015/03/18 17:04:54, Walter Cacau wrote: > On 2015/03/18 01:47:59, Evan Stade wrote: > > final_should_store_pan needs to come from the param in OnUnmaskResponse. (We > > don't really care if they uncheck the box and press cancel.) > > Shouldn't we care? IMHO, it is changing chrome state and it would be nice to > track. > > note: just opening the popup and closing will not trigger these events as there > was no attempt, but after the first attempt we will start recording because we > had a chance to save the pref. ok https://codereview.chromium.org/949323002/diff/80001/chrome/browser/ui/autofi... File chrome/browser/ui/autofill/card_unmask_prompt_controller_impl.cc (right): https://codereview.chromium.org/949323002/diff/80001/chrome/browser/ui/autofi... chrome/browser/ui/autofill/card_unmask_prompt_controller_impl.cc:119: ::UNMASK_PROMPT_CLOSED_FAILED_TO_UNMASK_RETRIABLE_FAILURE; on further reflection, we should perhaps log exactly which kind of failure (AutofillClient::GetRealPanResult) was last returned. That will tell us for example how often we get network errors vs. non-network permanent failures. saving the getrealpanresult also lets us condense unmasking_success_ and unmasking_allow_retry_ into one member var
https://codereview.chromium.org/949323002/diff/80001/tools/metrics/histograms... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/949323002/diff/80001/tools/metrics/histograms... tools/metrics/histograms/histograms.xml:2060: + <summary>Events tracking the usage of the unmasking prompt.</summary> nit: Please help users unfamiliar with this feature better understand this metric, by briefly explaining what triggers an unmasking prompt. https://codereview.chromium.org/949323002/diff/80001/tools/metrics/histograms... tools/metrics/histograms/histograms.xml:44820: + <int value="9" label="Did opt out to local save"/> nit: "opt out to" -> "opt out of" https://codereview.chromium.org/949323002/diff/80001/tools/metrics/histograms... tools/metrics/histograms/histograms.xml:44822: + label="Did not opt out of local save (local save stayed enabled)"/> Optional: Shorter labels tend to be better for readability, given the layout of the dashboard. For these labels in particular, I'd suggest something like: <int value="10" label="Accepted local save">Local save stayed enabled</int> The part that isn't the label will only be shown on hover, but that seems okay given that it's just rephrasing the text already present in the label. Picking another pair of long labels above, I'd suggest something like: <int value="4" label="Closed, success w/ 0 failures">Closed with a successful unmask on the first attempt.<int/> <int value="5" label="Closed, success w/ 1+ failures">Closed with a successful unmask after at least one failed attempt.<int/>
https://codereview.chromium.org/949323002/diff/60001/chrome/browser/ui/autofi... File chrome/browser/ui/autofill/card_unmask_prompt_controller_impl.cc (right): https://codereview.chromium.org/949323002/diff/60001/chrome/browser/ui/autofi... chrome/browser/ui/autofill/card_unmask_prompt_controller_impl.cc:70: AutofillMetrics::LogUnmaskPromptEvent( On 2015/03/18 21:29:07, Evan Stade wrote: > you can move this logging down to OnUnmaskDialogClosed so that (almost) all > logging is concentrated in one location Done. Also extracted that code to a method to make it clearer that it is only logging code. https://codereview.chromium.org/949323002/diff/80001/chrome/browser/ui/autofi... File chrome/browser/ui/autofill/card_unmask_prompt_controller_impl.cc (right): https://codereview.chromium.org/949323002/diff/80001/chrome/browser/ui/autofi... chrome/browser/ui/autofill/card_unmask_prompt_controller_impl.cc:119: ::UNMASK_PROMPT_CLOSED_FAILED_TO_UNMASK_RETRIABLE_FAILURE; On 2015/03/18 21:29:07, Evan Stade wrote: > on further reflection, we should perhaps log exactly which kind of failure > (AutofillClient::GetRealPanResult) was last returned. That will tell us for > example how often we get network errors vs. non-network permanent failures. > > saving the getrealpanresult also lets us condense unmasking_success_ and > unmasking_allow_retry_ into one member var Done. https://codereview.chromium.org/949323002/diff/80001/tools/metrics/histograms... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/949323002/diff/80001/tools/metrics/histograms... tools/metrics/histograms/histograms.xml:2060: + <summary>Events tracking the usage of the unmasking prompt.</summary> On 2015/03/18 22:48:55, Ilya Sherman wrote: > nit: Please help users unfamiliar with this feature better understand this > metric, by briefly explaining what triggers an unmasking prompt. Done. https://codereview.chromium.org/949323002/diff/80001/tools/metrics/histograms... tools/metrics/histograms/histograms.xml:44820: + <int value="9" label="Did opt out to local save"/> On 2015/03/18 22:48:55, Ilya Sherman wrote: > nit: "opt out to" -> "opt out of" Done. https://codereview.chromium.org/949323002/diff/80001/tools/metrics/histograms... tools/metrics/histograms/histograms.xml:44822: + label="Did not opt out of local save (local save stayed enabled)"/> On 2015/03/18 22:48:54, Ilya Sherman wrote: > Optional: Shorter labels tend to be better for readability, given the layout of > the dashboard. For these labels in particular, I'd suggest something like: > > <int value="10" label="Accepted local save">Local save stayed enabled</int> > > The part that isn't the label will only be shown on hover, but that seems okay > given that it's just rephrasing the text already present in the label. > > Picking another pair of long labels above, I'd suggest something like: > > <int value="4" label="Closed, success w/ 0 failures">Closed with a successful > unmask on the first attempt.<int/> > <int value="5" label="Closed, success w/ 1+ failures">Closed with a successful > unmask after at least one failed attempt.<int/> Done.
Histograms LGTM. Thanks. https://codereview.chromium.org/949323002/diff/100001/components/autofill/cor... File components/autofill/core/browser/autofill_metrics.cc (right): https://codereview.chromium.org/949323002/diff/100001/components/autofill/cor... components/autofill/core/browser/autofill_metrics.cc:352: GetRealPanResult metricResult; nit: hacker_case https://codereview.chromium.org/949323002/diff/100001/components/autofill/cor... components/autofill/core/browser/autofill_metrics.cc:357: } nit: No need for curly braces. https://codereview.chromium.org/949323002/diff/100001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/949323002/diff/100001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:44837: + Closed with successful unmask after failed attempts. nit: Please be consistent here vs. three lines up, in terms of "successful" vs. "a successful"
https://codereview.chromium.org/949323002/diff/120001/chrome/browser/ui/autof... File chrome/browser/ui/autofill/card_unmask_prompt_controller_impl.cc (right): https://codereview.chromium.org/949323002/diff/120001/chrome/browser/ui/autof... chrome/browser/ui/autofill/card_unmask_prompt_controller_impl.cc:107: LogEvents(); nit: I'd call this LogOnCloseEvents or something https://codereview.chromium.org/949323002/diff/120001/chrome/browser/ui/autof... chrome/browser/ui/autofill/card_unmask_prompt_controller_impl.cc:137: ::UNMASK_PROMPT_CLOSED_FAILED_TO_UNMASK_NON_RETRIABLE_FAILURE); this still doesn't log the exact error type https://codereview.chromium.org/949323002/diff/120001/chrome/browser/ui/autof... chrome/browser/ui/autofill/card_unmask_prompt_controller_impl.cc:141: { what is the purpose of this brace
https://codereview.chromium.org/949323002/diff/120001/chrome/browser/ui/autof... File chrome/browser/ui/autofill/card_unmask_prompt_controller_impl.cc (right): https://codereview.chromium.org/949323002/diff/120001/chrome/browser/ui/autof... chrome/browser/ui/autofill/card_unmask_prompt_controller_impl.cc:107: LogEvents(); On 2015/03/19 00:30:34, Evan Stade wrote: > nit: I'd call this LogOnCloseEvents or something Done. https://codereview.chromium.org/949323002/diff/120001/chrome/browser/ui/autof... chrome/browser/ui/autofill/card_unmask_prompt_controller_impl.cc:137: ::UNMASK_PROMPT_CLOSED_FAILED_TO_UNMASK_NON_RETRIABLE_FAILURE); On 2015/03/19 00:30:34, Evan Stade wrote: > this still doesn't log the exact error type AutofillMetrics::LogRealPanResult(result); takes care of that, right? IMHO, this one is more for getting the general feeling of how the user interacted, while GetRealPanResult is logging the actual results and the frequency we see them. https://codereview.chromium.org/949323002/diff/120001/chrome/browser/ui/autof... chrome/browser/ui/autofill/card_unmask_prompt_controller_impl.cc:141: { On 2015/03/19 00:30:34, Evan Stade wrote: > what is the purpose of this brace Done.
lgtm https://codereview.chromium.org/949323002/diff/120001/chrome/browser/ui/autof... File chrome/browser/ui/autofill/card_unmask_prompt_controller_impl.cc (right): https://codereview.chromium.org/949323002/diff/120001/chrome/browser/ui/autof... chrome/browser/ui/autofill/card_unmask_prompt_controller_impl.cc:137: ::UNMASK_PROMPT_CLOSED_FAILED_TO_UNMASK_NON_RETRIABLE_FAILURE); On 2015/03/19 00:39:25, Walter Cacau wrote: > On 2015/03/19 00:30:34, Evan Stade wrote: > > this still doesn't log the exact error type > > AutofillMetrics::LogRealPanResult(result); takes care of that, right? > > IMHO, this one is more for getting the general feeling of how the user > interacted, while GetRealPanResult is logging the actual results and the > frequency we see them. ok https://codereview.chromium.org/949323002/diff/140001/chrome/browser/ui/autof... File chrome/browser/ui/autofill/card_unmask_prompt_controller_impl_unittest.cc (right): https://codereview.chromium.org/949323002/diff/140001/chrome/browser/ui/autof... chrome/browser/ui/autofill/card_unmask_prompt_controller_impl_unittest.cc:23: CreditCard CreateCard() { we should really put this in autofill_test_utils at some point https://codereview.chromium.org/949323002/diff/140001/chrome/browser/ui/autof... chrome/browser/ui/autofill/card_unmask_prompt_controller_impl_unittest.cc:56: bool allow_retry) override {} is this the output of git cl format?
https://codereview.chromium.org/949323002/diff/140001/chrome/browser/ui/autof... File chrome/browser/ui/autofill/card_unmask_prompt_controller_impl_unittest.cc (right): https://codereview.chromium.org/949323002/diff/140001/chrome/browser/ui/autof... chrome/browser/ui/autofill/card_unmask_prompt_controller_impl_unittest.cc:23: CreditCard CreateCard() { On 2015/03/19 01:06:57, Evan Stade wrote: > we should really put this in autofill_test_utils at some point Acknowledged. https://codereview.chromium.org/949323002/diff/140001/chrome/browser/ui/autof... chrome/browser/ui/autofill/card_unmask_prompt_controller_impl_unittest.cc:56: bool allow_retry) override {} On 2015/03/19 01:06:57, Evan Stade wrote: > is this the output of git cl format? nope, my editor messing with identation
The CQ bit was checked by waltercacau@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from isherman@chromium.org, estade@chromium.org Link to the patchset: https://codereview.chromium.org/949323002/#ps180001 (title: "rebasing")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/949323002/180001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_gn_rel on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by waltercacau@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from isherman@chromium.org, estade@chromium.org Link to the patchset: https://codereview.chromium.org/949323002/#ps200001 (title: "fixing build")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/949323002/200001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_compile_rel on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/android_compile...)
The CQ bit was checked by waltercacau@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from isherman@chromium.org, estade@chromium.org Link to the patchset: https://codereview.chromium.org/949323002/#ps220001 (title: "fix android build")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/949323002/220001
Message was sent while issue was closed.
Committed patchset #12 (id:220001)
Message was sent while issue was closed.
Patchset 12 (id:??) landed as https://crrev.com/1b447a9e6add5a07a3f2357388aeedf08ef53f27 Cr-Commit-Position: refs/heads/master@{#321319} |