Closed Bug 1118863 Opened 5 years ago Closed 5 years ago

Add telemetry to know whether password saving is disabled globally

Categories

(Toolkit :: Password Manager, defect, P1)

defect
Points:
2

Tracking

()

RESOLVED FIXED
mozilla38
Iteration:
38.1 - 26 Jan
Tracking Status
firefox36 --- affected
firefox37 --- fixed
firefox38 --- fixed

People

(Reporter: MattN, Assigned: ally, Mentored)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 1 obsolete file)

We should report the value of the signon.rememberSignons preference with telemetry.
Flags: firefox-backlog+
If you can add a DXR link to this, or a pointer to a previous similar bug, this'd probably make a good Good First Bug.
Flags: needinfo?(MattN+bmo)
(In reply to Mike Hoye [:mhoye] from comment #1)
> If you can add a DXR link to this, or a pointer to a previous similar bug,
> this'd probably make a good Good First Bug.

I agree but I will be taking this bug in the iteration starting tomorrow as it's a priority to get these probes landed to kick off the password manager improvements.
Flags: needinfo?(MattN+bmo)
Points: --- → 2
Flags: qe-verify-
Priority: -- → P1
Assignee: nobody → ally
Attached patch isPasswordFillDisabledv1 (obsolete) — Splinter Review
from the IRC discussion this looks right. However, I still need to test it.
Comment on attachment 8552118 [details] [diff] [review]
isPasswordFillDisabledv1

Review of attachment 8552118 [details] [diff] [review]:
-----------------------------------------------------------------

::: toolkit/components/passwordmgr/nsLoginManager.js
@@ +211,5 @@
>      _gatherTelemetry : function() {
>        let numPasswordsHist = Services.telemetry.getHistogramById("PWMGR_NUM_SAVED_PASSWORDS");
>        numPasswordsHist.clear();
>        numPasswordsHist.add(this.countLogins("", "", ""));
> +      Services.telemetry.getHistogramById("PWMGR_ARE_SAVED_PW_DISABLED").add(this._remember);

I think you also want to clear it like I do with the other (now that you switched from a flag) since gather-telemetry can be called multiple times in a session.

This is recording the opposite of what it says now :P. I think we should rename the histogram to record the "positive" so that up-and-to-the-right on the graph means an increase in sessions with saving enabled. e.g. something like PWMGR_SAVING_ENABLED. Don't forget to update the probe description.
Attached patch isPWDEnabledv2Splinter Review
Attachment #8552118 - Attachment is obsolete: true
Attachment #8552720 - Flags: review?(MattN+bmo)
Comment on attachment 8552720 [details] [diff] [review]
isPWDEnabledv2

Review of attachment 8552720 [details] [diff] [review]:
-----------------------------------------------------------------

It works!

::: toolkit/components/passwordmgr/nsLoginManager.js
@@ +211,5 @@
>      _gatherTelemetry : function() {
>        let numPasswordsHist = Services.telemetry.getHistogramById("PWMGR_NUM_SAVED_PASSWORDS");
>        numPasswordsHist.clear();
>        numPasswordsHist.add(this.countLogins("", "", ""));
> +      let isPwdSavedEnabledHist = Services.telemetry.getHistogramById("PWMGR_SAVING_ENABLED");

Nit: add a newline above this one to separate the probes
Attachment #8552720 - Flags: review?(MattN+bmo) → review+
https://hg.mozilla.org/mozilla-central/rev/41197daad2a4
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla38
Iteration: --- → 38.1 - 26 Jan
Comment on attachment 8552720 [details] [diff] [review]
isPWDEnabledv2

Approval Request Comment
[Feature/regressing bug #]: Improvements to password manager in 2015 - Bug 1121127
[User impact if declined]: Possibly less ideal decisions based on lack of data.
[Describe test coverage new/current, TreeHerder]: No tests for the telemetry since it's outside the normal code flow so shouldn't break other aspects of password manager if an exception occurs. No problems have been reported on Nightly and data looks good there.
[Risks and why]: Low risk since it's new code only run during the telemetry gathering phase.
[String/UUID change made/needed]: None
Attachment #8552720 - Flags: approval-mozilla-beta?
Attachment #8552720 - Flags: approval-mozilla-aurora?
Comment on attachment 8552720 [details] [diff] [review]
isPWDEnabledv2

I'm not sure about the value of uplifting this patch to Beta at this point in the cycle. (I'll leave it to Sylvestre to make the call on that.) Happy to take this on Aurora.

Aurora+
Attachment #8552720 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Comment on attachment 8552720 [details] [diff] [review]
isPWDEnabledv2

Too late for beta.
Attachment #8552720 - Flags: approval-mozilla-beta? → approval-mozilla-beta-
You need to log in before you can comment on or make changes to this bug.