1375008 - Prioritize submission of Health ping

Status: RESOLVED FIXED

(Toolkit :: Telemetry, enhancement, P1)

Comment 1

[Kate Ustiuzhanina]

Attachment: prioritizeHealthPing.patch

Comment 2

[Georg Fritzsche [:gfritzsche]]

Comment on attachment 8889437 [details] [diff] [review]
prioritizeHealthPing.patch

Review of attachment 8889437 [details] [diff] [review]:
-----------------------------------------------------------------

This looks good overall, i just have some smaller comments and a test concern.

::: toolkit/components/telemetry/TelemetrySend.jsm
@@ +949,5 @@
>  
> +    currentPings =
> +      currentPings
> +        .filter(ping => ping.type === "health")
> +        .concat(currentPings.filter(ping => ping.type !== "health"));

Please add a comment on what and why (prioritize health pings to enable low-latency monitoring).

This way to write it is probably more clear (using the spread operator):
> currentPings = [
>   ... currentPings.filter(...),
>   ... currentPings.filter(...)
> ];

::: toolkit/components/telemetry/tests/unit/test_TelemetryHealthPing.js
@@ +115,5 @@
> +  // Fake now to be in throttled state
> +  let now = fakeNow(2050, 1, 2, 0, 0, 0);
> +  fakeMidnightPingFuzzingDelay(60 * 1000);
> +
> +  [PING_TYPE, PING_TYPE, "health", PING_TYPE].forEach(function (value) {

Why not just:
for (let value of [...]) {
  ...

@@ +121,5 @@
> +  });
> +
> +  // Move from throttled state
> +  fakeNow(futureDate(now, 5 * 60 * 1000));
> +  let pings = await PingServer.promiseNextPings(4);

This could end up waiting until the next SendScheduler activity is triggered.
We should make sure that this sends now to avoid this test taking long.

I think we could e.g.:
- just submitting another ping (ends up doing SendScheduler.triggerSendingPings(true))
- call TelemetrySend.notifyCanUpload()

@@ +122,5 @@
> +
> +  // Move from throttled state
> +  fakeNow(futureDate(now, 5 * 60 * 1000));
> +  let pings = await PingServer.promiseNextPings(4);
> +  Assert.equal(pings[0].type, "health", "Should have recorded a health ping.");

"Should have received the health ping first."

Comment 3

[Kate Ustiuzhanina]

Attachment: prioritizeHealthPing.patch

Added this lines to test. Test works file, although it worked locally fine too.

await TelemetryController.submitExternalPing(PING_TYPE, {});
await TelemetrySend.notifyCanUpload();
var scheduler = Cu.import("resource://gre/modules/TelemetrySend.jsm", {});
scheduler.SendScheduler.triggerSendingPings(true);

Comment 4

[Georg Fritzsche [:gfritzsche]]

Comment on attachment 8890398 [details] [diff] [review]
prioritizeHealthPing.patch

Review of attachment 8890398 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks, this looks good with the below addressed.

::: toolkit/components/telemetry/tests/unit/test_TelemetryHealthPing.js
@@ +113,5 @@
> +  TelemetryHealthPing.testReset();
> +
> +  let PING_TYPE = "priority-ping";
> +
> +  // Fake now to be in throttled state

Small nit: End comments with ".".
"Make sure sending is throttled to batch up pings."?

@@ +121,5 @@
> +  for (let value of [PING_TYPE, PING_TYPE, "health", PING_TYPE]) {
> +    TelemetryController.submitExternalPing(value, {});
> +  }
> +
> +  // Move from throttled state

"Now trigger sending pings again."?

@@ +124,5 @@
> +
> +  // Move from throttled state
> +  fakeNow(futureDate(now, 5 * 60 * 1000));
> +  await TelemetryController.submitExternalPing(PING_TYPE, {});
> +  await TelemetrySend.notifyCanUpload();

One of these two lines should be enough here.

Comment 5

[Kate Ustiuzhanina]

Attachment: prioritizeHealthPing.patch

Thank you for the review. 
Coul you, please, push on try?

Comment 6

[Georg Fritzsche [:gfritzsche]]

try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=9bfa87fb1d89e2a3b95b3367cde33a34143a0b5f

Comment 7

[Kate Ustiuzhanina]

Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=cc9c73618d0b81f13e390401ef06c1b070a2e55a

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/e51091c6c11a
Prioritize submission of Health ping. r=gfritzsche

Comment 9

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/e51091c6c11a