Revert some changes - and add a relevant unit test

Author: tejasd

firebase-perf/src/main/java/com/google/firebase/perf/logging/FirebaseSessionsEnforcementCheck.kt

@@ -17,47 +17,19 @@
 package com.google.firebase.perf.logging
 
 import com.google.firebase.perf.session.isLegacy
-import com.google.firebase.perf.v1.NetworkRequestMetric
-import com.google.firebase.perf.v1.TraceMetric
-import java.util.Collections
-import com.google.firebase.perf.v1.PerfSession as ProtoPerfSession
 
 class FirebaseSessionsEnforcementCheck {
   companion object {
     /** When enabled, failed preconditions will cause assertion errors for debugging. */
     @JvmStatic var enforcement: Boolean = false
     private var logger: AndroidLogger = AndroidLogger.getInstance()
 
-    @JvmStatic
-    fun filterLegacySessions(trace: TraceMetric): TraceMetric {
-      val updatedTrace = trace.toBuilder().clearPerfSessions()
-      filterLegacySessions(trace.perfSessionsList).forEach { updatedTrace.addPerfSessions(it) }
-      return updatedTrace.build()
-    }
-
-    @JvmStatic
-    fun filterLegacySessions(networkRequestMetric: NetworkRequestMetric): NetworkRequestMetric {
-      val updatedNetworkRequestMetric = networkRequestMetric.toBuilder().clearPerfSessions()
-      filterLegacySessions(networkRequestMetric.perfSessionsList).forEach { updatedNetworkRequestMetric.addPerfSessions(it) }
-      return updatedNetworkRequestMetric.build()
-    }
-
     @JvmStatic
     fun checkSession(sessionId: String, failureMessage: String) {
       if (sessionId.isLegacy()) {
-        logger.verbose("legacy session ${sessionId}: $failureMessage")
+        logger.verbose("Legacy Session ${sessionId}: $failureMessage")
         assert(!enforcement) { failureMessage }
       }
     }
-
-    private fun filterLegacySessions(sessions: List<ProtoPerfSession>): List<ProtoPerfSession> {
-      return if (sessions.count() > 0 && sessions[0].sessionId.isLegacy()) {
-        val updatedSessions = sessions.toMutableList<ProtoPerfSession>()
-        Collections.swap(updatedSessions, 0, 1)
-        updatedSessions
-      } else {
-        sessions
-      }
-    }
   }
 }

firebase-perf/src/main/java/com/google/firebase/perf/session/SessionManager.java

@@ -18,7 +18,6 @@
 import android.content.Context;
 import androidx.annotation.Keep;
 import androidx.annotation.VisibleForTesting;
-import com.google.firebase.perf.logging.FirebaseSessionsEnforcementCheck;
 import com.google.firebase.perf.session.gauges.GaugeManager;
 import com.google.firebase.perf.v1.GaugeMetadata;
 import com.google.firebase.perf.v1.GaugeMetric;

firebase-perf/src/main/java/com/google/firebase/perf/transport/TransportManager.java

@@ -14,11 +14,10 @@
 
 package com.google.firebase.perf.transport;
 
+import static com.google.firebase.perf.logging.FirebaseSessionsEnforcementCheck.checkSession;
 import static com.google.firebase.perf.util.AppProcessesProvider.getProcessName;
 import static java.util.concurrent.TimeUnit.MILLISECONDS;
 import static java.util.concurrent.TimeUnit.MINUTES;
-import static com.google.firebase.perf.logging.FirebaseSessionsEnforcementCheck.filterLegacySessions;
-import static com.google.firebase.perf.logging.FirebaseSessionsEnforcementCheck.checkSession;
 
 import android.annotation.SuppressLint;
 import android.content.Context;
@@ -302,7 +301,7 @@ public void log(final TraceMetric traceMetric) {
    */
   public void log(final TraceMetric traceMetric, final ApplicationProcessState appState) {
     executorService.execute(
-        () -> syncLog(PerfMetric.newBuilder().setTraceMetric(filterLegacySessions(traceMetric)), appState));
+        () -> syncLog(PerfMetric.newBuilder().setTraceMetric(traceMetric), appState));
   }
 
   /**
@@ -332,7 +331,7 @@ public void log(
     executorService.execute(
         () ->
             syncLog(
-                PerfMetric.newBuilder().setNetworkRequestMetric(filterLegacySessions(networkRequestMetric)), appState));
+                PerfMetric.newBuilder().setNetworkRequestMetric(networkRequestMetric), appState));
   }
 
   /**
@@ -358,7 +357,7 @@ public void log(final GaugeMetric gaugeMetric) {
    * {@link #isAllowedToDispatch(PerfMetric)}).
    */
   public void log(final GaugeMetric gaugeMetric, final ApplicationProcessState appState) {
-    checkSession(gaugeMetric.getSessionId(), "Logging GaugeMetric with Legacy Session ID.");
+    checkSession(gaugeMetric.getSessionId(), "log(GaugeMetric)");
     executorService.execute(
         () -> syncLog(PerfMetric.newBuilder().setGaugeMetric(gaugeMetric), appState));
   }

firebase-perf/src/test/java/com/google/firebase/perf/session/FirebaseSessionsTestHelper.kt

@@ -25,3 +25,5 @@ fun createTestSession(suffix: Int): PerfSession {
 }
 
 fun testSessionId(suffix: Int): String = "abc$suffix"
+
+fun testLegacySessionId(suffix: Int): String = "zabc$suffix"

firebase-perf/src/test/java/com/google/firebase/perf/session/PerfSessionTest.java

@@ -15,7 +15,9 @@
 package com.google.firebase.perf.session;
 
 import static com.google.common.truth.Truth.assertThat;
+import static com.google.firebase.perf.session.FirebaseSessionsHelperKt.isLegacy;
 import static com.google.firebase.perf.session.FirebaseSessionsTestHelperKt.createTestSession;
+import static com.google.firebase.perf.session.FirebaseSessionsTestHelperKt.testLegacySessionId;
 import static com.google.firebase.perf.session.FirebaseSessionsTestHelperKt.testSessionId;
 import static com.google.firebase.perf.util.Constants.PREFS_NAME;
 import static org.mockito.Mockito.mock;
@@ -224,6 +226,57 @@ public void testBuildAndSortMovesTheVerboseSessionToTop() {
     assertThat(PerfSession.isVerbose(perfSessions[0])).isTrue();
   }
 
+  @Test
+  public void testBuildAndSortMovesTheVerboseSessionToTop_legacySession() {
+    // Force all the sessions from now onwards to be non-verbose
+    forceNonVerboseSession();
+
+    // Next, create 3 non-verbose sessions, including a legacy session.
+    List<PerfSession> sessions = new ArrayList<>();
+    sessions.add(PerfSession.createWithId(testLegacySessionId(1)));
+    sessions.add(PerfSession.createWithId(testSessionId(2)));
+    sessions.add(PerfSession.createWithId(testSessionId(3)));
+
+    // Force all the sessions from now onwards to be verbose
+    forceVerboseSession();
+
+    // Next, create 2 verbose sessions
+    sessions.add(PerfSession.createWithId(testSessionId(4)));
+    sessions.add(PerfSession.createWithId(testSessionId(5)));
+
+    // Verify that the first session in the list of sessions was not verbose
+    assertThat(sessions.get(0).isVerbose()).isFalse();
+
+    com.google.firebase.perf.v1.PerfSession[] perfSessions =
+        PerfSession.buildAndSort(ImmutableList.copyOf(sessions));
+
+    // Verify that after building the proto objects for PerfSessions, the first session in the array
+    // of proto objects is a verbose session
+    assertThat(PerfSession.isVerbose(perfSessions[0])).isTrue();
+  }
+
+  @Test
+  public void testBuildAndSortKeepsLegacySessionAtTopWithNoVerboseSessions() {
+    // Force all the sessions from now onwards to be non-verbose
+    forceNonVerboseSession();
+
+    // Next, create 3 non-verbose sessions, including a legacy session.
+    List<PerfSession> sessions = new ArrayList<>();
+    sessions.add(PerfSession.createWithId(testLegacySessionId(1)));
+    sessions.add(PerfSession.createWithId(testSessionId(2)));
+    sessions.add(PerfSession.createWithId(testSessionId(3)));
+
+    // Verify that the first session in the list of sessions was legacy.
+    assertThat(isLegacy(sessions.get(0))).isTrue();
+
+    com.google.firebase.perf.v1.PerfSession[] perfSessions =
+        PerfSession.buildAndSort(ImmutableList.copyOf(sessions));
+
+    // Verify that after building the proto objects for PerfSessions, the first session in the array
+    // of proto objects is a legacy session.
+    assertThat(isLegacy(perfSessions[0].getSessionId())).isTrue();
+  }
+
   @Test
   public void testIsExpiredReturnsFalseWhenCurrentSessionLengthIsLessThanMaxSessionLength() {
     Timer mockTimer = mock(Timer.class);