With the desire to remove code that is not providing any value, I suggest we remove the FHR service from Fennec. FHR requires a non-trivial amount of code and additional network pings. I think we should examine whether or not we could use Telemetry or Unified Telemetry alone to accomplish the same tasks. As near as I can tell, we are using FHR for: 1. Usage data for Active Profiles, used to create MAU metrics 2. Session data used to estimate usage drop-off or churn. 3. Search count usage, as well as rank order of popular search engines. We could switch to Adjust for #1 and maybe #2, but we'd need to change the current code calling points. #3 is somewhat useful, but we still depend on the search partners to give us accurate usage counts.

I've wondered about this myself. I'm quite concerned that our current Telemetry approach is failing us for Background Services and possibly even for Fennec. By "current approach", I mean doing everything in Gecko. It's just not robust for things happening in the background. If my fears are correct, this would be less "remove FHR" and more "mutate FHR into a real Telemetry background service on Android".

(In reply to Nick Alexander :nalexander (PTO July 17-27) from comment #1) > If my fears are correct, this would be less "remove FHR" and more "mutate > FHR into a real Telemetry background service on Android". Maybe, but it depends on the data. If the core things we need to collect are only around when Gecko is running, current Telemetry might be fine (even though it's weak). If we decide to go full Java, I'd want a bug filed to kill Gecko's Telemetry in Fennec and use JNI (or other) to get Gecko to access the Java code. There can be only one.

Good, we're all on the same page! Last I checked desktop's measurements for active sessions are totally wrong: it uses the same measure as for idle, so Firefox is considered active if it's open while the user is using the machine. It also uses a tick-based measure. Perhaps gfritzsche knows if those are still true. If we could port Fennec-FHR's session start/stop to telemetry, that would be better. I'd also be happy using Adjust for this part.

I think this is probably the right thing to do, but I don't think this bug is the right place to design this out ;-) This is big enough that it needs a real PRD. That said, I'm going to respond a little bit. What does "desktop's measurements for active sessions" mean? Note that unified telemetry is two separate pieces, and they are mostly independent: * On the client, a gecko service for recording histograms, packaging that up into JSON pings, and uploading those to the server. * On the server, a mechanism for processing JSON pings into aggregate data either automatically or manually, using either heka realtime filters or spark python notebooks. Somebody needs to make a decision about whether the histogram data from gecko is useful to you at all. Vladan uses it, so I expect it is still useful. With that in mind, it's still possible to send that using Java instead of the gecko/desktop-focused telemetry JSMs.

(In reply to Benjamin Smedberg [:bsmedberg] from comment #4) > I think this is probably the right thing to do, but I don't think this bug > is the right place to design this out ;-) This is big enough that it needs a > real PRD. > > That said, I'm going to respond a little bit. > > What does "desktop's measurements for active sessions" mean? > > Note that unified telemetry is two separate pieces, and they are mostly > independent: > > * On the client, a gecko service for recording histograms, packaging that up > into JSON pings, and uploading those to the server. I suspect the uploader is one of the things we'd want to convert to an Android background process. I bet we'd want some form of JSON ping generation in Java too, but it doesn't need to be only in Java. A Java uploader could handle Gecko and Java generated pings. > * On the server, a mechanism for processing JSON pings into aggregate data > either automatically or manually, using either heka realtime filters or > spark python notebooks. This is still important. > Somebody needs to make a decision about whether the histogram data from > gecko is useful to you at all. Vladan uses it, so I expect it is still > useful. With that in mind, it's still possible to send that using Java > instead of the gecko/desktop-focused telemetry JSMs. The histogram data *is* useful for Fennec. This would likely still be part of the Gecko-based ping generation. I suppose a Java ping generator could make histograms too, but it might not be a high priority.

This bug is sounding increasingly like "morph the existing Android FHR implementation into a unified telemetry uploader".

(In reply to Richard Newman [:rnewman] from comment #3) > Good, we're all on the same page! > > Last I checked desktop's measurements for active sessions are totally wrong: > it uses the same measure as for idle, so Firefox is considered active if > it's open while the user is using the machine. It also uses a tick-based > measure. Perhaps gfritzsche knows if those are still true. On desktop we don't have an "active session" model like mobile FHR does. We do track "active ticks" though (somewhat broken on desktop) and total time in the session etc. > If we could port Fennec-FHR's session start/stop to telemetry, that would be > better. I think that transports to the new Telemetry model rather naturally. On desktop we split up sessions into subsession pings on certain events (addon disabled etc.). On Fennec this model could break up session on session start/stop instead. Then a Telemetry uploader service collects that data and takes care of persisting and uploading.

FHR is being shut down at the end of the year, so we could get moving on this bug (and uplift a fix). No point in sending data to nowhere.

Unconditionally: * about:healthreport. * Messaging parts of Fennec to support about:healthreport (data collection). * Bagheera client. * Settings and parts of the data reporting notification. Conditional on not reusing HealthReportUploadService and/or data storage for unified telemetry: * Uploader service. * Uploader client. * Database/Content Provider. Remember to clean up stored data. * 180 days of stored data and environments.

Caution: the adjust framework and the future unified telemetry system will still depend on the basic opt-out UI mechanism. My strong recommendation is that you do not remove the data collection messaging or preferences checkbox.

Given that we are retiring FHR completely now, we should talk for coordination between desktop and mobile removal. I'd like to avoid putting any work in temporary desktop- or mobile-only removal. We do use part of data reporting policy prefs etc., see e.g.: http://gecko.readthedocs.org/en/latest/toolkit/components/telemetry/telemetry/preferences.html We need to understand properly on how this affects mobile. about:healthreport is still supposed to be used, it will be backed by Telemetry data now. Conceptually, for user facing elements like settings and data choices, FHR is now the "base data"/"opt-out" set of Telemetry.

(In reply to Benjamin Smedberg [:bsmedberg] from comment #10) > Caution: the adjust framework and the future unified telemetry system will > still depend on the basic opt-out UI mechanism. My strong recommendation is > that you do not remove the data collection messaging or preferences checkbox. Yup. We already treat each of the components separately; simply disabling MOZ_SERVICES_HEALTHREPORT already does the right thing, so in this case we'd just be deleting some code that's no longer needed. See <https://dxr.mozilla.org/mozilla-central/source/mobile/android/base/DataReportingNotification.java>. Notably we will have already set the FHR upload pref for existing users, so it's not enough to just remove this clause to turn off uploads -- we also need to kill the uploader and ideally clean up that pref, presumably at the same time that we delete the FHR database, unless whoever's tackling UT (Chenxia?) decides to reuse that subsystem. (In reply to Georg Fritzsche [:gfritzsche] from comment #11) > We do use part of data reporting policy prefs etc., see e.g.: > http://gecko.readthedocs.org/en/latest/toolkit/components/telemetry/ > telemetry/preferences.html > We need to understand properly on how this affects mobile. There's no intersection between desktop's data reporting policy and Android's. Right now I believe the only places Fennec touches telemetry are: * The usual default Gecko prefs per channel apply. * Settings allows you to opt in or out by setting toolkit.telemetry.enabled. The FHR upload setting is entirely unrelated to Gecko. If we now have two layers of telemetry data, backed by two checkboxes (one opt-in and one opt-out), then probably we need to add a second Gecko-pref checkbox to Settings. > about:healthreport is still supposed to be used, it will be backed by > Telemetry data now. Has someone scoped out the work for doing that (in particular, doing that on Android)? I know I haven't. We have a messaging system that serializes the FHR payload to hand over to Gecko, so our wrapper differs from desktop's.

(In reply to Richard Newman [:rnewman] from comment #12) > (In reply to Benjamin Smedberg [:bsmedberg] from comment #10) > > Caution: the adjust framework and the future unified telemetry system will > > still depend on the basic opt-out UI mechanism. My strong recommendation is > > that you do not remove the data collection messaging or preferences checkbox. > > Yup. We already treat each of the components separately; simply disabling > MOZ_SERVICES_HEALTHREPORT already does the right thing, so in this case we'd > just be deleting some code that's no longer needed. > > See > <https://dxr.mozilla.org/mozilla-central/source/mobile/android/base/ > DataReportingNotification.java>. > > > Notably we will have already set the FHR upload pref for existing users, so > it's not enough to just remove this clause to turn off uploads -- we also > need to kill the uploader and ideally clean up that pref, presumably at the > same time that we delete the FHR database, unless whoever's tackling UT > (Chenxia?) decides to reuse that subsystem. I don't know if you need/should clean that pref, see below. > (In reply to Georg Fritzsche [:gfritzsche] from comment #11) > > > We do use part of data reporting policy prefs etc., see e.g.: > > http://gecko.readthedocs.org/en/latest/toolkit/components/telemetry/ > > telemetry/preferences.html > > We need to understand properly on how this affects mobile. > > There's no intersection between desktop's data reporting policy and > Android's. > > Right now I believe the only places Fennec touches telemetry are: > > * The usual default Gecko prefs per channel apply. > * Settings allows you to opt in or out by setting toolkit.telemetry.enabled. > > The FHR upload setting is entirely unrelated to Gecko. > > If we now have two layers of telemetry data, backed by two checkboxes (one > opt-in and one opt-out), then probably we need to add a second Gecko-pref > checkbox to Settings. On Desktop we kept the same terminology: "Firefox Health Report" for the "op-out" data, "Telemetry" for the "opt-in" data. That way nothing user-visible changed really, only the backing system used for the data reporting. It also allows to keep/re-use the state of the "opt-out"/"FHR" pref, so we don't need to notify users again. I'm not sure if that transfers well to Android though, but presumably the "FHR upload"/opt-out choice pref could just be kept for adjust/opt-out-Telemetry/...? > > about:healthreport is still supposed to be used, it will be backed by > > Telemetry data now. > > Has someone scoped out the work for doing that (in particular, doing that on > Android)? I know I haven't. We have a messaging system that serializes the > FHR payload to hand over to Gecko, so our wrapper differs from desktop's. No, that planning work (if & how to do it) needs to happen for Android.

I just realized Firefox 43 is fast approaching, so if we want to disable FHR uploads, for that, we'll need to get a simple and very low-risk patch together. We should still keep the data reporting pref and corresponding UI around, since we use that for opting out of Adjust data collection. So, to *just* disable the uploading of any data (not the collecting locally), what's the simplest thing we can do? Are there a few lines of code we can comment out? We can follow up in 44 or 45 with a more proper removal of this code.

I filed bug 1230206 to deal with this.

(In reply to :Margaret Leibovic from comment #14) > So, to *just* disable the uploading of any data (not the collecting > locally), what's the simplest thing we can do? Are there a few lines of code > we can comment out? Yes. Myself or mcomella can do this quite easily. (In reply to Georg Fritzsche [:gfritzsche] from comment #13) > I'm not sure if that transfers well to Android though, but presumably the > "FHR upload"/opt-out choice pref could just be kept for > adjust/opt-out-Telemetry/...? If the Android UT uploader is implemented as an Android (Java) service, then yes, we can directly use the same pref. If we use the Gecko uploader, then some additional work will be required to set a Gecko pref by asking the Java side for the current value (and keep them in sync or clean up the existing pref if desired). (When we say "pref" we mean an Android SharedPreference, by contrast to "Gecko pref".) There's currently nothing on the Gecko side that knows anything about the user's FHR setting, the data reporting notification, or their FHR data.

(In reply to Richard Newman [:rnewman] from comment #16) > (In reply to Georg Fritzsche [:gfritzsche] from comment #13) > > > I'm not sure if that transfers well to Android though, but presumably the > > "FHR upload"/opt-out choice pref could just be kept for > > adjust/opt-out-Telemetry/...? > > If the Android UT uploader is implemented as an Android (Java) service, then > yes, we can directly use the same pref. If we use the Gecko uploader, then > some additional work will be required to set a Gecko pref by asking the Java > side for the current value (and keep them in sync or clean up the existing > pref if desired). > > (When we say "pref" we mean an Android SharedPreference, by contrast to > "Gecko pref".) > > There's currently nothing on the Gecko side that knows anything about the > user's FHR setting, the data reporting notification, or their FHR data. From my perspective we should probably do that either way: unless we change the implications of the opt-out notification we should not show it again or forget the choices users made.

One open question: Do we ever need to work on historic data or incoming data from old clients? In that case we may want to keep some of the documentation around.

This patch removes the FHR from android/base/* 1. hg rm android/base/*/health 2. Removes code reporter and recorder code from BrowserApp and GeckoApp The only tricky part of #2 was changing the FENNEC_WAS_KILLED probe back to the pre SessionInformation check, but that was easy enough to find and do.

This patch removes HealthReportStatusListener from browser.js I left aboutHealthReport.[js|xhtml] alone since we still need it for opt-out reasons. The current patchset leaves the "show me the raw data" as blank, but we can show the telemetry data in there.

This patch removes code from android/services/* 1. hg rm android/services/*/healthreport 2. hg rm android/services/*/bagheera 3. hg rm android/services/manifests/HealthReport* 4. Update AndroidManifest.xmml.in to remove the HealthReport* includes 5. hg rm android/tests/*/healthreport 6. hg rm android/tests/*/test/TestDeflaftion and TestBoundedByteArrayEntity These were all decoupled well enough that we don't have any real issues, but I did move a use-permission into SyncAndroidManifest_permissions because testOrderedBroadcast depended on it. Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=a544c4efc27c

I left MOZ_HEALTH_REPORT and PREFS_HEALTHREPORT_UPLOAD_ENABLED because we use these to control the opt-out data collection. I left the Settings code/UI in place too.

(In reply to Mark Finkle (:mfinkle) from comment #19) > 1. hg rm android/base/*/health > 2. Removes code reporter and recorder code from BrowserApp and GeckoApp What's the plan for implementing UT? AIUI, we still need event recording (BHR) and environment stuff. Last time I talked to mcomella about this, we only planned to remove storage, upload, environment hashing, and measurements, and reuse the existing Java-side code for scheduling and capture. It seems foolish to throw out stable session start/end and environment capture code.

(In reply to Richard Newman [:rnewman] from comment #23) > (In reply to Mark Finkle (:mfinkle) from comment #19) > > > 1. hg rm android/base/*/health > > 2. Removes code reporter and recorder code from BrowserApp and GeckoApp > > What's the plan for implementing UT? > > AIUI, we still need event recording (BHR) and environment stuff. Last time I > talked to mcomella about this, we only planned to remove storage, upload, > environment hashing, and measurements, and reuse the existing Java-side code > for scheduling and capture. > > It seems foolish to throw out stable session start/end and environment > capture code. If we need the code, we can always add it back.

This patch removes BrowserHealthReporter and BrowserHealthRecorder. It leaves HealthRecorder (abstract), StubbedHealthRecorder (does nothing) and SessionInformation (for managing time-spent). These will not be needed in the UT MVP, but Richard convinced me to leave them in place to clearly show where the session recording hooks are applied to BrowserApp/GeckoApp. I stubbed out the GeckoPreference broadcast helpers since they have nothing to broadcast to, but again, we might need to broadcast to the UT service in the future. The integration hooks are in place, but there is no implementation left.

Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=43d1e4b7bc5f

I'm potentially re-using some of this for UT (or at least using it as a guide) so I'd prefer to leave it in – adding it back later will take more effort for me. As an alternative, if we remove the entry points, Proguard should optimize it all out, right?

(In reply to Michael Comella (:mcomella) from comment #27) > I'm potentially re-using some of this for UT (or at least using it as a > guide) so I'd prefer to leave it in – adding it back later will take more > effort for me. I'm not convinced. To counter, I don't want code in the shipping app that executes, but has no real value. > As an alternative, if we remove the entry points, Proguard should optimize > it all out, right? Maybe, but why bother. Folks, this code is going away. Let's make our peace with it.

Top crashes for Fx43: https://crash-stats.mozilla.com/topcrashers/?product=FennecAndroid&version=43.0&days=7 #10 crash on Fx43: java.lang.IllegalStateException: Not initialized. at org.mozilla.gecko.background.healthreport.ProfileInformationCache.ensureInitialized(ProfileInformationCache.java) #16 crash on Fx43: java.lang.IllegalStateException: Could not fetch Health Report content provider. at org.mozilla.gecko.health.BrowserHealthRecorder.<init>(BrowserHealthRecorder.java)

Comment on attachment 8705474 [details] [diff] [review] remove-android-base-fhr v0.2 Review of attachment 8705474 [details] [diff] [review]: ----------------------------------------------------------------- ::: mobile/android/base/java/org/mozilla/gecko/health/BrowserHealthRecorder.java @@ -334,5 @@ > - * Retrieve the stored profile creation time from the profile directory. > - * > - * @return the <code>created</code> value from the times.json file, or -1 on failure. > - */ > - protected static long getProfileInitTimeFromFile(final String profilePath) { Mentioned in the other bug: I think Michael will be pulling these straight out of the grave (and perhaps into Profile).

https://hg.mozilla.org/integration/fx-team/rev/acbe1452a667 https://hg.mozilla.org/integration/fx-team/rev/a9f337003e06 https://hg.mozilla.org/integration/fx-team/rev/d9661fecd939

https://hg.mozilla.org/mozilla-central/rev/acbe1452a667 https://hg.mozilla.org/mozilla-central/rev/a9f337003e06 https://hg.mozilla.org/mozilla-central/rev/d9661fecd939

(In reply to Mark Finkle (:mfinkle) from comment #29) > Top crashes for Fx43: > https://crash-stats.mozilla.com/topcrashers/ > ?product=FennecAndroid&version=43.0&days=7 > > #10 crash on Fx43: > java.lang.IllegalStateException: Not initialized. at > org.mozilla.gecko.background.healthreport.ProfileInformationCache. > ensureInitialized(ProfileInformationCache.java) These crashes are #20 and #10, respectively, on 43.0 (beta) so I am requesting uplift to Beta > #16 crash on Fx43: > java.lang.IllegalStateException: Could not fetch Health Report content > provider. at > org.mozilla.gecko.health.BrowserHealthRecorder.<init>(BrowserHealthRecorder. > java)

Comment on attachment 8705123 [details] [diff] [review] remove-android-chrome-fhr v0.1 Approval Request Comment [Feature/regressing bug #]:Two top crashes on Beta [User impact if declined]: Crashes [Describe test coverage new/current, TreeHerder]: Landed on nightly for a few days with no apparent issues [Risks and why]: Most of these patches is file removal. The remaining code paths are stubbed out, minimizing risk. [String/UUID change made/needed]: None Same request for the other two patches...

Comment on attachment 8705128 [details] [diff] [review] remove-android-services-fhr v0.1 see above

Comment on attachment 8705474 [details] [diff] [review] remove-android-base-fhr v0.2 see above

If approved, I will make the patch tweaks needed to land this on Beta, due to the source folder reorganization.

Comment on attachment 8705123 [details] [diff] [review] remove-android-chrome-fhr v0.1 As such the size of these patches gives me a lot of anxiety but I trust Mark's judgement on the risk involved. Let's uplift to Beta44 with the hopes that this helps improve Fennec stability.

Comment on attachment 8705474 [details] [diff] [review] remove-android-base-fhr v0.2 Beta44+, Aurora45+

These are going to need rebasing to work around the lack of Bug 1107811 on beta, and it looks like there's at least one other conflict in applying patch 3 to beta. Haven't tried uplifting to aurora yet.

They apply cleanly to aurora: remote: https://hg.mozilla.org/releases/mozilla-aurora/rev/5f72d47564e3 remote: https://hg.mozilla.org/releases/mozilla-aurora/rev/ac0788fda729 remote: https://hg.mozilla.org/releases/mozilla-aurora/rev/e0af556e825

Rebased for beta: remote: https://hg.mozilla.org/releases/mozilla-beta/rev/012e3bcabf6c remote: https://hg.mozilla.org/releases/mozilla-beta/rev/dbf26af1a8a6 remote: https://hg.mozilla.org/releases/mozilla-beta/rev/e146313ddb54

https://hg.mozilla.org/releases/mozilla-b2g44_v2_5/rev/012e3bcabf6c https://hg.mozilla.org/releases/mozilla-b2g44_v2_5/rev/dbf26af1a8a6 https://hg.mozilla.org/releases/mozilla-b2g44_v2_5/rev/e146313ddb54

Verified as fixed on 44.0b9 on a Xiaomi Mi i4 with 5.0.2 Android.