1118863 - Add telemetry to know whether password saving is disabled globally

Status: RESOLVED FIXED

(Toolkit :: Password Manager, defect, P1)

Description

[Matthew N. [:MattN]]

We should report the value of the signon.rememberSignons preference with telemetry.

Comment 1

[Mike Hoye [:mhoye]]

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.

Comment 2

[Matthew N. [:MattN]]

(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.

Comment 3

[Allison Naaktgeboren :ally]

Attachment: isPasswordFillDisabledv1

from the IRC discussion this looks right. However, I still need to test it.

Comment 4

[Matthew N. [:MattN]]

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.

Comment 5

[Allison Naaktgeboren :ally]

Attachment: isPWDEnabledv2

Comment 6

[Matthew N. [:MattN]]

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

Comment 7

[Allison Naaktgeboren :ally]

https://hg.mozilla.org/integration/fx-team/rev/41197daad2a4

Comment 8

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/41197daad2a4

Comment 9

[Matthew N. [:MattN]]

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

Comment 10

[Lawrence Mandel [:lmandel] (use needinfo)]

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+

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/507228e928da

Comment 12

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8552720 [details] [diff] [review]
isPWDEnabledv2

Too late for beta.