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

RESOLVED FIXED in Firefox 39

Status

()

defect
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: gfritzsche, Assigned: Dexter)

Tracking

Trunk
mozilla39
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox39 fixed)

Details

Attachments

(2 attachments, 9 obsolete attachments)

14.24 KB, patch
vladan
: review+
Details | Diff | Splinter Review
19.48 KB, patch
Dexter
: review+
Details | Diff | Splinter Review
We should audit that the TelemetryPing sending & persistence functionality is properly guarded with Telemetry.canSend checks.

Comment 1

4 years ago
Has canSend been updated to mean "FHR or telemetry"?
Reporter

Comment 2

4 years ago
Just
Assignee: nobody → gfritzsche
Reporter

Comment 3

4 years ago
(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
Reporter

Updated

4 years ago
Blocks: 1120356
No longer blocks: 1134269
Reporter

Updated

4 years ago
Blocks: 1127918
No longer blocks: 1120356
Reporter

Comment 4

4 years ago
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
Summary: Verify that TelemetryPing.* functions are guarded properly with Telemetry.canSend → Make TelemetryPing and TelemetrySession code use the "FHR enabled" & "Telemetry enabled" prefs properly
Reporter

Updated

4 years ago
Depends on: 1137257
Assignee

Updated

4 years ago
Depends on: 1137252
Assignee

Comment 5

4 years ago
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).
Assignee

Comment 6

4 years ago
Posted patch bug1134279.patch (obsolete) — Splinter Review
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.
Assignee: gfritzsche → alessio.placitelli
Status: NEW → ASSIGNED
Attachment #8575523 - Flags: review?(vdjeric)
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?
Attachment #8575523 - Flags: review?(vdjeric)
Flags: needinfo?(benjamin)
Assignee

Comment 8

4 years ago
Posted patch bug1134279.patch (obsolete) — Splinter Review
I forgot to attach the updated patch (was missing a change in Telemetry.h).
Attachment #8575523 - Attachment is obsolete: true

Comment 9

4 years ago
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.
Flags: needinfo?(benjamin)
Assignee

Comment 10

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

This patch should already deal with the changes we discussed today.
Attachment #8575959 - Flags: review?(vdjeric)
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)
Attachment #8575959 - Flags: review?(vdjeric) → review-
Reporter

Updated

4 years ago
Blocks: 1137252
No longer depends on: 1137252
Assignee

Comment 12

4 years ago
This patch renames canSend to isOfficialTelemetry and adds the new canRecordBase/Extended flags.
Attachment #8575959 - Attachment is obsolete: true
Attachment #8579280 - Flags: review?(vdjeric)
Assignee

Comment 13

4 years ago
Attachment #8579281 - Flags: review?(vdjeric)
Assignee

Comment 14

4 years ago
Updated the comments in Telemetry.cpp.
Attachment #8579280 - Attachment is obsolete: true
Attachment #8579280 - Flags: review?(vdjeric)
Attachment #8579284 - Flags: review?(vdjeric)
Assignee

Comment 15

4 years ago
(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 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"
Attachment #8579284 - Flags: review?(vdjeric)
(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 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
Attachment #8579281 - Flags: review?(vdjeric) → review+
Assignee

Comment 19

4 years ago
Thanks for the review, I've addressed your comments in this patch.
Attachment #8579284 - Attachment is obsolete: true
Assignee

Updated

4 years ago
Attachment #8580681 - Flags: review?(vdjeric)
Assignee

Comment 20

4 years ago
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 :)
Attachment #8580681 - Flags: feedback?(Pidgeot18)
Assignee

Comment 21

4 years ago
Attachment #8579281 - Attachment is obsolete: true
Attachment #8580685 - Flags: review+
Assignee

Comment 22

4 years ago
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?
Attachment #8580685 - Flags: feedback?(ydelendik)
Reporter

Comment 23

4 years ago
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.
Attachment #8580685 - Flags: feedback?(ydelendik)
Reporter

Comment 24

4 years ago
We can just guard for this centrally in _recordFlashPluginTelemetry() though.
Assignee

Comment 25

4 years ago
Thanks Georg, I've added the check in |_recordFlashPluginTelemetry|.
Attachment #8580685 - Attachment is obsolete: true
Attachment #8581560 - Flags: review+
Attachment #8580681 - Flags: review?(vdjeric) → review+
Assignee

Comment 26

4 years ago
This patch fixes broken Android tests and a compilation issue.
Attachment #8581560 - Attachment is obsolete: true
Assignee

Comment 27

4 years ago
Removed a misleading comment, r+ as discussed with Vladan.
Attachment #8582454 - Attachment is obsolete: true
Attachment #8582496 - Flags: review+
Assignee

Comment 28

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

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

Updated

4 years ago
Keywords: checkin-needed
Assignee

Comment 29

4 years ago
Rebased.
Attachment #8582496 - Attachment is obsolete: true
Attachment #8583113 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/2aa9f4d0385d
https://hg.mozilla.org/mozilla-central/rev/135c75e6fa1e
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla39
Depends on: 1148763

Updated

4 years ago
Depends on: 1150975
Reporter

Updated

4 years ago
Depends on: 1157282
Assignee

Updated

4 years ago
Attachment #8580681 - Flags: feedback?(Pidgeot18)
You need to log in before you can comment on or make changes to this bug.