Simultaneous view tracking, automatic Fragment tracking

[foooorsyth]

TLDR

In this PR I add lifecycle observation to the base SDK as a means to reliably and accurately track sessions, foreground/background status, and views. I also add the ability to track multiple views simultaneously, which is necessary for single Activity / multi-Fragment apps or any app in which multiple view components may displayed at the same time.

Motivations

1. The current implementation of view tracking (one view at a time) is of low value in apps that make heavy use of Fragments as opposed to "one Activity per screen" app architecture. In a "one Activity per app" application, Countly's view tracking is effectively useless. People have different preferences for app architecture, and an ideal view tracking implementation would be able to account for most if not all of them. In the below scenario, we should be able to know that SampleActivity, BlueFragment, and RedFragment are all visible at the same time.

2. The current Activity callbacks (onStart/onStop) make false assumptions with respect to the order in which they will be called. This can lead to the following:
   • The SDK thinking it is in the foreground when it is actually in the background, and vice versa
   • Incorrect Activity count
   • Incorrect view name reporting
   • Incorrect view durations
   • Combinations of the above

These are all active bugs. Most of the time, these incorrect assumptions will not lead to runtime error. Instead, incorrect information will simply be reported to the backend.

Example:

Let's say ActivityA starts ActivityB. The current implementation assumes that A.onStop will be called before B.onStart. This is not guaranteed. The inverse assumption is also made (that is to say that if B is popped off the back stack, B.onStop will be called before A.onStart) and this is also not guaranteed. You can imagine the chaos that can ensue in terms of view duration, name, activity count, and foreground/background reporting with incorrect assumptions here.

3. Hooking into ActivityLifecycleCallbacks on the Application object inside of Countly.init() can lead to misunderstanding the true state of the app if the users of the SDK call init() outside of Application.onCreate. If the user halts the SDK and re-inits for whatever reason, the SDK may receive unbalanced / lopsided Activity lifecycle callbacks and therefore misunderstand app state.

4. Manual callbacks in onStart/onStop is generally considered bad practice -- 20 other libraries are fighting for space in those Activity callbacks. There are greener pastures with Lifecycle observation.

Implementation

Instead of a boolean indicating whether or not automatic view tracking is enabled, I introduce the notion of 3 different tracking modes with TrackingMode (immediately deprecating two of them):

public enum TrackingMode {
    /** @deprecated */
    @Deprecated
    MANUAL,
    /** @deprecated */
    @Deprecated
    AUTOMATIC,
    LIFECYCLE
}

In MANUAL and AUTOMATIC modes, view tracking works like it did before (one view at a time). After calling Countly.sharedInstance().views().recordView once, any subsequent calls will "close out" the last view and start recording the new one.

In LIFECYCLE mode, Countly users are expected to instruct Countly to track components with the new public methods Countly.sharedInstance().trackLifecycle(LifecycleOwner) or Countly.sharedInstance().trackLifecycle(LifecycleOwner, Map<String, Object> viewSegmentation). Multiple LifecycleOwners can be tracked simultaneously in this mode. There is no automatic tracking -- users must opt-in in every Activity/Fragment.

Additionally, multiple named views can be tracked in this mode through the existing Countly.sharedInstance().views().recordView methods. Because multiple views can be tracked simultaneously, users must manually "close out" tracked views with the new method Countly.sharedInstance().views().endViewRecording(String... viewNames), which takes a names of the view(s) that the user wants to close out.

Tracking of sessions and foreground/background status has been moved to a ProcessLifecycleObserver, which conveniently calls onStart when the first Activity in an app starts, and onStop when the last Activity stops. We also cannot miss any state, as observers are always notified of state changes that occurred even before they ever started observing. The same applies for Activity and Fragment observation.

To live in harmony with ProGuard/R8, I added a simple PersistentName annotation. Users can annotate their Lifecycle-owning app components with this annotation to ensure that views get unobfuscated names. This is just here to save developers from having to mess with esoteric ProGuard syntax.

@PersistentName(name = "MainActivity")
public class MainActivity extends AppCompatActivity {
    @Override
    public void onCreate(Bundle savedInstanceState) {
        super.onCreate(saveInstanceState);
        Countly.sharedInstance().trackLifecycle(this);
        //...
    }
}

I'm happy to hop on a call with you guys at your convenience to go over the implementation. Please reach out to me via my work email (I believe you should have it).

[ArtursKadikis]

Just having a brief glance at this. Wanted to say that "Let's say ActivityA starts ActivityB. The current implementation assumes that A.onStop will be called before B.onStart." isn't really correct. Currently the SDK does not assume that or at least tries to not assume that.

How would your lifecycle approach get around "If the user halts the SDK and re-inits for whatever reason, the SDK may receive unbalanced / lopsided Activity lifecycle callbacks"?

The "onStart/onStop" callbacks is something that we have planned to move away from for some time and a view rework in general to so that it's usable in fragment situations.

[foooorsyth]

Just having a brief glance at this. Wanted to say that "Let's say ActivityA starts ActivityB. The current implementation assumes that A.onStop will be called before B.onStart." isn't really correct. Currently the SDK does not assume that or at least tries to not assume that.

The main issues comes from controlling of both sessions and foreground/background status in onStart/onStop. Here are the (abridged) current implementations:

    public synchronized void onStart(Activity activity) {
        ++activityCount_;
        if (activityCount_ == 1 && !moduleSessions.manualSessionControlEnabled) {
            //if we open the first activity
            //and we are not using manual session control,
            //begin a session
            moduleSessions.beginSessionInternal();
        }
        CrashDetails.inForeground();
    }

    public synchronized void onStop() {
        if (activityCount_ == 0) {
            throw new IllegalStateException("must call onStart before onStop");
        }
        --activityCount_;
        if (activityCount_ == 0 && !moduleSessions.manualSessionControlEnabled) {
            // if we don't use manual session control
            // Called when final Activity is stopped.
            // Sends an end session event to the server, also sends any unsent custom events.
            moduleSessions.endSessionInternal(null);
        }
        CrashDetails.inBackground();
    }

Let's consider the following sequence:

So what happens in this scenario? The crash details say that the app was in the background at the time of the crash (it wasn't). And the session ends prematurely (the session is inactive at the time of the crash).

Even without the Exception getting thrown, we still get two separate sessions despite the app never closing and reopening.

So let's consider the scenario where B.onStart gets called before A.onStop

This is even more troublesome -- despite B starting normally from A, Countly thinks that the app is in the background because an onStop was called most recently!

In my implementation, I put session tracking and foreground/background state changes in the ProcessLifecycleObserver to avoid these problems. ProcessLifecycleObserver.onStart() will only get called once -- when the first Activity is started. Similarly, ProcessLifecycleObserver.onStop() will also only get called once -- when the last Activity stops. We do not have to worry about the order of multiple onStart/onStop calls from multiple Activities, or keeping track of Activity count, which is the cause of the problems above.

How would your lifecycle approach get around "If the user halts the SDK and re-inits for whatever reason, the SDK may receive unbalanced / lopsided Activity lifecycle callbacks"?

LifecycleOwners maintain a history of their own state. When a new LifecycleObserver starts observing an owner, the owner fires Lifecycle events at it to bring it from INITIALIZED to whatever state the owner is at currently. Basically, every new observer that attaches get "brought up to speed" on the current state of the owner and gets moved through all state between INITIALIZED and the current state as if they had been observing from the very beginning of the owner's life. Therefore, no state is ever missed, even if Countly is initialized late in an application's life.

[iartem]

Overall motivation seems a bit exaggerated. Crash reporting background/foreground thing is of course invalid, but sessions are tracked correctly as of now. Could you @aimkey give a reproducible example when they don't or where activity count approach doesn't work?

Generally, I think pretty much everything related to views tracking should be merged, as it's a huge step up from what we have now. Though I haven't looked into backwards compatibility yet.

At the same time I think everything regarding session tracking should stay on ActivityLifecycleCallbacks as they just don't require adding neither Countly.sharedInstance().onStart() (which I believe should be finally removed in favour of ActivityLifecycleCallbacks), nor Countly.sharedInstance().trackLifecycle() - people forget to add such calls all the time. Unbinding views tracking from lifecycle tracking also seems to be a good idea.

[foooorsyth]

sessions are tracked correctly as of now. Could you @aimkey give a reproducible example when they don't or where activity count approach doesn't work?

I think you're right. I've been mislead by an old test device. The order of A.onPause -> B.onCreate -> B.onStart -> B.onResume -> A.onStop does seem to be guaranteed in the case of A starting B. I'm seeing this consistent behavior on all Pixel/Nexus phones that I have. I only see A.onStop before B.onStart on an old Galaxy S5 running a custom Lollipop ROM, so I'll chalk that up to the ROM. I'm hoping other OEMs don't break this contract. https://developer.android.com/guide/components/activities/activity-lifecycle#soafa

ProcessLifecycleOwner approach may still be worthwhile for maintaining sessions across orientation changes. Question: Is it intended behavior for a session to end and a new session to begin on an orientation change?

At the same time I think everything regarding session tracking should stay on ActivityLifecycleCallbacks as they just don't require adding neither Countly.sharedInstance().onStart() (which I believe should be finally removed in favour of ActivityLifecycleCallbacks), nor Countly.sharedInstance().trackLifecycle() - people forget to add such calls all the time.

With session tracking and foreground/background tracking in ProcessLifecycleObserver.onStart/onStop, both can be tracked without any required calls to Countly.sharedInstance.onStart or .trackLifecycle. Currently this is only the case in TrackingMode.LIFECYCLE as I didn't want to break any legacy behavior. Anyways, I think it's possible to do away with the TrackingModes entirely. See below...

Unbinding views tracking from lifecycle tracking also seems to be a good idea.

I think this is possible for both Activities and Fragments with ActivityLifecycleCallbacks combined with FragmentLifecycleCallbacks. Here's my plan:

1. Keep session/foreground tracking in the ProcessLifecycleObserver (unless a new session is desired on configuration changes, in which case I'll put it all in ActivityLifecycleCallbacks and drop the ProcessLifecycleObserver)
2. Delete TrackingMode. Go back to a boolean for automatic view tracking on/off (keeps the public API there the same)
3. Implement automatic tracking of all Activities and Fragments with ActivityLifecycleCallbacks and FragmentLifecycleCallbacks. Soft deprecate Countly.sharedInstance.onStart/onStop for backwards compatibility

Sound good? I'll re-add the WIP prefix and push another commit soon.

[iartem]

ProcessLifecycleOwner approach may still be worthwhile for maintaining sessions across orientation changes. Question: Is it intended behavior for a session to end and a new session to begin on an orientation change?

It's not exactly intended, it's rather a known limitation we live with. Session tracking won't ever be 100%-correct because of, for example, small dives into background because user switched to another app for a second. And we don't want to postpone any requests as it'd decrease probability they actually arrive to the server. It's true for all platforms, therefore it's mitigated on the server.

Sound good?

To me it is, but @ArtursKadikis may have something else in mind. Anyway, thanks for bringing that topic up and all your work! 👍

[foooorsyth]

Okay, I think I'm done reworking it. Let me know what you guys think.

TODO

• [x] Add implementation to :sdk
• [x] Javadoc new public APIs
• [x] Add tests for new APIs
• [x] Update sample app(s)
• [ ] Versioning? (left to maintainers/owners)

Overview

I added automatic tracking of all Activity callbacks (create, start, stop, configurationChange) along with automatic tracking of all Fragments as views when enableViewTracking is true. To facilitate this, I added the ability to track multiple views at the same time.

New Public APIs

On ModuleViews.Views

public synchronized <V> Countly recordView(@NonNull V view);
public synchronized <V> Countly recordView(@NonNull V view, @Nullable Map<String, Object> viewSegmentation);
public synchronized <V> Countly recordView(@NonNull V view, @Nullable Map<String, Object> viewSegmentation, @Nullable String persistentName);
public synchronized Countly endViewRecording(@NonNull String viewName);
public synchronized <V> Countly endViewRecording(@NonNull V view);
public synchronized <V> Countly addViewSegmentation(@NonNull V view, @NonNull Map<String, Object> viewSegmentation);
public synchronized <V> Countly addViewPersistentName(@NonNull V view, @NonNull String persistentName);

On CountlyConfig

// requires init with application reference
public CountlyConfig(Application application, String appKey, String serverURL);

The @DoNotTrack annotation can be used on Activities, Fragments, or any object tracked by identity to selectively opt-out of automatic view tracking even with enableViewTracking enabled. This is used in harmony with the existing Activity exception list.

The @PersistentName annotation can be used to give a class a name that persists even with ProGuard/R8. Of course this can be done with proguard-rules.pro, so this is merely a convenience annotation (ProGuard syntax is hell).

New behavior

Since multiple views can be tracked at any time, successive calls to Countly.sharedInstance().views().recordView does NOT "close out" the most recently recorded view. Instead, to report a view duration, users must call one of the variants of Countly.sharedInstance().views().endViewRecording with either a reference to their view object or the name of the view that they wish to close out. Of course, this is done automatically for Activities and Fragments if enableViewTracking is true.

Facilitating the automatic tracking of all Activity lifecycles is a reference to an Application. This is now mandatory -- you can't init without it.

Deprecations

All public manual Activity tracking functions are now no-ops. This is all done internally now.

@Deprecated public static void onCreate(android.app.Activity activity) { }
@Deprecated public synchronized void onStart(android.app.Activity activity) { }
@Deprecated public synchronized void onStop() { }
@Deprecated public synchronized void onConfigurationChanged(Configuration newConfig) { }

Since we need an Application reference to do this automatic tracking, CountlyConfig constructors that don't require an Application are also deprecated in favor of the new one (above).

@Deprecated public CountlyConfig();
@Deprecated public CountlyConfig(Context context, String appKey, String serverURL);

Test deletions

UtilsTest#testAPI was deleted because it does not appear to be deterministic (depends on the version of the OS in the Instrumentation environment)

DeviceInfoTest#testGetMetricsWithOverride_2 was deleted because it also does not appear to be deterministic. It compares JSON string to JSON string, but one of the strings is constructed by iterating over keys in an unordered map. Therefore, the string that it creates can vary iteration to iteration, causing the test to intermittently fail.

CountlyTests#testInit_twiceWithDifferentContext was deleted because this behavior is no longer supported. You need to init with an Application reference, so the Context reference will always be the same (can't init successively with different Contexts).

[ArtursKadikis]

Hello, this PR has good ideas and improvements over the current way of handling views. One thing I don't like is that this is currently not a drop-in upgrade and requires code modification when upgrading from the previous version. At the very least I would want at least 1 transitional version where both approaches would be valid and the old calls would be deprecated. This way the new view handling would be opt-in. Even though the "DeviceInfoTest#testGetMetricsWithOverride_2" test is not fully deterministic, it still has it's value. Though I agree that it would benefit from a better implementation. This PR does make a lot of changes, though most of it is related to the sample app, tests, and some unrelated minor things. It's hard for me to evaluate if it does not introduce some edge case issues or conflicts. I want to get a more intimate understanding of this whole workflow and do a more fine-grained checking if there are no hidden issues introduced. Therefore I will use this PR as a template and merge the required changes manually, while also making this opt-in. Thank you for your contribution.

[ArtursKadikis]

Simultaneous view tracking did not make it into the current release. I'll try to add it in one of the upcomming point releases