1374270 - Support sending health ping on shutdown via PingSender

Status: RESOLVED FIXED

(Toolkit :: Telemetry, enhancement, P1)

Comment 1

[Kate Ustiuzhanina]

Attachment: shutdownViaPingSender.patch

Comment 2

[Georg Fritzsche [:gfritzsche]]

Comment on attachment 8889431 [details] [diff] [review]
shutdownViaPingSender.patch

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

Alessio, can you take a look?

Comment 3

[Alessio Placitelli [:Dexter]]

Comment on attachment 8889431 [details] [diff] [review]
shutdownViaPingSender.patch

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

This looks good, there are just a a couple of observations below. I was also thinking about docs: are you mentioning anywhere, in the docs, that the ping is going to use the pingsender under certain circumstances (shutdown)?

::: toolkit/components/telemetry/tests/unit/test_TelemetryController.js
@@ +98,4 @@
>  
>    Services.prefs.setBoolPref(TelemetryUtils.Preferences.TelemetryEnabled, true);
>    Services.prefs.setBoolPref(TelemetryUtils.Preferences.FhrUploadEnabled, true);
> +  Preferences.set("toolkit.telemetry.healthping.enabled", false);

If this pref is also being used in TelemetryHealthPing.jsm and other tests, could you please add it to http://searchfox.org/mozilla-central/source/toolkit/components/telemetry/TelemetryUtils.jsm#36 and reference that instead?

::: toolkit/components/telemetry/tests/unit/test_TelemetryHealthPing.js
@@ +188,5 @@
> +
> +  TelemetryHealthPing.recordSendFailure("testFailure");
> +  let nextRequest = PingServer.promiseNextRequest();
> +
> +  await TelemetryController.testReset();

Random note here: please make sure that each TelemetryController.testReset or testSetup call has a corresponding call to testShutdown. Otherwise, things will get messy in the future (double pings being received, intermittent failures, etc.).

::: toolkit/components/telemetry/tests/unit/test_TelemetrySession.js
@@ +479,4 @@
>  
>    Services.prefs.setBoolPref(TelemetryUtils.Preferences.TelemetryEnabled, true);
>    Services.prefs.setBoolPref(TelemetryUtils.Preferences.FhrUploadEnabled, true);
> +  Preferences.set("toolkit.telemetry.healthping.enabled", false);

Please use the definition from TelemetryUtils.jsm

Comment 4

[Kate Ustiuzhanina]

(In reply to Alessio Placitelli [:Dexter] from comment #3)
> Comment on attachment 8889431 [details] [diff] [review]
> shutdownViaPingSender.patch
> 
> Review of attachment 8889431 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This looks good, there are just a a couple of observations below. I was also
> thinking about docs: are you mentioning anywhere, in the docs, that the ping
> is going to use the pingsender under certain circumstances (shutdown)?
> 
> ::: toolkit/components/telemetry/tests/unit/test_TelemetryController.js
> @@ +98,4 @@
> >  
> >    Services.prefs.setBoolPref(TelemetryUtils.Preferences.TelemetryEnabled, true);
> >    Services.prefs.setBoolPref(TelemetryUtils.Preferences.FhrUploadEnabled, true);
> > +  Preferences.set("toolkit.telemetry.healthping.enabled", false);
> 
> If this pref is also being used in TelemetryHealthPing.jsm and other tests,
> could you please add it to
> http://searchfox.org/mozilla-central/source/toolkit/components/telemetry/
> TelemetryUtils.jsm#36 and reference that instead?
> 
> ::: toolkit/components/telemetry/tests/unit/test_TelemetryHealthPing.js
> @@ +188,5 @@
> > +
> > +  TelemetryHealthPing.recordSendFailure("testFailure");
> > +  let nextRequest = PingServer.promiseNextRequest();
> > +
> > +  await TelemetryController.testReset();
> 
> Random note here: please make sure that each TelemetryController.testReset
> or testSetup call has a corresponding call to testShutdown. Otherwise,
> things will get messy in the future (double pings being received,
> intermittent failures, etc.).
> 
> ::: toolkit/components/telemetry/tests/unit/test_TelemetrySession.js
> @@ +479,4 @@
> >  
> >    Services.prefs.setBoolPref(TelemetryUtils.Preferences.TelemetryEnabled, true);
> >    Services.prefs.setBoolPref(TelemetryUtils.Preferences.FhrUploadEnabled, true);
> > +  Preferences.set("toolkit.telemetry.healthping.enabled", false);
> 
> Please use the definition from TelemetryUtils.jsm

Hi Alessio, 
Thank you for the review. Will update docs.

Comment 5

[Kate Ustiuzhanina]

Attachment: shutdownViaPingSender.patch

Move "toolkit.telemetry.healthping.enabled" pref to TelemetryUtils in pathch for this bug: https://bugzilla.mozilla.org/show_bug.cgi?id=1374270

Comment 6

[Alessio Placitelli [:Dexter]]

Comment on attachment 8890300 [details] [diff] [review]
shutdownViaPingSender.patch

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

Looks good, thanks Kate!

Comment 7

[Alessio Placitelli [:Dexter]]

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

Comment 8

[Alessio Placitelli [:Dexter]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=942b8c12f2f4c5fb620280fd079c76ff9d1baa0b

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/bb2eeb5a2f4a
Support sending health ping on shutdown via PingSender. r=Dexter

Comment 10

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Sorry, this had to be backed out for failing toolkit/components/telemetry/tests/unit/test_TelemetryHealthPing.js, at least on Android and Linux:

https://hg.mozilla.org/integration/mozilla-inbound/rev/6a51dec278cc313330d877902f1a650061680e91

Push with failure: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=bb2eeb5a2f4aedf6247b01260b00f41dd9691c21&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=retry&filter-resultStatus=usercancel
Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=118577793&repo=mozilla-inbound
[task 2017-07-27T18:10:17.775247Z] 18:10:17     INFO -  TEST-START | toolkit/components/telemetry/tests/unit/test_TelemetryHealthPing.js
[task 2017-07-27T18:15:17.775765Z] 18:15:17  WARNING -  TEST-UNEXPECTED-TIMEOUT | toolkit/components/telemetry/tests/unit/test_TelemetryHealthPing.js | Test timed out
[task 2017-07-27T18:15:17.776370Z] 18:15:17     INFO -  TEST-INFO took 300000ms

Please fix the issue and submit an updated patch. Thank you.

Comment 11

[Kate Ustiuzhanina]

(In reply to Sebastian Hengst [:aryx][:archaeopteryx] (needinfo on intermittent or backout) from comment #10)
> Sorry, this had to be backed out for failing
> toolkit/components/telemetry/tests/unit/test_TelemetryHealthPing.js, at
> least on Android and Linux:
> 
> https://hg.mozilla.org/integration/mozilla-inbound/rev/
> 6a51dec278cc313330d877902f1a650061680e91
> 
> Push with failure:
> https://treeherder.mozilla.org/#/jobs?repo=mozilla-
> inbound&revision=bb2eeb5a2f4aedf6247b01260b00f41dd9691c21&filter-
> resultStatus=testfailed&filter-resultStatus=busted&filter-
> resultStatus=exception&filter-resultStatus=retry&filter-
> resultStatus=usercancel
> Failure log:
> https://treeherder.mozilla.org/logviewer.html#?job_id=118577793&repo=mozilla-
> inbound
> [task 2017-07-27T18:10:17.775247Z] 18:10:17     INFO -  TEST-START |
> toolkit/components/telemetry/tests/unit/test_TelemetryHealthPing.js
> [task 2017-07-27T18:15:17.775765Z] 18:15:17  WARNING - 
> TEST-UNEXPECTED-TIMEOUT |
> toolkit/components/telemetry/tests/unit/test_TelemetryHealthPing.js | Test
> timed out
> [task 2017-07-27T18:15:17.776370Z] 18:15:17     INFO -  TEST-INFO took
> 300000ms
> 
> Please fix the issue and submit an updated patch. Thank you.

Hi Sebastian, thank you for the information. It seems that I know what could be wrong with Android, but what is problem with Linux tests? Is it the same (I can't see timeout on test_TelemetryHealthPing.js among Linnux tests.)

Comment 12

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Hi Kate, on Linux the error message is similar, see the tc-X(X7) for Linux in https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=f682a94b747439e679ccf3f5d2983b15152cfe93&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception

Be aware that there is also another health ping failure floating around which happens intermittently even without your patch applied: https://treeherder.mozilla.org/logviewer.html#?job_id=118775238&repo=autoland

Comment 13

[Kate Ustiuzhanina]

Attachment: shutdownViaPingSender.patch

Hi Alessio, it seems that I forget to check that we are not using Android, that is why PingServer never get shutdown ping and I get timeout error with this OS. 

Fixed with this patch.

Comment 14

[Alessio Placitelli [:Dexter]]

Comment on attachment 8891299 [details] [diff] [review]
shutdownViaPingSender.patch

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

Looks good with the nits below addressed.

::: toolkit/components/telemetry/tests/unit/test_TelemetryHealthPing.js
@@ +181,5 @@
>  });
>  
> +add_task(async function test_usePingSenderOnShutdown() {
> +  if (gIsAndroid ||
> +    (AppConstants.platform == "linux" && OS.Constants.Sys.bits == 32)) {

nit: please align the "(" before AppConstants to the "g" of gIsAndroid on the previous line

@@ +188,5 @@
> +    // Linux 32 bit (due to missing libraries). So skip it there too.
> +    // See bug 1310703 comment 78.
> +    return;
> +  }
> +  

nit: trailing whitespace

Comment 15

[Kate Ustiuzhanina]

Attachment: shutdownViaPingSender.patch

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/db29261fe4c7
Support sending health ping on shutdown via PingSender. r=Dexter

Comment 17

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

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