1236383 - Update FxA telemetry based on findings of bug 1206978

Status: RESOLVED FIXED

(Firefox :: Sync, defect)

Description

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

Bug 1206978/https://docs.google.com/a/mozilla.com/document/d/1NaowX6dzh0iVHf5xYwHodj5c8pstyTdBOkkXApTAc6M/edit?usp=sharing has some suggestions for improving the FxA telemetry (including removal of some probes, tweaks of others, and some new probes). This bug is to implement that.

Comment 1

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

CC Georg as I'll be asking for (non-urgent) review when he returns from PTO tomorrow.

Comment 2

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

Attachment: 0001-Bug-1236383-part-1-remove-and-rework-some-Sync-FxA-t.patch

Georg,
 If you see the document referenced in comment 0, you will see that of all the probes we collect for Sync and FxA, we've decided to remove some (where the value is low and we can get rough figured from the server), add some (where the results were ambigious), and make a few never expire (where we consider them "core health" indicators and bug 1234415 is to create a dashboard for them). I hope none of these are particularly controversial, but I'm happy to further explain any that are.

(I can't flag Georg at the moment but his PTO ends tomorrow (today for me ;) and I'll flag him then.

Kit,
  You landed these probes in the first place, so it makes sense for you to give it a quick look too if you don't mind.

Comment 3

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

Attachment: 0002-Bug-1236383-part-2-remove-the-now-unused-2nd-param-t.patch

This patch removes the second param to checkServerError as it is no longer used. Kit originally added this when the probes landed, so requesting review from him.

Comment 4

[Lina Butler [:lina]]

Comment on attachment 8703490 [details] [diff] [review]
0001-Bug-1236383-part-1-remove-and-rework-some-Sync-FxA-t.patch

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

Looks good! Just a few nits.

::: services/fxaccounts/FxAccountsClient.jsm
@@ -427,3 @@
>            Services.telemetry.getKeyedHistogramById(
>              "FXA_HAWK_ERRORS").add(path);
> -        } else if (error.code >= 500 || error.code in STATUS_CODE_TO_OTHER_ERRORS_LABEL) {

Could you remove `STATUS_CODE_TO_OTHER_ERRORS_LABEL`, too, please?

::: services/sync/modules/browserid_identity.js
@@ +496,4 @@
>            result = STATUS_OK;
>          } else {
>            result = LOGIN_FAILED_LOGIN_REJECTED;
> +          Services.telemetry.getHistogramById("WEAVE_HAS_NO_KEYS_WHEN_UNLOCKED").add(1);

I hope we see more useful numbers for this one. :-)

::: toolkit/components/telemetry/Histograms.json
@@ +9661,3 @@
>      "kind": "count",
>      "releaseChannelCollection": "opt-out",
> +    "description": "Errors encountered fetching Sync keys, including network errors."

Looks like this isn't recorded for network errors anymore.

Comment 5

[Lina Butler [:lina]]

Comment on attachment 8703491 [details] [diff] [review]
0002-Bug-1236383-part-2-remove-the-now-unused-2nd-param-t.patch

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

Thanks!

Comment 6

[Lina Butler [:lina]]

Comment on attachment 8703490 [details] [diff] [review]
0001-Bug-1236383-part-1-remove-and-rework-some-Sync-FxA-t.patch

Georg, welcome back! :-) Could you take a look at this when it's convenient, please?

Comment 7

[Georg Fritzsche [:gfritzsche]]

Thanks :)
Sure, i'll catch up on this and its background tomorrow.

Comment 8

[Georg Fritzsche [:gfritzsche]]

Comment on attachment 8703490 [details] [diff] [review]
0001-Bug-1236383-part-1-remove-and-rework-some-Sync-FxA-t.patch

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

I assume its sufficient for me to look over the Histograms.json changes.

The overview document listing the status of the individual existing probes is great.
Is there a similar overview listing out the probes that need to be added?

This seems to have a good plan for monitoring etc. in place and looks reasonable to me.
Did a data collection peer have a look over the plan to sign off on it (bsmedberg or ally, vladan is on PTO)?

::: toolkit/components/telemetry/Histograms.json
@@ +9600,5 @@
>      "description": "Time spent waiting for a navigator.requestMediaKeySystemAccess call to fail."
>    },
>    "FXA_CONFIGURED": {
>      "alert_emails": ["[e-mail]"],
> +    "expires_in_version": "never",

Add bug_numbers here too pointing to this bug?
E.g. the document linked here could be pretty helpful if anyone needs to dig.

@@ +9616,4 @@
>    },
>    "FXA_HAWK_ERRORS": {
>      "alert_emails": ["[e-mail]"],
> +    "expires_in_version": "49",

Add bug_numbers? Ditto for the ones below.

Comment 10

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

Attachment: 0002-Bug-1236383-part-1-remove-and-rework-some-Sync-FxA-t.patch

I've made the changes requested by both Kit and Georg and I'm carrying r+ from Kit forward.

Benjamin, I'm looking for data-steward approval - can you please have a look or delegate to someone else?

This patch is adjusting some of the telemetry for Sync/FxA. The reasoning for these changes can be found at https://docs.google.com/a/mozilla.com/document/d/1NaowX6dzh0iVHf5xYwHodj5c8pstyTdBOkkXApTAc6M/edit?usp=sharing, and the ongoing use and monitoring these probes can be found in bug 1232050 comment 10.

Summary:

* Keys WEAVE_CUSTOM_LEGACY_SERVER_CONFIGURATION, WEAVE_CUSTOM_FXA_SERVER_CONFIGURATION, WEAVE_CUSTOM_TOKEN_SERVER_CONFIGURATION, FXA_SERVER_ERRORS, TOKENSERVER_AUTH_ERRORS, WEAVE_CAN_FETCH_KEYS and WEAVE_FXA_KEY_FETCH_ERRORS have been removed.

* FXA_CONFIGURED and WEAVE_ENGINE_SYNC_ERRORS are set to "never" expire as they will be a key measurement for ongoing Sync health.

* FXA_UNVERIFIED_ACCOUNT_ERRORS has been replaced with FXA_SECURE_CREDENTIALS_SAVE_WITH_MP_LOCKED to more accurately measure what we want to measure. Expires in 49.

* WEAVE_HMAC_ERRORS has been replaced with WEAVE_FXA_KEY_FETCH_AUTH_ERRORS to more accurately measure what we want to measure. Expires in 49.

* WEAVE_STORAGE_AUTH_ERRORS has been replaced with WEAVE_HAS_NO_KEYS_WHEN_UNLOCKED to more accurately measure what we want to measure. Expires in 49.

* FXA_HAWK_ERRORS, WEAVE_ENGINE_APPLY_NEW_FAILURES, WEAVE_ENGINE_APPLY_FAILURES has been set to expire in 49 (was 47) as we continue to analyze the data given other changes that should shift these metrics.

Comment 11

[Benjamin Smedberg]

Comment on attachment 8706211 [details] [diff] [review]
0002-Bug-1236383-part-1-remove-and-rework-some-Sync-FxA-t.patch

As with WEAVE_CONFIGURED in the other bug, I expect that we should do something better with FX_CONFIGURED so that it's recorded properly in subsessions, so that deserves a followup.

Comment 12

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

(In reply to Benjamin Smedberg  [:bsmedberg] from comment #11)
> As with WEAVE_CONFIGURED in the other bug, I expect that we should do
> something better with FX_CONFIGURED so that it's recorded properly in
> subsessions, so that deserves a followup.

I opened bug 1238810

Comment 13

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/2e8b18c4a327
https://hg.mozilla.org/integration/fx-team/rev/b7d00293390b

Comment 14

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/2e8b18c4a327
https://hg.mozilla.org/mozilla-central/rev/b7d00293390b

Comment 17

[Mayur Patil]

[bugday-20160323]

Status: RESOLVED,FIXED -> UNVERIFIED

Comments:
STR: Not clear.
Developer specific testing

Component: 
Name			Firefox
Version			46.0b9
Build ID		20160322075646
Update Channel          beta
User Agent		Mozilla/5.0 (Windows NT 6.1; WOW64; rv:46.0) Gecko/20100101 Firefox/46.0
OS			Windows 7 SP1 x86_64

Expected Results: 
Developer specific testing

Actual Results: 
As expected