Skip to content

User action refactor #6120

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 29, 2024
Merged

User action refactor #6120

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
bad1a13
user action refactor
themiswang Jul 24, 2024
d4580f4
fix test
themiswang Jul 26, 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
10 changes: 7 additions & 3 deletions ...dTest/java/com/google/firebase/crashlytics/internal/common/CrashlyticsControllerTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -74,6 +74,8 @@ public class CrashlyticsControllerTest extends CrashlyticsTestCase {
private DataCollectionArbiter mockDataCollectionArbiter;
private CrashlyticsNativeComponent mockNativeComponent = mock(CrashlyticsNativeComponent.class);

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

@Override
protected void setUp() throws Exception {
super.setUp();
Expand Down Expand Up @@ -186,7 +188,8 @@ public CrashlyticsController build() {
sessionReportingCoordinator,
nativeComponent,
analyticsEventLogger,
mock(CrashlyticsAppQualitySessionsSubscriber.class));
mock(CrashlyticsAppQualitySessionsSubscriber.class),
diskWriteWorker);
return controller;
}
}
Expand Down Expand Up @@ -376,13 +379,14 @@ public void testLoggedExceptionsAfterCrashOk() {
*/
// FIXME: Validate this test works as intended
@SdkSuppress(minSdkVersion = 30) // ApplicationExitInfo
public void testLogStringAfterCrashOk() {
public void testLogStringAfterCrashOk() throws Exception {
final CrashlyticsController controller = builder().build();
controller.handleUncaughtException(
testSettingsProvider, Thread.currentThread(), new RuntimeException());

// This should not throw.
controller.writeToLog(System.currentTimeMillis(), "Hi");
diskWriteWorker.submit(() -> controller.writeToLog(System.currentTimeMillis(), "Hi"));
diskWriteWorker.await();
}

/**
Expand Down
14 changes: 13 additions & 1 deletion ...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 @@ -93,7 +93,7 @@ public void testCustomAttributes() throws Exception {

final String id = "id012345";
crashlyticsCore.setUserId(id);

crashlyticsCore.commonWorker.await();
assertEquals(id, metadata.getUserId());

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

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

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

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

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

Expand All @@ -130,23 +133,28 @@ public void testCustomAttributes() throws Exception {
final String key = "key" + i;
final String value = "value" + i;
crashlyticsCore.setCustomKey(key, value);
crashlyticsCore.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();
assertFalse(metadata.getCustomKeys().containsKey(key));

// should be able to update existing keys
crashlyticsCore.setCustomKey(key1, longValue);
crashlyticsCore.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();
assertEquals("", metadata.getCustomKeys().get(key1));

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

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

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

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

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

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

assertEquals(updatedStringValue, metadata.getCustomKeys().get(stringKey));
assertFalse(Boolean.parseBoolean(metadata.getCustomKeys().get(booleanKey)));
Expand Down
47 changes: 18 additions & 29 deletions .../src/main/java/com/google/firebase/crashlytics/internal/common/CrashlyticsController.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -86,6 +86,8 @@ class CrashlyticsController {

private final CrashlyticsWorker backgroundWorker;

private final CrashlyticsWorker diskWriteWorker;

private final IdManager idManager;
private final FileStore fileStore;

Expand Down Expand Up @@ -129,7 +131,8 @@ class CrashlyticsController {
SessionReportingCoordinator sessionReportingCoordinator,
CrashlyticsNativeComponent nativeComponent,
AnalyticsEventLogger analyticsEventLogger,
CrashlyticsAppQualitySessionsSubscriber sessionsSubscriber) {
CrashlyticsAppQualitySessionsSubscriber sessionsSubscriber,
CrashlyticsWorker diskWriteWorker) {
this.context = context;
this.backgroundWorker = commonWorker;
this.idManager = idManager;
Expand All @@ -143,6 +146,7 @@ class CrashlyticsController {
this.analyticsEventLogger = analyticsEventLogger;
this.sessionsSubscriber = sessionsSubscriber;
this.reportingCoordinator = sessionReportingCoordinator;
this.diskWriteWorker = diskWriteWorker;
}

private Context getContext() {
Expand Down Expand Up @@ -418,16 +422,9 @@ public Task<Void> then(@Nullable Settings appSettingsData)

/** Log a timestamped string to the log file. */
void writeToLog(final long timestamp, final String msg) {
backgroundWorker.submit(
new Callable<Void>() {
@Override
public Void call() throws Exception {
if (!isHandlingException()) {
logFileManager.writeToLog(timestamp, msg);
}
return null;
}
});
if (!isHandlingException()) {
logFileManager.writeToLog(timestamp, msg);
}
}

/** Log a caught exception - write out Throwable as event section of protobuf */
Expand All @@ -436,23 +433,15 @@ void writeNonFatalException(@NonNull final Thread thread, @NonNull final Throwab
// rather than the time at which the task executes.
final long timestampMillis = System.currentTimeMillis();

backgroundWorker.submit(
new Runnable() {
@Override
public void run() {
if (!isHandlingException()) {
long timestampSeconds = getTimestampSeconds(timestampMillis);
final String currentSessionId = getCurrentSessionId();
if (currentSessionId == null) {
Logger.getLogger()
.w("Tried to write a non-fatal exception while no session was open.");
return;
}
reportingCoordinator.persistNonFatalEvent(
ex, thread, currentSessionId, timestampSeconds);
}
}
});
if (!isHandlingException()) {
long timestampSeconds = getTimestampSeconds(timestampMillis);
final String currentSessionId = getCurrentSessionId();
if (currentSessionId == null) {
Logger.getLogger().w("Tried to write a non-fatal exception while no session was open.");
return;
}
reportingCoordinator.persistNonFatalEvent(ex, thread, currentSessionId, timestampSeconds);
}
}

void logFatalException(Thread thread, Throwable ex) {
Expand Down Expand Up @@ -941,7 +930,7 @@ private void writeApplicationExitInfoEventIfRelevant(String sessionId) {
if (applicationExitInfoList.size() != 0) {
final LogFileManager relevantSessionLogManager = new LogFileManager(fileStore, sessionId);
final UserMetadata relevantUserMetadata =
UserMetadata.loadFromExistingSession(sessionId, fileStore, backgroundWorker);
UserMetadata.loadFromExistingSession(sessionId, fileStore, diskWriteWorker);
reportingCoordinator.persistRelevantAppExitInfoEvent(
sessionId, applicationExitInfoList, relevantSessionLogManager, relevantUserMetadata);
} else {
Expand Down
36 changes: 21 additions & 15 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;

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

// region Constructors

Expand Down Expand Up @@ -153,7 +153,7 @@ public boolean onPreExecute(AppData appData, SettingsProvider settingsProvider)
initializationMarker = new CrashlyticsFileMarker(INITIALIZATION_MARKER_FILE_NAME, fileStore);

final UserMetadata userMetadata =
new UserMetadata(sessionIdentifier, fileStore, commonWorker);
new UserMetadata(sessionIdentifier, fileStore, diskWriteWorker);
final LogFileManager logFileManager = new LogFileManager(fileStore);
final StackTraceTrimmingStrategy stackTraceTrimmingStrategy =
new MiddleOutFallbackStrategy(
Expand Down Expand Up @@ -188,7 +188,8 @@ public boolean onPreExecute(AppData appData, SettingsProvider settingsProvider)
sessionReportingCoordinator,
nativeComponent,
analyticsEventLogger,
sessionsSubscriber);
sessionsSubscriber,
diskWriteWorker);

// If the file is present at this point, then the previous run's initialization
// did not complete, and we want to perform initialization synchronously this time.
Expand Down Expand Up @@ -321,7 +322,7 @@ public static String getVersion() {
* safe to invoke this method from the main thread.
*/
public void logException(@NonNull Throwable throwable) {
controller.writeNonFatalException(Thread.currentThread(), throwable);
commonWorker.submit(() -> controller.writeNonFatalException(Thread.currentThread(), throwable));
}

/**
Expand All @@ -336,11 +337,11 @@ public void logException(@NonNull Throwable throwable) {
*/
public void log(final String msg) {
final long timestamp = System.currentTimeMillis() - startTime;
controller.writeToLog(timestamp, msg);
commonWorker.submit(() -> controller.writeToLog(timestamp, msg));
}

public void setUserId(String identifier) {
controller.setUserId(identifier);
commonWorker.submit(() -> controller.setUserId(identifier));
}

/**
Expand All @@ -353,7 +354,7 @@ public void setUserId(String identifier) {
* @throws NullPointerException if key is null.
*/
public void setCustomKey(String key, String value) {
controller.setCustomKey(key, value);
commonWorker.submit(() -> controller.setCustomKey(key, value));
}

/**
Expand All @@ -370,7 +371,7 @@ public void setCustomKey(String key, String value) {
* @throws NullPointerException if any key in keysAndValues is null.
*/
public void setCustomKeys(Map<String, String> keysAndValues) {
controller.setCustomKeys(keysAndValues);
commonWorker.submit(() -> controller.setCustomKeys(keysAndValues));
}

// endregion
Expand All @@ -390,7 +391,7 @@ public void setCustomKeys(Map<String, String> keysAndValues) {
* @throws NullPointerException if key is null.
*/
public void setInternalKey(String key, String value) {
controller.setInternalKey(key, value);
commonWorker.submit(() -> controller.setInternalKey(key, value));
}

/** Logs a fatal Throwable on the Crashlytics servers on-demand. */
Expand All @@ -399,11 +400,16 @@ public void logFatalException(Throwable throwable) {
.d("Recorded on-demand fatal events: " + onDemandCounter.getRecordedOnDemandExceptions());
Logger.getLogger()
.d("Dropped on-demand fatal events: " + onDemandCounter.getDroppedOnDemandExceptions());
controller.setInternalKey(
ON_DEMAND_RECORDED_KEY, Integer.toString(onDemandCounter.getRecordedOnDemandExceptions()));
controller.setInternalKey(
ON_DEMAND_DROPPED_KEY, Integer.toString(onDemandCounter.getDroppedOnDemandExceptions()));
controller.logFatalException(Thread.currentThread(), throwable);
commonWorker.submit(
() -> {
controller.setInternalKey(
ON_DEMAND_RECORDED_KEY,
Integer.toString(onDemandCounter.getRecordedOnDemandExceptions()));
controller.setInternalKey(
ON_DEMAND_DROPPED_KEY,
Integer.toString(onDemandCounter.getDroppedOnDemandExceptions()));
controller.logFatalException(Thread.currentThread(), throwable);
});
}

// endregion
Expand Down
Loading
Loading