Skip to content

Commit 7be8dec

Browse files
themiswangthemiswang
committed
Feature rollouts user id fix (#5292)
For on-demand fatal, we will need to close the previous session and open a new session during app life cycle. We need to port over the previous UserMetadata state (including user id, custom keys etc) for the new session. Decide to merge to master later as a part of feature rollouts project. Original PR: #5275
1 parent b865d8f commit 7be8dec

4 files changed

Lines changed: 120 additions & 8 deletions

File tree

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

Lines changed: 36 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -14,6 +14,7 @@
1414

1515
package com.google.firebase.crashlytics.internal.common;
1616

17+
import static org.mockito.AdditionalMatchers.not;
1718
import static org.mockito.Mockito.any;
1819
import static org.mockito.Mockito.anyLong;
1920
import static org.mockito.Mockito.anyString;
@@ -38,6 +39,7 @@
3839
import com.google.firebase.crashlytics.internal.NativeSessionFileProvider;
3940
import com.google.firebase.crashlytics.internal.analytics.AnalyticsEventLogger;
4041
import com.google.firebase.crashlytics.internal.metadata.LogFileManager;
42+
import com.google.firebase.crashlytics.internal.metadata.UserMetadata;
4143
import com.google.firebase.crashlytics.internal.model.CrashlyticsReport;
4244
import com.google.firebase.crashlytics.internal.persistence.FileStore;
4345
import com.google.firebase.crashlytics.internal.settings.Settings;
@@ -52,6 +54,7 @@
5254
import java.util.TreeSet;
5355
import java.util.concurrent.Executor;
5456
import java.util.concurrent.TimeUnit;
57+
import org.junit.Test;
5558
import org.mockito.ArgumentCaptor;
5659

5760
public class CrashlyticsControllerTest extends CrashlyticsTestCase {
@@ -101,22 +104,33 @@ private class ControllerBuilder {
101104
private CrashlyticsNativeComponent nativeComponent = null;
102105
private AnalyticsEventLogger analyticsEventLogger;
103106
private SessionReportingCoordinator sessionReportingCoordinator;
107+
108+
private CrashlyticsBackgroundWorker backgroundWorker;
104109
private LogFileManager logFileManager = null;
105110

111+
private UserMetadata userMetadata = null;
112+
106113
ControllerBuilder() {
107114
dataCollectionArbiter = mockDataCollectionArbiter;
108115
nativeComponent = mockNativeComponent;
109116

110117
analyticsEventLogger = mock(AnalyticsEventLogger.class);
111118

112119
sessionReportingCoordinator = mockSessionReportingCoordinator;
120+
121+
backgroundWorker = new CrashlyticsBackgroundWorker(new SameThreadExecutorService());
113122
}
114123

115124
ControllerBuilder setDataCollectionArbiter(DataCollectionArbiter arbiter) {
116125
dataCollectionArbiter = arbiter;
117126
return this;
118127
}
119128

129+
ControllerBuilder setUserMetadata(UserMetadata userMetadata) {
130+
this.userMetadata = userMetadata;
131+
return this;
132+
}
133+
120134
public ControllerBuilder setNativeComponent(CrashlyticsNativeComponent nativeComponent) {
121135
this.nativeComponent = nativeComponent;
122136
return this;
@@ -153,13 +167,13 @@ public CrashlyticsController build() {
153167
final CrashlyticsController controller =
154168
new CrashlyticsController(
155169
testContext.getApplicationContext(),
156-
new CrashlyticsBackgroundWorker(new SameThreadExecutorService()),
170+
backgroundWorker,
157171
idManager,
158172
dataCollectionArbiter,
159173
testFileStore,
160174
crashMarker,
161175
appData,
162-
null,
176+
userMetadata,
163177
logFileManager,
164178
sessionReportingCoordinator,
165179
nativeComponent,
@@ -211,6 +225,26 @@ public void testFatalException_callsSessionReportingCoordinatorPersistFatal() th
211225
.persistFatalEvent(eq(fatal), eq(thread), eq(sessionId), anyLong());
212226
}
213227

228+
@Test
229+
public void testOnDemandFatal_callLogFatalException() {
230+
Thread thread = Thread.currentThread();
231+
Exception fatal = new RuntimeException("Fatal");
232+
Thread.UncaughtExceptionHandler exceptionHandler = mock(Thread.UncaughtExceptionHandler.class);
233+
UserMetadata mockUserMetadata = mock(UserMetadata.class);
234+
when(mockSessionReportingCoordinator.listSortedOpenSessionIds())
235+
.thenReturn(new TreeSet<>(Collections.singleton(SESSION_ID)).descendingSet());
236+
237+
final CrashlyticsController controller =
238+
builder()
239+
.setLogFileManager(new LogFileManager(testFileStore))
240+
.setUserMetadata(mockUserMetadata)
241+
.build();
242+
controller.enableExceptionHandling(SESSION_ID, exceptionHandler, testSettingsProvider);
243+
controller.logFatalException(thread, fatal);
244+
245+
verify(mockUserMetadata).setNewSession(not(eq(SESSION_ID)));
246+
}
247+
214248
public void testNativeCrashDataCausesNativeReport() throws Exception {
215249
final String sessionId = "sessionId_1_new";
216250
final String previousSessionId = "sessionId_0_previous";

‎firebase-crashlytics/src/androidTest/java/com/google/firebase/crashlytics/internal/metadata/MetaDataStoreTest.java‎

Lines changed: 52 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -14,7 +14,8 @@
1414

1515
package com.google.firebase.crashlytics.internal.metadata;
1616

17-
import com.google.common.truth.Truth;
17+
import static com.google.common.truth.Truth.assertThat;
18+
1819
import com.google.firebase.crashlytics.internal.CrashlyticsTestCase;
1920
import com.google.firebase.crashlytics.internal.common.CrashlyticsBackgroundWorker;
2021
import com.google.firebase.crashlytics.internal.persistence.FileStore;
@@ -151,6 +152,55 @@ public void testReadUserData_noStoredData() {
151152
assertNull(userData.getUserId());
152153
}
153154

155+
@Test
156+
public void testUpdateSessionId_notPersistUserIdToNewSessionIfNoUserIdSet() {
157+
UserMetadata userMetadata = new UserMetadata(SESSION_ID_1, fileStore, worker);
158+
userMetadata.setNewSession(SESSION_ID_2);
159+
assertThat(fileStore.getSessionFile(SESSION_ID_2, UserMetadata.USERDATA_FILENAME).exists())
160+
.isFalse();
161+
}
162+
163+
@Test
164+
public void testUpdateSessionId_notPersistCustomKeysToNewSessionIfNoCustomKeysSet() {
165+
UserMetadata userMetadata = new UserMetadata(SESSION_ID_1, fileStore, worker);
166+
userMetadata.setNewSession(SESSION_ID_2);
167+
assertThat(fileStore.getSessionFile(SESSION_ID_2, UserMetadata.KEYDATA_FILENAME).exists())
168+
.isFalse();
169+
}
170+
171+
@Test
172+
public void testUpdateSessionId_persistCustomKeysToNewSessionIfCustomKeysSet() {
173+
UserMetadata userMetadata = new UserMetadata(SESSION_ID_1, fileStore, worker);
174+
final Map<String, String> keys =
175+
new HashMap<String, String>() {
176+
{
177+
put(KEY_1, VALUE_1);
178+
put(KEY_2, VALUE_2);
179+
put(KEY_3, VALUE_3);
180+
}
181+
};
182+
userMetadata.setCustomKeys(keys);
183+
userMetadata.setNewSession(SESSION_ID_2);
184+
assertThat(fileStore.getSessionFile(SESSION_ID_2, UserMetadata.KEYDATA_FILENAME).exists())
185+
.isTrue();
186+
187+
MetaDataStore metaDataStore = new MetaDataStore(fileStore);
188+
assertThat(metaDataStore.readKeyData(SESSION_ID_2)).isEqualTo(keys);
189+
}
190+
191+
@Test
192+
public void testUpdateSessionId_persistUserIdToNewSessionIfUserIdSet() {
193+
String userId = "ThemisWang";
194+
UserMetadata userMetadata = new UserMetadata(SESSION_ID_1, fileStore, worker);
195+
userMetadata.setUserId(userId);
196+
userMetadata.setNewSession(SESSION_ID_2);
197+
assertThat(fileStore.getSessionFile(SESSION_ID_2, UserMetadata.USERDATA_FILENAME).exists())
198+
.isTrue();
199+
200+
MetaDataStore metaDataStore = new MetaDataStore(fileStore);
201+
assertThat(metaDataStore.readUserId(SESSION_ID_2)).isEqualTo(userId);
202+
}
203+
154204
// Keys
155205

156206
public void testWriteKeys() {
@@ -299,7 +349,7 @@ public void testWriteReadRolloutState() throws Exception {
299349
storeUnderTest.writeRolloutState(SESSION_ID_1, ROLLOUTS_STATE);
300350
List<RolloutAssignment> readRolloutsState = storeUnderTest.readRolloutsState(SESSION_ID_1);
301351

302-
Truth.assertThat(readRolloutsState).isEqualTo(ROLLOUTS_STATE);
352+
assertThat(readRolloutsState).isEqualTo(ROLLOUTS_STATE);
303353
}
304354

305355
public static void assertEqualMaps(Map<String, String> expected, Map<String, String> actual) {

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

Lines changed: 11 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -216,7 +216,7 @@ public Task<Void> call() throws Exception {
216216

217217
doWriteAppExceptionMarker(timestampMillis);
218218
doCloseSessions(settingsProvider);
219-
doOpenSession(new CLSUUID(idManager).toString());
219+
doOpenSession(new CLSUUID(idManager).toString(), isOnDemand);
220220

221221
// If automatic data collection is disabled, we'll need to wait until the next run
222222
// of the app.
@@ -498,7 +498,7 @@ void openSession(String sessionIdentifier) {
498498
new Callable<Void>() {
499499
@Override
500500
public Void call() throws Exception {
501-
doOpenSession(sessionIdentifier);
501+
doOpenSession(sessionIdentifier, /*isOnDemand=*/ false);
502502
return null;
503503
}
504504
});
@@ -550,7 +550,7 @@ boolean finalizeSessions(SettingsProvider settingsProvider) {
550550
* Not synchronized/locked. Must be executed from the single thread executor service used by this
551551
* class.
552552
*/
553-
private void doOpenSession(String sessionIdentifier) {
553+
private void doOpenSession(String sessionIdentifier, Boolean isOnDemand) {
554554
final long startedAtSeconds = getCurrentTimestampSeconds();
555555

556556
Logger.getLogger().d("Opening a new session with ID " + sessionIdentifier);
@@ -568,6 +568,14 @@ private void doOpenSession(String sessionIdentifier) {
568568
startedAtSeconds,
569569
StaticSessionData.create(appData, osData, deviceData));
570570

571+
// If is on-demand fatal, we need to update the session id for userMetadata
572+
// as well(since we don't really change the object to a new one for a new session).
573+
// all the information in the previous session is still in memory, but we do need to
574+
// manually writing them into persistence for the new session.
575+
if (isOnDemand && sessionIdentifier != null) {
576+
userMetadata.setNewSession(sessionIdentifier);
577+
}
578+
571579
logFileManager.setCurrentSession(sessionIdentifier);
572580
sessionsSubscriber.setSessionId(sessionIdentifier);
573581
reportingCoordinator.onBeginSession(sessionIdentifier, startedAtSeconds);

‎firebase-crashlytics/src/main/java/com/google/firebase/crashlytics/internal/metadata/UserMetadata.java‎

Lines changed: 21 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -47,7 +47,7 @@ public class UserMetadata {
4747

4848
private final MetaDataStore metaDataStore;
4949
private final CrashlyticsBackgroundWorker backgroundWorker;
50-
private final String sessionIdentifier;
50+
private String sessionIdentifier;
5151

5252
// The following references contain a marker bit, which is true if the data maintained in the
5353
// associated reference has been serialized since the last time it was updated.
@@ -87,6 +87,26 @@ public UserMetadata(
8787
this.backgroundWorker = backgroundWorker;
8888
}
8989

90+
/**
91+
* Refresh the userMetadata to reflect the status of the new session. This API is mainly for
92+
* on-demand fatal feature since we need to close and update to a new session. UserMetadata also
93+
* need to make this update instead of updating session id, we also need to manually writing the
94+
* into persistence for the new session.
95+
*/
96+
public void setNewSession(String sessionId) {
97+
synchronized (sessionIdentifier) {
98+
sessionIdentifier = sessionId;
99+
Map<String, String> keyData = customKeys.getKeys();
100+
if (getUserId() != null) {
101+
metaDataStore.writeUserData(sessionId, getUserId());
102+
}
103+
if (!keyData.isEmpty()) {
104+
metaDataStore.writeKeyData(sessionId, keyData);
105+
}
106+
// TODO(themis): adding feature rollouts later
107+
}
108+
}
109+
90110
@Nullable
91111
public String getUserId() {
92112
return userId.getReference();

0 commit comments

Comments
 (0)