Skip to content

Commit 917fa58

Browse files
tejasdtejasd
committed
Update legacy session enforcement and debugging logic (#7244)
Based on our discussion, updated this PR with the following changes: - Removed excessive logs for legacy sessions. - Changed the logs for sessions list to log only if *all* the sessions were legacy. - Added unit tests to verify the behaviour of verbose and legacy sessions, which keeps legacy sessions as the first *only* if there's no verbose session.
1 parent 22ec9a5 commit 917fa58

File tree

6 files changed

+67
-28
lines changed

6 files changed

+67
-28
lines changed

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

Lines changed: 5 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -16,7 +16,6 @@
1616

1717
package com.google.firebase.perf.logging
1818

19-
import com.google.firebase.perf.session.PerfSession
2019
import com.google.firebase.perf.session.isLegacy
2120
import com.google.firebase.perf.v1.PerfSession as ProtoPerfSession
2221

@@ -27,19 +26,16 @@ class FirebaseSessionsEnforcementCheck {
2726
private var logger: AndroidLogger = AndroidLogger.getInstance()
2827

2928
@JvmStatic
30-
fun checkSession(sessions: List<ProtoPerfSession>, failureMessage: String) {
31-
sessions.forEach { checkSession(it.sessionId, failureMessage) }
32-
}
33-
34-
@JvmStatic
35-
fun checkSession(session: PerfSession, failureMessage: String) {
36-
checkSession(session.sessionId(), failureMessage)
29+
fun checkSessionsList(sessions: List<ProtoPerfSession>, failureMessage: String) {
30+
if (sessions.count { it.sessionId.isLegacy() } == sessions.size) {
31+
sessions.forEach { checkSession(it.sessionId, failureMessage) }
32+
}
3733
}
3834

3935
@JvmStatic
4036
fun checkSession(sessionId: String, failureMessage: String) {
4137
if (sessionId.isLegacy()) {
42-
logger.debug("legacy session ${sessionId}: $failureMessage")
38+
logger.verbose("Contains Legacy Session ${sessionId}: $failureMessage")
4339
assert(!enforcement) { failureMessage }
4440
}
4541
}

‎firebase-perf/src/main/java/com/google/firebase/perf/session/FirebasePerformanceSessionSubscriber.kt‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -17,7 +17,7 @@
1717
package com.google.firebase.perf.session
1818

1919
import com.google.firebase.perf.config.ConfigResolver
20-
import com.google.firebase.perf.logging.FirebaseSessionsEnforcementCheck
20+
import com.google.firebase.perf.logging.FirebaseSessionsEnforcementCheck.Companion.checkSession
2121
import com.google.firebase.sessions.api.SessionSubscriber
2222

2323
class FirebasePerformanceSessionSubscriber(val configResolver: ConfigResolver) : SessionSubscriber {
@@ -30,7 +30,7 @@ class FirebasePerformanceSessionSubscriber(val configResolver: ConfigResolver) :
3030
override fun onSessionChanged(sessionDetails: SessionSubscriber.SessionDetails) {
3131
val currentPerfSession = SessionManager.getInstance().perfSession()
3232
// TODO(b/394127311): Add logic to deal with app start gauges if necessary.
33-
FirebaseSessionsEnforcementCheck.checkSession(currentPerfSession, "onSessionChanged")
33+
checkSession(currentPerfSession.sessionId(), "Existing session in onSessionChanged().")
3434

3535
val updatedSession = PerfSession.createWithId(sessionDetails.sessionId)
3636
SessionManager.getInstance().updatePerfSession(updatedSession)

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

Lines changed: 0 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -18,7 +18,6 @@
1818
import android.content.Context;
1919
import androidx.annotation.Keep;
2020
import androidx.annotation.VisibleForTesting;
21-
import com.google.firebase.perf.logging.FirebaseSessionsEnforcementCheck;
2221
import com.google.firebase.perf.session.gauges.GaugeManager;
2322
import com.google.firebase.perf.v1.GaugeMetadata;
2423
import com.google.firebase.perf.v1.GaugeMetric;
@@ -46,16 +45,13 @@ public static SessionManager getInstance() {
4645

4746
/** Returns the currently active PerfSession. */
4847
public final PerfSession perfSession() {
49-
FirebaseSessionsEnforcementCheck.checkSession(perfSession, "PerfSession.perfSession()");
50-
5148
return perfSession;
5249
}
5350

5451
private SessionManager() {
5552
// Creates a legacy session by default. This is a safety net to allow initializing
5653
// SessionManager - but the current implementation replaces it immediately.
5754
this(GaugeManager.getInstance(), PerfSession.createWithId(null));
58-
FirebaseSessionsEnforcementCheck.checkSession(perfSession, "SessionManager()");
5955
}
6056

6157
@VisibleForTesting
@@ -78,9 +74,6 @@ public void setApplicationContext(final Context appContext) {
7874
* @see PerfSession#isSessionRunningTooLong()
7975
*/
8076
public void stopGaugeCollectionIfSessionRunningTooLong() {
81-
FirebaseSessionsEnforcementCheck.checkSession(
82-
perfSession, "SessionManager.stopGaugeCollectionIfSessionRunningTooLong");
83-
8477
if (perfSession.isSessionRunningTooLong()) {
8578
gaugeManager.stopCollectingGauges();
8679
}
@@ -158,16 +151,12 @@ public void unregisterForSessionUpdates(WeakReference<SessionAwareObject> client
158151
}
159152

160153
private void logGaugeMetadataIfCollectionEnabled() {
161-
FirebaseSessionsEnforcementCheck.checkSession(
162-
perfSession, "logGaugeMetadataIfCollectionEnabled");
163154
if (perfSession.isVerbose()) {
164155
gaugeManager.logGaugeMetadata(perfSession.sessionId());
165156
}
166157
}
167158

168159
private void startOrStopCollectingGauges() {
169-
FirebaseSessionsEnforcementCheck.checkSession(perfSession, "startOrStopCollectingGauges");
170-
171160
if (perfSession.isVerbose()) {
172161
gaugeManager.startCollectingGauges(perfSession);
173162
} else {

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

Lines changed: 5 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -14,6 +14,8 @@
1414

1515
package com.google.firebase.perf.transport;
1616

17+
import static com.google.firebase.perf.logging.FirebaseSessionsEnforcementCheck.checkSession;
18+
import static com.google.firebase.perf.logging.FirebaseSessionsEnforcementCheck.checkSessionsList;
1719
import static com.google.firebase.perf.util.AppProcessesProvider.getProcessName;
1820
import static java.util.concurrent.TimeUnit.MILLISECONDS;
1921
import static java.util.concurrent.TimeUnit.MINUTES;
@@ -39,7 +41,6 @@
3941
import com.google.firebase.perf.config.ConfigResolver;
4042
import com.google.firebase.perf.logging.AndroidLogger;
4143
import com.google.firebase.perf.logging.ConsoleUrlGenerator;
42-
import com.google.firebase.perf.logging.FirebaseSessionsEnforcementCheck;
4344
import com.google.firebase.perf.metrics.validator.PerfMetricValidator;
4445
import com.google.firebase.perf.session.SessionManager;
4546
import com.google.firebase.perf.util.Constants;
@@ -300,8 +301,7 @@ public void log(final TraceMetric traceMetric) {
300301
* {@link #isAllowedToDispatch(PerfMetric)}).
301302
*/
302303
public void log(final TraceMetric traceMetric, final ApplicationProcessState appState) {
303-
FirebaseSessionsEnforcementCheck.checkSession(
304-
traceMetric.getPerfSessionsList(), "log TraceMetric");
304+
checkSessionsList(traceMetric.getPerfSessionsList(), "log(TraceMetric)");
305305
executorService.execute(
306306
() -> syncLog(PerfMetric.newBuilder().setTraceMetric(traceMetric), appState));
307307
}
@@ -330,8 +330,7 @@ public void log(final NetworkRequestMetric networkRequestMetric) {
330330
*/
331331
public void log(
332332
final NetworkRequestMetric networkRequestMetric, final ApplicationProcessState appState) {
333-
FirebaseSessionsEnforcementCheck.checkSession(
334-
networkRequestMetric.getPerfSessionsList(), "log NetworkRequestMetric");
333+
checkSessionsList(networkRequestMetric.getPerfSessionsList(), "log(NetworkRequestMetric)");
335334
executorService.execute(
336335
() ->
337336
syncLog(
@@ -361,7 +360,7 @@ public void log(final GaugeMetric gaugeMetric) {
361360
* {@link #isAllowedToDispatch(PerfMetric)}).
362361
*/
363362
public void log(final GaugeMetric gaugeMetric, final ApplicationProcessState appState) {
364-
FirebaseSessionsEnforcementCheck.checkSession(gaugeMetric.getSessionId(), "log GaugeMetric");
363+
checkSession(gaugeMetric.getSessionId(), "log(GaugeMetric)");
365364
executorService.execute(
366365
() -> syncLog(PerfMetric.newBuilder().setGaugeMetric(gaugeMetric), appState));
367366
}

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

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -25,3 +25,5 @@ fun createTestSession(suffix: Int): PerfSession {
2525
}
2626

2727
fun testSessionId(suffix: Int): String = "abc$suffix"
28+
29+
fun testLegacySessionId(suffix: Int): String = "zabc$suffix"

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

Lines changed: 53 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -15,7 +15,9 @@
1515
package com.google.firebase.perf.session;
1616

1717
import static com.google.common.truth.Truth.assertThat;
18+
import static com.google.firebase.perf.session.FirebaseSessionsHelperKt.isLegacy;
1819
import static com.google.firebase.perf.session.FirebaseSessionsTestHelperKt.createTestSession;
20+
import static com.google.firebase.perf.session.FirebaseSessionsTestHelperKt.testLegacySessionId;
1921
import static com.google.firebase.perf.session.FirebaseSessionsTestHelperKt.testSessionId;
2022
import static com.google.firebase.perf.util.Constants.PREFS_NAME;
2123
import static org.mockito.Mockito.mock;
@@ -224,6 +226,57 @@ public void testBuildAndSortMovesTheVerboseSessionToTop() {
224226
assertThat(PerfSession.isVerbose(perfSessions[0])).isTrue();
225227
}
226228

229+
@Test
230+
public void testBuildAndSortMovesTheVerboseSessionToTop_legacySession() {
231+
// Force all the sessions from now onwards to be non-verbose
232+
forceNonVerboseSession();
233+
234+
// Next, create 3 non-verbose sessions, including a legacy session.
235+
List<PerfSession> sessions = new ArrayList<>();
236+
sessions.add(PerfSession.createWithId(testLegacySessionId(1)));
237+
sessions.add(PerfSession.createWithId(testSessionId(2)));
238+
sessions.add(PerfSession.createWithId(testSessionId(3)));
239+
240+
// Force all the sessions from now onwards to be verbose
241+
forceVerboseSession();
242+
243+
// Next, create 2 verbose sessions
244+
sessions.add(PerfSession.createWithId(testSessionId(4)));
245+
sessions.add(PerfSession.createWithId(testSessionId(5)));
246+
247+
// Verify that the first session in the list of sessions was not verbose
248+
assertThat(sessions.get(0).isVerbose()).isFalse();
249+
250+
com.google.firebase.perf.v1.PerfSession[] perfSessions =
251+
PerfSession.buildAndSort(ImmutableList.copyOf(sessions));
252+
253+
// Verify that after building the proto objects for PerfSessions, the first session in the array
254+
// of proto objects is a verbose session
255+
assertThat(PerfSession.isVerbose(perfSessions[0])).isTrue();
256+
}
257+
258+
@Test
259+
public void testBuildAndSortKeepsLegacySessionAtTopWithNoVerboseSessions() {
260+
// Force all the sessions from now onwards to be non-verbose
261+
forceNonVerboseSession();
262+
263+
// Next, create 3 non-verbose sessions, including a legacy session.
264+
List<PerfSession> sessions = new ArrayList<>();
265+
sessions.add(PerfSession.createWithId(testLegacySessionId(1)));
266+
sessions.add(PerfSession.createWithId(testSessionId(2)));
267+
sessions.add(PerfSession.createWithId(testSessionId(3)));
268+
269+
// Verify that the first session in the list of sessions was legacy.
270+
assertThat(isLegacy(sessions.get(0))).isTrue();
271+
272+
com.google.firebase.perf.v1.PerfSession[] perfSessions =
273+
PerfSession.buildAndSort(ImmutableList.copyOf(sessions));
274+
275+
// Verify that after building the proto objects for PerfSessions, the first session in the array
276+
// of proto objects is a legacy session.
277+
assertThat(isLegacy(perfSessions[0].getSessionId())).isTrue();
278+
}
279+
227280
@Test
228281
public void testIsExpiredReturnsFalseWhenCurrentSessionLengthIsLessThanMaxSessionLength() {
229282
Timer mockTimer = mock(Timer.class);

0 commit comments

Comments
 (0)