Closed Bug 1180324 Opened 4 years ago Closed 4 years ago

Basic Fennec Sync health telemetry

Categories

(Firefox for Android :: Android Sync, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 42
Tracking Status
firefox42 --- fixed

People

(Reporter: nalexander, Assigned: ahmedibrahimkhali)

References

Details

Attachments

(1 file, 5 obsolete files)

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.
aminb: start with this ticket, but be sure to read the comments on the meta ticket that this one blocks (Bug 1180321).
Flags: needinfo?(me)
Attached patch sync_phases_telemetry.patch (obsolete) — Splinter Review
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.
Attachment #8634333 - Flags: review?(rnewman)
Attached patch sync_phases_telemetry.patch (obsolete) — Splinter Review
I've corrected the histogram names in the TelemetryContract since I forgot to refresh my patch queue with the latest changes.
Attachment #8634333 - Attachment is obsolete: true
Attachment #8634333 - Flags: review?(rnewman)
Attachment #8634337 - Flags: review?(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.
Attachment #8634337 - Flags: review?(rnewman) → review+
Attached patch sync_phases_telemetry.patch (obsolete) — Splinter Review
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.
Attachment #8634337 - Attachment is obsolete: true
Attachment #8638172 - Flags: review?(rnewman)
Attachment #8638172 - Flags: review?(rnewman) → review?(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".
Attachment #8638172 - Flags: review?(liuche) → review+
Attached patch sync_phases_telemetry.patch (obsolete) — Splinter Review
Added liuche's suggestion of renaming the "FENNEC_SYNC_NUMBER_OF_SYNCS_FAILED_TOO_SOON" historgram to "FENNEC_SYNC_NUMBER_OF_SYNCS_FAILED_BACKOFF".
Attachment #8638172 - Attachment is obsolete: true
Attachment #8640204 - Flags: review?(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
Attachment #8640204 - Flags: review?(liuche) → review+
Attached patch sync_phases_telemetry.patch (obsolete) — Splinter Review
Fixed histogram description.
Attachment #8640204 - Attachment is obsolete: true
Attachment #8641348 - Flags: review+
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
Attachment #8641348 - Attachment is obsolete: true
Attachment #8643981 - Flags: review+
Keywords: checkin-needed
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.
https://hg.mozilla.org/mozilla-central/rev/71b1b6b80f8f
Assignee: nobody → ahmedibrahimkhali
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 42
Flags: needinfo?(me)
Product: Android Background Services → Firefox for Android
You need to log in before you can comment on or make changes to this bug.