FHR is dead, and unified telemetry isn't implemented on mobile yet. Let's use Adjust to get session data in the meantime. From Adjust support: For us, session is counted as a time which user spent by having app in foreground. Once app goes in background, we internally save timestamp when that happened and next time app comes to foreground, we compare those two timestamps and check if there's 30 minutes or more difference between them. If yes - bringing app to foreground will be counted as a new session. If not, bringing app to foreground will be counted as the same session (or, as you will see in Adjust logs, it will be counted as a new sub-session of previous session). Since we obviously need information about moments when app goes to background / comes back to foreground, onResume and onPause methods are vital for us. Unfortunately, Android doesn't offer some system events which we can just subscribe onto and do everything internally w/o bothering the user for some extra effort. This is why we ask users to make a call to Adjust.onResume() in every activity onResume method and also to make a call to Adjust.onPause() in every activity onPause method. It sounds annoying probably, but this is the only way for us to keep above logic for session length counting consistent, no matter where user is in app at some moment when he decides to put it to background. In theory, there are three ways for you to achieve this (which you as experienced Android developers probably already know): * Brute force and overriding every activity onResume and onPause methods in order to make calls to adjust methods * Defining activity class which will each one of your classes inherit and then make these calls just in mother class * If your app is aiming devices with Android API level 14 or higher, you can take advantage of ActivityLifecycleCallbacks and also make calls to adjust methods in one place (you can check our example app on GitHub repo how this can be done: https://github.com/adjust/android_sdk/blob/master/Adjust/example/src/main/java/com/adjust/example/GlobalApplication.java#L60)

Review commit: https://reviewboard.mozilla.org/r/28093/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/28093/

I was unable to build and test this. When I add the appropriate flags to my mozconfig (MOZ_NATIVE_DEVICES & MOZ_INSTALL_TRACKING), I get compile errors: AdjustFactory.java:5: error: package ch.boye.httpclientandroidlib.client does not exist I'm talking to Nick atm to get it working.

Nick fixed this issue with: https://hg.mozilla.org/integration/fx-team/rev/1801cee7c4ef But I get more build errors: Locales.java:16: error: package android.support.v4.app does not exist 0:33.40 import android.support.v4.app.FragmentActivity;

I assume we're aiming for 43 here?

Comment on attachment 8698700 [details] MozReview Request: Bug 1232773 - Call Adjust.onPause/onResume. r=mfinkle https://reviewboard.mozilla.org/r/28093/#review25137 ::: mobile/android/base/java/org/mozilla/gecko/BrowserApp.java:892 (Diff revision 1) > + AdjustConstants.getAdjustHelper().onResume(); Add a blank line and a comment like: // Needed for Adjust to get accurate session measurements ::: mobile/android/base/java/org/mozilla/gecko/BrowserApp.java:914 (Diff revision 1) > + AdjustConstants.getAdjustHelper().onPause(); Add a blank line (above and below) and a comment like: // Needed for Adjust to get accurate session measurements

NI to Nick to help me with the build.

I just realized that we should be checking that Adjust is allowed (not opted out) before calling the onPause/onResume

Nick helped me with the build (bug 1220309 comment 17). I confirmed this works via Adjust's build-in logging. (In reply to Mark Finkle (:mfinkle) from comment #7) > I just realized that we should be checking that Adjust is allowed (not opted > out) before calling the onPause/onResume And I'll work on that.

Comment on attachment 8698700 [details] MozReview Request: Bug 1232773 - Call Adjust.onPause/onResume. r=mfinkle Review request updated; see interdiff: https://reviewboard.mozilla.org/r/28093/diff/1-2/

Review commit: https://reviewboard.mozilla.org/r/28203/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/28203/

Review commit: https://reviewboard.mozilla.org/r/28205/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/28205/

Finkle, should onReceive [1] also check if the user has Adjust enabled? [1]: https://mxr.mozilla.org/mozilla-central/source/mobile/android/base/java/org/mozilla/gecko/distribution/ReferrerReceiver.java#64

Comment on attachment 8699227 [details] MozReview Request: Bug 1232773 - Check if user is opt out from Adjust before accessing. r=mfinkle https://reviewboard.mozilla.org/r/28203/#review25247 Nice

(In reply to Michael Comella (:mcomella) from comment #12) > Finkle, should onReceive [1] also check if the user has Adjust enabled? > > [1]: > https://mxr.mozilla.org/mozilla-central/source/mobile/android/base/java/org/ > mozilla/gecko/distribution/ReferrerReceiver.java#64 Yeah, I guess so. You can do the same type of check in the AdjustHelper, right?

(In reply to Mark Finkle (:mfinkle) from comment #14) > (In reply to Michael Comella (:mcomella) from comment #12) > > Finkle, should onReceive [1] also check if the user has Adjust enabled? > > > > [1]: > > https://mxr.mozilla.org/mozilla-central/source/mobile/android/base/java/org/ > > mozilla/gecko/distribution/ReferrerReceiver.java#64 > > Yeah, I guess so. You can do the same type of check in the AdjustHelper, > right? Yep – done and folded.

https://hg.mozilla.org/integration/fx-team/rev/0aa77b78405e4f62e9f193f08e70f8946087458a Bug 1232773 - Add Adjust.onPause/onResume to record session info. r=mfinkle https://hg.mozilla.org/integration/fx-team/rev/07a32b55829d58f47ded2c0a8f9e98da9bdeb577 Bug 1232773 - Check if user is opt out from Adjust before accessing. r=mfinkle https://hg.mozilla.org/integration/fx-team/rev/a8f8ec75a2a9b81aa3963ff6c330133adbd095d5 Bug 1232773 - review: Comment on Adjust usage. r=me

https://hg.mozilla.org/mozilla-central/rev/0aa77b78405e https://hg.mozilla.org/mozilla-central/rev/07a32b55829d https://hg.mozilla.org/mozilla-central/rev/a8f8ec75a2a9

Comment on attachment 8698700 [details] MozReview Request: Bug 1232773 - Call Adjust.onPause/onResume. r=mfinkle Approval Request Comment [Feature/regressing bug #]: Removal of FHR to record session data [User impact if declined]: We will have no data for user session time in earlier versions of FF since FHR is being decomissioned. [Describe test coverage new/current, TreeHerder]: Tested locally. [Risks and why]: The main changes are extremely simple but the (existing) abstraction we used to jump through compile time flags adds some possibility of error (e.g. NullPointerException). However, I followed the paradigm set forth so it's a fairly low possibility. [String/UUID change made/needed]: None

Comment on attachment 8698700 [details] MozReview Request: Bug 1232773 - Call Adjust.onPause/onResume. r=mfinkle The uplift request in comment 18 applies to all of the patches landed in comment 16.

Comment on attachment 8698700 [details] MozReview Request: Bug 1232773 - Call Adjust.onPause/onResume. r=mfinkle Apparently we don't build with these flags in automation – I get build errors on a clobber: 1:09.14 /Users/mcomella/dev/moz/mobile/android/base/java/org/mozilla/gecko/adjust/AdjustHelper.java:8: error: cannot find symbol 1:09.14 import org.mozilla.gecko.GeckoSharedPrefs; 1:09.14 ^ 1:09.14 symbol: class GeckoSharedPrefs 1:09.14 location: package org.mozilla.gecko 1:09.15 /Users/mcomella/dev/moz/mobile/android/base/java/org/mozilla/gecko/adjust/AdjustHelper.java:9: error: package org.mozilla.gecko.preferences does not exist 1:09.15 import org.mozilla.gecko.preferences.GeckoPreferences; 1:09.15 ^ 1:09.18 Reading program jar [/Users/mcomella/dev/moz/objdir-droid-frontend/dist/exploded-aar/recyclerview-v7-23.0.1/recyclerview-v7-23.0.1-classes.jar] 1:09.25 /Users/mcomella/dev/moz/mobile/android/base/java/org/mozilla/gecko/adjust/AdjustHelper.java:68: error: cannot find symbol 1:09.25 final SharedPreferences prefs = GeckoSharedPrefs.forApp(context); 1:09.25 ^ 1:09.25 symbol: variable GeckoSharedPrefs 1:09.25 location: class AdjustHelper 1:09.25 /Users/mcomella/dev/moz/mobile/android/base/java/org/mozilla/gecko/adjust/AdjustHelper.java:69: error: cannot find symbol 1:09.25 return !prefs.getBoolean(GeckoPreferences.PREFS_HEALTHREPORT_UPLOAD_ENABLED, true); 1:09.25 ^ 1:09.25 symbol: variable GeckoPreferences 1:09.25 location: class AdjustHelper 1:09.27 Note: /Users/mcomella/dev/moz/mobile/android/base/java/org/mozilla/gecko/SysInfo.java uses or overrides a deprecated API. Nick, is there any way to easily add this configuration to automation or will we have to look into task cluster? In the mean-time, time to back this out – I'm assuming the dependencies are separated so we can't access Fennec code in Adjust*.

NI self to backout when tree opens.

Nick told me why the build failed in mach but not in gradle (which is why I missed this): 16:34 <@nalexander> mcomella: Adjust is built before the browser: https://dxr.mozilla.org/mozilla-central/source/mobile/android/base/moz.build?from=mobile%2Fandroid%2Fbase%2Fmoz.build#36 16:34 <@nalexander> mcomella: the pattern is: expose Adjust in AdjustHelper, then consume in BrowserAppl 16:35 <@nalexander> mcomella: so that we can have the dummy StubAdjustHelper when Adjust isn't compiled in.

(In reply to Michael Comella (:mcomella) from comment #22) > Nick told me why the build failed in mach but not in gradle (which is why I > missed this): > > 16:34 <@nalexander> mcomella: Adjust is built before the browser: > https://dxr.mozilla.org/mozilla-central/source/mobile/android/base/moz. > build?from=mobile%2Fandroid%2Fbase%2Fmoz.build#36 > 16:34 <@nalexander> mcomella: the pattern is: expose Adjust in AdjustHelper, > then consume in BrowserAppl > 16:35 <@nalexander> mcomella: so that we can have the dummy StubAdjustHelper > when Adjust isn't compiled in. I believe mcomella is unblocked. Mike, re-flag me if you hit snags. Sorry this is so gnarly :(

(In reply to Michael Comella (:mcomella) from comment #22) > Nick told me why the build failed in mach but not in gradle (which is why I > missed this): I filed bug 1233601.

https://hg.mozilla.org/integration/fx-team/rev/6587e9c69c362f1674b3e158be23311139411441 Bug 1232773 - Backout 3 changesets for build errors.

New patches build on bug 1233614.

Comment on attachment 8698700 [details] MozReview Request: Bug 1232773 - Call Adjust.onPause/onResume. r=mfinkle Review request updated; see interdiff: https://reviewboard.mozilla.org/r/28093/diff/2-3/

Comment on attachment 8698700 [details] MozReview Request: Bug 1232773 - Call Adjust.onPause/onResume. r=mfinkle https://reviewboard.mozilla.org/r/28093/#review25695

https://hg.mozilla.org/integration/fx-team/rev/0b4e0ce11778906d435ca401efd96db88cac27f4 Bug 1232773 - Call Adjust.onPause/onResume. r=mfinkle

Comment on attachment 8698700 [details] MozReview Request: Bug 1232773 - Call Adjust.onPause/onResume. r=mfinkle Please uplift with blocking bug 1233614. Approval Request Comment [Feature/regressing bug #]: Removal of FHR [User impact if declined]: We will be unable to identify user's session data because our previous implementation in FHR will be disabled shortly. [Describe test coverage new/current, TreeHerder]: Tested locally. [Risks and why]: Low – basic coding errors (e.g. NullPointerExceptions) but we're following an existing paradigm and I don't forsee issues. [String/UUID change made/needed]: None

Michael, your risk assessment on this one makes me anxious about uplifting to Beta44. NullPointerExceptions would impact the stability of 44 and therefore I want to suggest that, if this is only about removing code (that will not get used in 44) with the potential risk of introducing crashes, can we let this one ride the Aurora45 -> Beta45 train? Please let me know.

Comment on attachment 8698700 [details] MozReview Request: Bug 1232773 - Call Adjust.onPause/onResume. r=mfinkle Let's try with aurora.

(In reply to Ritu Kothari (:ritu) from comment #33) > Michael, your risk assessment on this one makes me anxious about uplifting > to Beta44. NullPointerExceptions would impact the stability of 44 and > therefore I want to suggest that, if this is only about removing code (that > will not get used in 44) with the potential risk of introducing crashes, can > we let this one ride the Aurora45 -> Beta45 train? Please let me know. Without this patch, we won't have accurate data for our usage metrics, so I am strongly in favor of uplifting it. Mike's assessment sounds pretty low risk to me. Looking at the patch, I'm not really sure where an NPE would happen, other than in Adjust's code, which should be pretty well tested if it's used by other apps.

(In reply to :Margaret Leibovic from comment #35) > (In reply to Ritu Kothari (:ritu) from comment #33) > > Michael, your risk assessment on this one makes me anxious about uplifting > > to Beta44. NullPointerExceptions would impact the stability of 44 and > > therefore I want to suggest that, if this is only about removing code (that > > will not get used in 44) with the potential risk of introducing crashes, can > > we let this one ride the Aurora45 -> Beta45 train? Please let me know. > > Without this patch, we won't have accurate data for our usage metrics, so I > am strongly in favor of uplifting it. > > Mike's assessment sounds pretty low risk to me. Looking at the patch, I'm > not really sure where an NPE would happen, other than in Adjust's code, > which should be pretty well tested if it's used by other apps. Margaret, thanks for providing more context on this one. If your assessment is that this is safe and needed in 44, then I will get it uplifted to m-b.

Comment on attachment 8698700 [details] MozReview Request: Bug 1232773 - Call Adjust.onPause/onResume. r=mfinkle This seems to be the only mechanism to get telemetry data on Fennec. I am a bit concerned about the size of the patch but approving it anyway. Beta44+

I think this needs to work around the lack of bug 1107811 on beta. Can we get a rebased patch?

(In reply to :Margaret Leibovic from comment #35) > (In reply to Ritu Kothari (:ritu) from comment #33) > > Michael, your risk assessment on this one makes me anxious about uplifting > > to Beta44. NullPointerExceptions would impact the stability of 44 and > > therefore I want to suggest that, if this is only about removing code (that > > will not get used in 44) with the potential risk of introducing crashes, can > > we let this one ride the Aurora45 -> Beta45 train? Please let me know. > > Mike's assessment sounds pretty low risk to me. Looking at the patch, I'm > not really sure where an NPE would happen, other than in Adjust's code, > which should be pretty well tested if it's used by other apps. I was most concerned with the complexity added by our AdjustHelper abstraction, which is used to work around the build system. It's a well-defined pattern we already use. I often think NPEs aren't feasible and they still happen so I generally just mention them as a possibility in my risk evaluations – perhaps I'm being too careful. Working on the rebased patch...

NI for uplift.