1124516 - Telemetry: Record the time in days each saved login was last used

Status: RESOLVED FIXED

(Toolkit :: Password Manager, defect, P1)

Description

[Chris Karlof [:ckarlof]]

"Used" means timeLastUsed as recorded by the password manager was update within the time frame.

Comment 1

[:Paolo Amadini]

I'll probably get to this in the next few days on top of bug 1125858.

Comment 2

[:Paolo Amadini]

We can record two different things:

- Distribution of timeLastUsed age across all passwords of all users:
   - One histogram for all users
   - Age on X axis
   - Number of passwords on Y axis
- Distribution per timeLastUsed age group
   - One histogram per login age group (day, week, month, year)
   - Number of passwords on X axis
   - Number of users on Y axis

The first graph would probably answer the "percentage per login age group" question, even in a more granular way. I'm not sure if the second one is just a different view or provides more useful insight.

Comment 3

[:Paolo Amadini]

At the engineering meeting we said we'll go with the first histogram only initially, updating the bug description accordingly.

Comment 4

[:Paolo Amadini]

Attachment: The patch

I also tested this manually by triggering the "gather-telemetry" notification from the Browser Console, and the results make sense using the current day as reference.

Comment 6

[Matthew N. [:MattN]]

Comment on attachment 8556518 [details] [diff] [review]
The patch

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

::: toolkit/components/passwordmgr/nsLoginManager.js
@@ +212,5 @@
>              }
>          }
>      },
>  
> +    /*

I guess you meant to make this in JS-doc format with two asterisks: /**

@@ +215,5 @@
>  
> +    /*
> +     * Collects statistics about the current logins and settings.  The telemetry
> +     * histograms used here are not accumulated, but are reset each time this
> +     * function is called.

…since it can be called multiple times in a session.

Nit: You could also note here something like the FLAG comment that this function may not be called at all in a session.

@@ +227,2 @@
>  
> +        let clearAndGetHistogram = function (histogramId) {

What's the advantage of this syntax over the traditional syntax:
function clearAndGetHistogram(histogramId) {

The disadvantage is that I don't immediately notice it's a helper function and I waste time reading it when trying to skim the logic of the outer function.

@@ +233,3 @@
>  
> +        clearAndGetHistogram("PWMGR_BLOCKLIST_NUM_SITES")
> +                            .add(this.getAllDisabledHosts({}).length);

The prevailing style in this file and the standard for new code (in browser/ at least) is to wrap *after* the period. (I personally preferred this style but gave up on that years ago)

@@ +255,5 @@
> +            usernamePresentHistogram.add(!!login.username);
> +
> +            // Not all logins may have the time last used set.
> +            login.QueryInterface(Ci.nsILoginMetaInfo);
> +            if (login.timeLastUsed) {

Hmm… where have you seen it not set? Both addLogin implementations set it and I see sync going through modifyLogin and addLogin so it should get set. I think we should record those in the error bucket of the histogram instead of ignoring them.

::: toolkit/components/passwordmgr/test/unit/test_telemetry.js
@@ +86,5 @@
> +  let snapshot = Services.telemetry.getHistogramById(histogramId).snapshot();
> +
> +  // Compute the actual ranges in the format { range1: value1, range2: value2 }.
> +  let actualNonZeroRanges = {};
> +  for (let [index, range] of Iterator(snapshot.ranges)) {

I believe Iterator is deprecated

Comment 7

[Matthew N. [:MattN]]

Comment on attachment 8556518 [details] [diff] [review]
The patch

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

::: toolkit/components/passwordmgr/test/unit/test_telemetry.js
@@ +107,5 @@
> + * Enable local telemetry recording for the duration of the tests, and prepare
> + * the test data that will be used by the following tests.
> + */
> +add_task(function test_initialize()
> +{

New code should have the brace on the same line as `function` like most code in the module.

Comment 8

[:Paolo Amadini]

(In reply to Matthew N. [:MattN] from comment #6)
> I guess you meant to make this in JS-doc format with two asterisks: /**

Updated, although the rest of this file consistently uses one asterisk only (but no @param tags).

> > +        let clearAndGetHistogram = function (histogramId) {
> 
> What's the advantage of this syntax over the traditional syntax:
> function clearAndGetHistogram(histogramId) {

No real advantage here, I've updated it to be a function statement.

If you're interested in a bit of background about why the "let" syntax is my default choice:
1) The other syntax can't be used in a conditional block, in strict mode.
2) As soon as you need "this", you need arrow functions (or bind) so you end up switching back to the "let" syntax anyways.

> The disadvantage is that I don't immediately notice it's a helper function
> and I waste time reading it when trying to skim the logic of the outer
> function.

Good point. By the way, an alternative to all the above is to place the helper function as a method on the object, which would solve the skimming problem more strongly.

> @@ +233,3 @@
> > +        clearAndGetHistogram("PWMGR_BLOCKLIST_NUM_SITES")
> > +                            .add(this.getAllDisabledHosts({}).length);
> 
> The prevailing style in this file and the standard for new code (in browser/
> at least) is to wrap *after* the period. (I personally preferred this style
> but gave up on that years ago)

Hm, wasn't updated on the latest fashion :-) The style guide used to recommend dot after line break, but it doesn't anymore and this is used inconsistently in this file too.

I've gone for a different style altogether (indented parameter list) that I think is clearer.

> > +            // Not all logins may have the time last used set.
> > +            login.QueryInterface(Ci.nsILoginMetaInfo);
> > +            if (login.timeLastUsed) {
> 
> Hmm… where have you seen it not set?

Uh, no, you're right, I thought it was not set on migration but we always set it. (In the test the first login ended up being in the future compared to the reference date, so it was ignored.)

We can ignore this case then, I believe we should trust the rest of the code to work correctly.

> should record those in the error bucket

Out of curiosity, are you saying that every histogram has an "error bucket", or is it a per-histogram convention like a special value?

Comment 9

[:Paolo Amadini]

Attachment: Updated patch

Comment 10

[Matthew N. [:MattN]]

Comment on attachment 8557104 [details] [diff] [review]
Updated patch

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

(In reply to :Paolo Amadini from comment #8)
> (In reply to Matthew N. [:MattN] from comment #6)
> > I guess you meant to make this in JS-doc format with two asterisks: /**
> 
> Updated, although the rest of this file consistently uses one asterisk only
> (but no @param tags).
> 
> > > +        let clearAndGetHistogram = function (histogramId) {
> > 
> > What's the advantage of this syntax over the traditional syntax:
> > function clearAndGetHistogram(histogramId) {
> 
> No real advantage here, I've updated it to be a function statement.
> 
> If you're interested in a bit of background about why the "let" syntax is my
> default choice:
> 1) The other syntax can't be used in a conditional block, in strict mode.

Yeah, this is really rare and kinda dirty anyways so I avoid it.

> 2) As soon as you need "this", you need arrow functions (or bind) so you end
> up switching back to the "let" syntax anyways.

Well, you can just bind using the function name e.g. clearAndGetHistogram.bind(this) with the syntax I suggested. It's only a problem if you need the reference to the function returned by bind which is not always the case.

> > @@ +233,3 @@
> > > +        clearAndGetHistogram("PWMGR_BLOCKLIST_NUM_SITES")
> > > +                            .add(this.getAllDisabledHosts({}).length);
> > 
> > The prevailing style in this file and the standard for new code (in browser/
> > at least) is to wrap *after* the period. (I personally preferred this style
> > but gave up on that years ago)
> 
> Hm, wasn't updated on the latest fashion :-) The style guide used to
> recommend dot after line break, but it doesn't anymore and this is used
> inconsistently in this file too.
> 
> I've gone for a different style altogether (indented parameter list) that I
> think is clearer.

OK, Gavin is the one who got me to change to period at the end many years ago (I think from Search code)

> > should record those in the error bucket
> 
> Out of curiosity, are you saying that every histogram has an "error bucket",
> or is it a per-histogram convention like a special value?

I thought it was the former but after re-reading https://developer.mozilla.org/en-US/docs/Mozilla/Performance/Adding_a_new_Telemetry_probe, I guess I misunderstood or things have changed. I thought there was a special bucket on probes (at least of some types) that counted all of the values outside the range but it turns out that values lower than the `low` value go in the "0" bucket and values higher than `high` go in the last bucket. (See the descriptions of `low` and `high` on that page.).

Comment 11

[:Paolo Amadini]

Attachment: Rebased patch

Comment 12

[:Paolo Amadini]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=2ffa9e6c31e4

Comment 13

[:Paolo Amadini]

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

Comment 14

[Carsten Book [:Tomcat]]

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

Comment 15

[Matthew N. [:MattN]]

Comment on attachment 8557533 [details] [diff] [review]
Rebased patch

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]: Added tests for the telemetry. 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 16

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

Comment on attachment 8557533 [details] [diff] [review]
Rebased patch

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 17

[Ryan VanderMeulen [:RyanVM]]

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

Comment 18

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8557533 [details] [diff] [review]
Rebased patch

Too late for beta.