Timings used in telemetry should be read from preferences, while defaulting at their currently hardcoded values. This change is intended to enable faster automated testing of telemetry.

Comment on attachment 8651151 [details] [diff] [review] telemetry-delays-prefs.patch This patch was discussed in bug 1168643. It now takes into account comment 25 of that bug, and was rebased against current m-c.

Comment on attachment 8651151 [details] [diff] [review] telemetry-delays-prefs.patch Review of attachment 8651151 [details] [diff] [review]: ----------------------------------------------------------------- As mentioned in bug 1168643, comment 25, we should also document these hidden prefs in toolkit/components/telemetry/docs/preferences.rst, in a "Testing" section. You can build the documentation with "mach build-docs", the output lands in <objdir>/docs/html/toolkit/components/telemetry/telemetry/preferences.html ::: toolkit/components/telemetry/TelemetrySession.jsm @@ +75,5 @@ > // Maximum number of content payloads that we are willing to store. > const MAX_NUM_CONTENT_PAYLOADS = 10; > > // Do not gather data more than once a minute > +const TELEMETRY_INTERVAL = Preferences.get("toolkit.telemetry.collectInterval", 60) * 1000; I don't think we need to change this. @@ +86,2 @@ > // When user is idle, execute a scheduler tick every 60 minutes. > +const SCHEDULER_TICK_IDLE_INTERVAL_MS = Preferences.get("toolkit.telemetry.idleTickInterval", 60 * 60) * 1000; I'd prefer these two to be toolkit.telemetry.scheduler.tickInterval & toolkit.telemetry.scheduler.idleTickInterval @@ +88,3 @@ > > // The tolerance we have when checking if it's midnight (15 minutes). > +const SCHEDULER_MIDNIGHT_TOLERANCE_MS = Preferences.get("toolkit.telemetry.scheduler.midnightTolerance", 15 * 60) * 1000; Sorry, missed this before - what do we need to change this for? I can't see a need to change this for testing.

Added "scheduler" in the paths of the 2 tickInterval prefs. Added corresponding doc. Kept "toolkit.telemetry.collectInterval" because we need to shorten all delays when testing to keep them consistent. Besides, it does no harm to keep it.

Comment on attachment 8651746 [details] [diff] [review] telemetry-delays-prefs-2.patch Review of attachment 8651746 [details] [diff] [review]: ----------------------------------------------------------------- So, in general i think if we change or land new code, there should be a good reason for it. Some smaller notes below, this is nearly done. ::: toolkit/components/telemetry/TelemetrySession.jsm @@ +75,5 @@ > // Maximum number of content payloads that we are willing to store. > const MAX_NUM_CONTENT_PAYLOADS = 10; > > // Do not gather data more than once a minute > +const TELEMETRY_INTERVAL = Preferences.get("toolkit.telemetry.collectInterval", 60) * 1000; This only used for triggering some memory report measurements, i think we don't need to change this now. @@ +88,3 @@ > > // The tolerance we have when checking if it's midnight (15 minutes). > +const SCHEDULER_MIDNIGHT_TOLERANCE_MS = Preferences.get("toolkit.telemetry.scheduler.midnightTolerance", 15 * 60) * 1000; I don't think we need to adjust this for testing. ::: toolkit/components/telemetry/docs/preferences.rst @@ +96,5 @@ > This is the only channel-specific version that we currently use for the minimum policy version. > + > +Testing > +------- > + Please add a short note that these prefs are for testing only. @@ +123,5 @@ > + Tolerance to midnight, when daily pings are sent (seconds). > + > +``toolkit.telemetry.idleTimeout`` > + > + Idle time before pinging (seconds). "Timeout until we decide whether a user is idle or not."? This is all this really does now.

Comment on attachment 8651775 [details] [diff] [review] telemetry-delays-prefs-3.patch Review of attachment 8651775 [details] [diff] [review]: ----------------------------------------------------------------- This looks good with the two things below changed. ::: toolkit/components/telemetry/TelemetrySession.jsm @@ +88,3 @@ > > // The tolerance we have when checking if it's midnight (15 minutes). > +const SCHEDULER_MIDNIGHT_TOLERANCE_MS = 900 * 1000; Lets leave this as |15 * 60 * 1000| (readability). @@ +91,5 @@ > > // Seconds of idle time before pinging. > // On idle-daily a gather-telemetry notification is fired, during it probes can > // start asynchronous tasks to gather data. > +const IDLE_TIMEOUT_SECONDS = Preferences.get("toolkit.telemetry.idleTimeout", 300); Lets leave this as |5 * 60|.

Modified according to last comment. Carrying on r+

Please provide a Try link.

Failures look unrelated to the patch (checked by gfritzche and myself).