Closed Bug 1118839 Opened 5 years ago Closed 5 years ago

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

Categories

(Toolkit :: Password Manager, defect, P1)

defect
Points:
3

Tracking

()

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

People

(Reporter: MattN, Assigned: MattN)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 1 obsolete file)

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.
Flags: firefox-backlog+
Iteration: --- → 38.1 - 26 Jan
Points: --- → 3
Flags: qe-verify-
/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
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": ["passwords-dev@mozilla.org"],

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.
https://reviewboard.mozilla.org/r/2525/#review1723

::: toolkit/components/telemetry/Histograms.json
(Diff revision 1)
> +    "alert_emails": ["passwords-dev@mozilla.org"],

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.
Attachment #8549417 - Flags: review?(dolske) → review+
(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. :)
Priority: -- → P1
(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": ["passwords-dev@mozilla.org"],
> 
> 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
Whiteboard: [fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/e41cbd7c6973
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → mozilla38
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
Attachment #8549417 - Flags: approval-mozilla-beta?
Attachment #8549417 - Flags: approval-mozilla-aurora?
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+
Attachment #8549417 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Comment on attachment 8549417 [details]
MozReview Request: bz://1118839/MattN

Too late for beta.
Attachment #8549417 - Flags: approval-mozilla-beta? → approval-mozilla-beta-
Attachment #8549417 - Attachment is obsolete: true
Attachment #8619059 - Flags: review+
You need to log in before you can comment on or make changes to this bug.