Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout) Assignee
	Description • 9 years ago

Since bug 1134279 landed, the https://dxr.mozilla.org/mozilla-central/source/toolkit/components/telemetry/tests/unit/test_TelemetryPing.js (test_TelemetrySendOldPings.js also fails, but I haven't investigated yet).

This happens because datareporting.healthreport.service.enabled doesn't exist (likely yet). The constant for this pref has been defined in the file, but it doesn't get turned on (because it's enabled on all branches using healthreport). So this patch set the preferences explicitly to true.

	Georg Fritzsche [:gfritzsche]
	Comment 1 • 9 years ago

What are the failures?
I can review this too, but i'd like to understand what happens.

	Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout) Assignee
	Comment 2 • 9 years ago

The tests fails for Thunderbird builds which ships without data reporting atm, see https://treeherder.mozilla.org/#/jobs?repo=comm-central&filter-searchStr=Ubuntu VM 12.04 comm-central opt test xpcshell

 TEST-UNEXPECTED-FAIL | toolkit/components/telemetry/tests/unit/test_TelemetryPing.js | xpcshell return code: 0

TEST-UNEXPECTED-FAIL | toolkit/components/telemetry/tests/unit/test_TelemetryPing.js | asyncSetup - [asyncSetup : 185] null == "e008dcfc-2f94-400b-8d18-fc9995e4d1e4"

What happens:

TelemetryPing.clientId is null: http://hg.mozilla.org/mozilla-central/file/078128c2600a/toolkit/components/telemetry/tests/unit/test_TelemetryPing.js#l185

This is derived from Impl._clientID. This would get set in http://hg.mozilla.org/mozilla-central/file/078128c2600a/toolkit/components/telemetry/TelemetryPing.jsm#l786 or even later in _delayedInitTask, but there is a check |!this.enableTelemetryRecording(testing)| before at http://hg.mozilla.org/mozilla-central/file/078128c2600a/toolkit/components/telemetry/TelemetryPing.jsm#l777

enableTelemetryRecording returns always false for environments without datareporting.healthreport.service.enabled http://hg.mozilla.org/mozilla-central/file/078128c2600a/toolkit/components/telemetry/TelemetryPing.jsm#l706 (but Firefox has it always set to true).

	Vladan Djeric (:vladan)
	Comment 3 • 9 years ago

Comment on attachment 8589244 [details] [diff] [review]
patch, v1

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

::: toolkit/components/telemetry/tests/unit/test_TelemetryPing.js
@@ +175,5 @@
>    loadAddonManager("xpcshell@tests.mozilla.org", "XPCShell", "1", "1.9.2");
>  
>    Services.prefs.setBoolPref(PREF_ENABLED, true);
>    Services.prefs.setBoolPref(PREF_FHR_UPLOAD_ENABLED, true);
> +  Services.prefs.setBoolPref(PREF_FHR_SERVICE_ENABLED, true);

So Thunderbird doesn't have the FHR service but it has the SessionRecorder?

	Georg Fritzsche [:gfritzsche]
	Comment 4 • 9 years ago

(In reply to Archaeopteryx [:aryx] from comment #2)
> enableTelemetryRecording returns always false for environments without
> datareporting.healthreport.service.enabled
> http://hg.mozilla.org/mozilla-central/file/078128c2600a/toolkit/components/
> telemetry/TelemetryPing.jsm#l706 (but Firefox has it always set to true).

Alessio, is this getting fixed in an active bug?
We don't want Telemetry to be off when we are disabling FHR.

	Alessio Placitelli [:Dexter]
	Comment 5 • 9 years ago

No, this isn't. I claim we should probably introduce a preference that enables/disables Telemetry base recording (opt-out) and stop relying on the FHR prefs. This would also fix bug 1148500.

	Georg Fritzsche [:gfritzsche]
	Comment 6 • 9 years ago

Ok, i filed bug 1153252 on this.

(In reply to Vladan Djeric (:vladan) -- please needinfo! from comment #3)
> ::: toolkit/components/telemetry/tests/unit/test_TelemetryPing.js
> @@ +175,5 @@
> >    loadAddonManager("xpcshell@tests.mozilla.org", "XPCShell", "1", "1.9.2");
> >  
> >    Services.prefs.setBoolPref(PREF_ENABLED, true);
> >    Services.prefs.setBoolPref(PREF_FHR_UPLOAD_ENABLED, true);
> > +  Services.prefs.setBoolPref(PREF_FHR_SERVICE_ENABLED, true);
> 
> So Thunderbird doesn't have the FHR service but it has the SessionRecorder?

Yes, we moved the SessionRecorder (bug 1147522) and client id implementation (bug 1137353) so that we still can use them when we turn FHR off.

Given the above i think we should flip the pref on for now (with a TODO comment pointing to bug 1153252). Sound ok vladan?

	Vladan Djeric (:vladan)
	Comment 7 • 9 years ago

Comment on attachment 8589244 [details] [diff] [review]
patch, v1

Sounds good

	Magnus Melin [:mkmelin]
	Comment 8 • 9 years ago

I landed this for you on inbound - https://hg.mozilla.org/integration/mozilla-inbound/rev/8e5fd409bfeb

	Phil Ringnalda (:philor)
	Comment 9 • 9 years ago

https://hg.mozilla.org/mozilla-central/rev/8e5fd409bfeb

	Lawrence Mandel [:lmandel] (use needinfo)
	Comment 10 • 9 years ago

Comment on attachment 8589244 [details] [diff] [review]
patch, v1

Approved for Aurora. For approval request see bug 1139460 comment 42. For approval comments see bug 1139460 comment 43.

	Georg Fritzsche [:gfritzsche]
	Comment 11 • 9 years ago

https://hg.mozilla.org/releases/mozilla-aurora/rev/7dee328009b7