Closed Bug 1301389 Opened 8 years ago Closed 8 years ago

Sync Ping shouldn't include FxA UID or Device ID multiple times

Categories

(Firefox :: Sync, defect, P1)

Product:
Firefox
Component:
Sync
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
Firefox 53
Tracking Flags:
Tracking Status
firefox53 --- fixed

People

(Reporter: tcsc, Assigned: tcsc)

References

Details

Attachments

(1 file)

Bug 1301389 - Only include the uid and device ID once in the sync ping.
58 bytes, text/x-review-board-request
markh
: review+
Details
Bug 1299784 comment 6 brings the point that it's very redundant to include these in every item in the sync ping's "syncs" array. Instead we should include it once and submit whatever we have (if we have anything) if it would change.
Priority: -- → P3
See Also: → 1314864
Priority: P3 → P2
Doing this simultaneously with 1314864 because we should have the format described somewhere before we land the server code expecting it.
Assignee: nobody → tchiovoloni
Status: NEW → ASSIGNED
Priority: P2 → P1
Comment on attachment 8809521 [details] Bug 1301389 - Only include the uid and device ID once in the sync ping. https://reviewboard.mozilla.org/r/92078/#review92062 ::: services/sync/tests/unit/test_telemetry.js (Diff revision 1) > } catch(ex) { > error = ex; > } > ok(!!error); > ok(!!ping); > - equal(ping.uid, "0".repeat(32)); It's difficult to get the "record" ping here, so I just ditched this check. It's not testing anything important anyway. ::: toolkit/components/telemetry/docs/data/sync-ping.rst:20 (Diff revision 1) > { > version: 4, > type: "sync", > ... common ping data > payload: { > version: 1, Not sure if I should bump the version. It's easier in the server telemetry code AFAICT for me to just handle both ignoring the version number, but if there were other consumers of the data, well, this *is* a backwards incompatible change.
Comment on attachment 8809521 [details] Bug 1301389 - Only include the uid and device ID once in the sync ping. https://reviewboard.mozilla.org/r/92078/#review93324 LGTM apart for the issue I raise below. ::: services/sync/modules/telemetry.js:494 (Diff revision 1) > log.warn("onSyncFinished but we aren't recording"); > return; > } > this.current.finished(error); > + if (this.payloads.length) { > + if (this.lastUID != this.current.uid || this.lastDeviceID != this.current.deviceID) { Even with the change from bug 1317589, I believe there will be cases where the first few syncs have no UID and then it suddenly appears - eg, startup for a user with a master-password, or where the first few syncs fail to grab a token. I think we should do either: * handle the "no uid" case slightly better - ie, a transition from "no uid" -> "valid uid" would not trigger this, but a transition between 2 valid UIDs would. * add observers for ONLOGIN and ONLOGOUT observers and unconditionally submit a ping at that time, and unconditionally using the last valid UID we saw (ie, while still taking into account that the first few syncs may not have a token) We should also manually tests these cases (ie, master-password and firefox starting without a network)
Attachment #8809521 - Flags: review?(markh)
Comment on attachment 8809521 [details] Bug 1301389 - Only include the uid and device ID once in the sync ping. https://reviewboard.mozilla.org/r/92078/#review93324 > Even with the change from bug 1317589, I believe there will be cases where the first few syncs have no UID and then it suddenly appears - eg, startup for a user with a master-password, or where the first few syncs fail to grab a token. > > I think we should do either: > * handle the "no uid" case slightly better - ie, a transition from "no uid" -> "valid uid" would not trigger this, but a transition between 2 valid UIDs would. > * add observers for ONLOGIN and ONLOGOUT observers and unconditionally submit a ping at that time, and unconditionally using the last valid UID we saw (ie, while still taking into account that the first few syncs may not have a token) > > We should also manually tests these cases (ie, master-password and firefox starting without a network) stoopid mozreview - the comment above had 2 bullet points :/
Comment on attachment 8809521 [details] Bug 1301389 - Only include the uid and device ID once in the sync ping. https://reviewboard.mozilla.org/r/92078/#review93324 > stoopid mozreview - the comment above had 2 bullet points :/ I'd rather not use the observer approach since it breaks TPS and seems like it generally might be less reliable to me. AFAICT we should be able to catch any changes that occur this way, although slower in the logout case (likely not until shutdown unless you login again, and we'd submit then with why=shutdown and not why=idchange). The master password approach appears to work from manual testing but I'm unclear on what you want me to test for firefox starting without a network. Do you mean e.g. you're logged into FxA already and we start without a network and try to sync, then sign on, and sync (and we shouldn't submit an idchange ping in this case?)
Comment on attachment 8809521 [details] Bug 1301389 - Only include the uid and device ID once in the sync ping. https://reviewboard.mozilla.org/r/92078/#review94824 This looks great, but I don't think it should land until your PR for the parquet view is merged (as otherwise I fear we will reject them)
Attachment #8809521 - Flags: review?(markh) → review+
Pushed by tchiovoloni@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/801e4543e990 Only include the uid and device ID once in the sync ping. r=markh
https://hg.mozilla.org/mozilla-central/rev/801e4543e990
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
status-firefox53: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 53
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: