1370668 - Can't assume that neither uid nor deviceID are going to be present in the Sync Ping bundle

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: Metrics, enhancement, P1)

Description

[:Grisha Kruglov]

If sync fails before we have a chance to set uid&deviceID (for example, while talking to the token server), background telemetry receiver will fail to process the resulting bundle because it makes an assumption that these two keys are going to be present in the bundle that it received.

Comment 1

[:Grisha Kruglov]

Attachment: Bug 1370668 - Do not assume that uid and deviceID will be present in the sync telemetry bundle

It's possible that a sync fails very early, for example while talking to the token server. In that case,
we wouldn't have uid/deviceID available in the sync telemetry bundle.

Review commit: https://reviewboard.mozilla.org/r/146358/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/146358/

Comment 2

[:Grisha Kruglov]

Comment on attachment 8875014 [details]
Bug 1370668 - Do not assume that uid and deviceID will be present in the sync telemetry bundle

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/146358/diff/1-2/

Comment 3

[Nick Alexander :nalexander [he/him]]

Comment on attachment 8875014 [details]
Bug 1370668 - Do not assume that uid and deviceID will be present in the sync telemetry bundle

https://reviewboard.mozilla.org/r/146358/#review150388

::: mobile/android/base/java/org/mozilla/gecko/telemetry/TelemetryBackgroundReceiver.java:182
(Diff revision 2)
>                          context.getFileStreamPath(SYNC_BUNDLE_STORE_DIR), DEFAULT_PROFILE);
>  
>                  long lastAttemptedSyncPingUpload = sharedPreferences.getLong(PREF_LAST_ATTEMPTED_UPLOADED, 0L);
> -                boolean idsChanged = setOrUpdateIDsIfChanged(sharedPreferences, uid, deviceID);
>  
> +                // Default to not upload just because we didn't receive uid or deviceID.

nit: rework this sentence.  The "just" is misleading.  Do you want:

// Never upload if we don't have a uid and a deviceID.  If we don't, it means that Sync failed very early on, probably due to network errors talking to the token server.

Or similar?

Comment 4

[:Grisha Kruglov]

(In reply to Nick Alexander :nalexander from comment #3)
> Comment on attachment 8875014 [details]
> Bug 1370668 - Do not assume that uid and deviceID will be present in the
> sync telemetry bundle
> 
> https://reviewboard.mozilla.org/r/146358/#review150388
> 
> :::
> mobile/android/base/java/org/mozilla/gecko/telemetry/
> TelemetryBackgroundReceiver.java:182
> (Diff revision 2)
> >                          context.getFileStreamPath(SYNC_BUNDLE_STORE_DIR), DEFAULT_PROFILE);
> >  
> >                  long lastAttemptedSyncPingUpload = sharedPreferences.getLong(PREF_LAST_ATTEMPTED_UPLOADED, 0L);
> > -                boolean idsChanged = setOrUpdateIDsIfChanged(sharedPreferences, uid, deviceID);
> >  
> > +                // Default to not upload just because we didn't receive uid or deviceID.
> 
> nit: rework this sentence.  The "just" is misleading.  Do you want:
> 
> // Never upload if we don't have a uid and a deviceID.  If we don't, it
> means that Sync failed very early on, probably due to network errors talking
> to the token server.
> 
> Or similar?

It's not really "never" though, since we might still decide to upload for other reasons (didn't upload for a while, got enough data collected to warrant an upload, etc).

Comment 5

[:Grisha Kruglov]

Comment on attachment 8875014 [details]
Bug 1370668 - Do not assume that uid and deviceID will be present in the sync telemetry bundle

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/146358/diff/2-3/

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/9581f9e1b988
Do not assume that uid and deviceID will be present in the sync telemetry bundle r=nalexander

Comment 7

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/9581f9e1b988