Support sending health ping on shutdown via PingSender

RESOLVED FIXED in Firefox 56

Status

()

Toolkit
Telemetry
P1
normal
RESOLVED FIXED
11 months ago
10 months ago

People

(Reporter: Kate Ustiuzhanina, Assigned: Kate Ustiuzhanina)

Tracking

(Blocks: 1 bug)

unspecified
mozilla56
Points:
2
Dependency tree / graph

Firefox Tracking Flags

(firefox56 fixed)

Details

Attachments

(1 attachment, 3 obsolete attachments)

Comment hidden (empty)
Blocks: 1372228
Summary: Support sending Helath ping on shutdown via PingSender → Support sending health ping on shutdown via PingSender
Priority: -- → P3
(Assignee)

Updated

10 months ago
Priority: P3 → P1
(Assignee)

Comment 1

10 months ago
Created attachment 8889431 [details] [diff] [review]
shutdownViaPingSender.patch
Attachment #8889431 - Flags: review?(gfritzsche)
Comment on attachment 8889431 [details] [diff] [review]
shutdownViaPingSender.patch

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

Alessio, can you take a look?
Attachment #8889431 - Flags: review?(gfritzsche) → review?(alessio.placitelli)
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
Attachment #8889431 - Flags: review?(alessio.placitelli) → feedback+
(Assignee)

Comment 4

10 months ago
(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.
(Assignee)

Comment 5

10 months ago
Created attachment 8890300 [details] [diff] [review]
shutdownViaPingSender.patch

Move "toolkit.telemetry.healthping.enabled" pref to TelemetryUtils in pathch for this bug: https://bugzilla.mozilla.org/show_bug.cgi?id=1374270
Attachment #8889431 - Attachment is obsolete: true
(Assignee)

Updated

10 months ago
Attachment #8890300 - Flags: review?(alessio.placitelli)
Comment on attachment 8890300 [details] [diff] [review]
shutdownViaPingSender.patch

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

Looks good, thanks Kate!
Attachment #8890300 - Flags: review?(alessio.placitelli) → review+
(Assignee)

Updated

10 months ago
Keywords: checkin-needed

Comment 9

10 months ago
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/bb2eeb5a2f4a
Support sending health ping on shutdown via PingSender. r=Dexter
Keywords: checkin-needed
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.
Flags: needinfo?(kustiuzhanina)
(Assignee)

Comment 11

10 months ago
(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.)
Flags: needinfo?(kustiuzhanina) → needinfo?(aryx.bugmail)
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
Flags: needinfo?(aryx.bugmail)
(Assignee)

Comment 13

10 months ago
Created attachment 8891299 [details] [diff] [review]
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.
Attachment #8890300 - Attachment is obsolete: true
Attachment #8891299 - Flags: review?(alessio.placitelli)
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
Attachment #8891299 - Flags: review?(alessio.placitelli) → review+
(Assignee)

Comment 15

10 months ago
Created attachment 8891371 [details] [diff] [review]
shutdownViaPingSender.patch
Attachment #8891299 - Attachment is obsolete: true
(Assignee)

Updated

10 months ago
Keywords: checkin-needed
Attachment #8891371 - Flags: review+

Comment 16

10 months ago
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/db29261fe4c7
Support sending health ping on shutdown via PingSender. r=Dexter
Keywords: checkin-needed

Comment 17

10 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/db29261fe4c7
Status: NEW → RESOLVED
Last Resolved: 10 months ago
status-firefox56: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
You need to log in before you can comment on or make changes to this bug.