1197292 - Timings used in telemetry should be read from preferences

Status: RESOLVED FIXED

(Toolkit :: Telemetry, defect)

Description

[André Reinald]

Attachment: telemetry-delays-prefs.patch

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 1

[André Reinald]

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 2

[Georg Fritzsche [:gfritzsche]]

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.

Comment 3

[André Reinald]

Attachment: telemetry-delays-prefs-2.patch

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 4

[Georg Fritzsche [:gfritzsche]]

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 5

[André Reinald]

Attachment: telemetry-delays-prefs-3.patch

Comment 6

[Georg Fritzsche [:gfritzsche]]

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|.

Comment 7

[André Reinald]

Attachment: telemetry-delays-prefs-4.patch

Modified according to last comment. Carrying on r+

Comment 8

[Ryan VanderMeulen [:RyanVM]]

Please provide a Try link.

Comment 9

[André Reinald]

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

Comment 10

[André Reinald]

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

Comment 11

[Pulsebot]

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

Comment 12

[Ryan VanderMeulen [:RyanVM]]

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