Closed Bug 1433485 Opened 4 years ago Closed 4 years ago

Review telemetry pref settings used for tests

Categories

(Testing :: General, defect, P1)

defect

Tracking

(firefox59 fixed, firefox60 fixed)

RESOLVED FIXED
mozilla60
Tracking Status
firefox59 --- fixed
firefox60 --- fixed

People

(Reporter: gbrown, Assigned: gbrown)

References

Details

Attachments

(1 file)

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.
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);
: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?
Flags: needinfo?(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.
Flags: needinfo?(gfritzsche)
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
Attachment #8946321 - Flags: review?(mcooper)
Attachment #8946321 - Flags: review?(gfritzsche)
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.
Attachment #8946321 - Flags: review?(mcooper) → review+
Priority: -- → P1
: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.
Flags: needinfo?(gfritzsche)
I'm going to press ahead here, on the strength of the existing review and earlier discussion.
Flags: needinfo?(gfritzsche)
Attachment #8946321 - Flags: review?(gfritzsche)
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 :)
Attachment #8946321 - Flags: review+
Thanks!
Pushed by gbrown@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/182c9af3ed1a
Update test prefs for telemetry; r=mythmon,dexter
https://hg.mozilla.org/mozilla-central/rev/182c9af3ed1a
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla60
You need to log in before you can comment on or make changes to this bug.