-
Notifications
You must be signed in to change notification settings - Fork 625
Implement SessionInitiator and hook it up to lifecycle events #4723
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -14,16 +14,35 @@ | |
|
||
package com.google.firebase.sessions | ||
|
||
import android.app.Application | ||
import android.util.Log | ||
import androidx.annotation.Discouraged | ||
import com.google.firebase.FirebaseApp | ||
import com.google.firebase.ktx.Firebase | ||
import com.google.firebase.ktx.app | ||
|
||
class FirebaseSessions internal constructor() { | ||
class FirebaseSessions internal constructor(firebaseApp: FirebaseApp) { | ||
init { | ||
val sessionInitiator = SessionInitiator(System::currentTimeMillis, this::initiateSessionStart) | ||
val context = firebaseApp.applicationContext.applicationContext | ||
if (context is Application) { | ||
context.registerActivityLifecycleCallbacks(sessionInitiator.activityLifecycleCallbacks) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should we check if the context is already registered for lifecycle callbacks? This is to ensure that we are not registering multiple times. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||
} else { | ||
Log.w(TAG, "Failed to register lifecycle callbacks, unexpected context ${context.javaClass}.") | ||
} | ||
mrober marked this conversation as resolved.
Show resolved
Hide resolved
|
||
} | ||
|
||
@Discouraged(message = "This will be replaced with a real API.") | ||
fun greeting(): String = "Matt says hi!" | ||
visumickey marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
private fun initiateSessionStart() { | ||
// TODO(mrober): Generate a session | ||
Log.i(TAG, "Initiate session start") | ||
} | ||
|
||
companion object { | ||
private const val TAG = "FirebaseSessions" | ||
|
||
@JvmStatic | ||
val instance: FirebaseSessions | ||
get() = getInstance(Firebase.app) | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -15,8 +15,10 @@ | |
package com.google.firebase.sessions | ||
|
||
import androidx.annotation.Keep | ||
import com.google.firebase.FirebaseApp | ||
import com.google.firebase.components.Component | ||
import com.google.firebase.components.ComponentRegistrar | ||
import com.google.firebase.components.Dependency | ||
import com.google.firebase.platforminfo.LibraryVersionComponent | ||
|
||
/** | ||
|
@@ -30,12 +32,14 @@ internal class FirebaseSessionsRegistrar : ComponentRegistrar { | |
listOf( | ||
Component.builder(FirebaseSessions::class.java) | ||
.name(LIBRARY_NAME) | ||
.factory { FirebaseSessions() } | ||
.add(Dependency.required(FirebaseApp::class.java)) | ||
.factory { container -> FirebaseSessions(container.get(FirebaseApp::class.java)) } | ||
.eagerInDefaultApp() | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why eager initialization? Should we go with lazy? Thoughts @davidmotson There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I set eager because I want the first session to start at app launch. Would lazy still be good enough for that? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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? |
||
.build(), | ||
LibraryVersionComponent.create(LIBRARY_NAME, BuildConfig.VERSION_NAME) | ||
) | ||
|
||
companion object { | ||
private const val LIBRARY_NAME = "fire-ses" | ||
private const val LIBRARY_NAME = "fire-sessions" | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,68 @@ | ||
/* | ||
* Copyright 2023 Google LLC | ||
* | ||
* Licensed under the Apache License, Version 2.0 (the "License"); | ||
* you may not use this file except in compliance with the License. | ||
* You may obtain a copy of the License at | ||
* | ||
* http://www.apache.org/licenses/LICENSE-2.0 | ||
* | ||
* Unless required by applicable law or agreed to in writing, software | ||
* distributed under the License is distributed on an "AS IS" BASIS, | ||
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
* See the License for the specific language governing permissions and | ||
* limitations under the License. | ||
*/ | ||
|
||
package com.google.firebase.sessions | ||
|
||
import android.app.Activity | ||
import android.app.Application.ActivityLifecycleCallbacks | ||
import android.os.Bundle | ||
|
||
/** | ||
* The [SessionInitiator] is responsible for calling the [initiateSessionStart] callback whenever a | ||
* session starts. This will happen at a cold start of the app, and when the app has been in the | ||
* background for a period of time (default 30 min) and then comes back to the foreground. | ||
* | ||
* @hide | ||
*/ | ||
internal class SessionInitiator( | ||
private val currentTimeMs: () -> Long, | ||
private val initiateSessionStart: () -> Unit | ||
visumickey marked this conversation as resolved.
Show resolved
Hide resolved
|
||
) { | ||
private var backgroundTimeMs = currentTimeMs() | ||
private val sessionTimeoutMs = 30 * 60 * 1000L // TODO(mrober): Get session timeout from settings | ||
visumickey marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
init { | ||
initiateSessionStart() | ||
} | ||
|
||
fun appBackgrounded() { | ||
backgroundTimeMs = currentTimeMs() | ||
} | ||
|
||
fun appForegrounded() { | ||
val intervalMs = currentTimeMs() - backgroundTimeMs | ||
visumickey marked this conversation as resolved.
Show resolved
Hide resolved
|
||
if (intervalMs > sessionTimeoutMs) { | ||
initiateSessionStart() | ||
} | ||
} | ||
|
||
internal val activityLifecycleCallbacks = | ||
object : ActivityLifecycleCallbacks { | ||
override fun onActivityResumed(activity: Activity) = appForegrounded() | ||
|
||
override fun onActivityPaused(activity: Activity) = appBackgrounded() | ||
|
||
override fun onActivityCreated(activity: Activity, savedInstanceState: Bundle?) = Unit | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Curious if this will be the place to kick start a session event? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do you mean onActivityCreated? We call initiateSessionStart() when the class is constructed for the cold boot case. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, that was my question. I get that part now. Will the code to start a new session be called in a later PR? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The |
||
|
||
override fun onActivityStarted(activity: Activity) = Unit | ||
|
||
override fun onActivityStopped(activity: Activity) = Unit | ||
|
||
override fun onActivityDestroyed(activity: Activity) = Unit | ||
|
||
override fun onActivitySaveInstanceState(activity: Activity, outState: Bundle) = Unit | ||
} | ||
} |
This file was deleted.
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,140 @@ | ||
/* | ||
* Copyright 2023 Google LLC | ||
* | ||
* Licensed under the Apache License, Version 2.0 (the "License"); | ||
* you may not use this file except in compliance with the License. | ||
* You may obtain a copy of the License at | ||
* | ||
* http://www.apache.org/licenses/LICENSE-2.0 | ||
* | ||
* Unless required by applicable law or agreed to in writing, software | ||
* distributed under the License is distributed on an "AS IS" BASIS, | ||
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
* See the License for the specific language governing permissions and | ||
* limitations under the License. | ||
*/ | ||
|
||
package com.google.firebase.sessions | ||
|
||
import com.google.common.truth.Truth.assertThat | ||
import org.junit.Test | ||
|
||
class SessionInitiatorTest { | ||
class FakeTime { | ||
var currentTimeMs = 0L | ||
private set | ||
|
||
fun addTimeMs(intervalMs: Long) { | ||
currentTimeMs += intervalMs | ||
} | ||
} | ||
visumickey marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
class SessionStartCounter { | ||
var count = 0 | ||
private set | ||
visumickey marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
fun initiateSessionStart() { | ||
count++ | ||
} | ||
} | ||
|
||
@Test | ||
fun coldStart_initiatesSession() { | ||
val sessionStartCounter = SessionStartCounter() | ||
|
||
// Simulate a cold start by simply constructing the SessionInitiator object | ||
SessionInitiator({ 0 }, sessionStartCounter::initiateSessionStart) | ||
|
||
assertThat(sessionStartCounter.count).isEqualTo(1) | ||
} | ||
|
||
@Test | ||
fun appForegrounded_largeInterval_initiatesSession() { | ||
val fakeTime = FakeTime() | ||
val sessionStartCounter = SessionStartCounter() | ||
|
||
val sessionInitiator = | ||
SessionInitiator(fakeTime::currentTimeMs, sessionStartCounter::initiateSessionStart) | ||
|
||
// First session on cold start | ||
assertThat(sessionStartCounter.count).isEqualTo(1) | ||
|
||
// Enough tome to initiate a new session, and then foreground | ||
fakeTime.addTimeMs(LARGE_INTERVAL_MS) | ||
sessionInitiator.appForegrounded() | ||
|
||
// Another session initiated | ||
assertThat(sessionStartCounter.count).isEqualTo(2) | ||
} | ||
|
||
@Test | ||
fun appForegrounded_smallInterval_doesNotInitiatesSession() { | ||
val fakeTime = FakeTime() | ||
val sessionStartCounter = SessionStartCounter() | ||
|
||
val sessionInitiator = | ||
SessionInitiator(fakeTime::currentTimeMs, sessionStartCounter::initiateSessionStart) | ||
|
||
// First session on cold start | ||
assertThat(sessionStartCounter.count).isEqualTo(1) | ||
|
||
// Not enough time to initiate a new session, and then foreground | ||
fakeTime.addTimeMs(SMALL_INTERVAL_MS) | ||
sessionInitiator.appForegrounded() | ||
|
||
// No new session | ||
assertThat(sessionStartCounter.count).isEqualTo(1) | ||
} | ||
|
||
@Test | ||
fun appForegrounded_background_foreground_largeIntervals_initiatesSessions() { | ||
val fakeTime = FakeTime() | ||
val sessionStartCounter = SessionStartCounter() | ||
|
||
val sessionInitiator = | ||
SessionInitiator(fakeTime::currentTimeMs, sessionStartCounter::initiateSessionStart) | ||
|
||
assertThat(sessionStartCounter.count).isEqualTo(1) | ||
|
||
fakeTime.addTimeMs(LARGE_INTERVAL_MS) | ||
sessionInitiator.appForegrounded() | ||
|
||
assertThat(sessionStartCounter.count).isEqualTo(2) | ||
|
||
sessionInitiator.appBackgrounded() | ||
fakeTime.addTimeMs(LARGE_INTERVAL_MS) | ||
sessionInitiator.appForegrounded() | ||
|
||
assertThat(sessionStartCounter.count).isEqualTo(3) | ||
} | ||
|
||
@Test | ||
fun appForegrounded_background_foreground_smallIntervals_doesNotInitiateNewSessions() { | ||
val fakeTime = FakeTime() | ||
val sessionStartCounter = SessionStartCounter() | ||
|
||
val sessionInitiator = | ||
SessionInitiator(fakeTime::currentTimeMs, sessionStartCounter::initiateSessionStart) | ||
|
||
// First session on cold start | ||
assertThat(sessionStartCounter.count).isEqualTo(1) | ||
|
||
fakeTime.addTimeMs(SMALL_INTERVAL_MS) | ||
sessionInitiator.appForegrounded() | ||
|
||
assertThat(sessionStartCounter.count).isEqualTo(1) | ||
|
||
sessionInitiator.appBackgrounded() | ||
fakeTime.addTimeMs(SMALL_INTERVAL_MS) | ||
sessionInitiator.appForegrounded() | ||
|
||
assertThat(sessionStartCounter.count).isEqualTo(1) | ||
|
||
assertThat(sessionStartCounter.count).isEqualTo(1) | ||
} | ||
|
||
companion object { | ||
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 | ||
visumickey marked this conversation as resolved.
Show resolved
Hide resolved
|
||
} | ||
} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. These tests are great 👍 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Suggestion: Make this part of the constructor for SessionInitiator instead of exposing the
activityLifecycleCallbacks
propertyThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.