move log and non-fatal presistence to disk write worker

Author: themiswang

firebase-crashlytics/src/androidTest/java/com/google/firebase/crashlytics/internal/common/SessionReportingCoordinatorTest.java

@@ -33,6 +33,8 @@
 
 import com.google.android.gms.tasks.Task;
 import com.google.android.gms.tasks.Tasks;
+import com.google.firebase.concurrent.TestOnlyExecutors;
+import com.google.firebase.crashlytics.internal.CrashlyticsWorker;
 import com.google.firebase.crashlytics.internal.metadata.LogFileManager;
 import com.google.firebase.crashlytics.internal.metadata.UserMetadata;
 import com.google.firebase.crashlytics.internal.model.CrashlyticsReport;
@@ -69,6 +71,8 @@ public class SessionReportingCoordinatorTest {
 
   private SessionReportingCoordinator reportingCoordinator;
 
+  private CrashlyticsWorker diskWriteWorker = new CrashlyticsWorker(TestOnlyExecutors.background());
+
   @Before
   public void setUp() {
     MockitoAnnotations.initMocks(this);
@@ -80,7 +84,8 @@ public void setUp() {
             reportSender,
             logFileManager,
             reportMetadata,
-            idManager);
+            idManager,
+            diskWriteWorker);
   }
 
   @Test
@@ -116,7 +121,8 @@ public void testFatalEvent_persistsHighPriorityEventWithAllThreadsForSessionId()
   }
 
   @Test
-  public void testNonFatalEvent_persistsNormalPriorityEventWithoutAllThreadsForSessionId() {
+  public void testNonFatalEvent_persistsNormalPriorityEventWithoutAllThreadsForSessionId()
+      throws Exception {
     final String eventType = "error";
     final String sessionId = "testSessionId";
     final long timestamp = System.currentTimeMillis();
@@ -126,6 +132,8 @@ public void testNonFatalEvent_persistsNormalPriorityEventWithoutAllThreadsForSes
     reportingCoordinator.onBeginSession(sessionId, timestamp);
     reportingCoordinator.persistNonFatalEvent(mockException, mockThread, sessionId, timestamp);
 
+    diskWriteWorker.await();
+
     final boolean expectedAllThreads = false;
     final boolean expectedHighPriority = false;
 
@@ -136,7 +144,7 @@ public void testNonFatalEvent_persistsNormalPriorityEventWithoutAllThreadsForSes
   }
 
   @Test
-  public void testNonFatalEvent_addsLogsToEvent() {
+  public void testNonFatalEvent_addsLogsToEvent() throws Exception {
     long timestamp = System.currentTimeMillis();
 
     mockEventInteractions();
@@ -149,14 +157,16 @@ public void testNonFatalEvent_addsLogsToEvent() {
     reportingCoordinator.onBeginSession(sessionId, timestamp);
     reportingCoordinator.persistNonFatalEvent(mockException, mockThread, sessionId, timestamp);
 
+    diskWriteWorker.await();
+
     verify(mockEventBuilder)
         .setLog(CrashlyticsReport.Session.Event.Log.builder().setContent(testLog).build());
     verify(mockEventBuilder).build();
     verify(logFileManager, never()).clearLog();
   }
 
   @Test
-  public void testNonFatalEvent_addsNoLogsToEventWhenNoneAvailable() {
+  public void testNonFatalEvent_addsNoLogsToEventWhenNoneAvailable() throws Exception {
     long timestamp = System.currentTimeMillis();
 
     mockEventInteractions();
@@ -168,6 +178,8 @@ public void testNonFatalEvent_addsNoLogsToEventWhenNoneAvailable() {
     reportingCoordinator.onBeginSession(sessionId, timestamp);
     reportingCoordinator.persistNonFatalEvent(mockException, mockThread, sessionId, timestamp);
 
+    diskWriteWorker.await();
+
     verify(mockEventBuilder, never()).setLog(any(CrashlyticsReport.Session.Event.Log.class));
     verify(mockEventBuilder).build();
     verify(logFileManager, never()).clearLog();
@@ -212,7 +224,7 @@ public void testFatalEvent_addsNoLogsToEventWhenNoneAvailable() {
   }
 
   @Test
-  public void testNonFatalEvent_addsSortedKeysToEvent() {
+  public void testNonFatalEvent_addsSortedKeysToEvent() throws Exception {
     final long timestamp = System.currentTimeMillis();
 
     mockEventInteractions();
@@ -243,6 +255,8 @@ public void testNonFatalEvent_addsSortedKeysToEvent() {
     reportingCoordinator.onBeginSession(sessionId, timestamp);
     reportingCoordinator.persistNonFatalEvent(mockException, mockThread, sessionId, timestamp);
 
+    diskWriteWorker.await();
+
     verify(mockEventAppBuilder).setCustomAttributes(expectedCustomAttributes);
     verify(mockEventAppBuilder).setInternalKeys(expectedCustomAttributes);
     verify(mockEventAppBuilder).build();
@@ -252,7 +266,7 @@ public void testNonFatalEvent_addsSortedKeysToEvent() {
   }
 
   @Test
-  public void testNonFatalEvent_addsNoKeysToEventWhenNoneAvailable() {
+  public void testNonFatalEvent_addsNoKeysToEventWhenNoneAvailable() throws Exception {
     final long timestamp = System.currentTimeMillis();
 
     mockEventInteractions();
@@ -266,6 +280,8 @@ public void testNonFatalEvent_addsNoKeysToEventWhenNoneAvailable() {
     reportingCoordinator.onBeginSession(sessionId, timestamp);
     reportingCoordinator.persistNonFatalEvent(mockException, mockThread, sessionId, timestamp);
 
+    diskWriteWorker.await();
+
     verify(mockEventAppBuilder, never()).setCustomAttributes(anyList());
     verify(mockEventAppBuilder, never()).build();
     verify(mockEventBuilder, never()).setApp(mockEventApp);
@@ -274,7 +290,7 @@ public void testNonFatalEvent_addsNoKeysToEventWhenNoneAvailable() {
   }
 
   @Test
-  public void testNonFatalEvent_addRolloutsEvent() {
+  public void testNonFatalEvent_addRolloutsEvent() throws Exception {
     long timestamp = System.currentTimeMillis();
     String sessionId = "testSessionId";
     mockEventInteractions();
@@ -287,6 +303,8 @@ public void testNonFatalEvent_addRolloutsEvent() {
     reportingCoordinator.onBeginSession(sessionId, timestamp);
     reportingCoordinator.persistNonFatalEvent(mockException, mockThread, sessionId, timestamp);
 
+    diskWriteWorker.await();
+
     verify(mockEventAppBuilder, never()).setCustomAttributes(anyList());
     verify(mockEventAppBuilder, never()).build();
     verify(mockEventBuilder, never()).setApp(mockEventApp);
@@ -417,35 +435,6 @@ public void testFatalEvent_addRolloutsToEvent() {
     verify(mockEventBuilder, times(2)).build();
   }
 
-  @Test
-  public void onLog_writesToLogFileManager() {
-    long timestamp = System.currentTimeMillis();
-    String log = "this is a log";
-
-    reportingCoordinator.onLog(timestamp, log);
-
-    verify(logFileManager).writeToLog(timestamp, log);
-  }
-
-  @Test
-  public void onCustomKey_writesToReportMetadata() {
-    final String key = "key";
-    final String value = "value";
-
-    reportingCoordinator.onCustomKey(key, value);
-
-    verify(reportMetadata).setCustomKey(key, value);
-  }
-
-  @Test
-  public void onUserId_writesUserToReportMetadata() {
-    final String userId = "testUser";
-
-    reportingCoordinator.onUserId(userId);
-
-    verify(reportMetadata).setUserId(userId);
-  }
-
   @Test
   public void testFinalizeSessionWithNativeEvent_createsCrashlyticsReportWithNativePayload() {
     byte[] testBytes = {0, 2, 20, 10};

firebase-crashlytics/src/main/java/com/google/firebase/crashlytics/internal/common/CrashlyticsCore.java

@@ -172,7 +172,8 @@ public boolean onPreExecute(AppData appData, SettingsProvider settingsProvider)
               stackTraceTrimmingStrategy,
               settingsProvider,
               onDemandCounter,
-              sessionsSubscriber);
+              sessionsSubscriber,
+              diskWriteWorker);
 
       controller =
           new CrashlyticsController(
@@ -337,7 +338,7 @@ public void logException(@NonNull Throwable throwable) {
    */
   public void log(final String msg) {
     final long timestamp = System.currentTimeMillis() - startTime;
-    commonWorker.submit(() -> controller.writeToLog(timestamp, msg));
+    diskWriteWorker.submit(() -> controller.writeToLog(timestamp, msg));
   }
 
   public void setUserId(String identifier) {

firebase-crashlytics/src/main/java/com/google/firebase/crashlytics/internal/common/CrashlyticsLifecycleEvents.java

@@ -25,27 +25,4 @@ interface CrashlyticsLifecycleEvents {
    * @param sessionId the identifier for the new session
    */
   void onBeginSession(@NonNull String sessionId, long timestamp);
-
-  /**
-   * Called when a message is logged by the user.
-   *
-   * @param timestamp the timestamp of the message (in milliseconds since app launch)
-   * @param log the log message
-   */
-  void onLog(long timestamp, String log);
-
-  /**
-   * Called when a custom key is set by the user.
-   *
-   * @param key
-   * @param value
-   */
-  void onCustomKey(String key, String value);
-
-  /**
-   * Called when a user ID is set by the user.
-   *
-   * @param userId
-   */
-  void onUserId(String userId);
 }

firebase-crashlytics/src/main/java/com/google/firebase/crashlytics/internal/common/SessionReportingCoordinator.java

@@ -23,6 +23,7 @@
 import androidx.annotation.VisibleForTesting;
 import com.google.android.gms.tasks.Task;
 import com.google.android.gms.tasks.Tasks;
+import com.google.firebase.crashlytics.internal.CrashlyticsWorker;
 import com.google.firebase.crashlytics.internal.Logger;
 import com.google.firebase.crashlytics.internal.metadata.LogFileManager;
 import com.google.firebase.crashlytics.internal.metadata.UserMetadata;
@@ -68,7 +69,8 @@ public static SessionReportingCoordinator create(
       StackTraceTrimmingStrategy stackTraceTrimmingStrategy,
       SettingsProvider settingsProvider,
       OnDemandCounter onDemandCounter,
-      CrashlyticsAppQualitySessionsSubscriber sessionsSubscriber) {
+      CrashlyticsAppQualitySessionsSubscriber sessionsSubscriber,
+      CrashlyticsWorker diskWriteWorker) {
     final CrashlyticsReportDataCapture dataCapture =
         new CrashlyticsReportDataCapture(
             context, idManager, appData, stackTraceTrimmingStrategy, settingsProvider);
@@ -77,7 +79,13 @@ public static SessionReportingCoordinator create(
     final DataTransportCrashlyticsReportSender reportSender =
         DataTransportCrashlyticsReportSender.create(context, settingsProvider, onDemandCounter);
     return new SessionReportingCoordinator(
-        dataCapture, reportPersistence, reportSender, logFileManager, userMetadata, idManager);
+        dataCapture,
+        reportPersistence,
+        reportSender,
+        logFileManager,
+        userMetadata,
+        idManager,
+        diskWriteWorker);
   }
 
   private final CrashlyticsReportDataCapture dataCapture;
@@ -87,19 +95,23 @@ public static SessionReportingCoordinator create(
   private final UserMetadata reportMetadata;
   private final IdManager idManager;
 
+  final CrashlyticsWorker diskWriteWorker;
+
   SessionReportingCoordinator(
       CrashlyticsReportDataCapture dataCapture,
       CrashlyticsReportPersistence reportPersistence,
       DataTransportCrashlyticsReportSender reportsSender,
       LogFileManager logFileManager,
       UserMetadata reportMetadata,
-      IdManager idManager) {
+      IdManager idManager,
+      CrashlyticsWorker diskWriteWorker) {
     this.dataCapture = dataCapture;
     this.reportPersistence = reportPersistence;
     this.reportsSender = reportsSender;
     this.logFileManager = logFileManager;
     this.reportMetadata = reportMetadata;
     this.idManager = idManager;
+    this.diskWriteWorker = diskWriteWorker;
   }
 
   @Override
@@ -110,21 +122,6 @@ public void onBeginSession(@NonNull String sessionId, long timestampSeconds) {
     reportPersistence.persistReport(capturedReport);
   }
 
-  @Override
-  public void onLog(long timestamp, String log) {
-    logFileManager.writeToLog(timestamp, log);
-  }
-
-  @Override
-  public void onCustomKey(String key, String value) {
-    reportMetadata.setCustomKey(key, value);
-  }
-
-  @Override
-  public void onUserId(String userId) {
-    reportMetadata.setUserId(userId);
-  }
-
   public void persistFatalEvent(
       @NonNull Throwable event, @NonNull Thread thread, @NonNull String sessionId, long timestamp) {
     Logger.getLogger().v("Persisting fatal event for session " + sessionId);
@@ -339,8 +336,19 @@ private void persistEvent(
             EVENT_THREAD_IMPORTANCE,
             MAX_CHAINED_EXCEPTION_DEPTH,
             includeAllThreads);
+    CrashlyticsReport.Session.Event finallizedEvent = addMetaDataToEvent(capturedEvent);
+
+    // Non-fatal case, non-fatal does not include all threads
+    // We move to diskWriteWorker for persistence
+    if (!includeAllThreads) {
+      diskWriteWorker.submit(
+          () -> {
+            reportPersistence.persistEvent(finallizedEvent, sessionId, isHighPriority);
+          });
+      return;
+    }
 
-    reportPersistence.persistEvent(addMetaDataToEvent(capturedEvent), sessionId, isHighPriority);
+    reportPersistence.persistEvent(finallizedEvent, sessionId, isHighPriority);
   }
 
   private boolean onReportSendComplete(@NonNull Task<CrashlyticsReportWithSessionId> task) {

firebase-crashlytics/src/test/java/com/google/firebase/crashlytics/internal/common/SessionReportingCoordinatorRobolectricTest.java

@@ -28,6 +28,8 @@
 import android.os.Build;
 import androidx.annotation.RequiresApi;
 import androidx.test.core.app.ApplicationProvider;
+import com.google.firebase.concurrent.TestOnlyExecutors;
+import com.google.firebase.crashlytics.internal.CrashlyticsWorker;
 import com.google.firebase.crashlytics.internal.metadata.LogFileManager;
 import com.google.firebase.crashlytics.internal.metadata.UserMetadata;
 import com.google.firebase.crashlytics.internal.model.CrashlyticsReport;
@@ -61,6 +63,8 @@ public class SessionReportingCoordinatorRobolectricTest {
 
   private SessionReportingCoordinator reportingCoordinator;
 
+  private CrashlyticsWorker diskWriteWorker = new CrashlyticsWorker(TestOnlyExecutors.background());
+
   @Before
   public void setUp() {
     MockitoAnnotations.initMocks(this);
@@ -72,7 +76,8 @@ public void setUp() {
             reportSender,
             logFileManager,
             reportMetadata,
-            idManager);
+            idManager,
+            diskWriteWorker);
     mockEventInteractions();
   }