1435067 - Use the pingSender for sending the sync ping

Status: RESOLVED FIXED

(Firefox :: Sync, enhancement, P2)

Description

[Thom Chiovoloni [:tcsc] (ex-moco)]

I guess you need to opt into this. Trivial patch incoming.

Comment 1

[Thom Chiovoloni [:tcsc] (ex-moco)]

Attachment: Bug 1435067 - Use the pingsender when sending the sync ping

Review commit: https://reviewboard.mozilla.org/r/217344/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/217344/

Comment 2

[Mark Hammond [:markh] [:mhammond]]

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.

Comment 3

[Thom Chiovoloni [:tcsc] (ex-moco)]

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.

Comment 4

[Georg Fritzsche [: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.

Comment 5

[Thom Chiovoloni [:tcsc] (ex-moco)]

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.

Comment 6

[Georg Fritzsche [:gfritzsche]]

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.

Comment 7

[Georg Fritzsche [:gfritzsche]]

Bug 1437120 was uplifted to beta, validated and is not blocking this bug anymore.
Thanks for waiting a bit!

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/3aa4c86bbf16
Use the pingsender when sending the sync ping r=gfritzsche,markh

Comment 9

[Andreea Pavel [:apavel]]

https://hg.mozilla.org/mozilla-central/rev/3aa4c86bbf16

Comment 10

[Chris H-C :chutten]

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

Comment 11

[Thom Chiovoloni [:tcsc] (ex-moco)]

Markh, can you check and see if our ping volume has dropped any using the script you used for bug 1439564?

Comment 12

[Mark Hammond [:markh] [:mhammond]]

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.

Comment 13

[Chris H-C :chutten]

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.

Comment 14

[Georg Fritzsche [:gfritzsche]]

I'm not sure how this could be related to the ping sender, sorry.