1134279 - Make TelemetryPing and TelemetrySession code use the "FHR enabled" & "Telemetry enabled" prefs properly

Status: RESOLVED FIXED

(Toolkit :: Telemetry, defect)

Description

[Georg Fritzsche [:gfritzsche]]

We should audit that the TelemetryPing sending & persistence functionality is properly guarded with Telemetry.canSend checks.

Comment 1

[Benjamin Smedberg]

Has canSend been updated to mean "FHR or telemetry"?

Comment 2

[Georg Fritzsche [:gfritzsche]]

Just

Comment 3

[Georg Fritzsche [:gfritzsche]]

(In reply to Benjamin Smedberg  [:bsmedberg] from comment #1)
> Has canSend been updated to mean "FHR or telemetry"?

No, i just checked a bit and it's not that simple.
canSend is used for whether we can send the "Telemetry" style data.
We need two different flags here:
* one for whether we are good to send the "release opt-in" data (Telemetry style)
* one for whether we can send & record data at all, bug 1127918 is about that

Comment 4

[Georg Fritzsche [:gfritzsche]]

We checked a little into what we are doing now:
* Telemetry.canSend seems unproblematic (true on official builds)
* Telemetry.canRecord should probably not be disabled anymore at all on desktop
* Checks on toolkit.telemetry.enabled / PREF_ENABLED need to be cross-checked - we turn off some things based on this (e.g. in TelemetrySession / TelemetryPing)

We also need to make sure that we are collecting the right dataset depending on the FHR & Telemetry prefs here:
* TelemetrySession.getHistogram
* TelemetrySesssion.getKeyedHistograms

Comment 5

[Alessio Placitelli [:Dexter]]

The "toolkit.telemetry.enabled" completely disables Telemetry when false. After bug 1137252 lands, disabling Telemetry will just disable sending pings to the servers (pings will still be persisted to the disk).

Comment 6

[Alessio Placitelli [:Dexter]]

Attachment: bug1134279.patch

This patch changes nsITelemetry by introducing the following flags:

- canSendBase: to check if we are allowed to send any official data (true on official builds if FHR or Telemetry are enabled at build time).
- canSendExtended: to check if we are allowed to send official extended data, aka Telemetry (true on official build if Telemetry is enabled at build time).
- canRecordExtended: to check if we are allowed to record extended telemetry data. This is basically the old |canRecord|, but renamed to comply to the new unified FHR/Telemetry.

Comment 7

[Vladan Djeric (:vladan)]

Comment on attachment 8575523 [details] [diff] [review]
bug1134279.patch

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

::: toolkit/components/telemetry/nsITelemetry.idl
@@ +255,2 @@
>     */
> +  attribute boolean canRecordExtended;

I'm wondering now if it even makes sense to have the canRecord* flag anymore?

If self-support requires both base (FRH) and extended (classic Telemetry) historical data, then we'll always be recording, and only the "send" permissions are important.

Benjamin:
- should we limit self-support to using only the base (FHR) data?
- should there be a pref to disable all gathering of data for self-support?

Comment 8

[Alessio Placitelli [:Dexter]]

Attachment: bug1134279.patch

I forgot to attach the updated patch (was missing a change in Telemetry.h).

Comment 9

[Benjamin Smedberg]

Self-support will probably only ever use the base data, but since they are mixed into the same ping, the question is really whether we're *recording* the extended data, right?

Once the data is recorded, sending is just a binary "do or don't" flag.

Comment 10

[Alessio Placitelli [:Dexter]]

Comment on attachment 8575959 [details] [diff] [review]
bug1134279.patch

This patch should already deal with the changes we discussed today.

Comment 11

[Vladan Djeric (:vladan)]

Comment on attachment 8575959 [details] [diff] [review]
bug1134279.patch

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

I think we miscommunicated :(

This is my understanding:

1. Self-support will need base data (FHR data) only.

2. The CanRecord flags exist so that base & extended probes can decide whether they are allowed to record a measurement (locally). Whether the build is allowed to send the ping is another matter.

3. In the unified Telemetry, a base probe (old FHR) should be allowed to record if datareporting.healthreport.service.enabled is preffed-on OR if self-support is preffed-on. An extended probe (old Telemetry) can record if toolkit.components.telemetry is preffed-on. For both of these, there is an additional requirement that they be in an official-style build that is allowed to report Telemetry/FHR data (MOZILLA_OFFICIAL + MOZ_TELEMETRY_REPORTING) **OR** if they are in a debug build (no MOZILLA_OFFICAL flag) **OR** that the build is currently running a test (testing=true in setupTelemetry). The same criteria applies when saving pings to disk (e.g. at shutdown).

4. Pings can only be sent from an official-style build that is allowed to report Telemetry/FHR data (MOZILLA_OFFICIAL + MOZ_TELEMETRY_REPORTING) **OR** if the build is currently running a test (the ping gets sent to a localhost test server). So the criteria for sending a ping are always the same, regardless of the content of the ping, whether it be base-only, or base + extended, or a Loop ping, etc.

So assuming I haven't gotten anything wrong in the statements above:

- I agree with Benjamin, CanSend should only indicate whether this is a release build compiled with the right flags or not. We don't need to separate it into Base vs Extended since it's the same ping being sent. 
- I see now there are multiple locations in the codebase where Telemetry::CanSend was misinterpreted.. Other parts of Firefox should have checked for Telemetry::CanRecord, the value of Telemetry::CanSend was irrelevant for them. We should rename Telemetry::CanSend to something like Telemetry::IsOfficialBuild() to prevent further confusion. We should also convert the existing callers of Telemetry::CanSend (Loop, PluginContent.jsm) to call Telemetry::CanRecordExtended instead of ::CanSend
- We need a Telemetry::CanRecordBase and Telemetry::CanRecordExtended
- We need comments clearly explaining the distinction between CanRecordBase and CanRecordExtended, in the .idl declaration and next to the Telemetry.cpp definitions. Devs neeed to know they need permission to add "base" Telemetry measurements
- I think unified Telemetry should not look at the MOZ_SERVICES_HEALTHREPORT flag. It's already hard to reason about Telemetry behaviour with all the build flags and use-cases. When we decommission the old FHR, we should get rid of MOZ_SERVICES_HEALTHREPORT altogether. 
- Eventually, we should also transition to a single pref indicating whether base or extended Telemetry or neither were opted into
- Why do we set canRecord both in TelemetryPing and TelemetrySession?
- Will these name changes break Thunderbird?
- As-is, the code would allow debug builds (without MOZ_TELEMETRY_REPORTING) to send extended Telemetry data (see TelemetyPing.jsm -- GetCanSendExtended would return true for MOZILLA_OFFICIAL + MOZ_SERVICES_HEALTHREPORT)

Comment 12

[Alessio Placitelli [:Dexter]]

Attachment: part 1 - Change the Telemetry flags (v2)

This patch renames canSend to isOfficialTelemetry and adds the new canRecordBase/Extended flags.

Comment 13

[Alessio Placitelli [:Dexter]]

Attachment: part 2 - Change how telemetry flags are used (v2)

Comment 14

[Alessio Placitelli [:Dexter]]

Attachment: part 1 - Change the Telemetry flags (v2)

Updated the comments in Telemetry.cpp.

Comment 15

[Alessio Placitelli [:Dexter]]

(In reply to Vladan Djeric (:vladan) -- please needinfo! from comment #11)
> Comment on attachment 8575959 [details] [diff] [review]
> bug1134279.patch
> 
> Review of attachment 8575959 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I think we miscommunicated :(

Whoops. Sorry about that.

> 3. In the unified Telemetry, a base probe (old FHR) should be allowed to
> record if datareporting.healthreport.service.enabled is preffed-on OR if
> self-support is preffed-on. An extended probe (old Telemetry) can record if
> toolkit.components.telemetry is preffed-on. For both of these, there is an
> additional requirement that they be in an official-style build that is
> allowed to report Telemetry/FHR data (MOZILLA_OFFICIAL +
> MOZ_TELEMETRY_REPORTING) **OR** if they are in a debug build (no
> MOZILLA_OFFICAL flag) **OR** that the build is currently running a test
> (testing=true in setupTelemetry). The same criteria applies when saving
> pings to disk (e.g. at shutdown).

Yes. Also, extended data recording should be coupled to base data recording. If the former is off, the latter can't be enabled.

> - We need comments clearly explaining the distinction between CanRecordBase
> and CanRecordExtended, in the .idl declaration and next to the Telemetry.cpp
> definitions. Devs neeed to know they need permission to add "base" Telemetry
> measurements

I made that clearer in the comments.

> - Why do we set canRecord both in TelemetryPing and TelemetrySession?

That's an error, I've changed that in this patch. |TelemetrySession.enableTelemetryRecording| will be removed by bug 1137252.

> - Will these name changes break Thunderbird?

I don't think so from a quick check, maybe I could flag somebody from #maildev to double check that?

Comment 16

[Vladan Djeric (:vladan)]

Comment on attachment 8579284 [details] [diff] [review]
part 1 - Change the Telemetry flags (v2)

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

::: toolkit/components/telemetry/Telemetry.cpp
@@ +3141,5 @@
>  NS_IMETHODIMP
> +TelemetryImpl::GetCanRecordBase(bool *ret) {
> +  *ret =
> +    Preferences::GetBool("datareporting.healthreport.service.enabled", false) ||
> +    Preferences::GetBool("browser.selfsupport.enabled", false);

- Currently, does turning FHR on/off take effect immediately or on next restart?
- Shouldn't we set these prefs at runtime from TelemetryPing.jsm, same as CanRecordExtended?

@@ +3153,5 @@
>  }
>  
> +NS_IMETHODIMP
> +TelemetryImpl::SetCanRecordExtended(bool canRecord) {
> +  mCanRecordExtended = !!canRecord;

does it make sense to use !! for a bool type?

::: toolkit/components/telemetry/Telemetry.h
@@ +179,2 @@
>   */
> +bool CanRecordExtended();

should we add a CanRecordBase for future use?

::: toolkit/components/telemetry/nsITelemetry.idl
@@ +254,5 @@
> +   * A flag indicating if Telemetry can record base data (FHR data). This is true if the
> +   * FHR data reporting service or self-support are enabled.
> +   *
> +   * In the unlikely event that adding a new base probe is needed, please talk to the
> +   * Telemetry team.

add this link https://wiki.mozilla.org/Firefox/Data_Collection

@@ +261,4 @@
>  
>    /**
> +   * A flag indicating if Telemetry is allowed to record extended data.
> +   * Set this to false to disable gathering of extended telemetry statistics.

"Set this to false" sounds like we want code outside Telemetry to set this flag (we don't).

How about "Returns false when the user hasn't opted into "extended Telemetry" on the Release channel, or when the user has explicitly opted out of Telemetry on Nightly/Aurora/Beta"

Comment 17

[Vladan Djeric (:vladan)]

(In reply to Alessio Placitelli [:Dexter] from comment #15)
> (In reply to Vladan Djeric (:vladan) -- please needinfo! from comment #11)
> > - Will these name changes break Thunderbird?
> 
> I don't think so from a quick check, maybe I could flag somebody from
> #maildev to double check that?

Yes please

Comment 18

[Vladan Djeric (:vladan)]

Comment on attachment 8579281 [details] [diff] [review]
part 2 - Change how telemetry flags are used (v2)

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

::: browser/components/loop/test/mochitest/browser_mozLoop_telemetry.js
@@ +12,5 @@
>   * Enable local telemetry recording for the duration of the tests.
>   */
>  add_task(function* test_initialize() {
> +  let oldCanRecord = Services.telemetry.canRecordExtended;
> +  Services.telemetry.canRecordExtended = true;

Ok now I see now why you wrote "set this to true" in the .idl comment.. maybe just add "set this to true in tests"

::: browser/modules/PluginContent.jsm
@@ +93,5 @@
>      if (!this.content || event.target != this.content.document) {
>        return;
>      }
>  
> +    if (Services.telemetry.canRecordExtended) {

these guys don't need to check for this, they're only accumulating to histograms, and the accumulate call itself checks this flag. Ping Yury Delendik please

::: toolkit/components/telemetry/TelemetrySession.jsm
@@ +1068,5 @@
>        return false;
>      }
>  #endif
>  
>      let enabled = Preferences.get(PREF_ENABLED, false);

TelemetryPing should be the only place checking this pref.. i.e. TelemetryPing.setup() should call TelemetrySession.setup(). gfritzsche said he'd fix this

Comment 19

[Alessio Placitelli [:Dexter]]

Attachment: part 1 - Change the Telemetry flags (v3)

Thanks for the review, I've addressed your comments in this patch.

Comment 20

[Alessio Placitelli [:Dexter]]

Comment on attachment 8580681 [details] [diff] [review]
part 1 - Change the Telemetry flags (v3)

:jcranmer, this patch changes the Telemetry interface renaming a couple of attributes to better reflect their meaning:

- |canRecord| was renamed to |canRecordExtended|
- |canSend| was renamed to |isOfficialTelemetry|

Would this name changes break Thunderbird?

Thanks for your input :)

Comment 21

[Alessio Placitelli [:Dexter]]

Attachment: part 2 - Change how telemetry flags are used (v3)

Comment 22

[Alessio Placitelli [:Dexter]]

Comment on attachment 8580685 [details] [diff] [review]
part 2 - Change how telemetry flags are used (v3)

Yury, due to changes in the Telemetry interfaces I needed to change |PluginContent.jsm|. It was checking |Telemetry.canSend| instead of |Telemetry.canRecord|.

I've removed this check now as |Telemetry.canRecord| is implicitly checked when accumulating an Histogram. Does it look ok to you?

Comment 23

[Georg Fritzsche [:gfritzsche]]

Comment on attachment 8580685 [details] [diff] [review]
part 2 - Change how telemetry flags are used (v3)

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

::: browser/modules/PluginContent.jsm
@@ +386,5 @@
>          shouldShowNotification = true;
>          break;
>      }
>  
> +    if (this._getPluginInfo(plugin).mimetype === "application/x-shockwave-flash") {

I think we should keep guarding that with .canRecord() - AFAIR the code in _recordFlashPluginTelemetry() can cause sync layout activity and we shouldn't pay for that when we don't record telemetry.

Comment 24

[Georg Fritzsche [:gfritzsche]]

We can just guard for this centrally in _recordFlashPluginTelemetry() though.

Comment 25

[Alessio Placitelli [:Dexter]]

Attachment: part 2 - Change how telemetry flags are used (v4)

Thanks Georg, I've added the check in |_recordFlashPluginTelemetry|.

Comment 26

[Alessio Placitelli [:Dexter]]

Attachment: part 2 - Change how telemetry flags are used (v5)

This patch fixes broken Android tests and a compilation issue.

Comment 27

[Alessio Placitelli [:Dexter]]

Attachment: part 2 - Change how telemetry flags are used (v5)

Removed a misleading comment, r+ as discussed with Vladan.

Comment 28

[Alessio Placitelli [:Dexter]]

Since this needs to land before bug 1127918, this is their shared try-push.

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

Comment 29

[Alessio Placitelli [:Dexter]]

Attachment: part 2 - Change how telemetry flags are used (v5)

Rebased.

Comment 30

[Georg Fritzsche [:gfritzsche]]

https://hg.mozilla.org/integration/fx-team/rev/2aa9f4d0385d
https://hg.mozilla.org/integration/fx-team/rev/135c75e6fa1e

Comment 31

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/2aa9f4d0385d
https://hg.mozilla.org/mozilla-central/rev/135c75e6fa1e