1329978 - Add pref to override TelemetrySend official check in sendingEnabled

Status: RESOLVED FIXED

(Toolkit :: Telemetry, defect, P1)

Description

[Georg Fritzsche [:gfritzsche]]

Automation tests need to override the send restriction that we have:
https://dxr.mozilla.org/mozilla-central/rev/845cc4dea57f6cc93f46810d24b1058b640c3b74/toolkit/components/telemetry/TelemetrySend.jsm#1038

Comment 1

[Georg Fritzsche [:gfritzsche]]

I would suggest we use a hidden boolean pref "toolkit.telemetry.send.overrideOfficialCheck".

This will need to add a check against the pref here:
https://dxr.mozilla.org/mozilla-central/rev/845cc4dea57f6cc93f46810d24b1058b640c3b74/toolkit/components/telemetry/TelemetrySend.jsm#1040
... of the form `Preferences.get(prefName, false)`.

We need to add documentation for the preference here:
https://dxr.mozilla.org/mozilla-central/rev/845cc4dea57f6cc93f46810d24b1058b640c3b74/toolkit/components/telemetry/docs/internals/preferences.rst#92

Comment 2

[John Dorlus [:Silne30]]

Attachment: new_pref.patch

This patch is a compilation of the new telemetry server stuff and an attempt to create a new pref. I am getting false positives so I know that I did something wrong.

Comment 3

[Alessio Placitelli [:Dexter]]

Comment on attachment 8827981 [details] [diff] [review]
new_pref.patch

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

I think you're seeing issues because of the location you added the pref check (which is commented in the current patch).

::: toolkit/components/telemetry/TelemetrySend.jsm
@@ +1041,5 @@
>     * @return {Boolean} True if pings can be send to the servers, false otherwise.
>     */
>    sendingEnabled(ping = null) {
>      // We only send pings from official builds, but allow overriding this for tests.
>      if (!Telemetry.isOfficialTelemetry && !this._testMode) {

The pref check should be here, not below. If you add it below this point, it won't get checked on unofficial builds, making the pref useless.

Just add it as "&& !IS_OVERRIDE_OFFICIAL_CHECK" after the _testMod check, on a new line.

@@ +1057,5 @@
>        return Preferences.get(PREF_FHR_UPLOAD_ENABLED, false);
>      }
>  
> +    //If the override is enabled, send pings regardless.
> +    //   if (PREF_SEND_OVERRIDE_OFFICIAL_CHECK) {

You were checking the pref name here, not the pref value which is in IS_OVERRIDE_OFFICIAL_CHECK.

Comment 4

[John Dorlus [:Silne30]]

Attachment: new_pref_revised.patch

Revision per notes above

Comment 5

[Alessio Placitelli [:Dexter]]

Comment on attachment 8828019 [details] [diff] [review]
new_pref_revised.patch

this patch looks corrupted, as it contains more than 1 header and different changesets for the same file.

Please attach a working patch.

Comment 6

[John Dorlus [:Silne30]]

Attachment: new_pref_git_revised.patch

Redid the patch. I think it had multiple revisions in the previous file because of the fact I exported commits to make a patch file. Some commits touched the same file. I think I made it work this time around though.

Comment 7

[Alessio Placitelli [:Dexter]]

Comment on attachment 8828110 [details] [diff] [review]
new_pref_git_revised.patch

John, this still has 4 "# HG changeset patch" sections and reports changes to TelemetrySend.jsm twice.

Comment 8

[Alessio Placitelli [:Dexter]]

I had a look at the raw file and, assuming the last export is the one with your latest changes, I see a problem:

>  else if (IS_OVERRIDE_OFFICIAL_CHECK)
>        return true;

You added this part to TelemetrySend.jsm: it's not needed and will result in unexpected behaviour, as it will basically ignore all the other telemetry prefs when enabled. Please remove it.

Moreover, it looks like TelemetrySend.jsm doesn't contain the preferences anymore (where is IS_OVERRIDE_OFFICIAL_CHECK defined?). I don't know if that's happening because of the way you're exporting the patches.

Comment 9

[John Dorlus [:Silne30]]

Assigning to Alessio to continue the work on this.

Comment 10

[Alessio Placitelli [:Dexter]]

Attachment: bug1329978.patch

This patch adds a new hidden preference (and the related test coverage) to override the isOfficialTelemetry check.

Comment 11

[Georg Fritzsche [:gfritzsche]]

Comment on attachment 8830298 [details] [diff] [review]
bug1329978.patch

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

::: toolkit/components/telemetry/TelemetrySend.jsm
@@ +45,5 @@
>  const PREF_BRANCH = "toolkit.telemetry.";
>  const PREF_SERVER = PREF_BRANCH + "server";
>  const PREF_UNIFIED = PREF_BRANCH + "unified";
>  const PREF_FHR_UPLOAD_ENABLED = "datareporting.healthreport.uploadEnabled";
> +const PREF_SEND_OVERRIDE_OFFICIAL_CHECK = PREF_BRANCH + "send.overrideOfficialCheck";

Nit: I think the `SEND` part is redundant here in this module.
Please also document the pref in preferences.rst, including that restart is required.

@@ +55,5 @@
>  // Changing this pref requires a restart.
>  const IS_UNIFIED_TELEMETRY = Preferences.get(PREF_UNIFIED, false);
>  
> +// Whether sending pings has been overridden.
> +const SEND_OVERRIDE_OFFICIAL_CHECK = Preferences.get(PREF_SEND_OVERRIDE_OFFICIAL_CHECK, false);

Ditto.

::: toolkit/components/telemetry/tests/unit/test_TelemetrySend.js
@@ +449,5 @@
> +  PingServer.resetPingHandler();
> +  yield TelemetryController.submitExternalPing(TEST_PING_TYPE, { test: "test" });
> +
> +  // Make sure we received the ping.
> +  yield PingServer.promiseNextPings(1);

`promiseNextPing()`.
Also check at least the ping type.

@@ +453,5 @@
> +  yield PingServer.promiseNextPings(1);
> +
> +  // Restore the test mode and disable the override.
> +  TelemetrySend.setTestModeEnabled(true);
> +  Preferences.set(PREF_SEND_OVERRIDE, false);

Nit: I think you'd want to reset the pref to default, not set it to an explicit value `false`.

@@ +457,5 @@
> +  Preferences.set(PREF_SEND_OVERRIDE, false);
> +});
> +
> +add_task(function* cleanup() {
> +  PingServer.stop();

yield ...

Comment 12

[Alessio Placitelli [:Dexter]]

Attachment: bug1329978.patch

Thanks Georg. This addresses your comments and moves the pref inside TelemetrySendImpl to ease testing.

Comment 13

[Alessio Placitelli [:Dexter]]

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

Comment 14

[Alessio Placitelli [:Dexter]]

Attachment: bug1329978.patch

The new test was failing on official/release builds on automation since isOfficialTelemetry is always true there. I've changed the test to not check for discarding the pings on official builds.

Carrying over the r+.

Comment 15

[Alessio Placitelli [:Dexter]]

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

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/4e8cdfa12989
Add pref to override TelemetrySend official check in sendingEnabled. r=gfritzsche

Comment 17

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/4e8cdfa12989