Bug 1075055 fixes the prefences. We need to record and submit telemetry when FHR is preffed on. We also need to check that we submit only the "FHR" bucket data when FHR is preffed on, but Telemetry preffed off.

This patch gathers the appropriate histograms based on Telemetry enabled status.

Comment on attachment 8574550 [details] [diff] [review] bug1127918.patch Review of attachment 8574550 [details] [diff] [review]: ----------------------------------------------------------------- - We shouldn't submit the Telemetry-specific measurements such as slowSQL and threadHangs if the user hasn't opted into Telemetry - How do the two FHR/Telemetry participation prefs interact with Telemetry.services.canSend?

As discussed this patch prevents extended statistics (opt-in data, aka Telemetry data) do be added to pings when Telemetry is disabled. I'm not sure what to do with data in |TelemetrySession.getSimpleMeasurements|.

I forgot to mention that this patch stacks on bug 1134279.

(In reply to Vladan Djeric (:vladan) -- please needinfo! from comment #2) > - How do the two FHR/Telemetry participation prefs interact with > Telemetry.services.canSend? |TelemetrySession.isTelemetryEnabled| could also check that |Telemetry.canSendExtended| (or we are testing) is true. But I think that this should be done as part of bug 1137252.

This patch takes care of the UITelemetry and addonManager as per our discussion.

Comment on attachment 8576718 [details] [diff] [review] bug1127918.patch - v3 Review of attachment 8576718 [details] [diff] [review]: ----------------------------------------------------------------- ::: toolkit/components/telemetry/TelemetrySession.jsm @@ +523,5 @@ > appTimestamps = o.TelemetryTimestamps.get(); > } catch (ex) {} > + > + // Only submit this if Telemetry is enabled. > + if (this.isTelemetryEnabled()) { we should check for Services.telemetry.canRecordExtended instead @@ +681,5 @@ > + * @return {Integer} A value from nsITelemetry.DATASET_*. > + */ > + getDatasetType: function() { > + return this.isTelemetryEnabled() ? Ci.nsITelemetry.DATASET_RELEASE_CHANNEL_OPTIN > + : Ci.nsITelemetry.DATASET_RELEASE_CHANNEL_OPTOUT; i think we should use Telemetry::CanRecordBase/Extended to implement getDataSetType @@ +978,4 @@ > }; > > + // Add Telemetry opt-in measurements for content. > + if (this.isTelemetryEnabled()) { Use .canRecordExtended here as well @@ +993,3 @@ > > + // Add Telemetry opt-in measurements for chrome process. > + if (this.isTelemetryEnabled()) { Same

Thanks for your review Vladan, this patch is rebased against latest bug 1134279.

Comment on attachment 8579306 [details] [diff] [review] bug1127918.patch - v4 Review of attachment 8579306 [details] [diff] [review]: ----------------------------------------------------------------- ::: toolkit/components/telemetry/TelemetrySession.jsm @@ +522,5 @@ > Cu.import("resource://gre/modules/TelemetryTimestamps.jsm", o); > appTimestamps = o.TelemetryTimestamps.get(); > } catch (ex) {} > + > + // Only submit this if Telemetry is enabled. we should start using the new terminology in comments :) "extended set" vs Telemetry @@ +529,5 @@ > + if (!IS_CONTENT_PROCESS) { > + ret.addonManager = AddonManagerPrivate.getSimpleMeasures(); > + } > + } catch (ex) {} > + try { you can just make this one big block: if (!IS_CONTENT_PROCESS && Telemetry.canRecordExtended) { try { ret.addonManager = AddonManagerPrivate.getSimpleMeasures(); ret.UITelemetry = UITelemetry.getSimpleMeasures(); } catch (ex) {} } @@ +974,3 @@ > }; > > + // Add Telemetry opt-in measurements for content. // Add extended set measurements common to chrome & content processes ::: toolkit/components/telemetry/tests/unit/test_TelemetrySession.js @@ +1140,5 @@ > + "chromeHangs", "threadHangStats", "log", "slowSQL", "fileIOReports", "lateWrites", > + "addonHistograms", "addonDetails", "UIMeasurements", "slowSQLStartup" > + ]; > + > + // Check that the payload does not contain extended statistics fields. Shouldn't we also check that the extended fields are present when canRecordExtended = true?

Thanks for your review Vladan.

Fixed |gatherMemory| gathering memory with canRecordExtended being false.

Comment on attachment 8582462 [details] [diff] [review] bug1127918.patch - v6 R+ as discussed with Vladan.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=348a280b7f1d (this stacks on bug 1134279, so the try push is for the both)

Rebased.