Implement SessionInitiator and hook it up to lifecycle events #4723

mrober · 2023-02-28T15:43:39Z

Implement SessionInitiator and hook it up to lifecycle events.

This is responsible for calling the initiateSessionStart callback whenever a session starts.

Used Longs for time, not go/goodtime, to keep the SDK light on older android api levels. Put the unit on every time variable.

Added unit tests. Used fakes, not mocks or stubs frameworks, similar to the iOS SDK.

google-oss-bot · 2023-02-28T15:44:13Z

	1 Warning
⚠️	Did you forget to add a changelog entry? (Add the 'no-changelog' label to the PR to silence this warning.)

Generated by 🚫 Danger

google-oss-bot · 2023-02-28T15:48:13Z

Coverage Report ¹

Affected Products

firebase-sessions
Overall coverage changed from ? (402c686) to 0.00% (76c2eab) by ?.
Filename Base (402c686) Merge (76c2eab) Diff
FirebaseSessions.kt ? 0.00% ?
FirebaseSessionsRegistrar.kt ? 0.00% ?
SessionInitiator.kt ? 0.00% ?

Test Logs

Merge (76c2eab): https://github.com/firebase/firebase-android-sdk/actions/runs/4298182811

https://storage.googleapis.com/firebase-sdk-metric-reports/QKu7xEbWGN.html

github-actions · 2023-02-28T15:50:23Z

Unit Test Results

0 tests 0 ✔️ 0s ⏱️
0 suites 0 💤
0 files 0 ❌

Results for commit c65a3ba.

♻️ This comment has been updated with latest results.

google-oss-bot · 2023-02-28T16:00:08Z

Size Report ¹

Affected Products

base
Type Base (402c686) Merge (76c2eab) Diff
apk (aggressive) ? 8.39 kB ? (?)
apk (release) ? 8.65 kB ? (?)
firebase-annotations
Type Base (402c686) Merge (76c2eab) Diff
apk (aggressive) ? 8.39 kB ? (?)
apk (release) ? 9.46 kB ? (?)
firebase-common
Type Base (402c686) Merge (76c2eab) Diff
aar ? 67.6 kB ? (?)
apk (aggressive) ? 111 kB ? (?)
apk (release) ? 1.26 MB ? (?)
firebase-common-ktx
Type Base (402c686) Merge (76c2eab) Diff
aar ? 13.2 kB ? (?)
apk (aggressive) ? 123 kB ? (?)
apk (release) ? 1.64 MB ? (?)
firebase-components
Type Base (402c686) Merge (76c2eab) Diff
aar ? 44.9 kB ? (?)
apk (aggressive) ? 23.1 kB ? (?)
apk (release) ? 596 kB ? (?)
firebase-sessions
Type Base (402c686) Merge (76c2eab) Diff
aar ? 12.5 kB ? (?)
apk (aggressive) ? 130 kB ? (?)
apk (release) ? 1.64 MB ? (?)

Test Logs

Merge (76c2eab): https://github.com/firebase/firebase-android-sdk/actions/runs/4298182811

https://storage.googleapis.com/firebase-sdk-metric-reports/6KqykGoB0x.html

firebase-sessions/src/main/kotlin/com/google/firebase/sessions/FirebaseSessions.kt

samedson · 2023-02-28T18:52:30Z

firebase-sessions/src/main/kotlin/com/google/firebase/sessions/FirebaseSessions.kt

+    val sessionInitiator = SessionInitiator(System::currentTimeMillis, this::initiateSessionStart)
+    val context = firebaseApp.applicationContext.applicationContext
+    if (context is Application) {
+      context.registerActivityLifecycleCallbacks(sessionInitiator.activityLifecycleCallbacks)


Suggestion: Make this part of the constructor for SessionInitiator instead of exposing the activityLifecycleCallbacks property

I did it this way so the SessionInitiator outer class wouldn't have any Context or Activity in the ctor to make the tests simpler.

samedson · 2023-02-28T18:53:27Z

firebase-sessions/src/test/kotlin/com/google/firebase/sessions/SessionInitiatorTest.kt

+    private const val SMALL_INTERVAL_MS = 3 * 1000L // not enough time to initiate a new session
+    private const val LARGE_INTERVAL_MS = 90 * 60 * 1000L // enough to initiate another session
+  }
+}


These tests are great 👍

samedson

Couple suggestions, but otherwise looks good!

visumickey

Thanks for making these changes. Since these are merged now - we can make these changes (if any) in a separate PR.

visumickey · 2023-02-28T23:40:47Z

firebase-sessions/src/main/kotlin/com/google/firebase/sessions/FirebaseSessions.kt

+    val sessionInitiator = SessionInitiator(System::currentTimeMillis, this::initiateSessionStart)
+    val context = firebaseApp.applicationContext.applicationContext
+    if (context is Application) {
+      context.registerActivityLifecycleCallbacks(sessionInitiator.activityLifecycleCallbacks)


Should we check if the context is already registered for lifecycle callbacks? This is to ensure that we are not registering multiple times.

Sam suggested moving this to the SessionInitiator ctor. If we do that, then that instance can only get registered once. Would that be enough to ensure this?

It depends on if the SessionInitator constructor will be called just once or more than once. Also, would the loss of a SessionInitiator object cause a deRegister to be called automatically? If so, this refactor should be fine.

firebase-sessions/src/main/kotlin/com/google/firebase/sessions/FirebaseSessions.kt

visumickey · 2023-03-01T00:41:07Z

firebase-sessions/src/main/kotlin/com/google/firebase/sessions/FirebaseSessionsRegistrar.kt

-        .factory { FirebaseSessions() }
+        .add(Dependency.required(FirebaseApp::class.java))
+        .factory { container -> FirebaseSessions(container.get(FirebaseApp::class.java)) }
+        .eagerInDefaultApp()


Why eager initialization? Should we go with lazy? Thoughts @davidmotson

I set eager because I want the first session to start at app launch. Would lazy still be good enough for that?

I think lazy should be fine. With eager my concern is that we might impact the startup time of the application. Especially since we would be dependent on libraries like Installations and Firelog.

The only caveat here would be the start time of a session. I think the start time of a session and the application start time are different from each other. So, I think lazy should be fine.

If I make it lazy, it never initiates on my test app if I never access the SDK. Are we sure that's what we want? When Crashlytics and Fireperf depend on it, will they always access it on every launch?

firebase-sessions/src/main/kotlin/com/google/firebase/sessions/SessionInitiator.kt

visumickey · 2023-03-01T01:01:45Z

firebase-sessions/src/main/kotlin/com/google/firebase/sessions/SessionInitiator.kt

+
+      override fun onActivityPaused(activity: Activity) = appBackgrounded()
+
+      override fun onActivityCreated(activity: Activity, savedInstanceState: Bundle?) = Unit


Curious if this will be the place to kick start a session event?

Do you mean onActivityCreated? We call initiateSessionStart() when the class is constructed for the cold boot case.

Yes, that was my question. I get that part now. Will the code to start a new session be called in a later PR?

The init { ... } block gets called as part of the ctor, and that calls initiateSessionStart() already. So we start a new session when this object is constructed, representing the cold boot. And we also call it on foreground after the session timeout elapsed.

firebase-sessions/src/test/kotlin/com/google/firebase/sessions/SessionInitiatorTest.kt

Implement SessionInitiator and hook it up to lifecycle events

7c02fe8

mrober requested review from samedson and davidmotson February 28, 2023 15:43

google-oss-bot added the size/L label Feb 28, 2023

mrober requested a review from visumickey February 28, 2023 15:48

samedson reviewed Feb 28, 2023

View reviewed changes

firebase-sessions/src/main/kotlin/com/google/firebase/sessions/FirebaseSessions.kt Show resolved Hide resolved

samedson reviewed Feb 28, 2023

View reviewed changes

samedson approved these changes Feb 28, 2023

View reviewed changes

Add a warning when failed to register lifecycle callbacks

c65a3ba

davidmotson approved these changes Feb 28, 2023

View reviewed changes

Fix formatting

59bf230

mrober merged commit e12458f into firebase-sessions Feb 28, 2023

mrober deleted the sessions-callback branch February 28, 2023 23:04

visumickey reviewed Mar 1, 2023

View reviewed changes

samedson added the api: sessions label Mar 10, 2023

firebase locked and limited conversation to collaborators Mar 31, 2023


		override fun onActivityPaused(activity: Activity) = appBackgrounded()

		override fun onActivityCreated(activity: Activity, savedInstanceState: Bundle?) = Unit

Implement SessionInitiator and hook it up to lifecycle events #4723

Implement SessionInitiator and hook it up to lifecycle events #4723

Uh oh!

Conversation

mrober commented Feb 28, 2023

Uh oh!

google-oss-bot commented Feb 28, 2023

Uh oh!

google-oss-bot commented Feb 28, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Coverage Report 1

Affected Products

firebase-sessions

Test Logs

Uh oh!

github-actions bot commented Feb 28, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Unit Test Results

Uh oh!

google-oss-bot commented Feb 28, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Size Report 1

Affected Products

base

firebase-annotations

firebase-common

firebase-common-ktx

firebase-components

firebase-sessions

Test Logs

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

samedson left a comment

Choose a reason for hiding this comment

Uh oh!

visumickey left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

google-oss-bot commented Feb 28, 2023 •

edited

Loading

Coverage Report ¹

`firebase-sessions`

github-actions bot commented Feb 28, 2023 •

edited

Loading

google-oss-bot commented Feb 28, 2023 •

edited

Loading

Size Report ¹

`base`

`firebase-annotations`

`firebase-common`

`firebase-common-ktx`

`firebase-components`

`firebase-sessions`