Closed
Bug 1374270
Opened 7 years ago
Closed 7 years ago
Support sending health ping on shutdown via PingSender
Categories
(Toolkit :: Telemetry, enhancement, P1)
Toolkit
Telemetry
Tracking
()
RESOLVED
FIXED
mozilla56
Tracking | Status | |
---|---|---|
firefox56 | --- | fixed |
People
(Reporter: katejimmelon, Assigned: katejimmelon)
References
Details
Attachments
(1 file, 3 obsolete files)
4.36 KB,
patch
|
Dexter
:
review+
|
Details | Diff | Splinter Review |
No description provided.
Updated•7 years ago
|
Blocks: 1372228
Summary: Support sending Helath ping on shutdown via PingSender → Support sending health ping on shutdown via PingSender
Updated•7 years ago
|
Priority: -- → P3
Assignee | ||
Updated•7 years ago
|
Priority: P3 → P1
Assignee | ||
Comment 1•7 years ago
|
||
Attachment #8889431 -
Flags: review?(gfritzsche)
Comment 2•7 years ago
|
||
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 3•7 years ago
|
||
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•7 years 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•7 years ago
|
||
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•7 years ago
|
Attachment #8890300 -
Flags: review?(alessio.placitelli)
Comment 6•7 years ago
|
||
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+
Comment 7•7 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=bcec1637ec22ebee52f05bfed4a6ff9b187b4f03
Comment 8•7 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=942b8c12f2f4c5fb620280fd079c76ff9d1baa0b
Assignee | ||
Updated•7 years ago
|
Keywords: checkin-needed
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
Comment 10•7 years ago
|
||
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•7 years 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)
Comment 12•7 years ago
|
||
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•7 years ago
|
||
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 14•7 years ago
|
||
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•7 years ago
|
||
Attachment #8891299 -
Attachment is obsolete: true
Assignee | ||
Updated•7 years ago
|
Keywords: checkin-needed
Updated•7 years ago
|
Attachment #8891371 -
Flags: review+
Comment 16•7 years 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•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/db29261fe4c7
Status: NEW → RESOLVED
Closed: 7 years 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.
Description
•