1433485 - Review telemetry pref settings used for tests

Status: RESOLVED FIXED

(Testing :: General, defect, P1)

Description

[Geoff Brown [:gbrown] (pto Apr 11-18)]

Let's follow-up on concerns raised in bug 1339568, comments 62 - 67.

There is concern that telemetry might not be effectively disabled during tests. Especially that pref datareporting.policy.dataSubmissionEnabled should be false, but is not.

Comment 1

[Geoff Brown [:gbrown] (pto Apr 11-18)]

I have confirmed that datareporting.policy.dataSubmissionEnabled is not modified for mochitests or reftests.

However, there are other telemetry related prefs being set during tests.

/* mochitests */

https://dxr.mozilla.org/mozilla-central/rev/e22e2c9eb81686e958a9448ea3d1e8cd766743e2/testing/profiles/prefs_general.js#244-265

// We want to collect telemetry, but we don't want to send in the results.
user_pref("toolkit.telemetry.server", "https://%(server)s/telemetry-dummy/");
// Don't send 'new-profile' ping on new profiles during tests, otherwise the testing framework
// might wait on the pingsender to finish and slow down tests.
user_pref("toolkit.telemetry.newProfilePing.enabled", false);
// Don't send 'bhr' ping during tests, otherwise the testing framework might
// wait on the pingsender to finish and slow down tests.
user_pref("toolkit.telemetry.bhrPing.enabled", false);
// Don't send the 'shutdown' ping using the pingsender on the first session using
// the 'pingsender' process. Valgrind marks the process as leaky (e.g. see bug 1364068
// for the 'new-profile' ping) but does not provide enough information
// to suppress the leak. Running locally does not reproduce the issue,
// so disable this until we rewrite the pingsender in Rust (bug 1339035).
user_pref("toolkit.telemetry.shutdownPingSender.enabledFirstSession", false);
// Don't send the 'first-shutdown' during tests, otherwise tests expecting
// main and subsession pings will fail.
user_pref("toolkit.telemetry.firstShutdownPing.enabled", false);

// A couple of preferences with default values to test that telemetry preference
// watching is working.
user_pref("toolkit.telemetry.test.pref1", true);
user_pref("toolkit.telemetry.test.pref2", false);


/* reftests */

https://dxr.mozilla.org/mozilla-central/rev/e22e2c9eb81686e958a9448ea3d1e8cd766743e2/layout/tools/reftest/reftest-preferences.js#86-90

// Skip data reporting policy notifications.
user_pref("datareporting.policy.dataSubmissionPolicyBypassNotification", true);

// Ensure that telemetry is disabled, so we don't connect to the telemetry
// server in the middle of the tests.
user_pref("toolkit.telemetry.enabled", false);
user_pref("datareporting.healthreport.uploadEnabled", false);
user_pref("experiments.enabled", false);

Comment 2

[Geoff Brown [:gbrown] (pto Apr 11-18)]

:gfritzsche - 

Should I set datareporting.policy.dataSubmissionEnabled = false? 
For both mochitests and reftests?
Are there other telemetry prefs that should be modified in the lists above? 
Can we do anything to make the mochitest and reftest prefs more consistent?

Comment 3

[Georg Fritzsche [:gfritzsche]]

I was not paying enough attention in the other bug, sorry.
The main upload preference that should default to false in all test environments is "datareporting.healthreport.uploadEnabled":
https://firefox-source-docs.mozilla.org/toolkit/components/telemetry/telemetry/internals/preferences.html#id1

We can also point "toolkit.telemetry.server" to a dummy address, but disabling .uploadEnabled instead saves us from unneeded connections etc.

Comment 4

[Geoff Brown [:gbrown] (pto Apr 11-18)]

Attachment: update telemetry prefs for tests

Thanks Georg.

:mythmon - I had to update browser_RecipeRunner.js to make it pass with this change, but I'm not familiar with the test. Is this change okay / expected?

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

Comment 5

[Michael Cooper [:mythmon]]

Comment on attachment 8946321 [details] [diff] [review]
update telemetry prefs for tests

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

This change is OK, and makes sense to me. The Reciperunner checks the value of that pref in a few places, so it makes sense the tests wouldn't work as expected when it is set to false.

Comment 6

[Geoff Brown [:gbrown] (pto Apr 11-18)]

:gfritzsche -- Do you have any concerns here, or did my r? just get buried in your review queue? I'm pretty sure this is the "right" thing to do, and tests continue to pass.

Comment 7

[Geoff Brown [:gbrown] (pto Apr 11-18)]

I'm going to press ahead here, on the strength of the existing review and earlier discussion.

Comment 8

[Alessio Placitelli [:Dexter]]

Comment on attachment 8946321 [details] [diff] [review]
update telemetry prefs for tests

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

I was about to steal this review from Georg: this looks good to me! Leaving my r+ for posterity :)

Comment 9

[Geoff Brown [:gbrown] (pto Apr 11-18)]

Thanks!

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/182c9af3ed1a
Update test prefs for telemetry; r=mythmon,dexter

Comment 11

[Natalia Csoregi [:nataliaCs]]

https://hg.mozilla.org/mozilla-central/rev/182c9af3ed1a

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/4c672d9530bf