Skip to content

Move log and non-fatal persistence to disk write worker #6133

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.

Sign up for GitHub

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 2 commits into from
Jul 30, 2024
Merged

Move log and non-fatal persistence to disk write worker #6133

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
2f347a1
move log and non-fatal presistence to disk write worker
themiswang Jul 29, 2024
6ef3aa9
address comments
themiswang Jul 30, 2024
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
34 changes: 19 additions & 15 deletions ...androidTest/java/com/google/firebase/crashlytics/internal/common/CrashlyticsCoreTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -70,6 +70,10 @@ public void whenAvailable(

private CrashlyticsCore crashlyticsCore;
private BreadcrumbSource mockBreadcrumbSource;
private static final CrashlyticsWorker commonWorker =
new CrashlyticsWorker(TestOnlyExecutors.background());
private static final CrashlyticsWorker diskWriteWorker =
new CrashlyticsWorker(TestOnlyExecutors.background());

@Override
protected void setUp() throws Exception {
Expand All @@ -93,7 +97,7 @@ public void testCustomAttributes() throws Exception {

final String id = "id012345";
crashlyticsCore.setUserId(id);
crashlyticsCore.commonWorker.await();
commonWorker.await();
assertEquals(id, metadata.getUserId());

final StringBuffer idBuffer = new StringBuffer(id);
Expand All @@ -104,13 +108,13 @@ public void testCustomAttributes() throws Exception {
final String superLongId = longId + "more chars";

crashlyticsCore.setUserId(superLongId);
crashlyticsCore.commonWorker.await();
commonWorker.await();
assertEquals(longId, metadata.getUserId());

final String key1 = "key1";
final String value1 = "value1";
crashlyticsCore.setCustomKey(key1, value1);
crashlyticsCore.commonWorker.await();
commonWorker.await();
assertEquals(value1, metadata.getCustomKeys().get(key1));

// Adding an existing key with the same value should return false
Expand All @@ -124,7 +128,7 @@ public void testCustomAttributes() throws Exception {

// test truncation of custom keys and attributes
crashlyticsCore.setCustomKey(superLongId, superLongValue);
crashlyticsCore.commonWorker.await();
commonWorker.await();
assertNull(metadata.getCustomKeys().get(superLongId));
assertEquals(longValue, metadata.getCustomKeys().get(longId));

Expand All @@ -133,28 +137,28 @@ public void testCustomAttributes() throws Exception {
final String key = "key" + i;
final String value = "value" + i;
crashlyticsCore.setCustomKey(key, value);
crashlyticsCore.commonWorker.await();
commonWorker.await();
assertEquals(value, metadata.getCustomKeys().get(key));
}
// should be full now, extra key, value pairs will be dropped.
final String key = "new key";
crashlyticsCore.setCustomKey(key, "some value");
crashlyticsCore.commonWorker.await();
commonWorker.await();
assertFalse(metadata.getCustomKeys().containsKey(key));

// should be able to update existing keys
crashlyticsCore.setCustomKey(key1, longValue);
crashlyticsCore.commonWorker.await();
commonWorker.await();
assertEquals(longValue, metadata.getCustomKeys().get(key1));

// when we set a key to null, it should still exist with an empty value
crashlyticsCore.setCustomKey(key1, null);
crashlyticsCore.commonWorker.await();
commonWorker.await();
assertEquals("", metadata.getCustomKeys().get(key1));

// keys and values are trimmed.
crashlyticsCore.setCustomKey(" " + key1 + " ", " " + longValue + " ");
crashlyticsCore.commonWorker.await();
commonWorker.await();
assertTrue(metadata.getCustomKeys().containsKey(key1));
assertEquals(longValue, metadata.getCustomKeys().get(key1));
}
Expand Down Expand Up @@ -205,7 +209,7 @@ public void testBulkCustomKeys() throws Exception {
keysAndValues.put(intKey, String.valueOf(intValue));

crashlyticsCore.setCustomKeys(keysAndValues);
crashlyticsCore.commonWorker.await();
commonWorker.await();

assertEquals(stringValue, metadata.getCustomKeys().get(stringKey));
assertEquals(trimmedValue, metadata.getCustomKeys().get(trimmedKey));
Expand All @@ -226,7 +230,7 @@ public void testBulkCustomKeys() throws Exception {
addlKeysAndValues.put(key, value);
}
crashlyticsCore.setCustomKeys(addlKeysAndValues);
crashlyticsCore.commonWorker.await();
commonWorker.await();

// Ensure all keys have been set
assertEquals(UserMetadata.MAX_ATTRIBUTES, metadata.getCustomKeys().size(), DELTA);
Expand All @@ -244,7 +248,7 @@ public void testBulkCustomKeys() throws Exception {
extraKeysAndValues.put(key, value);
}
crashlyticsCore.setCustomKeys(extraKeysAndValues);
crashlyticsCore.commonWorker.await();
commonWorker.await();

// Make sure the extra keys were not added
for (int i = UserMetadata.MAX_ATTRIBUTES; i < UserMetadata.MAX_ATTRIBUTES + 10; ++i) {
Expand All @@ -270,7 +274,7 @@ public void testBulkCustomKeys() throws Exception {
updatedKeysAndValues.put(intKey, String.valueOf(updatedIntValue));

crashlyticsCore.setCustomKeys(updatedKeysAndValues);
crashlyticsCore.commonWorker.await();
commonWorker.await();

assertEquals(updatedStringValue, metadata.getCustomKeys().get(stringKey));
assertFalse(Boolean.parseBoolean(metadata.getCustomKeys().get(booleanKey)));
Expand Down Expand Up @@ -443,8 +447,8 @@ CrashlyticsCore build(Context context) {
new FileStore(context),
mock(CrashlyticsAppQualitySessionsSubscriber.class),
mock(RemoteConfigDeferredProxy.class),
new CrashlyticsWorker(TestOnlyExecutors.background()),
new CrashlyticsWorker(TestOnlyExecutors.background()));
commonWorker,
diskWriteWorker);
return crashlyticsCore;
}
}
Expand Down
61 changes: 25 additions & 36 deletions ...java/com/google/firebase/crashlytics/internal/common/SessionReportingCoordinatorTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -69,6 +71,8 @@ public class SessionReportingCoordinatorTest {

private SessionReportingCoordinator reportingCoordinator;

private CrashlyticsWorker diskWriteWorker = new CrashlyticsWorker(TestOnlyExecutors.background());

@Before
public void setUp() {
MockitoAnnotations.initMocks(this);
Expand All @@ -80,7 +84,8 @@ public void setUp() {
reportSender,
logFileManager,
reportMetadata,
idManager);
idManager,
diskWriteWorker);
}

@Test
Expand Down Expand Up @@ -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();
Expand All @@ -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;

Expand All @@ -136,7 +144,7 @@ public void testNonFatalEvent_persistsNormalPriorityEventWithoutAllThreadsForSes
}

@Test
public void testNonFatalEvent_addsLogsToEvent() {
public void testNonFatalEvent_addsLogsToEvent() throws Exception {
long timestamp = System.currentTimeMillis();

mockEventInteractions();
Expand All @@ -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();
Expand All @@ -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();
Expand Down Expand Up @@ -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();
Expand Down Expand Up @@ -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();
Expand All @@ -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();
Expand All @@ -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);
Expand All @@ -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();
Expand All @@ -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);
Expand Down Expand Up @@ -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};
Expand Down
13 changes: 9 additions & 4 deletions ...lytics/src/main/java/com/google/firebase/crashlytics/internal/common/CrashlyticsCore.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -96,8 +96,8 @@ public class CrashlyticsCore {

private final RemoteConfigDeferredProxy remoteConfigDeferredProxy;

@VisibleForTesting final CrashlyticsWorker commonWorker;
@VisibleForTesting final CrashlyticsWorker diskWriteWorker;
private final CrashlyticsWorker commonWorker;
private final CrashlyticsWorker diskWriteWorker;

// region Constructors

Expand Down Expand Up @@ -172,7 +172,8 @@ public boolean onPreExecute(AppData appData, SettingsProvider settingsProvider)
stackTraceTrimmingStrategy,
settingsProvider,
onDemandCounter,
sessionsSubscriber);
sessionsSubscriber,
diskWriteWorker);

controller =
new CrashlyticsController(
Expand Down Expand Up @@ -337,7 +338,11 @@ public void logException(@NonNull Throwable throwable) {
*/
public void log(final String msg) {
final long timestamp = System.currentTimeMillis() - startTime;
commonWorker.submit(() -> controller.writeToLog(timestamp, msg));
// queuing up on common worker to maintain the order
commonWorker.submit(
() -> {
diskWriteWorker.submit(() -> controller.writeToLog(timestamp, msg));
});
}

public void setUserId(String identifier) {
Expand Down
51 changes: 0 additions & 51 deletions ...main/java/com/google/firebase/crashlytics/internal/common/CrashlyticsLifecycleEvents.java
View file
Open in desktop

This file was deleted.

Loading
Loading