Don't rely on datareporting.healthreport.uploadEnabled to control telemetry submission on Android

RESOLVED FIXED in Firefox 40

Status

()

defect
--
major
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: jchen, Assigned: gfritzsche)

Tracking

unspecified
mozilla41
All
Android
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox40 fixed, firefox41 fixed)

Details

(Whiteboard: [uplift])

Attachments

(1 attachment, 1 obsolete attachment)

It appears we stopped getting Fennec telemetry since around the 4-18 Nightly.

One cause I see is that the "datareporting.healthreport.uploadEnabled" pref is *not* set in Fennec, but there may be other causes.
Can someone look into this, Vladan? We didn't notice this for about a month, so we should get it fixed asap.
Flags: needinfo?(vdjeric)
I don't think "datareporting.healthreport.uploadEnabled" controls the FHR system in Fennec, which is a Java background service. That said, Telemetry is not connected to that pref either, and we do depend on Gecko's Telemetry system to upload telemetry.
Flags: needinfo?(rnewman)
(In reply to Mark Finkle (:mfinkle) from comment #2)
> I don't think "datareporting.healthreport.uploadEnabled" controls the FHR
> system in Fennec, which is a Java background service. That said, Telemetry
> is not connected to that pref either, and we do depend on Gecko's Telemetry
> system to upload telemetry.

Unified Telemetry (FHR + Telemetry) does look at the "datareporting.healthreport.uploadEnabled" pref:

http://hg.mozilla.org/mozilla-central/annotate/4fb7ff694bf5/toolkit/components/telemetry/TelemetryController.jsm#l1186
Flags: needinfo?(vdjeric) → needinfo?(gfritzsche)
(In reply to Vladan Djeric (:vladan) -- please needinfo! from comment #3)

> Unified Telemetry (FHR + Telemetry) does look at the
> "datareporting.healthreport.uploadEnabled" pref:
> 
> http://hg.mozilla.org/mozilla-central/annotate/4fb7ff694bf5/toolkit/
> components/telemetry/TelemetryController.jsm#l1186

Fennec is not using Unified Telemetry, right?
(In reply to Vladan Djeric (:vladan) -- please needinfo! from comment #3)

> Unified Telemetry (FHR + Telemetry) does look at the
> "datareporting.healthreport.uploadEnabled" pref:

Guess that's the bug then: don't do that. :D

Fennec ships Gecko's telemetry but not Gecko's FHR.
Assignee: nobody → gfritzsche
Blocks: 1069869
Status: NEW → ASSIGNED
Flags: needinfo?(rnewman)
Summary: No Fennec telemetry since 4-18 → Don't rely on datareporting.healthreport.uploadEnabled to control telemetry submission
Clearly we've specified that this is how desktop unified telemetry is supposed to behave. So what you want is an android-specific exception.
Summary: Don't rely on datareporting.healthreport.uploadEnabled to control telemetry submission → Don't rely on datareporting.healthreport.uploadEnabled to control telemetry submission on Android
(In reply to Benjamin Smedberg  [:bsmedberg] from comment #6)
> Clearly we've specified that this is how desktop unified telemetry is
> supposed to behave. So what you want is an android-specific exception.

If the pref is unused for any Java FHR impl, but could be used to easily re-enable Telemetry then let's add it to mobile.js

Richard - Sound safe?
Flags: needinfo?(rnewman)
Safe for these purposes, potentially confusing for users or distributions that think they can use that pref to control health report upload on Android, or for future developers.

If you take that avenue, comment the heck out of the pref value.


Some alternate solutions:

* don't check d.h.uE if toolkit.telemetry.unified is false (which is the case for Fennec).

* introduce a second pref that can unilaterally allow telemetry upload, and check that alongside.

* don't assume that the _absence_ of d.h.uE (which I presume is the case for Fennec) means no telemetry upload. There are a few different alternatives: look for a second pref; assume true; maybe others.
Flags: needinfo?(rnewman)
(In reply to Richard Newman [:rnewman] from comment #8)
> * don't check d.h.uE if toolkit.telemetry.unified is false (which is the
> case for Fennec).

That is what should probably happen (unless someone objects). That would also cover other uses, say Thunderbird and B2G.
Flags: needinfo?(gfritzsche)
Blocks: 1120356
No longer blocks: 1069869
Looking at the implementation, this was simply a bug in the send condition check.
Attachment #8607998 - Flags: review?(vdjeric)
Attachment #8607998 - Flags: review?(vdjeric) → review+
Attachment #8607998 - Attachment is obsolete: true
Attachment #8608844 - Flags: review+
Keywords: checkin-needed
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/5f9055c07cdd
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla41
Whiteboard: [uplift]
Comment on attachment 8608844 [details] [diff] [review]
Don't rely on datareporting.healthreport.uploadEnabled to control telemetry submission on Android

Approval Request Comment
[Feature/regressing bug #]:
Unified Telemetry, https://wiki.mozilla.org/Unified_Telemetry
This is part of the first (main) batch of uplifts to 40 to enable shipping on that train, see bug 1120356, comment 2.
[User impact if declined]:
Data & measurement insight projects delayed or blocked with direct impact on projects depending on this.
[Describe test coverage new/current, TreeHerder]:
We have good automation coverage of the feature.
We also had manual tests of the main tasks as well as confirmation of correct behavior on Nightly for the patches here.
[Risks and why]:
Low-risk - these patches are rather isolated to Telemetry and have been on Nightly for a while with no bad reports.
We intend to track on-going data quality and other issues during the 40 aurora & beta and flip the new behavior off when it doesn't meet the requirements.
[String/UUID change made/needed]:
The only string changes were to the about:telemetry page.
We decided that we can live with missing translations on that page for a cycle as that page is not exactly user-facing.
Attachment #8608844 - Flags: approval-mozilla-aurora?
Comment on attachment 8608844 [details] [diff] [review]
Don't rely on datareporting.healthreport.uploadEnabled to control telemetry submission on Android

Unified telemetry is an important new feature. It is blocking some other projects. Taking it.
Attachment #8608844 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
You need to log in before you can comment on or make changes to this bug.