Closed Bug 1124465 Opened 6 years ago Closed 6 years ago

Add telemetry probe for usage of the password capture dialog

Categories

(Toolkit :: Password Manager, defect, P1)

defect
Points:
3

Tracking

()

RESOLVED FIXED
mozilla39
Tracking Status
firefox38 --- fixed
firefox39 --- fixed

People

(Reporter: ally, Assigned: liuche)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 1 obsolete file)

:liuche suggests measuring the ui states.
Priority: -- → P1
Assignee: nobody → liuche
Points: --- → 3
Attached file MozReview Request: bz://1124465/liuche (obsolete) —
/r/3631 - Bug 1124465 - WIP Add telemetry probe for usage of the password capture dialog. r=MattN

Pull down this commit:

hg pull review -r 2ca8f8296bf80356136edbfc5f485ccaa5d13d85
AFAICT, there isn't a way to differentiate between "removing" the dialog by hitting the X and "dismissing" by tapping outside of the dialog - the prompt API notification lumps the two together.

Need to add UI telemetry for the Mobile telemetry dashboard, and remove some looging.
Comment on attachment 8561702 [details]
MozReview Request: bz://1124465/liuche

I'm inclined to leave off the "dismiss" probe on Desktop because the "dismissed" notification will catch the user clicking on the "X", clicking outside of the doorhanger, and also clicking "Not Now." (On mobile, "Not now" is a valid button that you can attach callbacks to.)

Is there a better way to list the telemtry enums, instead of mirroring them in mobile and desktop?
Attachment #8561702 - Flags: feedback?(MattN+bmo)
Attachment #8561702 - Attachment is obsolete: true
Attachment #8561702 - Flags: feedback?(MattN+bmo)
Comment on attachment 8561702 [details]
MozReview Request: bz://1124465/liuche

https://reviewboard.mozilla.org/r/3631/
Attachment #8561702 - Attachment is obsolete: false
Attachment #8561702 - Flags: review?(MattN+bmo)
Removed Notification bar and modal dialog telemetry measurements, and added measurement for every prompt open.
Comment on attachment 8561702 [details]
MozReview Request: bz://1124465/liuche

https://reviewboard.mozilla.org/r/3629/#review2935

::: toolkit/components/telemetry/Histograms.json
(Diff revision 1)
> +  "PWMGR_PROMPT_SELECTION_STATE": {

I would think we would want to have separate histograms for the remember vs. update prompt because as-is we can't measure the success rate of the two prompts since we can't differentiate between dismissing the remember prompt vs. dismissing the update prompt.
Attachment #8561702 - Flags: review?(MattN+bmo)
Comment on attachment 8561702 [details]
MozReview Request: bz://1124465/liuche

/r/3631 - Bug 1124465 - Add telemetry probe for usage of the password capture dialog. r=MattN

Pull down this commit:

hg pull review -r 0f61b46164d35bd2462738f0141bba41cf891084
Attachment #8561702 - Flags: review?(MattN+bmo)
I split this into two probes, and dropped the enum count down to 5 - if we're presenting more than 5 options, we're being awful. However, if we do change the options to new options, we might want to have some spare enums, but I don't think the new UI mocks have any options that are semantically different from add, update, not now, never.
Attachment #8561702 - Flags: review?(MattN+bmo)
Comment on attachment 8561702 [details]
MozReview Request: bz://1124465/liuche

https://reviewboard.mozilla.org/r/3629/#review3121

::: toolkit/components/telemetry/Histograms.json
(Diff revision 2)
> +  "PWMGR_SAVE_SELECTION_STATE": {

"SAVE_SELECTION_STATE" doesn't make me think about the action a user takes on a doorhanger. How about PWMGR_PROMPT_REMEMBER_ACTION and PWMGR_PROMPT_UPDATE_ACTION. Note that I used "remember" instead of "save" as I believe it aligns better with what we call the dialog in conversation and in the code.

::: toolkit/components/passwordmgr/nsLoginManagerPrompter.js
(Diff revision 2)
> +const PROMPT_RES_ADD = 2;
> +const PROMPT_RES_NOTNOW = 3;
> +const PROMPT_RES_NEVER = 4;

What is the significance of _RES_ in the variable names? It's not clear to me.

::: toolkit/components/passwordmgr/nsLoginManagerPrompter.js
(Diff revision 2)
> +const PROMPT_RES_DISPLAYED = 1;

I thought we wanted the count of values in the histogram to be the sum of the options the user could have chosen? What value is PROMPT_RES_DISPLAYED providing? Is this because you're not using PROMPT_RES_NOTNOW on desktop? I was thinking we had a way to record all times the popup gets hidden on desktop and we would count that the same as "not now".
Comment on attachment 8561702 [details]
MozReview Request: bz://1124465/liuche

/r/3631 - Bug 1124465 - Add telemetry probe for usage of the password capture dialog. r=MattN

Pull down this commit:

hg pull review -r 6a3cda4c4c0ae8f181dffe408ec3254bd976141e
Attachment #8561702 - Flags: review?(MattN+bmo)
Attachment #8561702 - Flags: review?(MattN+bmo) → review+
Comment on attachment 8561702 [details]
MozReview Request: bz://1124465/liuche

https://reviewboard.mozilla.org/r/3629/#review3429

::: mobile/android/components/LoginManagerPrompter.js
(Diff revision 3)
> +const PROMPT_DISPLAYED = 1;

Enumerated histograms start from 0 and I think it makes sense to have PROMPT_DISPLAYED = 0 since it's handled differently than the rest of the buckets.

::: mobile/android/components/LoginManagerPrompter.js
(Diff revision 3)
> +

Nit: extra newline

::: mobile/android/components/LoginManagerPrompter.js
(Diff revision 3)
> +        UITelemetry.addEvent("action.1", "dialog", null, "login-new-displayed");

Someone else should probably review UITelemetry for Android

::: toolkit/components/passwordmgr/nsLoginManagerPrompter.js
(Diff revision 3)
>    promptToChangePassword : function (aOldLogin, aNewLogin) {
>      var notifyObj = this._getPopupNote() || this._getNotifyBox();
> +    Services.telemetry.getHistogramById("PWMGR_PROMPT_UPDATE_ACTION").add(PROMPT_DISPLAYED);

I think you should move this to just before
`aNotifyObj.show(browser, "password-change"…`
as
`promptHistogram.add(PROMPT_DISPLAYED);` so the code is closer together and we don't accidentally skew the data with UI changes and from other applications (e.g. Thunderbird).

::: toolkit/components/telemetry/Histograms.json
(Diff revision 3)
> +    "description": "Action taken by user through prompt for creating a login"

I still think getting rid of PROMPT_DISPLAYED is probably not too hard but I'm guessing that you think this is is good enough since you're not accumulating PROMPT_NOTNOW on desktop.

Can you list the applicable values in the description and make it clear that the value of 0 is special and is always accumulated each time the prompt is shown?
https://reviewboard.mozilla.org/r/3629/#review3423

> I thought we wanted the count of values in the histogram to be the sum of the options the user could have chosen? What value is PROMPT_RES_DISPLAYED providing? Is this because you're not using PROMPT_RES_NOTNOW on desktop? I was thinking we had a way to record all times the popup gets hidden on desktop and we would count that the same as "not now".

Android doesn't fully implement the nsILoginPrompter interface, so there are several places where probes would need to be added, including some Android callbacks for handling View dismissal, and there are other places that dismiss after n page loads. I think it's better to just count the times the prompt is shown and calculate the numbers for the other actions because that's dead simple, and we don't run the risk of missing counts.
Attachment #8561702 - Flags: review+ → review?(margaret.leibovic)
Comment on attachment 8561702 [details]
MozReview Request: bz://1124465/liuche

/r/3631 - Bug 1124465 - Add telemetry probe for usage of the password capture dialog. r=MattN, margaret

Pull down this commit:

hg pull review -r d26580c8ce7c811209fdf9dd960ecd8cf9a409b6
Attachment #8561702 - Flags: review?(margaret.leibovic)
Comment on attachment 8561702 [details]
MozReview Request: bz://1124465/liuche

https://reviewboard.mozilla.org/r/3629/#review3741

::: mobile/android/components/LoginManagerPrompter.js
(Diff revision 4)
> +        UITelemetry.addEvent("action.1", "dialog", null, "login-new-displayed");

I'm not convinced that we need this redundant UITelemetry, since we can filter on Fennec on telemetry.mozilla.org.
https://reviewboard.mozilla.org/r/3629/#review3743

> I'm not convinced that we need this redundant UITelemetry, since we can filter on Fennec on telemetry.mozilla.org.

Talked about this in person, and I agree that the Fennec UITelemetry is redundant. Removing that part of this commit.
https://hg.mozilla.org/integration/fx-team/rev/3eb0cc94e217
Target Milestone: --- → mozilla39
https://hg.mozilla.org/mozilla-central/rev/3eb0cc94e217
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
OS: Mac OS X → All
Hardware: x86 → All
Comment on attachment 8561702 [details]
MozReview Request: bz://1124465/liuche

Approval Request Comment
[Feature/regressing bug #]: 2015 Passwords work (including upcoming doorhanger changes)
[User impact if declined]: We may not understand the impact of our upcoming doorhanger changes (starting in 39) if we don't have a telemetry baseline.
[Describe test coverage new/current, TreeHerder]: No tests but this has been on m-c for weeks.
[Risks and why]: Low risk since there haven't been issues in the last few weeks.
[String/UUID change made/needed]: None
Attachment #8561702 - Flags: approval-mozilla-aurora?
Attachment #8561702 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Attachment #8561702 - Attachment is obsolete: true
You need to log in before you can comment on or make changes to this bug.