Skip to content

Fix logs, breadcrumbs and custom keys for ANRs #2718

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
tejasd merged 62 commits into from
Jun 23, 2021
Merged

Fix logs, breadcrumbs and custom keys for ANRs #2718

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
62 commits
Select commit Hold shift + click to select a range
90322b3
Add collectAnrs to FeatureSettings
tejasd Apr 13, 2021
4e186a6
Add unit test to test settings received when anr collection is enabled
tejasd Apr 13, 2021
62e8c18
Cleanup json file
tejasd Apr 13, 2021
eb08e6b
Add ability to get the startTimestampMs of a session
tejasd Apr 14, 2021
ec3e5c9
Merge branch 'crashlytics/anrs' into td/anr-2
tejasd Apr 14, 2021
c5f5f26
Change Long to long
tejasd Apr 14, 2021
4d68ae4
Merge branch 'crashlytics/anrs' into td/anr
tejasd Apr 20, 2021
0422970
Add ApplicationExitInfo to CrashlyticsReport
tejasd Apr 21, 2021
b928e3f
Comment cleanup
tejasd Apr 21, 2021
20b4f30
Revert makeEvents refactoring
tejasd Apr 21, 2021
2c8f5a9
More cleanup
tejasd Apr 21, 2021
b0f9cc4
Merge branch 'crashlytics/anrs' into td/anr
tejasd Apr 22, 2021
3a56029
Merge branch 'crashlytics/anrs' into td/anr
tejasd Apr 23, 2021
3452ee4
Merge branch 'crashlytics/anrs' into td/anr-2
tejasd Apr 23, 2021
d1eeea8
AppExitInfo json transform
tejasd Apr 26, 2021
490733e
Merge branch 'crashlytics/anrs' into td/anr-2
tejasd Apr 26, 2021
5bb61f5
Merge branch 'crashlytics/anrs' into td/anr
tejasd Apr 26, 2021
1047983
AppExitInfo json transform
tejasd Apr 26, 2021
47511bc
Merge branch 'crashlytics/anrs' into td/anr-2
tejasd Apr 28, 2021
76f8438
Merge branch 'td/anr-2' into td/anr
tejasd Apr 28, 2021
2e17dee
Collect and Persist ANRs + robolectric tests fpr CrashlyticsControlle…
tejasd May 4, 2021
cb840c3
Add missing feature to read an app exit info when finalizing report +…
tejasd May 4, 2021
c780da6
Fix NPE and add relevant unit test
tejasd May 4, 2021
6c51f32
Add copyright to new test classes
tejasd May 4, 2021
91cda29
Merge branch 'crashlytics/anrs' into td/anr
tejasd May 11, 2021
dbc72dc
Merge branch 'crashlytics/anrs' into td/anr
tejasd May 14, 2021
b27e040
Add ApplicationExitInfo to Event.Application.Executor and make Thread…
tejasd May 14, 2021
bac6dcd
Merge branch 'crashlytics/anrs' into td/anr
tejasd May 14, 2021
08794f0
Refactor the persistence of AppExitInfo to move it to the Event
tejasd May 14, 2021
a085edc
Some cleanup
tejasd May 14, 2021
e069f72
Some cleanup + additional unit test
tejasd May 17, 2021
012d806
Try to fix tests
tejasd May 25, 2021
6a2dc78
Fix a broken unit test
tejasd May 25, 2021
2810b7b
Merge branch 'crashlytics/anrs' into td/anr
tejasd May 25, 2021
6a391ec
Merge branch 'crashlytics/anrs' into td/anr
tejasd May 26, 2021
8bad7a4
Revert Crashlytics androidTest manifest
tejasd May 27, 2021
eeb550b
Revert Crashlytics androidTest manifest
tejasd May 27, 2021
dbe533a
java format
tejasd May 27, 2021
d603cba
Clean up old usage of AppExitInfo
tejasd May 31, 2021
95a3c56
Add custom keys and logs to ANR event
tejasd May 31, 2021
c22350d
Fix unit test + cleanup with orientation in ANR event
tejasd May 31, 2021
8d00013
Revert orientation for ANRs
tejasd May 31, 2021
e32e503
Merge branch 'crashlytics/anrs' into td/anr
tejasd Jun 4, 2021
f27bbc7
Add ability to add logs and breadcrumbs to ANR events
tejasd Jun 4, 2021
5129de9
Revert "Add ability to add logs and breadcrumbs to ANR events"
tejasd Jun 4, 2021
21d4f2a
Merge branch 'crashlytics/anrs' into td/anr
tejasd Jun 4, 2021
621e2da
Merge branch 'crashlytics/anrs' into td/anr-2
tejasd Jun 4, 2021
619982b
Merge branch 'td/anr' into td/anr-2
tejasd Jun 4, 2021
6bfec5b
Restore logs changes for ANRs
tejasd Jun 4, 2021
fcbd3d1
Change test device to API 30
tejasd Jun 4, 2021
8f1af2c
Change device to API 30
tejasd Jun 4, 2021
e92a556
Fix custom keys for ANRs
tejasd Jun 15, 2021
ca67b3f
Merge branch 'crashlytics/anrs' into td/anr
tejasd Jun 15, 2021
339e3be
Merge branch 'td/anr' into td/anr-2
tejasd Jun 15, 2021
da62a3b
Cleanup
tejasd Jun 15, 2021
b676522
Revert addLogsAndReportMetadata to addLogsAndCustomKeys
tejasd Jun 15, 2021
167f2c1
Clean up unit test
tejasd Jun 23, 2021
fa7caa1
Merge branch 'crashlytics/anrs' into td/anr
tejasd Jun 23, 2021
91b903c
Merge branch 'td/anr' into td/anr-2
tejasd Jun 23, 2021
6af964a
Add comment about device orientation
tejasd Jun 23, 2021
bf4d680
Merge branch 'td/anr' into td/anr-2
tejasd Jun 23, 2021
2c8ee59
Fix merge conflict
tejasd Jun 23, 2021
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: 9 additions & 1 deletion .../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 @@ -895,7 +895,15 @@ private void writeApplicationExitInfoEventIfRelevant(String sessionId) {
// Passes the latest applicationExitInfo to ReportCoordinator, which persists it if it
// happened during the session.
if (applicationExitInfoList.size() != 0) {
reportingCoordinator.persistAppExitInfoEvent(sessionId, applicationExitInfoList.get(0));
final LogFileManager relevantSessionLogManager =
new LogFileManager(context, logFileDirectoryProvider, sessionId);
final UserMetadata relevantUserMetadata = new UserMetadata();
relevantUserMetadata.setCustomKeys(new MetaDataStore(getFilesDir()).readKeyData(sessionId));
reportingCoordinator.persistAppExitInfoEvent(
sessionId,
applicationExitInfoList.get(0),
Copy link

@pydichandra pydichandra Jun 22, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am assuming single app_exit_info entry gets created for app exit (and not multiple app_exit_info entries eg., ANR and OOM after an app exit). Is this assumption correct?

Copy link
Contributor Author

@tejasd tejasd Jun 22, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That assumption is most likely correct., but the documentation doesn't explicitly mention it, and I haven't looked at the implementation yet - but it does imply (https://developer.android.com/reference/kotlin/android/app/ApplicationExitInfo#gettraceinputstream)

However, we're currently only collecting ANRs, and so it's ok to only collect the last app_exit_info.

relevantSessionLogManager,
relevantUserMetadata);
}
}
}
Expand Down
19 changes: 17 additions & 2 deletions ...ain/java/com/google/firebase/crashlytics/internal/common/SessionReportingCoordinator.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -134,7 +134,11 @@ public void persistNonFatalEvent(
}

@RequiresApi(api = Build.VERSION_CODES.R)
public void persistAppExitInfoEvent(String sessionId, ApplicationExitInfo applicationExitInfo) {
public void persistAppExitInfoEvent(
String sessionId,
ApplicationExitInfo applicationExitInfo,
LogFileManager logFileManagerForSession,
UserMetadata userMetadataForSession) {
long sessionStartTime = reportPersistence.getStartTimestampMillis(sessionId);
// ApplicationExitInfo did not occur during the session.
if (applicationExitInfo.getTimestamp() < sessionStartTime) {
Expand All @@ -150,7 +154,11 @@ public void persistAppExitInfoEvent(String sessionId, ApplicationExitInfo applic
dataCapture.captureAnrEventData(convertApplicationExitInfo(applicationExitInfo));

Logger.getLogger().d("Persisting anr for session " + sessionId);
reportPersistence.persistEvent(addLogsAndCustomKeysToEvent(capturedEvent), sessionId, true);
reportPersistence.persistEvent(
addLogsAndCustomKeysToEvent(
capturedEvent, logFileManagerForSession, userMetadataForSession),
sessionId,
true);
}

public void finalizeSessionWithNativeEvent(
Expand Down Expand Up @@ -218,6 +226,13 @@ public Task<Void> sendReports(@NonNull Executor reportSendCompleteExecutor) {

private CrashlyticsReport.Session.Event addLogsAndCustomKeysToEvent(
CrashlyticsReport.Session.Event capturedEvent) {
return addLogsAndCustomKeysToEvent(capturedEvent, logFileManager, reportMetadata);
}

private CrashlyticsReport.Session.Event addLogsAndCustomKeysToEvent(
CrashlyticsReport.Session.Event capturedEvent,
LogFileManager logFileManager,
UserMetadata reportMetadata) {
final CrashlyticsReport.Session.Event.Builder eventBuilder = capturedEvent.toBuilder();
final String content = logFileManager.getLogString();

Expand Down
18 changes: 14 additions & 4 deletions ...com/google/firebase/crashlytics/internal/common/CrashlyticsControllerRobolectricTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@
import androidx.test.core.app.ApplicationProvider;
import com.google.firebase.crashlytics.internal.ProviderProxyNativeComponent;
import com.google.firebase.crashlytics.internal.analytics.AnalyticsEventLogger;
import com.google.firebase.crashlytics.internal.log.LogFileManager;
import com.google.firebase.crashlytics.internal.persistence.FileStore;
import com.google.firebase.crashlytics.internal.settings.SettingsDataProvider;
import com.google.firebase.crashlytics.internal.settings.model.FeaturesSettingsData;
Expand All @@ -54,6 +55,7 @@ public class CrashlyticsControllerRobolectricTest {
@Mock private File mockFilesDirectory;
@Mock private SessionReportingCoordinator mockSessionReportingCoordinator;
@Mock private DataCollectionArbiter mockDataCollectionArbiter;
@Mock private LogFileManager.DirectoryProvider mockLogFileDirecotryProvider;

@Before
public void setUp() {
Expand All @@ -72,7 +74,9 @@ public void testDoCloseSession_enabledAnrs_doesNotPersistsAppExitInfoIfItDoesntE
controller.doCloseSessions(mockSettingsDataProvider);
// Since we haven't added any app exit info to the shadow activity manager, there won't exist a
// single app exit info, and so this method won't be called.
verify(mockSessionReportingCoordinator, never()).persistAppExitInfoEvent(eq(sessionId), any());
verify(mockSessionReportingCoordinator, never())
.persistAppExitInfoEvent(
eq(sessionId), any(), any(LogFileManager.class), any(UserMetadata.class));
}

@Test
Expand All @@ -86,7 +90,11 @@ public void testDoCloseSession_enabledAnrs_persistsAppExitInfoIfItExists() {
mockSettingsData(true);
controller.doCloseSessions(mockSettingsDataProvider);
verify(mockSessionReportingCoordinator)
.persistAppExitInfoEvent(eq(sessionId), eq(testApplicationExitInfo));
.persistAppExitInfoEvent(
eq(sessionId),
eq(testApplicationExitInfo),
any(LogFileManager.class),
any(UserMetadata.class));
}

@Test
Expand All @@ -98,7 +106,9 @@ public void testDoCloseSession_disabledAnrs_doesNotPersistsAppExitInfo() {
.thenReturn(Collections.singletonList(sessionId));
mockSettingsData(false);
controller.doCloseSessions(mockSettingsDataProvider);
verify(mockSessionReportingCoordinator, never()).persistAppExitInfoEvent(eq(sessionId), any());
verify(mockSessionReportingCoordinator, never())
.persistAppExitInfoEvent(
eq(sessionId), any(), any(LogFileManager.class), any(UserMetadata.class));
}

private void mockSettingsData(boolean collectAnrs) {
Expand Down Expand Up @@ -130,7 +140,7 @@ private CrashlyticsController createController() {
appData,
null,
null,
null,
mockLogFileDirecotryProvider,
mockSessionReportingCoordinator,
new ProviderProxyNativeComponent(() -> null),
mock(AnalyticsEventLogger.class));
Expand Down
11 changes: 8 additions & 3 deletions ...ogle/firebase/crashlytics/internal/common/SessionReportingCoordinatorRobolectricTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,6 +53,8 @@ public class SessionReportingCoordinatorRobolectricTest {
@Mock private CrashlyticsReport.Session.Event.Builder mockEventBuilder;
@Mock private CrashlyticsReport.Session.Event.Application mockEventApp;
@Mock private CrashlyticsReport.Session.Event.Application.Builder mockEventAppBuilder;
@Mock private LogFileManager mockLogFileManager;
@Mock UserMetadata mockUserMetadata;

private SessionReportingCoordinator reportingCoordinator;

Expand All @@ -78,7 +80,8 @@ public void testAppExitInfoEvent_persistIfAnrWithinSession() {
ApplicationExitInfo testApplicationExitInfo = addAppExitInfo(ApplicationExitInfo.REASON_ANR);

reportingCoordinator.onBeginSession(sessionId, sessionStartTimestamp);
reportingCoordinator.persistAppExitInfoEvent(sessionId, testApplicationExitInfo);
reportingCoordinator.persistAppExitInfoEvent(
sessionId, testApplicationExitInfo, mockLogFileManager, mockUserMetadata);

verify(dataCapture).captureAnrEventData(convertApplicationExitInfo(testApplicationExitInfo));
verify(reportPersistence).persistEvent(any(), eq(sessionId), eq(true));
Expand All @@ -95,7 +98,8 @@ public void testAppExitInfoEvent_notPersistIfAnrBeforeSession() {
ApplicationExitInfo testApplicationExitInfo = addAppExitInfo(ApplicationExitInfo.REASON_ANR);

reportingCoordinator.onBeginSession(sessionId, sessionStartTimestamp);
reportingCoordinator.persistAppExitInfoEvent(sessionId, testApplicationExitInfo);
reportingCoordinator.persistAppExitInfoEvent(
sessionId, testApplicationExitInfo, mockLogFileManager, mockUserMetadata);

verify(dataCapture, never())
.captureAnrEventData(convertApplicationExitInfo(testApplicationExitInfo));
Expand All @@ -114,7 +118,8 @@ public void testAppExitInfoEvent_notPersistIfAppExitInfoNotAnrButWithinSession()
addAppExitInfo(ApplicationExitInfo.REASON_DEPENDENCY_DIED);

reportingCoordinator.onBeginSession(sessionId, sessionStartTimestamp);
reportingCoordinator.persistAppExitInfoEvent(sessionId, testApplicationExitInfo);
reportingCoordinator.persistAppExitInfoEvent(
sessionId, testApplicationExitInfo, mockLogFileManager, mockUserMetadata);

verify(dataCapture, never())
.captureAnrEventData(convertApplicationExitInfo(testApplicationExitInfo));
Expand Down