Add Application Open and Application Backgrounded Events #620

carloskelly13 · 2019-07-16T19:36:20Z

Addresses LIB-1156 and LIB-1143.

…round

codecov-io · 2019-07-16T19:40:28Z

Codecov Report

Merging #620 into master will decrease coverage by 0.13%.
The diff coverage is 87.83%.

@@            Coverage Diff             @@
##           master     #620      +/-   ##
==========================================
- Coverage   83.27%   83.14%   -0.14%     
==========================================
  Files          30       31       +1     
  Lines        2463     2509      +46     
  Branches      283      291       +8     
==========================================
+ Hits         2051     2086      +35     
- Misses        329      335       +6     
- Partials       83       88       +5

Impacted Files	Coverage Δ
...src/main/java/com/segment/analytics/Analytics.java	`75.53% <100%> (-0.03%)`	⬇️
...analytics/AnalyticsActivityLifecycleCallbacks.java	`86.56% <86.56%> (ø)`
.../src/main/java/com/segment/analytics/ValueMap.java	`86.39% <0%> (-2.73%)`	⬇️
...ain/java/com/segment/analytics/internal/Utils.java	`74.13% <0%> (-1.15%)`	⬇️
...cs/src/main/java/com/segment/analytics/Traits.java	`92.3% <0%> (-1.1%)`	⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 93c7d5b...064c69a. Read the comment docs.

f2prateek · 2019-07-17T21:52:33Z

analytics/src/main/java/com/segment/analytics/Analytics.java

+            if (shutdown) {
+              return;
+            }
+            track(


Same as segmentio/analytics-ios#831 (comment) - I don't believe any properties should be collected here, as per https://segment.com/docs/spec/mobile/#application-backgrounded.

f2prateek

I think there's a bug in the behaviour here, particularly for applications with multiple activities.

Currently, I believe that "Application Opened" will be fired any time a new activity in the existing app is opened (regardless of whethere the user already had the app open or not).

Similarly, if the user exits an activity (e.g. by hitting the back button in a multi activity application), it would fire a "Application Backgrounded" event, even though they might still remain in the same app.

carloskelly13 · 2019-07-17T22:18:08Z

That is a good point. Since we don't have access to android.arch.lifecycle I could use the atomic boolean like we did for onActivityCreated to ensure it only fires for the first activity. Otherwise the user would have to manually add the tracking.

f2prateek · 2019-07-17T22:26:49Z

👍 I think there are tradeoffs of both approaches:

android.arch.lifecycle approach: likely more reliable as they've handled edge cases for us, but requires adding a new dependency.
boolean approach: likely less reliable as we might not account properly for all edge cases, but doesn't require a dependency.

I think the boolean approach is best for now as it mimics what we already do, but we should consider the android.arch.lifecycle approach in the future to make this more bullet proof.

carloskelly13 · 2019-07-17T22:31:15Z

Sounds good. I will update that!

carloskelly13 · 2019-07-17T22:32:18Z

It looks like it is part of Jetpack AndroidX so there are other dependencies to add when using android.arch.lifecycle. https://developer.android.com/jetpack/androidx/releases/lifecycle

According to the docs it either has a Java 8 dependency or a Kotlin one and the annotation processor differs on each.

f2prateek · 2019-07-18T00:40:40Z

Oof that sucks - yeah I think the boolean approach is definitely the right one for now!

carloskelly13 · 2019-07-18T22:21:48Z

analytics/src/main/java/com/segment/analytics/Analytics.java

    activityLifecycleCallback =
        new Application.ActivityLifecycleCallbacks() {
          final AtomicBoolean trackedApplicationLifecycleEvents = new AtomicBoolean(false);
+          final AtomicInteger numberOfActivities = new AtomicInteger(1);
+          final AtomicBoolean isChangingActivityConfigurations = new AtomicBoolean(false);
+          final AtomicBoolean firstLaunch = new AtomicBoolean(false);


Android has new application lifecycle callbacks via androidx.lifecycle, but we can't use them due to the dependency requirements AndroidX places. Additionally that breaks the React Native wrapper as AndroidX deps won't work with new RN projects out of the box. We use the traditional method of activity reference counting to detect if we are launching a new activity or resuming to foreground.

carloskelly13 · 2019-07-18T22:29:12Z

analytics/src/main/java/com/segment/analytics/Analytics.java

            if (!trackedApplicationLifecycleEvents.getAndSet(true)
                && shouldTrackApplicationLifecycleEvents) {
+              numberOfActivities.set(0);
+              firstLaunch.set(true);


These reset the atomic variables for native Android apps. onActivityCreated is not called when this library is used by the React Native wrapper.

f2prateek · 2019-07-18T22:29:30Z

analytics/src/test/java/com/segment/analytics/AnalyticsTest.java

@@ -896,19 +883,6 @@ protected boolean matchesSafely(TrackPayload payload) {
                        payload.properties().getInt("build", -1) == 101;
                  }
                }));
-    verify(integration)


why was this removed?

f2prateek · 2019-07-18T22:29:36Z

analytics/src/test/java/com/segment/analytics/AnalyticsTest.java

@@ -796,19 +796,6 @@ protected boolean matchesSafely(TrackPayload payload) {
                        payload.properties().getInt("build", -1) == 100;
                  }
                }));
-    verify(integration)


why was this removed?

Both were removed as ApplicationOpened is no longer called during trackApplicationLifecycleEventsInstalled. It is now called in onActivityResumed and depending if it's a cold start or not, the correct Properties are passed.

Got it - shouldn't this test be moved somewhere else then?

I see it is moved, never mind!

carloskelly13 · 2019-07-18T22:31:09Z

analytics/src/main/java/com/segment/analytics/Analytics.java

+              if (firstLaunch.get()) {
+                properties.putValue("version", currentVersion).putValue("build", currentBuild);
+              }
+              properties.putValue("from_background", !firstLaunch.getAndSet(false));


On first launch we want to ensure from_background is false, subsequent calls, this will be true.

carloskelly13 · 2019-07-18T22:33:23Z

analytics/src/test/java/com/segment/analytics/AnalyticsTest.java

+                    return payload.event().equals("Application Opened")
+                        && payload.properties().getString("version").equals("1.0.0")
+                        && payload.properties().getInt("build", -1) == 100
+                        && !payload.properties().getBoolean("from_background", true);


This is the first integration test that ensures that on first start from_background is false, the correct build params are part of the cold start Application Opened tracking event.

carloskelly13 · 2019-07-18T22:33:49Z

analytics/src/test/java/com/segment/analytics/AnalyticsTest.java

+                new NoDescriptionMatcher<TrackPayload>() {
+                  @Override
+                  protected boolean matchesSafely(TrackPayload payload) {
+                    return payload.event().equals("Application Backgrounded");


This is the second integration test that launches the app then sends it to background.

carloskelly13 · 2019-07-18T22:34:25Z

analytics/src/test/java/com/segment/analytics/AnalyticsTest.java

+                  @Override
+                  protected boolean matchesSafely(TrackPayload payload) {
+                    return payload.event().equals("Application Opened")
+                        && payload.properties().getBoolean("from_background", false);


This is the third integration test that verifies when we send the app to the background, then resume it to foreground the from_background param is true.

f2prateek · 2019-07-19T22:37:38Z

analytics/src/main/java/com/segment/analytics/AnalyticsActivityLifecycleCallbacks.java

+import java.util.concurrent.atomic.AtomicBoolean;
+import java.util.concurrent.atomic.AtomicInteger;
+
+public class AnalyticsActivityLifecycleCallbacks implements Application.ActivityLifecycleCallbacks {


This should be a private class.

(or at least package protected)

f2prateek

LGTM! I think let's make the internal class package protected, and add a bit more internal documentation for how the lifecycle tracking works - and this should be good to merge!

This reverts commit 1cfb025.

fathyb · 2019-07-23T00:13:29Z

I merged by accident, sorry! Made a revert PR under #621

carloskelly13 added 4 commits July 11, 2019 13:57

LIB-1156: Added support for tracking event when app opened from backg…

704ce64

…round

Ran formatter.

bfa60cb

Added Backgrounded events

cbd6df2

Run formatter.

2a07484

f2prateek reviewed Jul 17, 2019

View reviewed changes

f2prateek suggested changes Jul 17, 2019

View reviewed changes

Updated lifecycle methods and added integration tests.

cd95037

carloskelly13 commented Jul 18, 2019

View reviewed changes

f2prateek reviewed Jul 18, 2019

View reviewed changes

carloskelly13 commented Jul 18, 2019

View reviewed changes

carloskelly13 added 2 commits July 19, 2019 16:22

Separate out Lifecycle Callbacks into separate class

6bb753b

Run formatter.

5df65ff

f2prateek reviewed Jul 19, 2019

View reviewed changes

Made analytics lifecycle class package protected.

064c69a

f2prateek approved these changes Jul 22, 2019

View reviewed changes

f2prateek requested review from bsneed and fathyb July 22, 2019 18:48

f2prateek approved these changes Jul 22, 2019

View reviewed changes

fathyb approved these changes Jul 23, 2019

View reviewed changes

fathyb merged commit 1cfb025 into segmentio:master Jul 23, 2019

fathyb added a commit that referenced this pull request Jul 23, 2019

Revert "Add Application Open and Application Backgrounded Events (#620)"

df256e7

This reverts commit 1cfb025.

fathyb mentioned this pull request Jul 23, 2019

Revert "Add Application Open and Application Backgrounded Events" #621

Closed

bsneed mentioned this pull request May 15, 2020

Update docs to reflect recent Android changes. segmentio/segment-docs#873

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Application Open and Application Backgrounded Events #620

Add Application Open and Application Backgrounded Events #620

carloskelly13 commented Jul 16, 2019 •

edited

Loading

codecov-io commented Jul 16, 2019 •

edited

Loading

f2prateek Jul 17, 2019

f2prateek left a comment

carloskelly13 commented Jul 17, 2019

f2prateek commented Jul 17, 2019

carloskelly13 commented Jul 17, 2019

carloskelly13 commented Jul 17, 2019 •

edited

Loading

f2prateek commented Jul 18, 2019

carloskelly13 Jul 18, 2019

carloskelly13 Jul 18, 2019

f2prateek Jul 18, 2019

f2prateek Jul 18, 2019

carloskelly13 Jul 18, 2019

f2prateek Jul 19, 2019

f2prateek Jul 19, 2019

carloskelly13 Jul 18, 2019

carloskelly13 Jul 18, 2019 •

edited

Loading

carloskelly13 Jul 18, 2019

carloskelly13 Jul 18, 2019

f2prateek Jul 19, 2019

f2prateek Jul 19, 2019

f2prateek left a comment

fathyb commented Jul 23, 2019

Add Application Open and Application Backgrounded Events #620

Add Application Open and Application Backgrounded Events #620

Conversation

carloskelly13 commented Jul 16, 2019 • edited Loading

codecov-io commented Jul 16, 2019 • edited Loading

Codecov Report

Choose a reason for hiding this comment

f2prateek left a comment

Choose a reason for hiding this comment

carloskelly13 commented Jul 17, 2019

f2prateek commented Jul 17, 2019

carloskelly13 commented Jul 17, 2019

carloskelly13 commented Jul 17, 2019 • edited Loading

f2prateek commented Jul 18, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

carloskelly13 Jul 18, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

f2prateek left a comment

Choose a reason for hiding this comment

fathyb commented Jul 23, 2019

carloskelly13 commented Jul 16, 2019 •

edited

Loading

codecov-io commented Jul 16, 2019 •

edited

Loading

carloskelly13 commented Jul 17, 2019 •

edited

Loading

carloskelly13 Jul 18, 2019 •

edited

Loading