1141769 - Implement new style(unified) FHR/Telemetry password manager probes

Status: RESOLVED WONTFIX

(Toolkit :: Password Manager, defect, P2)

Description

[Allison Naaktgeboren :ally]

targeting 39, as that will be the first firefox version using the new system.


approval for fhr probes: https://bugzilla.mozilla.org/show_bug.cgi?id=1124895#c15

Comment 1

[Matthew N. [:MattN]]

We should probably separate simply adding the
> "releaseChannelCollection": "opt-out",
line to Histograms.json (and maybe hooking up daily collection to the pwmgr gather method) from the probes which need to actually be implemented still.

Comment 2

[Allison Naaktgeboren :ally]

Attachment: newFHRforPasswords

I imagine this should look pretty straightforward.

However I am unclear about the fate of the passive probes from collectDailyData() in old FHR. I would think that it, like the rest of FHR, would go away entirely, but then it is not clear where to do the periodic collection of the passive data. I am unable to find any examples of ported passive problems in the new entries.

Comment 3

[Matthew N. [:MattN]]

(In reply to Allison Naaktgeboren please NEEDINFO? :ally from comment #2)
> I would think that it, like the rest of FHR,
> would go away entirely, but then it is not clear where to do the periodic
> collection of the passive data. I am unable to find any examples of ported
> passive problems in the new entries.

We have gather-telemetry for this and it's already used in password manager.

Comment 4

[Benjamin Smedberg]

I think that we're trying to discourage passive collection in general. Some things end up in the environment. The details depend on the use-cases and how you're building the reporting.

gather-telemetry is pretty horrible in general and we're trying to either remove or rewrite it. Georg should provide more details.

Comment 5

[Allison Naaktgeboren :ally]

(In reply to Matthew N. [:MattN] from comment #3)
> (In reply to Allison Naaktgeboren please NEEDINFO? :ally from comment #2)
> > I would think that it, like the rest of FHR,
> > would go away entirely, but then it is not clear where to do the periodic
> > collection of the passive data. I am unable to find any examples of ported
> > passive problems in the new entries.
> 
> We have gather-telemetry for this and it's already used in password manager.

I saw, the total saved password count is there, but I'm aware that I'm not supposed to be adding to it, which is why I'm so interested in what happened to previously passive fhr probes.

Comment 6

[Matthew N. [:MattN]]

(In reply to Benjamin Smedberg  [:bsmedberg] from comment #4)
> gather-telemetry is pretty horrible in general and we're trying to either
> remove or rewrite it. Georg should provide more details.

I got that impression before but it's the best we have AFAIK and this bug wouldn't be adding a new usage of it since we are already using it for telemetry in password manager. I don't think we need to block this bug on the future of gather-telemetry.

Comment 7

[Georg Fritzsche [:gfritzsche]]

We will look through the current uses of gather-telemetry and change them as soon as we find the time.
Note that gather-telemetry might never fire during a session and you would not submit any data in that case.

For the use-case of collectDailyData() here:
https://hg.mozilla.org/mozilla-central/annotate/102d0e9aa9e1/toolkit/components/passwordmgr/LoginManagerParent.jsm#l98
... this looks like it can just directly record in Histograms (which is cheap) instead of waiting for a collection signal.

Comment 8

[Georg Fritzsche [:gfritzsche]]

Comment on attachment 8601143 [details] [diff] [review]
newFHRforPasswords

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

::: toolkit/components/passwordmgr/LoginManagerParent.jsm
@@ +105,5 @@
>      yield m.setDailyLastNumeric("numSavedPasswords", loginsCount);
>  
> +    //TODO anaaktge: check with Greorg that recordDailyData() functions are still called for passive probes. We still need enabled
> +    // total saved logins already collected by PWMGR_NUM_SAVED_PASSWORDS elsewhere
> +    Services.telemetry.getHistogramById("PWMGR_IS_ENABLED").add(enabled);

recordDailyData() will not be called anymore.
Just submitting this on startup is probably enough information.

If we really need to track this in finer resolution we could consider submitting it on changes too or adding it to the environment.

@@ +187,5 @@
>  #ifndef ANDROID
>  #ifdef MOZ_SERVICES_HEALTHREPORT
>      if (aTopic == "LoginStats:NewSavedPassword") {
>        recordFHRDailyCounter("numNewSavedPasswordsInSession");
> +      Services.telemetry.getHistogramById("PWMGR_NUM_NEW_SAVED_PASSWORDS_IN_SESSION").add(1);

You don't want this guarded by |#ifdef MOZ_SERVICES_HEALTHREPORT| - if we build without the Healthreport implementation this would stop recording anything.
Dito for the probes below.

::: toolkit/components/telemetry/Histograms.json
@@ +7943,5 @@
>      "description": "Whether a saved login has a username"
>    },
> +  "PWMGR_IS_ENABLED": {
> +    "expires_in_version": "never",
> +    "kind": "boolean",

Wouldn't "flag" make more sense here?
Or do you want to track enabling/disabling during sessions/subsessions?

Comment 9

[Allison Naaktgeboren :ally]

(In reply to Georg Fritzsche [:gfritzsche] from comment #8)
> Comment on attachment 8601143 [details] [diff] [review]
> newFHRforPasswords
> 
> Review of attachment 8601143 [details] [diff] [review]:

> > +  "PWMGR_IS_ENABLED": {
> > +    "expires_in_version": "never",
> > +    "kind": "boolean",
> 
> Wouldn't "flag" make more sense here?
> Or do you want to track enabling/disabling during sessions/subsessions?

Yes, the idea is to track enabling/disabling during sessions. :)

Comment 10

[Georg Fritzsche [:gfritzsche]]

(In reply to Allison Naaktgeboren please NEEDINFO? :ally from comment #9)
> > > +  "PWMGR_IS_ENABLED": {
> > > +    "expires_in_version": "never",
> > > +    "kind": "boolean",
> > 
> > Wouldn't "flag" make more sense here?
> > Or do you want to track enabling/disabling during sessions/subsessions?
> 
> Yes, the idea is to track enabling/disabling during sessions. :)

Ok, what kind of question do you want to answer with that?

Does it need to be correlated to other measurements etc.?
Or is it just about knowing that this was flipped during a session?

Comment 11

[Matthew N. [:MattN]]

Comment on attachment 8601143 [details] [diff] [review]
newFHRforPasswords

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

::: toolkit/components/telemetry/Histograms.json
@@ +7943,5 @@
>      "description": "Whether a saved login has a username"
>    },
> +  "PWMGR_IS_ENABLED": {
> +    "expires_in_version": "never",
> +    "kind": "boolean",

> Wouldn't "flag" make more sense here?
PWMGR_SAVING_ENABLED is the flag version. If we can see this flag change over time for a user in a report then we probably don't need to record it both as a flag and boolean.

@@ +7949,5 @@
> +    "description": "If the password manager is enabled"
> +  },
> +  "PWMGR_NUM_NEW_SAVED_PASSWORDS_IN_SESSION": {
> +    "expires_in_version": "never",
> +    "kind": "count",

We already get this from PWMGR_PROMPT_REMEMBER_ACTION so I'm not sure we need to duplicate this info.

@@ +7955,5 @@
> +    "description": "Number of new passwords saved this session"
> +  },
> +  "PWMGR_NUM_SUCCESSFUL_AUTOFILLs": {
> +    "expires_in_version": "never",
> +    "kind": "count",

Ditto with PWMGR_FORM_AUTOFILL_RESULT

@@ +7961,5 @@
> +    "description": "Number of times the password manager successfully fills password fields"
> +  },
> +  "PWMGR_TOTAL_NUM_LOGINS_ENCOUNTERED": {
> +    "expires_in_version": "never",
> +    "kind": "count",

Ditto with PWMGR_FORM_AUTOFILL_RESULT

Comment 12

[Allison Naaktgeboren :ally]

(In reply to Matthew N. [:MattN] from comment #11)
> Comment on attachment 8601143 [details] [diff] [review]
> newFHRforPasswords
> 
> Review of attachment 8601143 [details] [diff] [review]:
> -----------------------------------------------------------------

Er, so if I read this correctly, we need none of these new probes?

Comment 13

[Georg Fritzsche [:gfritzsche]]

Ok, reading through the implementation here, i don't think the current probes can substitute the FHR provider as they work now.

If "gather-telemetry" is not fired, then this never submits any data.
I'd recommend to just submit those values directly where possible and let Telemetry accumulate them.

(In reply to Matthew N. [:MattN] from comment #11)
> ::: toolkit/components/telemetry/Histograms.json
> @@ +7943,5 @@
> >      "description": "Whether a saved login has a username"
> >    },
> > +  "PWMGR_IS_ENABLED": {
> > +    "expires_in_version": "never",
> > +    "kind": "boolean",
> 
> > Wouldn't "flag" make more sense here?
> PWMGR_SAVING_ENABLED is the flag version. If we can see this flag change
> over time for a user in a report then we probably don't need to record it
> both as a flag and boolean.

With the new/modified Telemetry system, a full browser sessions data is broken into subsessions by different events. Events are "passing midnight" and changing important environment data (active addons, important prefs, ...).

Histograms reset between subsessions, so as implemented the PWMGR_SAVING_ENABLED boolean only shows up in the first subsession it is submitted to.
You would see different values for each separate subsessions this is submitted to.

Other options are:
* if you want to know it was toggled during a subsession, add a flag that answers that exact question
* if it's important to correlate other measurements more exactly to the "enabled" state, we could add it to the environment and make it trigger a new subsession

I hope that helps, happy to talk through it with both of you more real-time if that helps.

Comment 14

[Matthew N. [:MattN]]

(In reply to Allison Naaktgeboren please NEEDINFO? :ally from comment #12)
> Er, so if I read this correctly, we need none of these new probes?

Well, we would need to make the existing equivalent ones opt-out though. Have you checked that the data is actually available like I'm suggesting?

(In reply to Georg Fritzsche [:gfritzsche] from comment #13)
> If "gather-telemetry" is not fired, then this never submits any data.
> I'd recommend to just submit those values directly where possible and let
> Telemetry accumulate them.

When though? Surely having every component do this on startup is not good for performance. That is one of the advantages of gather-telemetry and why I think we should just make it more reliable.

> (In reply to Matthew N. [:MattN] from comment #11)
> > ::: toolkit/components/telemetry/Histograms.json
> > @@ +7943,5 @@
> > >      "description": "Whether a saved login has a username"
> > >    },
> > > +  "PWMGR_IS_ENABLED": {
> > > +    "expires_in_version": "never",
> > > +    "kind": "boolean",
> > 
> > > Wouldn't "flag" make more sense here?
> > PWMGR_SAVING_ENABLED is the flag version. If we can see this flag change
> > over time for a user in a report then we probably don't need to record it
> > both as a flag and boolean.
> 
> With the new/modified Telemetry system, a full browser sessions data is
> broken into subsessions by different events. Events are "passing midnight"
> and changing important environment data (active addons, important prefs,
> ...).
> 
> Histograms reset between subsessions, so as implemented the
> PWMGR_SAVING_ENABLED boolean only shows up in the first subsession it is
> submitted to.
> You would see different values for each separate subsessions this is
> submitted to.
> 
> Other options are:
> * if you want to know it was toggled during a subsession, add a flag that
> answers that exact question
> * if it's important to correlate other measurements more exactly to the
> "enabled" state, we could add it to the environment and make it trigger a
> new subsession

It seems like we should fire gather-telemetry whenever we start a new subsession.

Comment 15

[Allison Naaktgeboren :ally]

Attachment: newFHRforPasswords v2

So here is what we're going to do: we're going to add the new fhr probes, PWMGR_NUM_SAVED_PASSWORDS will be an experiment on fhr+gatherTelemetry(). 

They gather data under different circumstances and on largely different populations and are subject to different data governance rules. For example, FHR for passwords has a 6 month limit, as agreed to with the Firefox Data Stewards. Telemetry does not. 

Fighting about the fate of gather_telemetry() is out of scope for this bug.  :) 

MattN has raised some potentially interesting points, but as a its a brave new system we don't have enough information. So let's do an experiment and find out. The total # passwords probe has been converted to FHR and left in gather telemetry as an experiment. We know that gather_telemetry() may not run during a session. We already have a good sense of what that number is, so have a less pressing need to collect it. 

If we do not get sufficient data from it as an FHR probe in its current location, or have problems using the data, we can determine if allowing fhr probes in gather_telemetry() is not good enough.

If the experiment is not deemed worthwhile, I can move it back to the constructor.

Comment 16

[Matthew N. [:MattN]]

Comment on attachment 8603577 [details] [diff] [review]
newFHRforPasswords v2

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

(In reply to Allison Naaktgeboren please NEEDINFO? :ally from comment #15)
> So here is what we're going to do: we're going to add the new fhr probes,
> PWMGR_NUM_SAVED_PASSWORDS will be an experiment on fhr+gatherTelemetry(). 
> 
> They gather data under different circumstances and on largely different
> populations and are subject to different data governance rules. For example,
> FHR for passwords has a 6 month limit, as agreed to with the Firefox Data
> Stewards. Telemetry does not. 

So are you saying this is the reason we need to duplicate probes? Why can't the data just be anonymized/aggregated after 6 months? I really don't want to have multiple overlapping probes as it adds confusion.

> Fighting about the fate of gather_telemetry() is out of scope for this bug. 
> :) 

Fine but it could be a dependency.

> MattN has raised some potentially interesting points, but as a its a brave
> new system we don't have enough information. So let's do an experiment and
> find out. The total # passwords probe has been converted to FHR and left in
> gather telemetry as an experiment. We know that gather_telemetry() may not
> run during a session. We already have a good sense of what that number is,
> so have a less pressing need to collect it. 
> 
> If we do not get sufficient data from it as an FHR probe in its current
> location, or have problems using the data, we can determine if allowing fhr
> probes in gather_telemetry() is not good enough.
> 
> If the experiment is not deemed worthwhile, I can move it back to the
> constructor.

We could just call _gatherTelemetry from another event in that case (or have FHR call it when gathering data or starting a new subsession).

Georg & Ally: can we discuss this tomorrow morning PDT on Vidyo?

::: toolkit/components/passwordmgr/LoginManagerParent.jsm
@@ +178,5 @@
>        return parent.initializationPromise;
>      });
>  
> +    let enabled = Services.prefs.getBoolPref("signon.rememberSignons")
> +    Services.telemetry.getHistogramById("PWMGR_IS_ENABLED_FLAG").add(enabled);

Doesn't this have a similar problem as gether-telemetry does in that this is only called once per session instead of once per subsession or do flag histograms work differently? I really don't think we should duplicate PWMGR_SAVING_ENABLED. If you don't want to fix gather-telemetry before landing (which I think is something that should be done for new FHR anyways) then we should just call _gatherTelemetry upon initialization (and risk startup performance regressions – the reason we do this up gather-telemetry in the first place).

::: toolkit/components/telemetry/Histograms.json
@@ +7735,5 @@
> +  "PWMGR_IS_ENABLED_FLAG": {
> +    "expires_in_version": "never",
> +    "kind": "flag",
> +    "releaseChannelCollection": "opt-out",
> +    "description": "If the password manager is enabled"

It sounds like Georg is saying this (or probably the existing probe) should be reported as part of the telemetry environment so it gets reported with every subsession. My main question is: how does that affects reporting on the value? Will I still be able to see the proportion of users with the pref set to false change over time on a dashboard? If not, why not?

Comment 17

[Georg Fritzsche [:gfritzsche]]

(In reply to Matthew N. [:MattN] from comment #16)
> > Fighting about the fate of gather_telemetry() is out of scope for this bug. 
> > :) 
> 
> Fine but it could be a dependency.

I agree that we may have use-cases that need an improved version of that for expensive accumulations.
I don't think the probes here need that though as they are not expensive. But let's sync about this later today.

Comment 18

[Georg Fritzsche [:gfritzsche]]

Comment on attachment 8603577 [details] [diff] [review]
newFHRforPasswords v2

Cancelling per yesterdays discussion.
We can probably port most or all of the existing probes to provide FHR equivalent behavior, provided people looking at them now get a heads-up.

Comment 19

[Allison Naaktgeboren :ally]

one of the concerns coming out of last week's discussion was the impact of calling gather_telemetry() on startup. To that end, I have kicked off talos runs to see what shakes out.

base (fxteam trunk): https://treeherder.mozilla.org/#/jobs?repo=try&revision=f074fe0af4a4
gather_telemetry on startup: https://treeherder.mozilla.org/#/jobs?repo=try&revision=e12d3367b313

Comment 20

[Allison Naaktgeboren :ally]

we've resolved the talos concerns. the old patch has rotted so badly ive given up saving it. Starting anew.

Comment 21

[Allison Naaktgeboren :ally]

Attachment: FHR #2, v3

Largely based on MattN's comment 11.

Once MattN is happy with it, we'll send it to Georg for a quick once over.

Comment 22

[Matthew N. [:MattN]]

Comment on attachment 8621248 [details] [diff] [review]
FHR #2, v3

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

Thanks Ally! r=me with 2 comments:

I think PWMGR_NUM_SAVED_PASSWORDS should also be changed to opt-out since that is recorded in current FHR via numSavedPasswords.

::: toolkit/components/telemetry/Histograms.json
@@ +7958,3 @@
>      "description": "Action taken by user through prompt for creating a login. (0=Prompt displayed [always recorded], 1=Add login, 2=Don't save now, 3=Never save)"
>    },
>    "PWMGR_PROMPT_UPDATE_ACTION" : {

Since this is the complement of PWMGR_PROMPT_REMEMBER_ACTION, can we have it in FHR too?

Comment 23

[Allison Naaktgeboren :ally]

Puts on data steward hat: I think that's ok, subject to the same 6 month limitation as the original fhr collection.

Comment 24

[Allison Naaktgeboren :ally]

Attachment: FHR#2 v4 (v3 + MattN's requests)

30 seconds to write, 2 hours to test. Some days I dislike FHR.

The good news is that should be a really easy review for you, Georg.

Comment 25

[Georg Fritzsche [:gfritzsche]]

Comment on attachment 8621927 [details] [diff] [review]
FHR#2 v4 (v3 + MattN's requests)

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

::: toolkit/components/passwordmgr/nsLoginManager.js
@@ +102,5 @@
>        this._initStorage();
>      }
>  
>      Services.obs.addObserver(this._observer, "gather-telemetry", false);
> +    this._gatherTelemetry(new Date().getTime());

This can introduce some noise now:
It will always be collected on startup, but on some sessions PWMGR_NUM_SAVED_PASSWORDS (and the other histograms in _gatherTelemetry) will be reset & overwritten from gather-telemetry.

I think it would be much saner for reasoning to always trigger _gatherTelemetry at startup and not register for "gather-telemetry" anymore.

Otherwise the relationship between the histograms collected in _gatherHistograms() and those accumulated "live" (like PWMGR_PROMPT_REMEMBER_ACTION) is ambiguous.

Comment 26

[Georg Fritzsche [:gfritzsche]]

(In reply to Georg Fritzsche [:gfritzsche] from comment #25)
> Comment on attachment 8621927 [details] [diff] [review]
> FHR#2 v4 (v3 + MattN's requests)
> 
> Review of attachment 8621927 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: toolkit/components/passwordmgr/nsLoginManager.js
> @@ +102,5 @@
> >        this._initStorage();
> >      }
> >  
> >      Services.obs.addObserver(this._observer, "gather-telemetry", false);
> > +    this._gatherTelemetry(new Date().getTime());
> 
> This can introduce some noise now:
> It will always be collected on startup, but on some sessions
> PWMGR_NUM_SAVED_PASSWORDS (and the other histograms in _gatherTelemetry)
> will be reset & overwritten from gather-telemetry.
> 
> I think it would be much saner for reasoning to always trigger
> _gatherTelemetry at startup and not register for "gather-telemetry" anymore.
> 
> Otherwise the relationship between the histograms collected in
> _gatherHistograms() and those accumulated "live" (like
> PWMGR_PROMPT_REMEMBER_ACTION) is ambiguous.

Matt, Ally, what do you think?

Comment 27

[Allison Naaktgeboren :ally]

I defer to MattN, who has a much more nuanced understanding of gatherTelemetry's data

Comment 28

[Allison Naaktgeboren :ally]

fwiw, I am ok removing the listner

Comment 29

[Matthew N. [:MattN]]

The main reason for leaving it is because it provides a clean way of testing this code. If you want to remove the registration then you'll need to modify the tests to call _gatherTelemetry instead of sending the notification

Comment 30

[Georg Fritzsche [:gfritzsche]]

In case this wasn't clear: let's remove it then and fix the tests.

Comment 31

[Allison Naaktgeboren :ally]

It was pointed out to me last week during Whistler that the 6 month expiration on this fhr collection, as spelled out in https://bugzilla.mozilla.org/show_bug.cgi?id=1124895#c15 , expires at the end of q3, and new FHR, on its most current schedule, will not have hit release by that point.

Thus, we no longer need this bug.