1205705 - Collect about:accounts telemetry on Fennec

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Nick Alexander :nalexander [he/him]]

We're about to transition to a web-based FxA sign in flow.  There is some data we might like.  Off the top, I can think of:

* how we got to about:accounts (first run, Sync preference, notification);
* whether we were able to load fxa-content-server at all; (that is, did we replace the spinner with the network error message).
* how long it took to load fxa-content-server. (that is, how long was the spinner visible).

Since the fxa-content-server loads different pages differently, we should include which endpoint we were hitting for the last few (/signup, /signin are faster than /settings, anecdotally).

Comment 1

[Nick Alexander :nalexander [he/him]]

Attachment: MozReview Request: Bug 1205705 - Part 1: Always resolve or reject the LOADED message promise. r=markh,mfinkle

Bug 1205705 - Part 1: Always resolve or reject the LOADED message promise. r?markh,mfinkle

This is hygiene that completes the set of paths through this part of
the code.  If we wrapper.{init,retry}, we are guaranteed to have a new
promise; and now that promise will always be fulfilled.  It is
technically possible, but not anticipated, for an in-flight promise to
be replaced.  Such a situation should not occur, but if it does, the
obsolete promise will still exist but never be fulfilled (since
loading or errors only touch the most recent promise).  Eventually it
will be safely garbage collected.

Comment 2

[Nick Alexander :nalexander [he/him]]

Attachment: MozReview Request: Bug 1205705 - Part 2: Collect client-side fxa-content-server LOADED Telemetry. r=mfinkle r=ally

Bug 1205705 - Part 2: Collect client-side fxa-content-server LOADED Telemetry. r?mfinkle,ally

This collects client-side fxa-content-server data.  The data covers
only the about:accounts experience until:

* the fxa-content-server provides the LOADED message; or
* connection failure is observed.

Nota bene: a healthy fxa-content-server always delivers the LOADED
message!  In future, we might want to timeout the load (and observe
said timeouts) separately.

We collect no data after the fxa-content-server LOADED message.  The
intention is for the server-side metrics flow to capture the valuable
"bounce rate" metrics, since the fxa-content-server team are in
position to quickly improve the web-based UI flow.

The client-side data collected is intended to answer the following
questions:

1) How many remote content loads started;
2) How many loads completed;
3) What proportion of loads made it to the LOADED message, as opposed
to failed;
4) How long it took each successful load to observe the LOADED
message;
5) How long it took each failing load to observe failure.

All of these are keyed by the fxa-content-server endpoint path (like
'settings' or 'profile/avatar'), since I observe differences between
the time-to-LOADED for each endpoint path.

There is a privacy trade-off here.  Mozilla is collecting data to
understand the user experience when about:accounts is connecting to
the specific fxa-content-server hosted by Mozilla at
accounts.firefox.com.  However, we don't want to observe what
alternate servers users might be using, so we can't collect the whole
URL.  Here, we filter the data based on whether the user is /not/
using accounts.firefox.com, and then record just the endpoint path.
Other collected data could expose that the user is using Firefox
Accounts, and together, that leaks the number of users not using
accounts.firefox.com.  We accept this leak: Mozilla already collects
data about whether Sync (both legacy and FxA) is using a custom server
in various situations: see the WEAVE_CUSTOM_* Telemetry histograms.

Comment 3

[Nick Alexander :nalexander [he/him]]

For the record, I can verify that this calls the Telemetry methods (with reasonable histograms and keys), but I can't actually see any data being collected in about:telemetry!  It may be that my local build has telemetry disabled in some way.  Advice on how to test this more thoroughly appreciated.

Comment 4

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 8663144 [details]
MozReview Request: Bug 1205705 - Part 1: Always resolve or reject the LOADED message promise. r=markh,mfinkle

https://reviewboard.mozilla.org/r/19729/#review17755

Comment 5

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 8663145 [details]
MozReview Request: Bug 1205705 - Part 2: Collect client-side fxa-content-server LOADED Telemetry. r=mfinkle r=ally

https://reviewboard.mozilla.org/r/19731/#review17757

Code part looks OK. The telemetry looks OK too, but I assume you have thought more about "can I look at the resultant data and be able to get value from it" part.

Comment 6

[Mark Hammond [:markh] [:mhammond]]

Comment on attachment 8663144 [details]
MozReview Request: Bug 1205705 - Part 1: Always resolve or reject the LOADED message promise. r=markh,mfinkle

https://reviewboard.mozilla.org/r/19729/#review17797

::: mobile/android/chrome/content/aboutAccounts.js:144
(Diff revision 1)
> +        loadedDeferred.reject(new Error("Failed in onStateChange!"));

I just said something very similar in that other review :) LGTM, but I wonder (without fully thinking it through) if it could be in show(), thus making it more difficult to have places where this isn't done but should be? Feel free to ignore if that's not viable.

Comment 7

[Nick Alexander :nalexander [he/him]]

markh: I'll digest your comments more thoroughly today, but I meant to ask you: could you take a look at part 2 of this (the actual telemetry) and let me know if I should remove the FENNEC_ prefix?  I think Desktop might care to collect some of this data, possibly in the future.  Thanks!

Comment 8

[Allison Naaktgeboren :ally]

Comment on attachment 8663145 [details]
MozReview Request: Bug 1205705 - Part 2: Collect client-side fxa-content-server LOADED Telemetry. r=mfinkle r=ally

https://reviewboard.mozilla.org/r/19731/#review17869

::: mobile/android/chrome/content/aboutAccounts.js:73
(Diff revision 1)
> +  // alternate servers users might be using, so we can't collect the whole URL.

good.

::: mobile/android/chrome/content/aboutAccounts.js:80
(Diff revision 1)
> +  // WEAVE_CUSTOM_* Telemetry histograms.

That was not our finest data collection moment. This does not expand the risk beyond the pre-existing probes, so it's acceptable.

::: toolkit/components/telemetry/Histograms.json:8739
(Diff revision 1)
> +  "FENNEC_ABOUT_ACCOUNTS_CONTENT_SERVER_LOAD_STARTED_COUNT": {

missing expires_in_version value, _especially_ for fhr-replacement probes. Please remember that "never" is no longer an acceptable value. How about version 50?

::: toolkit/components/telemetry/Histograms.json:8750
(Diff revision 1)
> +    "releaseChannelCollection": "opt-out",

same

::: toolkit/components/telemetry/Histograms.json:8762
(Diff revision 1)
> +  "FENNEC_ABOUT_ACCOUNTS_CONTENT_SERVER_FAILURE_TIME_MS": {

same

::: toolkit/components/telemetry/Histograms.json:8764
(Diff revision 1)
> +    "keyed": true,

keyed opt-out probes are dicey. Are settings and signin the only possible values for keys for all of these keyed probes?

Comment 9

[Nick Alexander :nalexander [he/him]]

ally: ah, I removed the expiry I had (52) due to what you said to liuche:

17:06 ally: liuche:  please remember no expiration never in new probes 

I interpreted that as "no expiration, never" in new probes.  But you meant "no expiration == never", i.e., they always expire.  I'll make it 50, I choose 52 just since that's the next ESR.

As to the keyed opt-out probes: no, there are many other possible values, and we grow new ones frequently.  They are endpoints on accounts.firefox.com; right now I know of: signup, signin, force_auth, settings, profile/avatar{/change}, legal (IIRC).

Thanks for the review, will re-flag shortly.

Comment 10

[Nick Alexander :nalexander [he/him]]

Comment on attachment 8663144 [details]
MozReview Request: Bug 1205705 - Part 1: Always resolve or reject the LOADED message promise. r=markh,mfinkle

Bug 1205705 - Part 1: Always resolve or reject the LOADED message promise. r=markh,mfinkle

This is hygiene that completes the set of paths through this part of
the code.  If we wrapper.{init,retry}, we are guaranteed to have a new
promise; and now that promise will always be fulfilled.  It is
technically possible, but not anticipated, for an in-flight promise to
be replaced.  Such a situation should not occur, but if it does, the
obsolete promise will still exist but never be fulfilled (since
loading or errors only touch the most recent promise).  Eventually it
will be safely garbage collected.

Comment 11

[Nick Alexander :nalexander [he/him]]

Comment on attachment 8663145 [details]
MozReview Request: Bug 1205705 - Part 2: Collect client-side fxa-content-server LOADED Telemetry. r=mfinkle r=ally

Bug 1205705 - Part 2: Collect client-side fxa-content-server LOADED Telemetry. r=mfinkle r=ally

This collects client-side fxa-content-server data.  The data covers
only the about:accounts experience until:

* the fxa-content-server provides the LOADED message; or
* connection failure is observed.

Nota bene: a healthy fxa-content-server always delivers the LOADED
message!  In future, we might want to timeout the load (and observe
said timeouts) separately.

We collect no data after the fxa-content-server LOADED message.  The
intention is for the server-side metrics flow to capture the valuable
"bounce rate" metrics, since the fxa-content-server team are in
position to quickly improve the web-based UI flow.

The client-side data collected is intended to answer the following
questions:

1) How many remote content loads started;
2) How many loads completed;
3) What proportion of loads made it to the LOADED message, as opposed
to failed;
4) How long it took each successful load to observe the LOADED
message;
5) How long it took each failing load to observe failure.

All of these are keyed by the fxa-content-server endpoint path (like
'settings' or 'profile/avatar'), since I observe differences between
the time-to-LOADED for each endpoint path.

There is a privacy trade-off here.  Mozilla is collecting data to
understand the user experience when about:accounts is connecting to
the specific fxa-content-server hosted by Mozilla at
accounts.firefox.com.  However, we don't want to observe what
alternate servers users might be using, so we can't collect the whole
URL.  Here, we filter the data based on whether the user is /not/
using accounts.firefox.com, and then record just the endpoint path.
Other collected data could expose that the user is using Firefox
Accounts, and together, that leaks the number of users not using
accounts.firefox.com.  We accept this leak: Mozilla already collects
data about whether Sync (both legacy and FxA) is using a custom server
in various situations: see the WEAVE_CUSTOM_* Telemetry histograms.

Comment 12

[Nick Alexander :nalexander [he/him]]

I've incorporated ally's

    "expires_in_version": "50",

throughout.  Although markh hasn't responded to https://bugzilla.mozilla.org/show_bug.cgi?id=1205705#c6, I dropped the FENNEC_ prefix, since it's likely Desktop will want something similar in the future.  I have an open item to address markh's more general about:accounts feedback elsewhere.

bsmedberg: ally isn't clear if "keyed opt-out probes" are accepted.  Can you tell me?  There's no particular reason this should be opt-out; I just copied this from other, similar, probes.

Comment 13

[Mark Hammond [:markh] [:mhammond]]

(In reply to Nick Alexander :nalexander from comment #12)
> Although markh hasn't responded to
> https://bugzilla.mozilla.org/show_bug.cgi?id=1205705#c6, I dropped the
> FENNEC_ prefix, since it's likely Desktop will want something similar in the
> future.  I have an open item to address markh's more general about:accounts
> feedback elsewhere.

Ack - sorry about that - I started looking at this, got distracted, then forgot to come back. I think dropping the prefix is good.

Comment 14

[Allison Naaktgeboren :ally]

(In reply to Nick Alexander :nalexander from comment #12)
> I've incorporated ally's
> 
>     "expires_in_version": "50",
> 
> throughout.  Although markh hasn't responded to
> https://bugzilla.mozilla.org/show_bug.cgi?id=1205705#c6, I dropped the
> FENNEC_ prefix, since it's likely Desktop will want something similar in the
> future.  I have an open item to address markh's more general about:accounts
> feedback elsewhere.
> 
> bsmedberg: ally isn't clear if "keyed opt-out probes" are accepted.  Can you
> tell me?  There's no particular reason this should be opt-out; I just copied
> this from other, similar, probes.

If there's no particular reason for this to be opt-out, then it probably shouldn't be opt out. Let's remove that and life gets easier for everyone involved. :)

Comment 15

[Allison Naaktgeboren :ally]

Comment on attachment 8663145 [details]
MozReview Request: Bug 1205705 - Part 2: Collect client-side fxa-content-server LOADED Telemetry. r=mfinkle r=ally

https://reviewboard.mozilla.org/r/19731/#review18525

p=ally with removal of the release collection opt out.

Comment 16

[Benjamin Smedberg]

"keyed" is a red herring. The question is about opt-out/release metrics in general. The bar for release metrics is very high: a clear question tied to user benefit, and a person who is committed to monitoring the incoming data and acting on it. Unless you need the release metrics, we should use the default telemetry metrics.

Comment 17

[Nick Alexander :nalexander [he/him]]

https://hg.mozilla.org/integration/fx-team/rev/1d8058a6dfc237db11e88ffcdb20362fa0dd9df0
Bug 1205705 - Part 1: Always resolve or reject the LOADED message promise. r=markh,mfinkle

https://hg.mozilla.org/integration/fx-team/rev/b508dd619d4df66c579aeaa42b7f105cf08fa609
Bug 1205705 - Part 2: Collect client-side fxa-content-server LOADED Telemetry. r=mfinkle,ally p=ally

Comment 18

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/1d8058a6dfc2
https://hg.mozilla.org/mozilla-central/rev/b508dd619d4d