Move shouldLogSession decision into SessionFirelogPublisher

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

@@ -60,14 +60,14 @@ internal data class SessionInfo(
   /** What order this Session came in this run of the app. For the first Session this will be 0. */
   val sessionIndex: Int,
 
-  /** Tracks when the event was initiated */
-  var eventTimestampUs: Long,
+  /** Tracks when the event was initiated. */
+  val eventTimestampUs: Long,
 
   /** Data collection status of the dependent product SDKs. */
-  var dataCollectionStatus: DataCollectionStatus = DataCollectionStatus(),
+  val dataCollectionStatus: DataCollectionStatus = DataCollectionStatus(),
 
   /** Identifies a unique device+app installation: go/firebase-installations */
-  var firebaseInstallationId: String = "",
+  val firebaseInstallationId: String = "",
 )
 
 /** Contains the data collection state for all dependent SDKs and sampling info */

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

@@ -32,16 +32,13 @@ internal object SessionEvents {
       .ignoreNullValues(true)
       .build()
 
-  /**
-   * Construct a Session Start event.
-   *
-   * Some mutable fields, e.g. firebaseInstallationId, get populated later.
-   */
+  /** Construct a Session Start event. */
   fun buildSession(
     firebaseApp: FirebaseApp,
     sessionDetails: SessionDetails,
     sessionsSettings: SessionsSettings,
     subscribers: Map<SessionSubscriber.Name, SessionSubscriber> = emptyMap(),
+    firebaseInstallationId: String = "",
   ) =
     SessionEvent(
       eventType = EventType.SESSION_START,
@@ -56,6 +53,7 @@ internal object SessionEvents {
             crashlytics = toDataCollectionState(subscribers[SessionSubscriber.Name.CRASHLYTICS]),
             sessionSamplingRate = sessionsSettings.samplingRate,
           ),
+          firebaseInstallationId,
         ),
       applicationInfo = getApplicationInfo(firebaseApp)
     )

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

@@ -22,6 +22,7 @@ import com.google.firebase.FirebaseApp
 import com.google.firebase.app
 import com.google.firebase.installations.FirebaseInstallationsApi
 import com.google.firebase.sessions.api.FirebaseSessionsDependencies
+import com.google.firebase.sessions.api.SessionSubscriber
 import com.google.firebase.sessions.settings.SessionsSettings
 import kotlin.coroutines.CoroutineContext
 import kotlinx.coroutines.CoroutineScope
@@ -47,22 +48,28 @@ internal class SessionFirelogPublisher(
    * This will pull all the necessary information about the device in order to create a full
    * [SessionEvent], and then upload that through the Firelog interface.
    */
-  fun logSession(sessionDetails: SessionDetails) {
+  fun logSession(sessionDetails: SessionDetails) =
     CoroutineScope(backgroundDispatcher).launch {
-      val sessionEvent =
-        SessionEvents.buildSession(
-          firebaseApp,
-          sessionDetails,
-          sessionSettings,
-          FirebaseSessionsDependencies.getRegisteredSubscribers(),
+      // TODO(mrober): Get subscriber.isDataCollectionEnabled information from all processes.
+      // This will get the subscribers for the current process, not the union of all.
+      // It's possible only one subscriber in another process has data collection enabled.
+      val subscribers = FirebaseSessionsDependencies.getRegisteredSubscribers()
+
+      if (shouldLogSession(subscribers)) {
+        attemptLoggingSessionEvent(
+          SessionEvents.buildSession(
+            firebaseApp,
+            sessionDetails,
+            sessionSettings,
+            subscribers,
+            getFirebaseInstallationId(),
+          )
         )
-      sessionEvent.sessionData.firebaseInstallationId = getFid()
-      attemptLoggingSessionEvent(sessionEvent)
+      }
     }
-  }
 
   /** Attempts to write the given [SessionEvent] to firelog. Failures are logged and ignored. */
-  private suspend fun attemptLoggingSessionEvent(sessionEvent: SessionEvent) {
+  private fun attemptLoggingSessionEvent(sessionEvent: SessionEvent) {
     try {
       eventGDTLogger.log(sessionEvent)
       Log.d(TAG, "Successfully logged Session Start event: ${sessionEvent.sessionData.sessionId}")
@@ -71,8 +78,51 @@ internal class SessionFirelogPublisher(
     }
   }
 
-  /** Gets the Firebase Installation ID for the current app installation. */
-  private suspend fun getFid() =
+  /** Determines if the SDK should log a session to Firelog. */
+  private suspend fun shouldLogSession(
+    subscribers: Map<SessionSubscriber.Name, SessionSubscriber>
+  ): Boolean {
+    if (subscribers.isEmpty()) {
+      Log.d(
+        SessionLifecycleService.TAG,
+        "Sessions SDK did not have any dependent SDKs register as dependencies. Events will not be sent."
+      )
+      return false
+    }
+
+    if (subscribers.values.none { it.isDataCollectionEnabled }) {
+      Log.d(
+        SessionLifecycleService.TAG,
+        "Data Collection is disabled for all subscribers. Skipping this Session Event"
+      )
+      return false
+    }
+
+    Log.d(SessionLifecycleService.TAG, "Data Collection is enabled for at least one Subscriber")
+
+    // This will cause remote settings to be fetched if the cache is expired.
+    sessionSettings.updateSettings()
+
+    if (!sessionSettings.sessionsEnabled) {
+      Log.d(SessionLifecycleService.TAG, "Sessions SDK disabled. Events will not be sent.")
+      return false
+    }
+
+    if (!shouldCollectEvents()) {
+      Log.d(SessionLifecycleService.TAG, "Sessions SDK has dropped this session due to sampling.")
+      return false
+    }
+
+    return true
+  }
+
+  /**
+   * Gets the Firebase Installation ID for the current app installation.
+   *
+   * Only call this after checking [shouldLogSession] to ensure data collection is enabled for at
+   * least one subscriber. Otherwise this could cause a network request for no reason.
+   */
+  private suspend fun getFirebaseInstallationId() =
     try {
       firebaseInstallations.id.await()
     } catch (ex: Exception) {
@@ -81,8 +131,16 @@ internal class SessionFirelogPublisher(
       ""
     }
 
+  /** Calculate whether we should sample events using [SessionsSettings] data. */
+  private fun shouldCollectEvents(): Boolean {
+    // Sampling rate of 1 means the SDK will send every event.
+    return randomValueForSampling <= sessionSettings.samplingRate
+  }
+
   internal companion object {
-    const val TAG = "SessionFirelogPublisher"
+    private const val TAG = "SessionFirelogPublisher"
+
+    private val randomValueForSampling: Double = Math.random()
 
     val instance: SessionFirelogPublisher
       get() = Firebase.app.get(SessionFirelogPublisher::class.java)

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

@@ -80,6 +80,7 @@ internal object SessionLifecycleClient {
 
       CoroutineScope(Dispatchers.instance.backgroundDispatcher).launch {
         FirebaseSessionsDependencies.getRegisteredSubscribers().values.forEach { subscriber ->
+          // Notify subscribers, regardless of sampling and data collection state.
           subscriber.onSessionChanged(SessionSubscriber.SessionDetails(sessionId))
           Log.d(TAG, "Notified ${subscriber.sessionSubscriberName} of new session $sessionId")
         }

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

@@ -30,7 +30,6 @@ import android.os.Messenger
 import android.util.Log
 import com.google.firebase.Firebase
 import com.google.firebase.app
-import com.google.firebase.sessions.api.FirebaseSessionsDependencies
 import com.google.firebase.sessions.settings.SessionsSettings
 import java.util.concurrent.atomic.AtomicReference
 import kotlinx.coroutines.CoroutineScope
@@ -44,7 +43,7 @@ import kotlinx.coroutines.launch
 internal class SessionLifecycleService : Service() {
 
   /** The thread that will be used to process all lifecycle messages from connected clients. */
-  private var handlerThread: HandlerThread = HandlerThread("FirebaseSessions_HandlerThread")
+  private val handlerThread: HandlerThread = HandlerThread("FirebaseSessions_HandlerThread")
 
   /** The handler that will process all lifecycle messages from connected clients . */
   private var messageHandler: MessageHandler? = null
@@ -111,18 +110,17 @@ internal class SessionLifecycleService : Service() {
      * given [Message]. This will determine if the foregrounding should result in the creation of a
      * new session.
      */
-    private fun handleForegrounding(msg: Message) =
-      CoroutineScope(Dispatchers.instance.backgroundDispatcher).launch {
-        Log.d(TAG, "Activity foregrounding at ${msg.getWhen()}")
-        if (!hasForegrounded) {
-          Log.d(TAG, "Cold start detected.")
-          hasForegrounded = true
-          newSession()
-        } else if (isSessionRestart(msg.getWhen())) {
-          Log.d(TAG, "Session too long in background. Creating new session.")
-          newSession()
-        }
+    private fun handleForegrounding(msg: Message) {
+      Log.d(TAG, "Activity foregrounding at ${msg.getWhen()}")
+      if (!hasForegrounded) {
+        Log.d(TAG, "Cold start detected.")
+        hasForegrounded = true
+        newSession()
+      } else if (isSessionRestart(msg.getWhen())) {
+        Log.d(TAG, "Session too long in background. Creating new session.")
+        newSession()
       }
+    }
 
     /**
      * Handles a backgrounding event by any activity owned by the application as specified by the
@@ -144,37 +142,7 @@ internal class SessionLifecycleService : Service() {
     }
 
     /** Generates a new session id and sends it everywhere it's needed */
-    private suspend fun newSession() {
-      val subscribers = FirebaseSessionsDependencies.getRegisteredSubscribers()
-
-      if (subscribers.isEmpty()) {
-        Log.d(
-          TAG,
-          "Sessions SDK did not have any dependent SDKs register as dependencies. Events will not be sent."
-        )
-        return
-      }
-
-      if (subscribers.values.none { it.isDataCollectionEnabled }) {
-        Log.d(TAG, "Data Collection is disabled for all subscribers. Skipping this Session Event")
-        return
-      }
-
-      Log.d(TAG, "Data Collection is enabled for at least one Subscriber")
-
-      // This will cause remote settings to be fetched if the cache is expired.
-      SessionsSettings.instance.updateSettings()
-
-      if (!SessionsSettings.instance.sessionsEnabled) {
-        Log.d(TAG, "Sessions SDK disabled. Events will not be sent.")
-        return
-      }
-
-      if (!shouldCollectEvents()) {
-        Log.d(TAG, "Sessions SDK has dropped this session due to sampling.")
-        return
-      }
-
+    private fun newSession() {
       SessionGenerator.instance.generateNewSession()
       Log.d(TAG, "Generated new session ${SessionGenerator.instance.currentSession.sessionId}")
       broadcastSession()
@@ -228,16 +196,6 @@ internal class SessionLifecycleService : Service() {
         sessionId.let { datastore.updateSessionId(it) }
       }
     }
-
-    /** Calculate whether we should sample events using [SessionsSettings] data. */
-    private fun shouldCollectEvents(): Boolean {
-      // Sampling rate of 1 means the SDK will send every event.
-      return randomValueForSampling <= SessionsSettings.instance.samplingRate
-    }
-
-    private companion object {
-      private val randomValueForSampling: Double = Math.random()
-    }
   }
 
   override fun onCreate() {

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

@@ -20,23 +20,41 @@ import androidx.test.ext.junit.runners.AndroidJUnit4
 import com.google.common.truth.Truth.assertThat
 import com.google.firebase.FirebaseApp
 import com.google.firebase.concurrent.TestOnlyExecutors
+import com.google.firebase.sessions.api.FirebaseSessionsDependencies
+import com.google.firebase.sessions.api.SessionSubscriber
 import com.google.firebase.sessions.settings.SessionsSettings
 import com.google.firebase.sessions.testing.FakeEventGDTLogger
 import com.google.firebase.sessions.testing.FakeFirebaseApp
 import com.google.firebase.sessions.testing.FakeFirebaseInstallations
+import com.google.firebase.sessions.testing.FakeSessionSubscriber
 import com.google.firebase.sessions.testing.FakeSettingsProvider
 import com.google.firebase.sessions.testing.TestSessionEventData
 import kotlinx.coroutines.ExperimentalCoroutinesApi
 import kotlinx.coroutines.asCoroutineDispatcher
 import kotlinx.coroutines.test.runCurrent
 import kotlinx.coroutines.test.runTest
 import org.junit.After
+import org.junit.Before
 import org.junit.Test
 import org.junit.runner.RunWith
 
 @OptIn(ExperimentalCoroutinesApi::class)
 @RunWith(AndroidJUnit4::class)
 class SessionFirelogPublisherTest {
+  @Before
+  fun setUp() {
+    val crashlyticsSubscriber =
+      FakeSessionSubscriber(sessionSubscriberName = SessionSubscriber.Name.CRASHLYTICS)
+    FirebaseSessionsDependencies.addDependency(SessionSubscriber.Name.CRASHLYTICS)
+    FirebaseSessionsDependencies.register(crashlyticsSubscriber)
+  }
+
+  @After
+  fun cleanUp() {
+    FirebaseApp.clearInstancesForTest()
+    FirebaseSessionsDependencies.reset()
+  }
+
   @Test
   fun logSession_populatesFid() = runTest {
     val fakeFirebaseApp = FakeFirebaseApp()
@@ -61,13 +79,7 @@ class SessionFirelogPublisherTest {
 
     runCurrent()
 
-    System.out.println("FakeEventGDTLogger: $fakeEventGDTLogger")
     assertThat(fakeEventGDTLogger.loggedEvent!!.sessionData.firebaseInstallationId)
       .isEqualTo("FaKeFiD")
   }
-
-  @After
-  fun cleanUp() {
-    FirebaseApp.clearInstancesForTest()
-  }
 }