1127918 - Record & submit the appropriate telemetry datasets when FHR is enabled

Status: RESOLVED FIXED

(Toolkit :: Telemetry, defect)

Description

[Georg Fritzsche [:gfritzsche]]

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.

Comment 1

[Alessio Placitelli [:Dexter]]

Attachment: bug1127918.patch

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

Comment 2

[Vladan Djeric (:vladan)]

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?

Comment 3

[Alessio Placitelli [:Dexter]]

Attachment: bug1127918.patch - v2

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|.

Comment 4

[Alessio Placitelli [:Dexter]]

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

Comment 5

[Alessio Placitelli [:Dexter]]

(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.

Comment 6

[Alessio Placitelli [:Dexter]]

Attachment: bug1127918.patch - v3

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

Comment 7

[Vladan Djeric (:vladan)]

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

Comment 8

[Alessio Placitelli [:Dexter]]

Attachment: bug1127918.patch - v4

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

Comment 9

[Vladan Djeric (:vladan)]

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?

Comment 10

[Alessio Placitelli [:Dexter]]

Attachment: bug1127918.patch - v5

Thanks for your review Vladan.

Comment 11

[Alessio Placitelli [:Dexter]]

Attachment: bug1127918.patch - v6

Fixed |gatherMemory| gathering memory with canRecordExtended being false.

Comment 12

[Alessio Placitelli [:Dexter]]

Comment on attachment 8582462 [details] [diff] [review]
bug1127918.patch - v6

R+ as discussed with Vladan.

Comment 13

[Alessio Placitelli [:Dexter]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=348a280b7f1d

(this stacks on bug 1134279, so the try push is for the both)

Comment 14

[Alessio Placitelli [:Dexter]]

Attachment: bug1127918.patch - v6

Rebased.

Comment 15

[Georg Fritzsche [:gfritzsche]]

https://hg.mozilla.org/integration/fx-team/rev/386d8cc19d8d

Comment 16

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/386d8cc19d8d