Many sync pings are missing device IDs
Categories
(Firefox :: Sync, defect, P1)
Tracking
()
Tracking | Status | |
---|---|---|
firefox-esr68 | --- | unaffected |
firefox75 | --- | unaffected |
firefox76 | --- | unaffected |
firefox77 | --- | fixed |
firefox78 | --- | fixed |
People
(Reporter: markh, Assigned: rfkelly)
References
(Regression)
Details
(Keywords: regression)
Attachments
(1 file)
47 bytes,
text/x-phabricator-request
|
pascalc
:
approval-mozilla-beta+
|
Details | Review |
I've many sync pings for my personal account which have, say:
"devices": [
{
"id": "00000000000000000000000000000000",
"type": "desktop",
"os": "Darwin",
"version": "77.0a1"
},
{
"id": "00000000000000000000000000000000",
"type": "desktop"
}
],
...
"uid": "2b...",
The ping reports a sync did happen (so we should have a token) and given we report the uid, we should be able to report the device IDs.
This is true for all recent pings, then they start appearing (on May 2). Without digging deeper and without real evidence, I'm suspecting bug 1604844
Reporter | ||
Comment 1•5 years ago
|
||
(oh - the top-level deviceID
field is also missing entirely, which I suspect is just a direct result of whatever the underlying issue is)
Assignee | ||
Comment 2•5 years ago
|
||
I'm suspecting bug 1604844
Oof, yep, the lack of a return
on this method would probably explain it:
Assignee | ||
Updated•5 years ago
|
Assignee | ||
Comment 3•5 years ago
|
||
The regressing patch landed in 77, flipping some flags to indicate this.
Assignee | ||
Comment 4•5 years ago
|
||
Assignee | ||
Comment 5•5 years ago
|
||
Mark, are you able to share instructions for inspecting sync pings from your own device, for QA purposes? I wasn't able to inspect this information via about:telemetry
.
Reporter | ||
Comment 6•5 years ago
|
||
It should be about:telemetry, then "current data" -> "archived ping data", then "ping type" can be selected where "sync" is an option - then "raw payload" to see the content.
Assignee | ||
Comment 7•5 years ago
|
||
Thanks, that did the trick! (I didn't realize the "current data" thing was a drop-down, for some reason).
The fix here seems to have resolved the issue.
The very first sync ping submitted after signing in has valid-looking deviceID
fields in the individual items in the devices
array, but not in the top-level fields of the ping. Subsequent pings appear to contain the deviceID
in the top-level fields as well.
Updated•5 years ago
|
Comment 9•5 years ago
|
||
bugherder |
Assignee | ||
Comment 10•5 years ago
|
||
Comment on attachment 9146437 [details]
Bug 1636053 - correctly include deviceID in the sync ping. r?markh
Beta/Release Uplift Approval Request
-
User impact if declined: No user-visible impact, but it will make analysis of sync-related telemetry pings much more difficult, because we won't be able to distinguish between pings sent from different devices on the same account.
-
Is this code covered by automated tests?: Yes
-
Has the fix been verified in Nightly?: Yes
-
Needs manual test from QE?: No
-
If yes, steps to reproduce: IMHO additional manual verification of this is not a good use of someone's time, but if desired, here's what I did to confirm the fix:
-
Sign in to Firefox and enable sync
-
Select "Accounts toolbar icon -> sync now" to force a sync
-
Restart the browser to flush the sync telemetry ping
-
Go to about:telemetry, change "current data" to "archived ping data", then change "ping type" to "sync"
-
Select "raw payload" to view the contents of the most recent sync ping
-
Find the "devices" field in the resulting JSON, and check that all of the items therein have an "id" field that is not a string of 32 zeros.
-
List of other uplifts needed: None
-
Risk to taking this patch: Low
-
Why is the change risky/not risky? (and alternatives if risky): It's a one-line fix for a pretty scrutable bug (failing to return a value from a method) and the affected code is all internal to the same file as the fix itself, so it seems very low risk to me.
-
String changes made/needed: None
Comment 11•5 years ago
|
||
Comment on attachment 9146437 [details]
Bug 1636053 - correctly include deviceID in the sync ping. r?markh
Low risk and has tests, uplift approved for the next beta, thanks.
![]() |
||
Comment 12•5 years ago
|
||
bugherder uplift |
Updated•5 years ago
|
Description
•