Closed Bug 1232773 Opened 9 years ago Closed 9 years ago

Call Adjust.onPause and Adjust.onResume methods

Categories

(Firefox for Android Graveyard :: General, defect)

Product:
Firefox for Android Graveyard
Component:
General
Version:
35 Branch
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(firefox44 fixed, firefox45 fixed, firefox46 fixed, b2g-v2.5 fixed, fennec44+)

Status:
RESOLVED FIXED
Milestone:
Firefox 46
Tracking Flags:
Tracking Status
firefox44 --- fixed
firefox45 --- fixed
firefox46 --- fixed
b2g-v2.5 --- fixed
fennec 44+ ---

People

(Reporter: Margaret, Assigned: mcomella)

References

Details

Attachments

(2 files, 2 obsolete files)

MozReview Request: Bug 1232773 - Call Adjust.onPause/onResume. r=mfinkle
58 bytes, text/x-review-board-request
mfinkle
: review+
Sylvestre
: approval-mozilla-aurora+
ritu
: approval-mozilla-beta+
Details
(44 branch) Call Adjust.onPause and Adjust.onResume methods
4.40 KB, patch
Details | Diff | Splinter Review 
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)
Summary: Call Adjust.onPause and Adjust.onResume events → Call Adjust.onPause and Adjust.onResume methods
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?
tracking-fennec: --- → ?
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
Attachment #8698700 - Flags: review?(mark.finkle) → review+
NI to Nick to help me with the build.
Flags: needinfo?(nalexander)
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.
Flags: needinfo?(nalexander)
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/
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
Attachment #8699227 - Flags: review?(mark.finkle) → review+
(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?
Flags: needinfo?(mark.finkle)
(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.
tracking-fennec: ? → 44+
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
Attachment #8698700 - Flags: approval-mozilla-aurora?
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.
Attachment #8698700 - Flags: approval-mozilla-beta?
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*.
Flags: needinfo?(nalexander)
Attachment #8698700 - Flags: approval-mozilla-beta?
Attachment #8698700 - Flags: approval-mozilla-aurora?
NI self to backout when tree opens.
Flags: needinfo?(michael.l.comella)
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 :(
Flags: needinfo?(nalexander)
(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.
Flags: needinfo?(michael.l.comella)
Status: RESOLVED → REOPENED
status-firefox46: fixed → affected
Resolution: FIXED → ---
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/
Attachment #8698700 - Attachment description: MozReview Request: Bug 1232773 - Add Adjust.onPause/onResume to record session info. r=mfinkle → MozReview Request: Bug 1232773 - Call Adjust.onPause/onResume. r=mfinkle
Attachment #8698700 - Flags: review+ → review?(mark.finkle)
Comment on attachment 8698700 [details]
MozReview Request: Bug 1232773 - Call Adjust.onPause/onResume. r=mfinkle

https://reviewboard.mozilla.org/r/28093/#review25695
Attachment #8698700 - Flags: review?(mark.finkle) → review+
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
Attachment #8698700 - Flags: approval-mozilla-beta?
Attachment #8698700 - Flags: approval-mozilla-aurora?
https://hg.mozilla.org/mozilla-central/rev/0b4e0ce11778
Status: REOPENED → RESOLVED
Closed: 9 years ago → 9 years ago
status-firefox46: affected → fixed
Resolution: --- → FIXED
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.
Flags: needinfo?(michael.l.comella)
Comment on attachment 8698700 [details]
MozReview Request: Bug 1232773 - Call Adjust.onPause/onResume. r=mfinkle

Let's try with aurora.
Attachment #8698700 - Flags: approval-mozilla-aurora? → approval-mozilla-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+
Attachment #8698700 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
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.
Flags: needinfo?(michael.l.comella) → needinfo?(wkocher)
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: