Skip to content

Commit a94ddbf

Browse files
mrobermrober
committed
Revert "Move log and non-fatal persistence to disk write worker (#6133)"
This reverts commit 10fe142.
1 parent b4af47d commit a94ddbf

File tree

6 files changed

+131
-89
lines changed

6 files changed

+131
-89
lines changed

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

Lines changed: 15 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -70,10 +70,6 @@ public void whenAvailable(
7070

7171
private CrashlyticsCore crashlyticsCore;
7272
private BreadcrumbSource mockBreadcrumbSource;
73-
private static final CrashlyticsWorker commonWorker =
74-
new CrashlyticsWorker(TestOnlyExecutors.background());
75-
private static final CrashlyticsWorker diskWriteWorker =
76-
new CrashlyticsWorker(TestOnlyExecutors.background());
7773

7874
@Override
7975
protected void setUp() throws Exception {
@@ -97,7 +93,7 @@ public void testCustomAttributes() throws Exception {
9793

9894
final String id = "id012345";
9995
crashlyticsCore.setUserId(id);
100-
commonWorker.await();
96+
crashlyticsCore.commonWorker.await();
10197
assertEquals(id, metadata.getUserId());
10298

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

110106
crashlyticsCore.setUserId(superLongId);
111-
commonWorker.await();
107+
crashlyticsCore.commonWorker.await();
112108
assertEquals(longId, metadata.getUserId());
113109

114110
final String key1 = "key1";
115111
final String value1 = "value1";
116112
crashlyticsCore.setCustomKey(key1, value1);
117-
commonWorker.await();
113+
crashlyticsCore.commonWorker.await();
118114
assertEquals(value1, metadata.getCustomKeys().get(key1));
119115

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

129125
// test truncation of custom keys and attributes
130126
crashlyticsCore.setCustomKey(superLongId, superLongValue);
131-
commonWorker.await();
127+
crashlyticsCore.commonWorker.await();
132128
assertNull(metadata.getCustomKeys().get(superLongId));
133129
assertEquals(longValue, metadata.getCustomKeys().get(longId));
134130

@@ -137,28 +133,28 @@ public void testCustomAttributes() throws Exception {
137133
final String key = "key" + i;
138134
final String value = "value" + i;
139135
crashlyticsCore.setCustomKey(key, value);
140-
commonWorker.await();
136+
crashlyticsCore.commonWorker.await();
141137
assertEquals(value, metadata.getCustomKeys().get(key));
142138
}
143139
// should be full now, extra key, value pairs will be dropped.
144140
final String key = "new key";
145141
crashlyticsCore.setCustomKey(key, "some value");
146-
commonWorker.await();
142+
crashlyticsCore.commonWorker.await();
147143
assertFalse(metadata.getCustomKeys().containsKey(key));
148144

149145
// should be able to update existing keys
150146
crashlyticsCore.setCustomKey(key1, longValue);
151-
commonWorker.await();
147+
crashlyticsCore.commonWorker.await();
152148
assertEquals(longValue, metadata.getCustomKeys().get(key1));
153149

154150
// when we set a key to null, it should still exist with an empty value
155151
crashlyticsCore.setCustomKey(key1, null);
156-
commonWorker.await();
152+
crashlyticsCore.commonWorker.await();
157153
assertEquals("", metadata.getCustomKeys().get(key1));
158154

159155
// keys and values are trimmed.
160156
crashlyticsCore.setCustomKey(" " + key1 + " ", " " + longValue + " ");
161-
commonWorker.await();
157+
crashlyticsCore.commonWorker.await();
162158
assertTrue(metadata.getCustomKeys().containsKey(key1));
163159
assertEquals(longValue, metadata.getCustomKeys().get(key1));
164160
}
@@ -209,7 +205,7 @@ public void testBulkCustomKeys() throws Exception {
209205
keysAndValues.put(intKey, String.valueOf(intValue));
210206

211207
crashlyticsCore.setCustomKeys(keysAndValues);
212-
commonWorker.await();
208+
crashlyticsCore.commonWorker.await();
213209

214210
assertEquals(stringValue, metadata.getCustomKeys().get(stringKey));
215211
assertEquals(trimmedValue, metadata.getCustomKeys().get(trimmedKey));
@@ -230,7 +226,7 @@ public void testBulkCustomKeys() throws Exception {
230226
addlKeysAndValues.put(key, value);
231227
}
232228
crashlyticsCore.setCustomKeys(addlKeysAndValues);
233-
commonWorker.await();
229+
crashlyticsCore.commonWorker.await();
234230

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

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

276272
crashlyticsCore.setCustomKeys(updatedKeysAndValues);
277-
commonWorker.await();
273+
crashlyticsCore.commonWorker.await();
278274

279275
assertEquals(updatedStringValue, metadata.getCustomKeys().get(stringKey));
280276
assertFalse(Boolean.parseBoolean(metadata.getCustomKeys().get(booleanKey)));
@@ -447,8 +443,8 @@ CrashlyticsCore build(Context context) {
447443
new FileStore(context),
448444
mock(CrashlyticsAppQualitySessionsSubscriber.class),
449445
mock(RemoteConfigDeferredProxy.class),
450-
commonWorker,
451-
diskWriteWorker);
446+
new CrashlyticsWorker(TestOnlyExecutors.background()),
447+
new CrashlyticsWorker(TestOnlyExecutors.background()));
452448
return crashlyticsCore;
453449
}
454450
}

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

Lines changed: 36 additions & 25 deletions
Original file line numberDiff line numberDiff line change
@@ -33,8 +33,6 @@
3333

3434
import com.google.android.gms.tasks.Task;
3535
import com.google.android.gms.tasks.Tasks;
36-
import com.google.firebase.concurrent.TestOnlyExecutors;
37-
import com.google.firebase.crashlytics.internal.CrashlyticsWorker;
3836
import com.google.firebase.crashlytics.internal.metadata.LogFileManager;
3937
import com.google.firebase.crashlytics.internal.metadata.UserMetadata;
4038
import com.google.firebase.crashlytics.internal.model.CrashlyticsReport;
@@ -71,8 +69,6 @@ public class SessionReportingCoordinatorTest {
7169

7270
private SessionReportingCoordinator reportingCoordinator;
7371

74-
private CrashlyticsWorker diskWriteWorker = new CrashlyticsWorker(TestOnlyExecutors.background());
75-
7672
@Before
7773
public void setUp() {
7874
MockitoAnnotations.initMocks(this);
@@ -84,8 +80,7 @@ public void setUp() {
8480
reportSender,
8581
logFileManager,
8682
reportMetadata,
87-
idManager,
88-
diskWriteWorker);
83+
idManager);
8984
}
9085

9186
@Test
@@ -121,8 +116,7 @@ public void testFatalEvent_persistsHighPriorityEventWithAllThreadsForSessionId()
121116
}
122117

123118
@Test
124-
public void testNonFatalEvent_persistsNormalPriorityEventWithoutAllThreadsForSessionId()
125-
throws Exception {
119+
public void testNonFatalEvent_persistsNormalPriorityEventWithoutAllThreadsForSessionId() {
126120
final String eventType = "error";
127121
final String sessionId = "testSessionId";
128122
final long timestamp = System.currentTimeMillis();
@@ -132,8 +126,6 @@ public void testNonFatalEvent_persistsNormalPriorityEventWithoutAllThreadsForSes
132126
reportingCoordinator.onBeginSession(sessionId, timestamp);
133127
reportingCoordinator.persistNonFatalEvent(mockException, mockThread, sessionId, timestamp);
134128

135-
diskWriteWorker.await();
136-
137129
final boolean expectedAllThreads = false;
138130
final boolean expectedHighPriority = false;
139131

@@ -144,7 +136,7 @@ public void testNonFatalEvent_persistsNormalPriorityEventWithoutAllThreadsForSes
144136
}
145137

146138
@Test
147-
public void testNonFatalEvent_addsLogsToEvent() throws Exception {
139+
public void testNonFatalEvent_addsLogsToEvent() {
148140
long timestamp = System.currentTimeMillis();
149141

150142
mockEventInteractions();
@@ -157,16 +149,14 @@ public void testNonFatalEvent_addsLogsToEvent() throws Exception {
157149
reportingCoordinator.onBeginSession(sessionId, timestamp);
158150
reportingCoordinator.persistNonFatalEvent(mockException, mockThread, sessionId, timestamp);
159151

160-
diskWriteWorker.await();
161-
162152
verify(mockEventBuilder)
163153
.setLog(CrashlyticsReport.Session.Event.Log.builder().setContent(testLog).build());
164154
verify(mockEventBuilder).build();
165155
verify(logFileManager, never()).clearLog();
166156
}
167157

168158
@Test
169-
public void testNonFatalEvent_addsNoLogsToEventWhenNoneAvailable() throws Exception {
159+
public void testNonFatalEvent_addsNoLogsToEventWhenNoneAvailable() {
170160
long timestamp = System.currentTimeMillis();
171161

172162
mockEventInteractions();
@@ -178,8 +168,6 @@ public void testNonFatalEvent_addsNoLogsToEventWhenNoneAvailable() throws Except
178168
reportingCoordinator.onBeginSession(sessionId, timestamp);
179169
reportingCoordinator.persistNonFatalEvent(mockException, mockThread, sessionId, timestamp);
180170

181-
diskWriteWorker.await();
182-
183171
verify(mockEventBuilder, never()).setLog(any(CrashlyticsReport.Session.Event.Log.class));
184172
verify(mockEventBuilder).build();
185173
verify(logFileManager, never()).clearLog();
@@ -224,7 +212,7 @@ public void testFatalEvent_addsNoLogsToEventWhenNoneAvailable() {
224212
}
225213

226214
@Test
227-
public void testNonFatalEvent_addsSortedKeysToEvent() throws Exception {
215+
public void testNonFatalEvent_addsSortedKeysToEvent() {
228216
final long timestamp = System.currentTimeMillis();
229217

230218
mockEventInteractions();
@@ -255,8 +243,6 @@ public void testNonFatalEvent_addsSortedKeysToEvent() throws Exception {
255243
reportingCoordinator.onBeginSession(sessionId, timestamp);
256244
reportingCoordinator.persistNonFatalEvent(mockException, mockThread, sessionId, timestamp);
257245

258-
diskWriteWorker.await();
259-
260246
verify(mockEventAppBuilder).setCustomAttributes(expectedCustomAttributes);
261247
verify(mockEventAppBuilder).setInternalKeys(expectedCustomAttributes);
262248
verify(mockEventAppBuilder).build();
@@ -266,7 +252,7 @@ public void testNonFatalEvent_addsSortedKeysToEvent() throws Exception {
266252
}
267253

268254
@Test
269-
public void testNonFatalEvent_addsNoKeysToEventWhenNoneAvailable() throws Exception {
255+
public void testNonFatalEvent_addsNoKeysToEventWhenNoneAvailable() {
270256
final long timestamp = System.currentTimeMillis();
271257

272258
mockEventInteractions();
@@ -280,8 +266,6 @@ public void testNonFatalEvent_addsNoKeysToEventWhenNoneAvailable() throws Except
280266
reportingCoordinator.onBeginSession(sessionId, timestamp);
281267
reportingCoordinator.persistNonFatalEvent(mockException, mockThread, sessionId, timestamp);
282268

283-
diskWriteWorker.await();
284-
285269
verify(mockEventAppBuilder, never()).setCustomAttributes(anyList());
286270
verify(mockEventAppBuilder, never()).build();
287271
verify(mockEventBuilder, never()).setApp(mockEventApp);
@@ -290,7 +274,7 @@ public void testNonFatalEvent_addsNoKeysToEventWhenNoneAvailable() throws Except
290274
}
291275

292276
@Test
293-
public void testNonFatalEvent_addRolloutsEvent() throws Exception {
277+
public void testNonFatalEvent_addRolloutsEvent() {
294278
long timestamp = System.currentTimeMillis();
295279
String sessionId = "testSessionId";
296280
mockEventInteractions();
@@ -303,8 +287,6 @@ public void testNonFatalEvent_addRolloutsEvent() throws Exception {
303287
reportingCoordinator.onBeginSession(sessionId, timestamp);
304288
reportingCoordinator.persistNonFatalEvent(mockException, mockThread, sessionId, timestamp);
305289

306-
diskWriteWorker.await();
307-
308290
verify(mockEventAppBuilder, never()).setCustomAttributes(anyList());
309291
verify(mockEventAppBuilder, never()).build();
310292
verify(mockEventBuilder, never()).setApp(mockEventApp);
@@ -435,6 +417,35 @@ public void testFatalEvent_addRolloutsToEvent() {
435417
verify(mockEventBuilder, times(2)).build();
436418
}
437419

420+
@Test
421+
public void onLog_writesToLogFileManager() {
422+
long timestamp = System.currentTimeMillis();
423+
String log = "this is a log";
424+
425+
reportingCoordinator.onLog(timestamp, log);
426+
427+
verify(logFileManager).writeToLog(timestamp, log);
428+
}
429+
430+
@Test
431+
public void onCustomKey_writesToReportMetadata() {
432+
final String key = "key";
433+
final String value = "value";
434+
435+
reportingCoordinator.onCustomKey(key, value);
436+
437+
verify(reportMetadata).setCustomKey(key, value);
438+
}
439+
440+
@Test
441+
public void onUserId_writesUserToReportMetadata() {
442+
final String userId = "testUser";
443+
444+
reportingCoordinator.onUserId(userId);
445+
446+
verify(reportMetadata).setUserId(userId);
447+
}
448+
438449
@Test
439450
public void testFinalizeSessionWithNativeEvent_createsCrashlyticsReportWithNativePayload() {
440451
byte[] testBytes = {0, 2, 20, 10};

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

Lines changed: 4 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -99,8 +99,8 @@ public class CrashlyticsCore {
9999

100100
private final RemoteConfigDeferredProxy remoteConfigDeferredProxy;
101101

102-
private final CrashlyticsWorker commonWorker;
103-
private final CrashlyticsWorker diskWriteWorker;
102+
@VisibleForTesting final CrashlyticsWorker commonWorker;
103+
@VisibleForTesting final CrashlyticsWorker diskWriteWorker;
104104

105105
// region Constructors
106106

@@ -175,8 +175,7 @@ public boolean onPreExecute(AppData appData, SettingsProvider settingsProvider)
175175
stackTraceTrimmingStrategy,
176176
settingsProvider,
177177
onDemandCounter,
178-
sessionsSubscriber,
179-
diskWriteWorker);
178+
sessionsSubscriber);
180179

181180
controller =
182181
new CrashlyticsController(
@@ -341,11 +340,7 @@ public void logException(@NonNull Throwable throwable) {
341340
*/
342341
public void log(final String msg) {
343342
final long timestamp = System.currentTimeMillis() - startTime;
344-
// queuing up on common worker to maintain the order
345-
commonWorker.submit(
346-
() -> {
347-
diskWriteWorker.submit(() -> controller.writeToLog(timestamp, msg));
348-
});
343+
commonWorker.submit(() -> controller.writeToLog(timestamp, msg));
349344
}
350345

351346
public void setUserId(String identifier) {

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

Lines changed: 51 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,51 @@
1+
// Copyright 2020 Google LLC
2+
//
3+
// Licensed under the Apache License, Version 2.0 (the "License");
4+
// you may not use this file except in compliance with the License.
5+
// You may obtain a copy of the License at
6+
//
7+
// http://www.apache.org/licenses/LICENSE-2.0
8+
//
9+
// Unless required by applicable law or agreed to in writing, software
10+
// distributed under the License is distributed on an "AS IS" BASIS,
11+
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
12+
// See the License for the specific language governing permissions and
13+
// limitations under the License.
14+
15+
package com.google.firebase.crashlytics.internal.common;
16+
17+
import androidx.annotation.NonNull;
18+
19+
/** This class defines Crashlytics lifecycle events */
20+
interface CrashlyticsLifecycleEvents {
21+
22+
/**
23+
* Called when a new session is opened.
24+
*
25+
* @param sessionId the identifier for the new session
26+
*/
27+
void onBeginSession(@NonNull String sessionId, long timestamp);
28+
29+
/**
30+
* Called when a message is logged by the user.
31+
*
32+
* @param timestamp the timestamp of the message (in milliseconds since app launch)
33+
* @param log the log message
34+
*/
35+
void onLog(long timestamp, String log);
36+
37+
/**
38+
* Called when a custom key is set by the user.
39+
*
40+
* @param key
41+
* @param value
42+
*/
43+
void onCustomKey(String key, String value);
44+
45+
/**
46+
* Called when a user ID is set by the user.
47+
*
48+
* @param userId
49+
*/
50+
void onUserId(String userId);
51+
}

0 commit comments

Comments
 (0)