1118839 - Add telemetry to measure the number of saved passwords for a user

Status: RESOLVED FIXED

(Toolkit :: Password Manager, defect, P1)

Description

[Matthew N. [:MattN]]

When telemetry is gathered, we can use getAllLogins and report the length of the array. We may have to handle if the MP isn't unlocked.

Comment 1

[Matthew N. [:MattN]]

Attachment: MozReview Request: bz://1118839/MattN

Comment 2

[Matthew N. [:MattN]]

/r/2527 - Bug 1118839 - Add telemetry to measure the number of saved passwords for a user. r=dolske

Pull down this commit:

hg pull review -r e36779f44f22bf5729c607c5449f04a06e3a75dc

Comment 3

[Matthew N. [:MattN]]

https://reviewboard.mozilla.org/r/2525/#review1677

::: toolkit/components/passwordmgr/nsLoginManager.js
(Diff revision 1)
> +      numPasswordsHist.add(this.countLogins("", "", ""));

I double-checked that this counts both form and HTTP-auth (non-form) logins.

::: toolkit/components/telemetry/Histograms.json
(Diff revision 1)
> +  "PWMGR_NUM_SAVED_PASSWORDS": {
> +    "alert_emails": ["[e-mail]"],

I think this should be low enough volume and relevant enough that I think it's worth testing alert emails to the mailing list.

::: toolkit/components/telemetry/Histograms.json
(Diff revision 1)
> +    "kind": "exponential",
> +    "high": 750,
> +    "n_buckets" : 50,

I came up with these values based on:
a) the numbers from https://docs.google.com/a/mozilla.com/spreadsheets/d/1JPHgC4CXCylnv-5ygrbsihbICIaYjd-suB56ibtG5Js/edit
b) not having mroe than 50 buckets (since the current telemetry dashboard doesn't handle more than that well.
c) exponential is probably more important than linear since I think we'll care most about people going from 0/1 => a few passwords => most passwords.
d) I wanted to just use a COUNT histogram but the dashboard sums all the counts together globally so it's harder to see an increase of passwords per user over time.

Comment 4

[Justin Dolske [:Dolske]]

https://reviewboard.mozilla.org/r/2525/#review1723

::: toolkit/components/telemetry/Histograms.json
(Diff revision 1)
> +    "alert_emails": ["[e-mail]"],

Eh, I don't think this is likely to be very useful. I'd remove it.

::: toolkit/components/passwordmgr/nsLoginManager.js
(Diff revision 1)
> +      let numPasswordsHist = Services.telemetry.getHistogramById("PWMGR_NUM_SAVED_PASSWORDS");

Is it worth considering counting the exception list too?

I pondered counting form and http-auth longs separately, but I can't think of a particular reason to do so (and it just complicates getting an actual total). Plus, I suspect that this will become less important with the upcoming changes, as I suspect we'll want to make the difference less important.

Comment 5

[Justin Dolske [:Dolske]]

(In reply to Justin Dolske [:Dolske] from comment #4)

> > +      let numPasswordsHist = Services.telemetry.getHistogramById("PWMGR_NUM_SAVED_PASSWORDS");
> 
> Is it worth considering counting the exception list too?

Apparently so! Bug 1118871. :)

Comment 6

[Matthew N. [:MattN]]

(In reply to Justin Dolske [:Dolske] from comment #4)
> https://reviewboard.mozilla.org/r/2525/#review1723
> 
> ::: toolkit/components/telemetry/Histograms.json
> (Diff revision 1)
> > +    "alert_emails": ["[e-mail]"],
> 
> Eh, I don't think this is likely to be very useful. I'd remove it.

OK, I can see it being useful if we make a change that causes people to lose their passwords but hopefully that is rare. I removed it.

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

Comment 7

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/e41cbd7c6973

Comment 8

[Matthew N. [:MattN]]

Comment on attachment 8549417 [details]
MozReview Request: bz://1118839/MattN

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 9

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

Comment on attachment 8549417 [details]
MozReview Request: bz://1118839/MattN

Given that we're at the end of the Beta 36 cycle, I'm not sure that there is a lot of value in uplifting this change to Beta. (I'll let Sylvestre make the call on Beta.) I am happy to take this on Aurora 37. 

Aurora+

Comment 10

[Ryan VanderMeulen [:RyanVM][PTO June 24-28]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/bfd779b9cace

Comment 11

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8549417 [details]
MozReview Request: bz://1118839/MattN

Too late for beta.

Comment 13

[Matthew N. [:MattN]]

Attachment: MozReview Request: Bug 1118839 - Add telemetry to measure the number of saved passwords for a user. r=dolske