Closed Bug 1636053 Opened 5 years ago Closed 5 years ago

Many sync pings are missing device IDs

Categories

(Firefox :: Sync, defect, P1)

defect

Tracking

()

RESOLVED FIXED
Firefox 78
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)

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

(oh - the top-level deviceID field is also missing entirely, which I suspect is just a direct result of whatever the underlying issue is)

I'm suspecting bug 1604844

Oof, yep, the lack of a return on this method would probably explain it:

Assignee: nobody → rfkelly

The regressing patch landed in 77, flipping some flags to indicate this.

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.

Flags: needinfo?(markh)

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.

Flags: needinfo?(markh)

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.

Pushed by rkelly@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/f603312246a6 correctly include deviceID in the sync ping. r=markh
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 78

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

Attachment #9146437 - Flags: approval-mozilla-beta?

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.

Attachment #9146437 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Has Regression Range: --- → yes
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: