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)
Firefox
Sync
Tracking
()
RESOLVED
FIXED
Firefox 53
Tracking | Status | |
---|---|---|
firefox53 | --- | fixed |
People
(Reporter: tcsc, Assigned: tcsc)
References
Details
Attachments
(1 file)
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.
Updated•8 years ago
|
Priority: -- → P3
Updated•8 years ago
|
Priority: P3 → P2
Assignee | ||
Comment 1•8 years ago
|
||
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 hidden (mozreview-request) |
Assignee | ||
Comment 3•8 years ago
|
||
mozreview-review |
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 4•8 years ago
|
||
mozreview-review |
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 5•8 years ago
|
||
mozreview-review-reply |
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 hidden (mozreview-request) |
Assignee | ||
Comment 7•8 years ago
|
||
mozreview-review-reply |
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 hidden (mozreview-request) |
Comment 9•8 years ago
|
||
mozreview-review |
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+
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 12•8 years ago
|
||
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
Comment 13•8 years ago
|
||
bugherder |
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.
Description
•