Skip to content
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 Conditional Extraction of ApplicationExitInfo Trace Data for Enhanced Logging #95

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

jaredsburrows
Copy link

@jaredsburrows jaredsburrows commented Nov 4, 2024

Closes #90

@murki

This PR updates the ApplicationExitInfo.toFields() function to conditionally extract and include trace data in the app termination log when available. Previously, only basic application exit information was logged. Now, the trace data field (_app_exit_trace) will be included in the metadata when a relevant exit reason and non-empty trace data exist.

This change improves the accuracy and usefulness of app termination logs by only including trace data when meaningful, specifically for ANR and native crash scenarios. By avoiding the addition of empty or irrelevant trace data, this update enhances the quality of diagnostics information for developers.

Refactored ApplicationExitInfo.toFields():

  • Converted the toFields function from using a direct mapOf to initializing a mutable map (fields), enabling conditional addition of _app_exit_trace.
  • Added logic to check if ApplicationExitInfo.reason is either REASON_ANR or REASON_CRASH_NATIVE. If so, getTraceInputStream() is called to retrieve any available trace data.

Conditional Trace Data Extraction:

  • Used getTraceInputStream() with a check on traceStream.available() to ensure only non-empty streams are processed.
  • _app_exit_trace is added to the fields map only when valid trace data is found, avoiding unnecessary empty fields in the log.

Improved Logging Precision:

  • Ensures that trace data is only included in the event log when relevant and available, reducing noise in the logs and improving clarity in app exit event reporting.

References:

Copy link

github-actions bot commented Nov 4, 2024

All contributors have signed the CLA ✍️ ✅
Posted by the CLA Assistant Lite bot.

@jaredsburrows jaredsburrows force-pushed the pr/jaredsburrows/extract-getTraceInputStream branch from 2c21458 to 48f0a67 Compare November 4, 2024 08:14
@jaredsburrows
Copy link
Author

I have read the CLA Document and I hereby sign the CLA.

@jaredsburrows jaredsburrows force-pushed the pr/jaredsburrows/extract-getTraceInputStream branch from 48f0a67 to bcde542 Compare November 4, 2024 08:21

// Add trace data if available for REASON_ANR or REASON_CRASH_NATIVE
if (reason == ApplicationExitInfo.REASON_ANR
|| reason == ApplicationExitInfo.REASON_CRASH_NATIVE) {
Copy link
Contributor

@murki murki Nov 4, 2024

Choose a reason for hiding this comment

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

according to the docs: "tombstone traces will be returned for REASON_CRASH_NATIVE, with an InputStream containing a protobuf with [this schema]"(https://android.googlesource.com/platform/system/core/+/refs/heads/master/debuggerd/proto/tombstone.proto)

we should parse the stream using that proto and extract its properties individually

Copy link
Author

Choose a reason for hiding this comment

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

We can generate the parseFrom Tombestone by adding the proto do this repo. We would need to keep tombstone.proto up to date to have the latest fields to parse the inputstream.

@murki
Copy link
Contributor

murki commented Nov 4, 2024

@jaredsburrows were you able to test this in the wild? If so it would be helpful to link to a session that can be verified

@jaredsburrows jaredsburrows force-pushed the pr/jaredsburrows/extract-getTraceInputStream branch from bcde542 to d3bc58c Compare November 4, 2024 17:33
// Add trace data if available for REASON_ANR or REASON_CRASH_NATIVE
if (reason == ApplicationExitInfo.REASON_ANR || reason == ApplicationExitInfo.REASON_CRASH_NATIVE) {
runCatching {
getTraceInputStream()?.takeIf { it.available() > 0 }?.bufferedReader()?.use { reader ->
Copy link
Contributor

Choose a reason for hiding this comment

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

Another thing I noticed from the docs:

Note that because these traces are kept in a separate global circular buffer, crashes may be overwritten by newer crashes (including from other applications)

Something else that we might need to think about if and how to handle

@jaredsburrows jaredsburrows force-pushed the pr/jaredsburrows/extract-getTraceInputStream branch from d3bc58c to 20801e1 Compare November 5, 2024 23:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Enrich instrumentation data for App Terminations with ANR and CRASH_NATIVE reasons
2 participants