-
Notifications
You must be signed in to change notification settings - Fork 566
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.
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
Add collection of ANRs to Crashlytics #2756
Conversation
…n instead of Report. (#2668)
Coverage ReportAffected SDKs
Test Logs
NotesHTML coverage reports can be produced locally with Head commit (133e2b76) is created by Prow via merging commits: 91681dd 3ebad98. |
Binary Size ReportAffected SDKs
Test Logs
NotesHead commit (133e2b76) is created by Prow via merging commits: 91681dd 3ebad98. |
@@ -74,7 +78,8 @@ dependencies { | |||
annotationProcessor 'com.google.auto.value:auto-value:1.6.5' | |||
|
|||
testImplementation 'androidx.test:runner:1.3.0' | |||
testImplementation "org.robolectric:robolectric:$robolectricVersion" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider updating $robolectricVersion
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Deferred.
@@ -47,6 +47,12 @@ public String eventToJson(@NonNull CrashlyticsReport.Session.Event event) { | |||
return CRASHLYTICS_REPORT_JSON_ENCODER.encode(event); | |||
} | |||
|
|||
@NonNull | |||
public String appExitInfoToJson( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Rename to applicationExitInfoToJson
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
ActivityManager activityManager = | ||
(ActivityManager) context.getSystemService(Context.ACTIVITY_SERVICE); | ||
List<ApplicationExitInfo> applicationExitInfoList = | ||
activityManager.getHistoricalProcessExitReasons(null, 0, 0); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Change maxEvents to 1, and add a TODO to change this later if start linking ApplicationExitInfo to other events.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
UserMetadata userMetadataForSession) { | ||
long sessionStartTime = reportPersistence.getStartTimestampMillis(sessionId); | ||
// ApplicationExitInfo did not occur during the session. | ||
if (applicationExitInfo.getTimestamp() < sessionStartTime) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This might cause a bug where an ANR is sent repeatedly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does not cause a bug.
|
||
/** If an ApplicationExitInfo exists relevant to the session, writes that event. */ | ||
private void writeApplicationExitInfoEventIfRelevant(String sessionId) { | ||
if (android.os.Build.VERSION.SDK_INT >= Build.VERSION_CODES.R) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add log when it's not API 30
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
@tejasd: The following test failed, say
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I had some Qs answered in synchronous code review with Tejas and Vivian. LGTM!!
Macrobenchmark ReportAffected SDKsMeasurements are for head commit (3ebad98). Diffing against base commit (91681dd) is working in progress.
|
getHistoricalProcessExitReasons
to collect ANR data