Closed Bug 1370668 Opened 2 years ago Closed 2 years ago

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

Categories

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

enhancement

Tracking

()

RESOLVED FIXED
Firefox 55
Tracking Status
firefox55 --- fixed

People

(Reporter: Grisha, Assigned: Grisha)

References

Details

Attachments

(1 file)

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 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?
Attachment #8875014 - Flags: review?(nalexander) → review+
(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).
Pushed by gkruglov@mozilla.com:
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
https://hg.mozilla.org/mozilla-central/rev/9581f9e1b988
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 55
You need to log in before you can comment on or make changes to this bug.