Closed Bug 1435067 Opened 2 years ago Closed 2 years ago

Use the pingSender for sending the sync ping

Categories

(Firefox :: Sync, enhancement, P2)

enhancement

Tracking

()

RESOLVED FIXED
Firefox 60
Tracking Status
firefox60 --- fixed

People

(Reporter: tcsc, Assigned: tcsc)

Details

Attachments

(1 file)

I guess you need to opt into this. Trivial patch incoming.
Comment on attachment 8947636 [details]
Bug 1435067 - Use the pingsender when sending the sync ping

https://reviewboard.mozilla.org/r/217344/#review223130

LGTM, but I think we should also get review from Georg.
Attachment #8947636 - Flags: review?(markh) → review+
Priority: -- → P2
Comment on attachment 8947636 [details]
Bug 1435067 - Use the pingsender when sending the sync ping

We'd like to do this, if there's no reason not to.
Attachment #8947636 - Flags: review?(gfritzsche)
Comment on attachment 8947636 [details]
Bug 1435067 - Use the pingsender when sending the sync ping

https://reviewboard.mozilla.org/r/217344/#review225530

This looks fine, but lets hold this back from landing until bug 1437120 is understood.
This patch would likely shadow what's going on there.
Attachment #8947636 - Flags: review?(gfritzsche) → review+
Comment on attachment 8947636 [details]
Bug 1435067 - Use the pingsender when sending the sync ping

https://reviewboard.mozilla.org/r/217344/#review225664

::: services/sync/modules/telemetry.js:499
(Diff revision 1)
>      // know that the ping was built. We don't end up submitting them, however.
>      let numEvents = record.events ? record.events.length : 0;
>      if (record.syncs.length || numEvents) {
>        log.trace(`submitting ${record.syncs.length} sync record(s) and ` +
>                  `${numEvents} event(s) to telemetry`);
> -      TelemetryController.submitExternalPing("sync", record);
> +      TelemetryController.submitExternalPing("sync", record, { usePingSender: true });

Opening a mozreview issue to block this, so that I don't forget and land it accidentally.
Bug 1436657 was fixed, but i'd like us verify this by looking at Nightly data first in bug 1439564.
Landing this after we should then see a direct improvement in latency.
Bug 1437120 was uplifted to beta, validated and is not blocking this bug anymore.
Thanks for waiting a bit!
Pushed by tchiovoloni@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/3aa4c86bbf16
Use the pingsender when sending the sync ping r=gfritzsche,markh
https://hg.mozilla.org/mozilla-central/rev/3aa4c86bbf16
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 60
Could this have caused http://alerts.telemetry.mozilla.org/index.html#/detectors/1/metrics/1422/alerts/?from=2018-03-02&to=2018-03-02 ? There's a slight increase in the number of TELEMETRY_PENDING_LOAD_FAILURE_READ counts (exhibits as a sharp increase on the 95th %ile: https://mzl.la/2Hk6dkm ).

before: https://mzl.la/2p2anGg
after: https://mzl.la/2p0P6N2
Flags: needinfo?(tchiovoloni)
Flags: needinfo?(gfritzsche)
Markh, can you check and see if our ping volume has dropped any using the script you used for bug 1439564?
Flags: needinfo?(tchiovoloni) → needinfo?(markh)
Bug 1437120 landed on Nightly on Feb 15. This landed on Mar 02. https://gist.github.com/mhammond/56e58de91226a942fee19c7431c384b2 shows an increase around the middle of Feb but shows nothing significant at the start of March, which implies to me that this bug hasn't caused pings to be lost.
Flags: needinfo?(markh)
That's good to hear, thank you. Well, it's bad that I can't blame this alert on losing sync pings, but it's good we're not actually losing pings :S

I have some load failures in my main pings of the recent past. I'll turn telemetry logging all the way on and see if I can catch these read failures in the act.
I'm not sure how this could be related to the ping sender, sorry.
Flags: needinfo?(gfritzsche)
You need to log in before you can comment on or make changes to this bug.