1180324 - Basic Fennec Sync health telemetry

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: Android Sync, defect)

Description

[Nick Alexander :nalexander [he/him]]

This is a good first part of Bug 1180321.

The goal is to use TelemetryWrapper to track how many syncs start, how many syncs finish, and what the status of those syncs are (success, failure, error).  For bonus points, it would be nice to know how many syncs are triggered but rejected for being too soon after a recent sync.

The sync code is all delegate (callback) based, so there are good hooks for most of these conditions already.  The relevant code is all around https://dxr.mozilla.org/mozilla-central/source/mobile/android/base/fxa/sync/FxAccountSyncAdapter.java.  (I don't care about old sync telemetry, since it's being deprecated.)

We might include the telemetry calls in that file; or we might put them in the SessionCallback (https://dxr.mozilla.org/mozilla-central/source/mobile/android/base/fxa/sync/FxAccountSyncAdapter.java#130); or they might go in the SyncDelegate (https://dxr.mozilla.org/mozilla-central/source/mobile/android/base/fxa/sync/FxAccountSyncAdapter.java#86).  I think the SyncDelegate looks most promising, but I haven't dug into it yet.

Comment 1

[Nick Alexander :nalexander [he/him]]

aminb: start with this ticket, but be sure to read the comments on the meta ticket that this one blocks (Bug 1180321).

Comment 2

[Ahmed Khalil]

Attachment: sync_phases_telemetry.patch

This patch tracks the questions of "How many sync started ?" and "How many sync Finished?", If the patch was in the right direction, I'll add other telemetries like 
- How many times a sync is rejected after a recent sync.
- How many times sync has failed because of an error.

Comment 3

[Ahmed Khalil]

Attachment: sync_phases_telemetry.patch

I've corrected the histogram names in the TelemetryContract since I forgot to refresh my patch queue with the latest changes.

Comment 4

[Richard Newman [:rnewman]]

Comment on attachment 8634337 [details] [diff] [review]
sync_phases_telemetry.patch

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

::: mobile/android/base/fxa/sync/FxAccountSyncAdapter.java
@@ +89,5 @@
>      @Override
>      public void handleSuccess() {
>        Logger.info(LOG_TAG, "Sync succeeded.");
>        super.handleSuccess();
> +      TelemetryWrapper.addToHistogram(TelemetryContract.SYNC_FINISHED, 1);

I'm not 100% sure about the name for this. After all, dying with an error finishes the sync, so this is really "finished with no unhandled errors", which we might term "SUCCEEDED" or "COMPLETED".

Think about it and make a decision.

Comment 5

[Ahmed Khalil]

Attachment: sync_phases_telemetry.patch

I've renamed the SYNC_FINISHED to SYNC_COMPLETED and added SYNC_FAILED for handling failure cases and also added SYNC_FAILED_TOO_SOON, for indicating a too soon resync after a recent sync.

Comment 6

[Chenxia Liu [:liuche]]

Comment on attachment 8638172 [details] [diff] [review]
sync_phases_telemetry.patch

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

r+ with one comment.

::: toolkit/components/telemetry/Histograms.json
@@ +8318,5 @@
> +    "expires_in_version": "45",
> +    "kind": "count",
> +    "description": "Counts the number of times that a sync has failed with errors."
> +  },
> +  "FENNEC_SYNC_NUMBER_OF_SYNCS_FAILED_TOO_SOON": {

I would rename this to be SYNC_FAILED_BACKOFF, to indicate that this is a failure when we are trying to sync before the server backoff interval has passed, and update this everywhere else. Also update the description - "...trying to sync before server backoff interval has passed".

Comment 7

[Ahmed Khalil]

Attachment: sync_phases_telemetry.patch

Added liuche's suggestion of renaming the "FENNEC_SYNC_NUMBER_OF_SYNCS_FAILED_TOO_SOON" historgram to "FENNEC_SYNC_NUMBER_OF_SYNCS_FAILED_BACKOFF".

Comment 8

[Chenxia Liu [:liuche]]

Comment on attachment 8640204 [details] [diff] [review]
sync_phases_telemetry.patch

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

Great, one last little typo, but after that you can just transfer the r+ over to the new patch, push to try, and request checkin-needed. No need to flag me for review again.

::: toolkit/components/telemetry/Histograms.json
@@ +8321,5 @@
> +  },
> +  "FENNEC_SYNC_NUMBER_OF_SYNCS_FAILED_BACKOFF": {
> +    "expires_in_version": "45",
> +    "kind": "count",
> +    "description": "Counts the number of times that a sync has failed because of trying to sync before server interval backoff has passed."

Nit: "backoff interval" not interval backoff

Comment 9

[Ahmed Khalil]

Attachment: sync_phases_telemetry.patch

Fixed histogram description.

Comment 10

[Ahmed Khalil]

Attachment: sync_phases_telemetry.patch

Changed the commit message to point to the ticket, and changed the reviewer name to "liuche".

Also posted the patch to the Treeherder and the build was success. 

https://treeherder.mozilla.org/#/jobs?repo=try&revision=293118e4688e

Comment 11

[Ryan VanderMeulen [:RyanVM]]

For future reference, your push to Try was on top of a revision from early July. Please make sure you push to something more current in the future as it basically invalidates your testing by not doing so.

Comment 12

[Pulsebot]

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

Comment 13

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/71b1b6b80f8f