-
-
Notifications
You must be signed in to change notification settings - Fork 466
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?
Conversation
|
🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
sentry-android-core/src/main/java/io/sentry/android/core/AnrV2Integration.java
Outdated
Show resolved
Hide resolved
sentry-android-core/src/main/java/io/sentry/android/core/anr/AnrProfileManager.java
Show resolved
Hide resolved
sentry-android-core/src/main/java/io/sentry/android/core/anr/AnrCulpritIdentifier.java
Show resolved
Hide resolved
sentry-android-core/src/main/java/io/sentry/android/core/anr/AggregatedStackTrace.java
Outdated
Show resolved
Hide resolved
Performance metrics 🚀
|
| Revision | Plain | With Sentry | Diff |
|---|---|---|---|
| a416a65 | 295.53 ms | 373.74 ms | 78.21 ms |
| cf708bd | 408.35 ms | 458.98 ms | 50.63 ms |
| e59e22a | 374.68 ms | 442.14 ms | 67.46 ms |
| d15471f | 361.89 ms | 378.07 ms | 16.18 ms |
| d364ace | 384.53 ms | 453.51 ms | 68.98 ms |
| b3d8889 | 371.69 ms | 432.96 ms | 61.26 ms |
| 951caf7 | 323.66 ms | 392.82 ms | 69.16 ms |
| bdbe1f4 | 380.66 ms | 464.44 ms | 83.78 ms |
| a416a65 | 333.78 ms | 410.37 ms | 76.59 ms |
| 539ca63 | 313.51 ms | 355.43 ms | 41.92 ms |
App size
| Revision | Plain | With Sentry | Diff |
|---|---|---|---|
| a416a65 | 1.58 MiB | 2.12 MiB | 555.26 KiB |
| cf708bd | 1.58 MiB | 2.11 MiB | 539.71 KiB |
| e59e22a | 1.58 MiB | 2.20 MiB | 635.34 KiB |
| d15471f | 1.58 MiB | 2.13 MiB | 559.54 KiB |
| d364ace | 1.58 MiB | 2.11 MiB | 539.75 KiB |
| b3d8889 | 1.58 MiB | 2.10 MiB | 535.06 KiB |
| 951caf7 | 1.58 MiB | 2.13 MiB | 558.77 KiB |
| bdbe1f4 | 1.58 MiB | 2.11 MiB | 538.88 KiB |
| a416a65 | 1.58 MiB | 2.12 MiB | 555.26 KiB |
| 539ca63 | 1.58 MiB | 2.12 MiB | 551.41 KiB |
Previous results on branch: markushi/feat/anr-profiling
Startup times
| Revision | Plain | With Sentry | Diff |
|---|---|---|---|
| fca8df8 | 326.79 ms | 379.69 ms | 52.90 ms |
| c10e603 | 367.92 ms | 393.50 ms | 25.58 ms |
| 31581b9 | 350.00 ms | 420.63 ms | 70.63 ms |
| 2cee1ab | 318.29 ms | 361.00 ms | 42.71 ms |
| 4c0ffee | 314.94 ms | 377.79 ms | 62.86 ms |
| 83a9ec4 | 333.84 ms | 390.30 ms | 56.47 ms |
| 00299fd | 359.87 ms | 424.85 ms | 64.98 ms |
App size
| Revision | Plain | With Sentry | Diff |
|---|---|---|---|
| fca8df8 | 1.58 MiB | 2.29 MiB | 723.68 KiB |
| c10e603 | 1.58 MiB | 2.29 MiB | 723.72 KiB |
| 31581b9 | 1.58 MiB | 2.19 MiB | 624.94 KiB |
| 2cee1ab | 1.58 MiB | 2.29 MiB | 723.68 KiB |
| 4c0ffee | 1.58 MiB | 2.29 MiB | 723.67 KiB |
| 83a9ec4 | 1.58 MiB | 2.29 MiB | 723.99 KiB |
| 00299fd | 1.58 MiB | 2.29 MiB | 723.50 KiB |
sentry-android-core/src/main/java/io/sentry/android/core/anr/AnrCulpritIdentifier.java
Show resolved
Hide resolved
…ntry-java into markushi/feat/anr-profiling
sentry-android-core/src/main/java/io/sentry/android/core/AnrV2Integration.java
Outdated
Show resolved
Hide resolved
sentry-android-core/src/main/java/io/sentry/android/core/AnrV2Integration.java
Outdated
Show resolved
Hide resolved
sentry-android-core/src/main/java/io/sentry/android/core/AnrV2Integration.java
Outdated
Show resolved
Hide resolved
sentry-android-core/src/main/java/io/sentry/android/core/anr/AnrStackTrace.java
Outdated
Show resolved
Hide resolved
sentry-android-core/src/main/java/io/sentry/android/core/anr/AnrProfilingIntegration.java
Show resolved
Hide resolved
|
@sentry review |
…ntry-java into markushi/feat/anr-profiling
sentry-android-core/src/main/java/io/sentry/android/core/AnrV2Integration.java
Outdated
Show resolved
Hide resolved
sentry-android-core/src/main/java/io/sentry/android/core/anr/AnrProfilingIntegration.java
Show resolved
Hide resolved
sentry-android-core/src/main/java/io/sentry/android/core/anr/StackTraceConverter.java
Outdated
Show resolved
Hide resolved
sentry-android-core/src/main/java/io/sentry/android/core/ApplicationExitInfoEventProcessor.java
Outdated
Show resolved
Hide resolved
sentry-android-core/src/main/java/io/sentry/android/core/anr/StackTraceConverter.java
Outdated
Show resolved
Hide resolved
sentry-android-core/src/main/java/io/sentry/android/core/AnrV2Integration.java
Outdated
Show resolved
Hide resolved
sentry-android-core/src/main/java/io/sentry/android/core/anr/AnrProfilingIntegration.java
Show resolved
Hide resolved
sentry-android-core/src/main/java/io/sentry/android/core/ApplicationExitInfoEventProcessor.java
Show resolved
Hide resolved
sentry-android-core/src/main/java/io/sentry/android/core/anr/AnrCulpritIdentifier.java
Show resolved
Hide resolved
sentry-android-core/src/main/java/io/sentry/android/core/anr/AnrProfilingIntegration.java
Show resolved
Hide resolved
sentry-samples/sentry-samples-android/src/main/AndroidManifest.xml
Outdated
Show resolved
Hide resolved
sentry-samples/sentry-samples-android/src/main/AndroidManifest.xml
Outdated
Show resolved
Hide resolved
sentry-android-core/src/test/java/io/sentry/android/core/anr/AnrProfilingIntegrationTest.kt
Outdated
Show resolved
Hide resolved
sentry-android-core/src/test/java/io/sentry/android/core/anr/AnrProfilingIntegrationTest.kt
Outdated
Show resolved
Hide resolved
sentry-android-core/src/test/java/io/sentry/android/core/anr/AnrProfileRotationHelperTest.kt
Outdated
Show resolved
Hide resolved
sentry-android-core/src/test/java/io/sentry/android/core/anr/AnrProfileManagerTest.kt
Outdated
Show resolved
Hide resolved
sentry-android-core/src/main/java/io/sentry/android/core/AndroidOptionsInitializer.java
Show resolved
Hide resolved
| return; | ||
| } | ||
|
|
||
| if (options.isEnableAnrProfiling() && hasOnlySystemFrames(event)) { |
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.
wondering whether we should guard this behind isEnableAnrProfiling or actually just do it for all ANRs going forward? I think AEI also gives quite a lot of noise with just system frames, right?
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.
Yes, exactly - we should do this everywhere in the long run. I'm still would guarded right now - otherwise we'll have a breaking change in default behavior. I can create a follow up ticket for the next major.
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 think we've done this quite a lot in the past (breaking behaviour change that affects grouping) so I'm not opposed to doing this now and not wait for the next major 😅 If it improves things I think I'd rather do it sooner. But your call here (we could also wait to get some adoption and see how it performs before doing this for everyone)
| final StackTraceElement stackTraceElement = stack[0]; | ||
| final String message = | ||
| stackTraceElement.getClassName() + "." + stackTraceElement.getMethodName(); | ||
| final AnrException exception = new AnrException(message); |
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.
is the new AnrException type intentional here? Because it's a new type it will create new groups (even if the profile-derived stacktrace matches the one from AEI). Not sure if we should keep it as ApplicationNotResponding?
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.
Yes, I think I looked into this early on, but ApplicationNotResponding requires a non-null Thread object, which seemed to be widely used and hard to refactor. But that's a good point, let me check that again.
sentry-android-core/src/main/java/io/sentry/android/core/ApplicationExitInfoEventProcessor.java
Show resolved
Hide resolved
| options); | ||
| chunk.setSentryProfile(profile); | ||
|
|
||
| final SentryId profilerId = Sentry.getCurrentScopes().captureProfileChunk(chunk); |
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.
just curious, but is there a way to send both the ANR event and the profile chunk in the same envelope?
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.
claude says yes, this should be valid for the backend😅
Yes, this works. The client can send both in the same envelope.
During split_envelope() at relay-server/src/services/processor.rs:357-365, Relay splits them apart before any event-type routing:
- ProfileChunk items are extracted into their own ProcessingGroup::ProfileChunk envelope
- The remaining error event goes into ProcessingGroup::Error
- Both are processed independently through their respective pipelines
We'd need to attach the profile directly to the event (or via hint) and then do something similar as we do for e.g. attachments (
sentry-java/sentry/src/main/java/io/sentry/SentryClient.java
Lines 422 to 432 in abf451a
| if (attachments != null) { | |
| for (final Attachment attachment : attachments) { | |
| final SentryEnvelopeItem attachmentItem = | |
| SentryEnvelopeItem.fromAttachment( | |
| options.getSerializer(), | |
| options.getLogger(), | |
| attachment, | |
| options.getMaxAttachmentSize()); | |
| envelopeItems.add(attachmentItem); | |
| } | |
| } |
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.
yeah, or replay_recording, too.
I guess this will be better in terms of not sending one without the other, but I don't have a strong preference right now, we can probably create a followup issue if you don't feel like doing it now.
| private void clearStacks() throws IOException { | ||
| numCollectedStacks.set(0); | ||
| getProfileManager().clear(); | ||
| } | ||
|
|
||
| private void addStackTrace(@NotNull final AnrStackTrace trace) throws IOException { | ||
| numCollectedStacks.incrementAndGet(); | ||
| getProfileManager().add(trace); |
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: A race condition between addStackTrace() and close() allows the profiling thread to write to the profileManager after it has been closed, causing an IOException and data loss.
Severity: HIGH
Suggested Fix
Ensure the profiling thread has fully terminated before closing the AnrProfileManager. In the close() method, after interrupting the thread via onBackground(), add a thread.join() to wait for its completion before proceeding to acquire the lock and close the manager. This will prevent the race condition where the thread attempts to use a closed resource.
Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent.
Verify if this is a real issue. If it is, propose a fix; if not, explain why it's not
valid.
Location:
sentry-android-core/src/main/java/io/sentry/android/core/anr/AnrProfilingIntegration.java#L229-L236
Potential issue: A race condition exists in the `AnrProfilingIntegration` shutdown
sequence. The `getProfileManager()` method acquires a lock to retrieve the
`profileManager` instance but releases the lock before returning the reference. A
separate profiling thread can then call `addStackTrace()`, which uses this reference to
add a trace. Concurrently, the `close()` method can be called, which interrupts the
profiling thread (without waiting for it to terminate), acquires the same lock, and
closes the `profileManager`. If the profiling thread attempts to call `.add()` on the
manager after it has been closed, an `IOException` is thrown, leading to the loss of the
stack trace data.
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.
Cursor Bugbot has reviewed your changes and found 2 potential issues.
Bugbot Autofix is OFF. To automatically fix reported issues with Cloud Agents, enable Autofix in the Cursor dashboard.
| if (options.getCacheDirPath() == null) { | ||
| logger.log(SentryLevel.WARNING, "ANR Profiling is enabled but cacheDirPath is not set"); | ||
| return; | ||
| } |
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.
Misleading warning logged when ANR profiling is disabled
Low Severity
The cacheDirPath == null check on line 60 happens before the isEnableAnrProfiling() check on line 65. This means the warning "ANR Profiling is enabled but cacheDirPath is not set" is logged even when ANR profiling is not enabled, since register() is called for all integrations. The feature-flag check and cacheDirPath check are in the wrong order.
| } | ||
| } | ||
| return true; | ||
| } |
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.
hasOnlySystemFrames true when no frames exist
Medium Severity
hasOnlySystemFrames returns true when the event has exceptions but none of them have actual stack frames (e.g., null stacktraces or empty frame lists). The loop body simply never executes, so the method falls through to return true. This incorrectly triggers the "system-frames-only-anr" fingerprint for frameless exceptions, conflating "no frames at all" with "only system frames."


📜 Description
Adds ANR (Application Not Responding) profiling integration that profiles the main thread when an ANR is detected and reports the captured profiles to Sentry.
Key Changes:
AnrProfilingIntegrationto capture profiles during ANR eventsAnrV2Integrationnow takes care of matching and capturing the profile on the next start💡 Motivation and Context
This feature enables better ANR diagnostics by capturing profiling data at the time of ANR detection, allowing developers to identify performance bottlenecks and problematic code paths causing application hangs.
Example event: https://sentry-sdks.sentry.io/issues/7229210096/events/4598ff6fcc0f402d8ecca615005e7f64/
💚 How did you test it?
📝 Checklist
sendDefaultPIIis enabled.🔮 Next steps