-
-
Notifications
You must be signed in to change notification settings - Fork 464
feat(anr): Profile main thread when ANR and report ANR profiles to Sentry #4899
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
base: main
Are you sure you want to change the base?
Changes from 3 commits
b286ad5
a62b5e8
ae66f73
f226d84
7d423a4
5824f8f
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -12,9 +12,19 @@ | |
| import io.sentry.ILogger; | ||
| import io.sentry.IScopes; | ||
| import io.sentry.Integration; | ||
| import io.sentry.ProfileChunk; | ||
| import io.sentry.ProfileContext; | ||
| import io.sentry.SentryEvent; | ||
| import io.sentry.SentryExceptionFactory; | ||
| import io.sentry.SentryLevel; | ||
| import io.sentry.SentryOptions; | ||
| import io.sentry.SentryStackTraceFactory; | ||
| import io.sentry.android.core.anr.AggregatedStackTrace; | ||
| import io.sentry.android.core.anr.AnrCulpritIdentifier; | ||
| import io.sentry.android.core.anr.AnrException; | ||
| import io.sentry.android.core.anr.AnrProfile; | ||
| import io.sentry.android.core.anr.AnrProfileManager; | ||
| import io.sentry.android.core.anr.StackTraceConverter; | ||
| import io.sentry.android.core.cache.AndroidEnvelopeCache; | ||
| import io.sentry.android.core.internal.threaddump.Lines; | ||
| import io.sentry.android.core.internal.threaddump.ThreadDumpParser; | ||
|
|
@@ -28,6 +38,7 @@ | |
| import io.sentry.protocol.Message; | ||
| import io.sentry.protocol.SentryId; | ||
| import io.sentry.protocol.SentryThread; | ||
| import io.sentry.protocol.profiling.SentryProfile; | ||
| import io.sentry.transport.CurrentDateProvider; | ||
| import io.sentry.transport.ICurrentDateProvider; | ||
| import io.sentry.util.HintUtils; | ||
|
|
@@ -41,6 +52,7 @@ | |
| import java.io.InputStreamReader; | ||
| import java.util.ArrayList; | ||
| import java.util.Collections; | ||
| import java.util.HashMap; | ||
| import java.util.List; | ||
| import java.util.concurrent.TimeUnit; | ||
| import org.jetbrains.annotations.ApiStatus; | ||
|
|
@@ -284,6 +296,8 @@ private void reportAsSentryEvent( | |
| } | ||
| } | ||
|
|
||
| applyAnrProfile(isBackground, anrTimestamp, event); | ||
|
|
||
| final @NotNull SentryId sentryId = scopes.captureEvent(event, hint); | ||
| final boolean isEventDropped = sentryId.equals(SentryId.EMPTY_ID); | ||
| if (!isEventDropped) { | ||
|
|
@@ -299,6 +313,67 @@ private void reportAsSentryEvent( | |
| } | ||
| } | ||
|
|
||
| private void applyAnrProfile( | ||
| final boolean isBackground, final long anrTimestamp, final @NotNull SentryEvent event) { | ||
|
|
||
| // as of now AnrProfilingIntegration only generates profiles in foreground | ||
| if (isBackground) { | ||
| return; | ||
| } | ||
|
|
||
| @Nullable AnrProfile anrProfile = null; | ||
| try { | ||
| final AnrProfileManager provider = new AnrProfileManager(options); | ||
| anrProfile = provider.load(); | ||
| } catch (Throwable t) { | ||
| options.getLogger().log(SentryLevel.INFO, "Could not retrieve ANR profile"); | ||
| } | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Bug: Concurrent File Access Causes Data CorruptionCreating a new |
||
|
|
||
| if (anrProfile != null) { | ||
| options.getLogger().log(SentryLevel.INFO, "ANR profile found"); | ||
| // TODO maybe be less strict around the end timestamp | ||
| if (anrTimestamp >= anrProfile.startTimeMs && anrTimestamp <= anrProfile.endtimeMs) { | ||
| final SentryProfile profile = StackTraceConverter.convert(anrProfile); | ||
| final ProfileChunk chunk = | ||
| new ProfileChunk( | ||
| new SentryId(), | ||
| new SentryId(), | ||
| null, | ||
| new HashMap<>(0), | ||
| anrTimestamp / 1000.0d, | ||
| ProfileChunk.PLATFORM_JAVA, | ||
| options); | ||
| chunk.setSentryProfile(profile); | ||
|
|
||
| options.getLogger().log(SentryLevel.DEBUG, ""); | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||
| scopes.captureProfileChunk(chunk); | ||
|
|
||
| final @Nullable AggregatedStackTrace culprit = | ||
| AnrCulpritIdentifier.identify(anrProfile.stacks); | ||
| if (culprit != null) { | ||
| // TODO if quality is low (e.g. when culprit is pollNative()) | ||
| // consider throwing the ANR using a static fingerprint to reduce noise | ||
| final @NotNull StackTraceElement[] stack = culprit.getStack(); | ||
| if (stack.length > 0) { | ||
| final StackTraceElement stackTraceElement = culprit.getStack()[0]; | ||
| final String message = | ||
| stackTraceElement.getClassName() + "." + stackTraceElement.getMethodName(); | ||
| final AnrException exception = new AnrException(message); | ||
| exception.setStackTrace(stack); | ||
|
|
||
| // TODO should this be re-used from somewhere else? | ||
| final SentryExceptionFactory factory = | ||
| new SentryExceptionFactory(new SentryStackTraceFactory(options)); | ||
| event.setExceptions(factory.getSentryExceptions(exception)); | ||
| event.getContexts().setProfile(new ProfileContext(chunk.getProfilerId())); | ||
| } | ||
| } | ||
| } else { | ||
| options.getLogger().log(SentryLevel.DEBUG, "ANR profile found, but doesn't match"); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| private @NotNull ParseResult parseThreadDump( | ||
| final @NotNull ApplicationExitInfo exitInfo, final boolean isBackground) { | ||
| final byte[] dump; | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,54 @@ | ||
| package io.sentry.android.core.anr; | ||
|
|
||
| import java.util.Arrays; | ||
| import org.jetbrains.annotations.ApiStatus; | ||
|
|
||
| @ApiStatus.Internal | ||
| public class AggregatedStackTrace { | ||
| // the number of frames of the stacktrace | ||
| final int depth; | ||
|
|
||
| // the quality of the stack trace, higher means better | ||
| final int quality; | ||
|
|
||
| private final StackTraceElement[] stack; | ||
|
|
||
| // 0 is the most detailed frame in the stacktrace | ||
| private final int stackStartIdx; | ||
| private final int stackEndIdx; | ||
|
|
||
| // the total number of times this exact stacktrace was captured | ||
| int count; | ||
|
|
||
| // first time the stacktrace occured | ||
| private long startTimeMs; | ||
|
|
||
| // last time the stacktrace occured | ||
| private long endTimeMs; | ||
|
|
||
| public AggregatedStackTrace( | ||
| final StackTraceElement[] stack, | ||
| final int stackStartIdx, | ||
| final int stackEndIdx, | ||
| final long timestampMs, | ||
| final int quality) { | ||
| this.stack = stack; | ||
| this.stackStartIdx = stackStartIdx; | ||
| this.stackEndIdx = stackEndIdx; | ||
| this.depth = stackEndIdx - stackStartIdx; | ||
cursor[bot] marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
| this.startTimeMs = timestampMs; | ||
| this.endTimeMs = timestampMs; | ||
| this.count = 1; | ||
| this.quality = quality; | ||
| } | ||
|
|
||
| public void add(long timestampMs) { | ||
| this.startTimeMs = Math.min(startTimeMs, timestampMs); | ||
| this.endTimeMs = Math.max(endTimeMs, timestampMs); | ||
| this.count++; | ||
| } | ||
|
|
||
| public StackTraceElement[] getStack() { | ||
| return Arrays.copyOfRange(stack, stackStartIdx, stackEndIdx + 1); | ||
| } | ||
| } | ||
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.
Bug:
AnrProfilingIntegrationandAnrV2Integrationconcurrently access the sameQueueFilewithout synchronization.Severity: CRITICAL | Confidence: 1.00
🔍 Detailed Analysis
The
AnrProfilingIntegration(writer) andAnrV2Integration(reader) concurrently access the same underlyingQueueFileatoptions.getCacheDirPath() + "anr_profile"without any synchronization. This violates the explicit contract ofQueueFilewhich states, "Only one instance should access a given file at a time." Concurrent unsynchronized access can lead to data corruption, lost stack traces, or file format inconsistencies due to conflicting writes and reads to the shared file header and data structures.💡 Suggested Fix
Implement a synchronization mechanism (e.g., file-level locking) to ensure exclusive access to the
QueueFile, or refactor to use a singleQueueFileinstance managed by a central component.🤖 Prompt for AI Agent
Did we get this right? 👍 / 👎 to inform future reviews.
Reference_id: 2623394