Skip to content

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

Merged
merged 3 commits into from
Feb 28, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Copy link
Contributor

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 property

Copy link
Contributor Author

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.

Copy link
Contributor

Choose a reason for hiding this comment

The 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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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?

Copy link
Contributor

Choose a reason for hiding this comment

The 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}.")
}
}

@Discouraged(message = "This will be replaced with a real API.")
fun greeting(): String = "Matt says hi!"

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)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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

/**
Expand All @@ -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()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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?

Copy link
Contributor

Choose a reason for hiding this comment

The 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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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
) {
private var backgroundTimeMs = currentTimeMs()
private val sessionTimeoutMs = 30 * 60 * 1000L // TODO(mrober): Get session timeout from settings

init {
initiateSessionStart()
}

fun appBackgrounded() {
backgroundTimeMs = currentTimeMs()
}

fun appForegrounded() {
val intervalMs = currentTimeMs() - backgroundTimeMs
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
Copy link
Contributor

Choose a reason for hiding this comment

The 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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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.

Copy link
Contributor

Choose a reason for hiding this comment

The 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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


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
}
}

class SessionStartCounter {
var count = 0
private set

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
}
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These tests are great 👍