Closed
Bug 1124465
Opened 10 years ago
Closed 10 years ago
Add telemetry probe for usage of the password capture dialog
Categories
(Toolkit :: Password Manager, defect, P1)
Toolkit
Password Manager
Tracking
()
RESOLVED
FIXED
mozilla39
People
(Reporter: ally, Assigned: liuche)
References
(Blocks 1 open bug)
Details
Attachments
(1 file, 1 obsolete file)
(deleted),
text/x-review-board-request
|
Details |
:liuche suggests measuring the ui states.
Reporter | ||
Updated•10 years ago
|
Blocks: password-telemetry
Updated•10 years ago
|
Priority: -- → P1
Assignee | ||
Updated•10 years ago
|
Assignee: nobody → liuche
Assignee | ||
Updated•10 years ago
|
Points: --- → 3
Assignee | ||
Comment 1•10 years ago
|
||
/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
Assignee | ||
Comment 2•10 years ago
|
||
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.
Assignee | ||
Comment 3•10 years ago
|
||
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)
Assignee | ||
Updated•10 years ago
|
Attachment #8561702 -
Attachment is obsolete: true
Attachment #8561702 -
Flags: feedback?(MattN+bmo)
Assignee | ||
Comment 4•10 years ago
|
||
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)
Assignee | ||
Comment 5•10 years ago
|
||
Removed Notification bar and modal dialog telemetry measurements, and added measurement for every prompt open.
Comment 6•10 years ago
|
||
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)
Assignee | ||
Comment 7•10 years ago
|
||
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)
Assignee | ||
Comment 8•10 years ago
|
||
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.
Updated•10 years ago
|
Attachment #8561702 -
Flags: review?(MattN+bmo)
Comment 9•10 years ago
|
||
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".
Assignee | ||
Comment 10•10 years ago
|
||
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)
Updated•10 years ago
|
Attachment #8561702 -
Flags: review?(MattN+bmo) → review+
Comment 11•10 years ago
|
||
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?
Assignee | ||
Comment 12•10 years ago
|
||
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.
Assignee | ||
Updated•10 years ago
|
Attachment #8561702 -
Flags: review+ → review?(margaret.leibovic)
Assignee | ||
Comment 13•10 years ago
|
||
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
Updated•10 years ago
|
Attachment #8561702 -
Flags: review?(margaret.leibovic)
Comment 14•10 years ago
|
||
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.
Assignee | ||
Comment 15•10 years ago
|
||
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.
Assignee | ||
Comment 16•10 years ago
|
||
Target Milestone: --- → mozilla39
Comment 17•10 years ago
|
||
Updated•10 years ago
|
OS: Mac OS X → All
Hardware: x86 → All
Comment 18•10 years ago
|
||
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?
Updated•10 years ago
|
Attachment #8561702 -
Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Comment 19•10 years ago
|
||
status-firefox38:
--- → fixed
Assignee | ||
Comment 20•9 years ago
|
||
Attachment #8561702 -
Attachment is obsolete: true
Assignee | ||
Comment 21•9 years ago
|
||
You need to log in
before you can comment on or make changes to this bug.
Description
•