Vladan Djeric (:vladan) Reporter
	Description • 9 years ago

idle-daily pings haven't been useful since the first AWS Telemetry backend and they've been superceded by "daily" pings in unified Telemetry

	Benjamin Smedberg
	Comment 1 • 9 years ago

What are the consequences of this for FHR users? Since this isn't a regression, I don't think it needs to block.

	Vladan Djeric (:vladan) Reporter
	Comment 2 • 9 years ago

(In reply to Benjamin Smedberg  [:bsmedberg] from comment #1)
> What are the consequences of this for FHR users? Since this isn't a
> regression, I don't think it needs to block.

There are no consequence to FHR users -- idle-daily wasn't used by anyone. Are you confusing idle-daily with the new "daily" ping?

There's no downside, we just remove an unneeded behavior from Firefox & related tests

	Benjamin Smedberg
	Comment 3 • 9 years ago

No, I'm mainly asking about relative priority. We should absolutely remove idle-daily (I thought we already had) but I don't see why this would block uplift.

	Vladan Djeric (:vladan) Reporter
	Comment 4 • 9 years ago

(In reply to Benjamin Smedberg  [:bsmedberg] from comment #3)
> No, I'm mainly asking about relative priority. We should absolutely remove
> idle-daily (I thought we already had) but I don't see why this would block
> uplift.

I marked it as a blocker because I see it as an easy bit of housekeeping that should have landed together with daily pings

	Mark Reid [:mreid]
	Comment 5 • 9 years ago

Aside: does that mean I can delete idle-daily submissions from the server data store? If so, I'll file a separate bug to do so.

	Vladan Djeric (:vladan) Reporter
	Comment 6 • 9 years ago

(In reply to Mark Reid [:mreid] from comment #5)
> Aside: does that mean I can delete idle-daily submissions from the server
> data store? If so, I'll file a separate bug to do so.

Yes, but let's not do that until we're confident about the quality of the new pings.. just in case idle-daily ends up being useful for debugging issues (e.g. compare idle-daily rates before & after to determine if we have telemetry initialization or sending bugs in the new telemetry).

	Alessio Placitelli [:Dexter] Assignee
	Comment 7 • 9 years ago

This patch removes "idle-daily" pings. I've not killed gather-telemetry, as this will be done by bug 1127907.

	Alessio Placitelli [:Dexter] Assignee
	Comment 8 • 9 years ago

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

	Vladan Djeric (:vladan) Reporter
	Comment 9 • 9 years ago

Can we keep the idle-daily event to check for persisted pings? it's nice to have a fallback in case of an issue with daily ping sends

	Alessio Placitelli [:Dexter] Assignee
	Comment 10 • 9 years ago

That's a good idea. The "idle-daily" event itself is not being removed by this patch, as "gather-telemetry" still uses it.

Maybe we can file a followup bug and move "idle-daily" handling to TelemetryScheduler (removing it from TelemetrySession after we deal with gather-telemetry)? So that the logic driving telemetry actions is encapsulated within that object?

	Alessio Placitelli [:Dexter] Assignee
	Comment 11 • 9 years ago

I erroneously removed test_TelemetryPing_idle.js in the last patch, which wasn't really testing idleDaily pings but rather "gather-telemetry" being correctly notified.

R+ on this as discussed on IRC with :gfritzsche.

	Georg Fritzsche [:gfritzsche]
	Comment 12 • 9 years ago

This has test failures in test_TelemetryPingBuildID.js:

 0:03.49 LOG: Thread-28 INFO toolkit/components/telemetry/tests/unit/test_TelemetryPingBuildID.js | Starting test_firstRun
 0:03.49 LOG: Thread-28 INFO (xpcshell/head.js) | test test_firstRun pending (2)
 0:03.81 PROCESS_OUTPUT: Thread-28 (pid:4155) Full command: ['/Users/gfritzsche/moz/fxteam/obj/dist/NightlyDebug.app/Contents/MacOS/xpcshell', '-g', '/Users/gfritzsche/moz/fxteam/obj/dist/NightlyDebug.app/Contents/Resources', '-a', '/Users/gfritzsche/moz/fxteam/obj/dist/NightlyDebug.app/Contents/Resources', '-r', '/Users/gfritzsche/moz/fxteam/obj/dist/NightlyDebug.app/Contents/Resources/components/httpd.manifest', '-m', '-s', '-e', 'const _HTTPD_JS_PATH = "/Users/gfritzsche/moz/fxteam/obj/dist/NightlyDebug.app/Contents/Resources/components/httpd.js";', '-e', 'const _HEAD_JS_PATH = "/Users/gfritzsche/moz/fxteam/testing/xpcshell/head.js";', '-e', 'const _TESTING_MODULES_DIR = "/Users/gfritzsche/moz/fxteam/obj/_tests/modules/";', '-f', '/Users/gfritzsche/moz/fxteam/testing/xpcshell/head.js', '-p', '/var/folders/tp/mgx9wlds46j6q3ck3xhjzm2w0000gn/T/tmpe4g0Ly', '-e', 'const _SERVER_ADDR = "localhost"', '-e', u'const _HEAD_FILES = ["/Users/gfritzsche/moz/fxteam/obj/_tests/xpcshell/toolkit/components/telemetry/tests/unit/head.js"];', '-e', 'const _TAIL_FILES = [];', '-e', 'const _JSDEBUGGER_PORT = 0;', '-e', u'const _TEST_FILE = ["/Users/gfritzsche/moz/fxteam/obj/_tests/xpcshell/toolkit/components/telemetry/tests/unit/test_TelemetryPingBuildID.js"];', '-e', u'const _TEST_NAME = "toolkit/components/telemetry/tests/unit/test_TelemetryPingBuildID.js"', '-e', '_execute_test(); quit(0);']
(pid:4155) "1428320362572	Toolkit.Telemetry	TRACE	TelemetrySession::setupChromeProcess"
 0:03.50 LOG: Thread-28 INFO (xpcshell/head.js) | test run_next_test 0 finished (2)
 0:03.50 LOG: Thread-28 INFO "CONSOLE_MESSAGE: (info) 1428320362572	Toolkit.Telemetry	TRACE	TelemetrySession::setupChromeProcess"
 0:03.81 PROCESS_OUTPUT: Thread-28 (pid:4155) "1428320362682	Toolkit.Telemetry	INFO	TelemetrySession::_loadSessionData - Cannot load session data file /var/folders/tp/mgx9wlds46j6q3ck3xhjzm2w0000gn/T/tmp5PM7s3/datareporting/session-state.json: Unix error 2 during operation open on file /var/folders/tp/mgx9wlds46j6q3ck3xhjzm2w0000gn/T/tmp5PM7s3/datareporting/session-state.json (No such file or directory) No traceback available"
 0:03.61 LOG: Thread-28 INFO "CONSOLE_MESSAGE: (info) 1428320362682	Toolkit.Telemetry	INFO	TelemetrySession::_loadSessionData - Cannot load session data file /var/folders/tp/mgx9wlds46j6q3ck3xhjzm2w0000gn/T/tmp5PM7s3/datareporting/session-state.json: Unix error 2 during operation open on file /var/folders/tp/mgx9wlds46j6q3ck3xhjzm2w0000gn/T/tmp5PM7s3/datareporting/session-state.json (No such file or directory) No traceback available"
 0:03.81 PROCESS_OUTPUT: Thread-28 (pid:4155) "1428320362741	Toolkit.Telemetry	TRACE	TelemetrySession::gatherMemory"
 0:03.66 LOG: Thread-28 INFO "CONSOLE_MESSAGE: (info) 1428320362741	Toolkit.Telemetry	TRACE	TelemetrySession::gatherMemory"
 0:03.81 PROCESS_OUTPUT: Thread-28 (pid:4155) "1428320362763	Toolkit.Telemetry	TRACE	TelemetryEnvironment::constructor"
 0:03.81 PROCESS_OUTPUT: Thread-28 (pid:4155) "1428320362766	Toolkit.Telemetry	TRACE	TelemetryEnvironment::_getGFXData - Only one display adapter detected."
 0:03.81 PROCESS_OUTPUT: Thread-28 (pid:4155) "1428320362767	Toolkit.Telemetry	ERROR	TelemetryEnvironment::_isDefaultBrowser - Could not obtain shell service"
 0:03.81 PROCESS_OUTPUT: Thread-28 (pid:4155) "1428320362770	Toolkit.Telemetry	TRACE	TelemetryEnvironment::registerChangeListener for TelemetrySession::onEnvironmentChange"
 0:03.81 PROCESS_OUTPUT: Thread-28 (pid:4155) "1428320362770	Toolkit.Telemetry	TRACE	TelemetryScheduler::init"
 0:03.81 PROCESS_OUTPUT: Thread-28 (pid:4155) "1428320362770	Toolkit.Telemetry	TRACE	TelemetryScheduler::_rescheduleTimeout - isUserIdle: false"
 0:03.81 PROCESS_OUTPUT: Thread-28 (pid:4155) "1428320362770	Toolkit.Telemetry	TRACE	TelemetryScheduler::_rescheduleTimeout - scheduling next tick for Mon Apr 06 2015 13:44:22 GMT+0200 (CEST)"
 0:03.81 PROCESS_OUTPUT: Thread-28 (pid:4155) "*************************"
 0:03.81 PROCESS_OUTPUT: Thread-28 (pid:4155) "A coding exception was thrown and uncaught in a Task."
 0:03.81 PROCESS_OUTPUT: Thread-28 (pid:4155) "Full message: ReferenceError: idleService is not defined"
 0:03.81 PROCESS_OUTPUT: Thread-28 (pid:4155) "Full stack: TelemetryScheduler.init@resource://gre/modules/TelemetrySession.jsm:439:5"
 0:03.81 PROCESS_OUTPUT: Thread-28 (pid:4155) "setupChromeProcess/this._delayedInitTask<@resource://gre/modules/TelemetrySession.jsm:1568:9"
 0:03.81 PROCESS_OUTPUT: Thread-28 (pid:4155) "_run_next_test@/Users/gfritzsche/moz/fxteam/testing/xpcshell/head.js:1377:9"
 0:03.81 PROCESS_OUTPUT: Thread-28 (pid:4155) "do_execute_soon/<.run@/Users/gfritzsche/moz/fxteam/testing/xpcshell/head.js:646:9"
 0:03.81 PROCESS_OUTPUT: Thread-28 (pid:4155) "_do_main@/Users/gfritzsche/moz/fxteam/testing/xpcshell/head.js:207:5"
 0:03.81 PROCESS_OUTPUT: Thread-28 (pid:4155) "_execute_test@/Users/gfritzsche/moz/fxteam/testing/xpcshell/head.js:506:5"
 0:03.81 PROCESS_OUTPUT: Thread-28 (pid:4155) "@-e:1:1"
 0:03.81 PROCESS_OUTPUT: Thread-28 (pid:4155) "*************************"
 0:03.69 LOG: Thread-28 ERROR Unexpected exception ReferenceError: idleService is not defined at resource://gre/modules/TelemetrySession.jsm:439
TelemetryScheduler.init@resource://gre/modules/TelemetrySession.jsm:439:5
setupChromeProcess/this._delayedInitTask<@resource://gre/modules/TelemetrySession.jsm:1568:9
_run_next_test@/Users/gfritzsche/moz/fxteam/testing/xpcshell/head.js:1377:9
do_execute_soon/<.run@/Users/gfritzsche/moz/fxteam/testing/xpcshell/head.js:646:9
_do_main@/Users/gfritzsche/moz/fxteam/testing/xpcshell/head.js:207:5
_execute_test@/Users/gfritzsche/moz/fxteam/testing/xpcshell/head.js:506:5
@-e:1:1

	Alessio Placitelli [:Dexter] Assignee
	Comment 13 • 9 years ago

(In reply to Georg Fritzsche [:gfritzsche] from comment #12)
> This has test failures in test_TelemetryPingBuildID.js:

Yes, the patch needs to be rebased against the latest m-c.

	Alessio Placitelli [:Dexter] Assignee
	Comment 14 • 9 years ago

Rebased.

	Vladan Djeric (:vladan) Reporter
	Comment 15 • 9 years ago

(In reply to Alessio Placitelli [:Dexter] from comment #10)
> That's a good idea. The "idle-daily" event itself is not being removed by
> this patch, as "gather-telemetry" still uses it.

This patch removes the "idle" listener which calls TelemetryPing.send(). That call to TelemetryPing.send() would also send persisted pings after sending the idle-daily ping. I'd like us to continue sending persisted pings on idle-daily.

> Maybe we can file a followup bug and move "idle-daily" handling to
> TelemetryScheduler (removing it from TelemetrySession after we deal with
> gather-telemetry)? So that the logic driving telemetry actions is
> encapsulated within that object?

Yes, that would be good.

	Alessio Placitelli [:Dexter] Assignee
	Comment 16 • 9 years ago

Changed the patch to send pending pings on idle-daily.

This patch also adds test coverage for that and moves |decodeRequestPayload| to head.js.

	Georg Fritzsche [:gfritzsche]
	Comment 17 • 9 years ago

Comment on attachment 8589087 [details] [diff] [review]
bug1139754.patch - v3

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

::: toolkit/components/telemetry/TelemetrySession.jsm
@@ +101,1 @@
>  const IDLE_TIMEOUT_SECONDS = 5 * 60;

This is unused.

@@ +1850,5 @@
>        // Enqueue to main-thread, otherwise components may be inited by the
>        // idle-daily category and miss the gather-telemetry notification.
>        Services.tm.mainThread.dispatch((function() {
> +        // Notify that data should be gathered now. We are keeping this behaviour for
> +        // now but it will be removed as soon as bug 1127907 lands.

Mark it with a TODO too:
TODO: We are keeping this ...

@@ +1856,2 @@
>        }).bind(this), Ci.nsIThread.DISPATCH_NORMAL);
> +      // Try to send any pending persisted ping now.

Add something like:
"TODO: This is just a fallback for now.
Remove this when we have ping send scheduling properly factored out and driven independently of this module."

::: toolkit/components/telemetry/tests/unit/head.js
@@ +17,5 @@
>  
>  let gOldAppInfo = null;
>  let gGlobalScope = this;
>  
> +function decodeRequestPayload(request) {

Add a comment explaining the behavior.

::: toolkit/components/telemetry/tests/unit/test_TelemetryPing_idle.js
@@ +44,5 @@
> +    gHttpServer.registerPrefixHandler("/submit/telemetry/", request => resolve(request));
> +  });
> +
> +  let gatherPromise = new Promise(resolve => {
> +    Services.obs.addObserver(function observeTelemetry() {

Nit: Less nesting etc. with defer()?
let p = PromiseUtils.defer();
addObserver(p.resolve, "gather-telemetry");
yield p;
removeObserver(p.resolve, ...

@@ +55,2 @@
>    TelemetrySession.observe(null, "idle-daily", null);
> +  yield gatherPromise;

Assert.ok(true, "Received gather-telemetry notification.");

	Alessio Placitelli [:Dexter] Assignee
	Comment 19 • 9 years ago

(In reply to Georg Fritzsche [:gfritzsche] from comment #17)
> Comment on attachment 8589087 [details] [diff] [review]
> bug1139754.patch - v3
> 
> Review of attachment 8589087 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: toolkit/components/telemetry/TelemetrySession.jsm
> @@ +101,1 @@
> >  const IDLE_TIMEOUT_SECONDS = 5 * 60;
> 
> This is unused.

This is used by the idleService in TelemetryScheduler.

	Georg Fritzsche [:gfritzsche]
	Comment 20 • 9 years ago

Comment on attachment 8589145 [details] [diff] [review]
bug1139754.patch - v4

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

::: toolkit/components/telemetry/TelemetrySession.jsm
@@ +1850,4 @@
>        // Enqueue to main-thread, otherwise components may be inited by the
>        // idle-daily category and miss the gather-telemetry notification.
>        Services.tm.mainThread.dispatch((function() {
> +        // Notify that data should be gathered now. TODO: We are keeping this behaviour for

Nit: TODO on a new line.

	Alessio Placitelli [:Dexter] Assignee
	Comment 21 • 9 years ago

Moved the TODO to a new line.

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

	Georg Fritzsche [:gfritzsche]
	Comment 22 • 9 years ago

https://hg.mozilla.org/integration/fx-team/rev/0b7c79fbd6e9

	Ryan VanderMeulen [:RyanVM]
	Comment 23 • 9 years ago

https://hg.mozilla.org/mozilla-central/rev/0b7c79fbd6e9