-
-
Notifications
You must be signed in to change notification settings - Fork 465
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 |
|---|---|---|---|
| 27d7cf8 | 309.43 ms | 364.27 ms | 54.85 ms |
| f064536 | 335.52 ms | 408.79 ms | 73.27 ms |
| fc5ccaf | 256.80 ms | 322.36 ms | 65.56 ms |
| e59e22a | 368.02 ms | 432.00 ms | 63.98 ms |
| b750b96 | 408.98 ms | 480.32 ms | 71.34 ms |
| fcec2f2 | 328.91 ms | 387.75 ms | 58.84 ms |
| 9139b91 | 351.35 ms | 355.63 ms | 4.28 ms |
| 951caf7 | 323.66 ms | 392.82 ms | 69.16 ms |
| 18c0bc2 | 306.73 ms | 349.77 ms | 43.03 ms |
| dba088c | 365.46 ms | 366.31 ms | 0.85 ms |
App size
| Revision | Plain | With Sentry | Diff |
|---|---|---|---|
| 27d7cf8 | 1.58 MiB | 2.12 MiB | 549.42 KiB |
| f064536 | 1.58 MiB | 2.20 MiB | 633.90 KiB |
| fc5ccaf | 1.58 MiB | 2.13 MiB | 557.54 KiB |
| e59e22a | 1.58 MiB | 2.20 MiB | 635.34 KiB |
| b750b96 | 1.58 MiB | 2.10 MiB | 533.19 KiB |
| fcec2f2 | 1.58 MiB | 2.12 MiB | 551.50 KiB |
| 9139b91 | 1.58 MiB | 2.13 MiB | 559.52 KiB |
| 951caf7 | 1.58 MiB | 2.13 MiB | 558.77 KiB |
| 18c0bc2 | 1.58 MiB | 2.13 MiB | 557.33 KiB |
| dba088c | 1.58 MiB | 2.13 MiB | 558.99 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
Show resolved
Hide resolved
sentry-android-core/src/main/java/io/sentry/android/core/AnrV2Integration.java
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 |
sentry-android-core/src/main/java/io/sentry/android/core/AnrV2Integration.java
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
Show resolved
Hide resolved
markushi
left a comment
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.
Addressing bot comments:
Concurrent QueueFile Access (Comments on AnrV2Integration.java:327, :367):
✅ Working as designed. The reader uses AnrProfileRotationHelper.getLastFile() which reads from a rotated/archived file, while the writer writes to the current file. They access different files.
AnrProfileManager file descriptor leak (AnrProfileManager.java:50):
✅ Already fixed. The code uses try-with-resources: try (final AnrProfileManager provider = new AnrProfileManager(options, lastFile))
lowQualityPackages initialization (AnrCulpritIdentifier.java:85):
✅ Already fixed. The field uses a static initializer block.
Stack depth calculation (AggregatedStackTrace.java:38):
✅ Already fixed.
Incomplete stack trace handling (AnrCulpritIdentifier.java:138):
✅ Already fixed. Code has if (stackTraceMap.isEmpty()) return null; guard at line 140.
Empty debug log (AnrV2Integration.java:346):
✅ Already fixed.
Missing rotate() call (AnrV2Integration.java:424):
✅ Working as designed. Rotation should only happen once after app start, not on every ANR event read.
Stack trace serialization (AnrStackTrace.java:35):
✅ Already fixed.
Missing listener removal (AnrProfilingIntegration.java:79):
✅ Already fixed.
Profile ID mismatch (AnrV2Integration.java:418):
✅ Working as designed. We intentionally return chunk.getProfilerId() to reference the chunk_id as profiler_id.
hasOnlySystemFrames() fingerprinting (AnrV2Integration.java:321, :497):
✅ FIXED in this update. Modified to return false when exceptions list is null/empty, preventing incorrect fingerprinting when profiling fails.
sentry-android-core/src/main/java/io/sentry/android/core/anr/AnrProfilingIntegration.java
Show resolved
Hide resolved
|
|
||
| if (this.options == null) { | ||
| 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.
Dead code due to requireNonNull throwing before null check
Low Severity
The if (this.options == null) return; check is unreachable dead code. Objects.requireNonNull on line 51-54 throws an IllegalArgumentException when the ternary expression evaluates to null (i.e., when options is not a SentryAndroidOptions). The null check can never execute because the exception is thrown first.
sentry-android-core/src/main/java/io/sentry/android/core/AnrV2Integration.java
Show resolved
Hide resolved
Semver Impact of This PR🟡 Minor (new features) 📋 Changelog PreviewThis is how your changes will appear in the changelog. New Features ✨
Bug Fixes 🐛
Internal Changes 🔧Deps
Other
🤖 This preview updates automatically when you update the PR. |
| ### Feature | ||
| ### Features | ||
|
|
||
| - Add new experimental option to capture profiles for ANRs ([#4899](https://github.com/getsentry/sentry-java/pull/4899)) |
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.
- 🚫 The changelog entry seems to be part of an already released section
## 8.31.0.
Consider moving the entry to the## Unreleasedsection, please.
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.
| final long now = SystemClock.uptimeMillis(); | ||
| final long diff = now - lastMainThreadExecutionTime; | ||
|
|
||
| if (diff < 1000) { |
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.
Hardcoded value instead of defined constant
Low Severity
The comparison diff < 1000 uses a hardcoded value instead of the defined constant THRESHOLD_SUSPICION_MS (which equals 1000) declared on line 32. This inconsistency between the hardcoded literal and the constant makes the threshold relationship unclear and creates maintenance risk if the value needs to change.
|
|
||
| if (currentFile.exists()) { | ||
| currentFile.renameTo(oldFile); | ||
| } |
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.
File rotation ignores operation failures silently
Medium Severity
The performRotationIfNeeded method ignores the return values of oldFile.delete() and currentFile.renameTo(oldFile). If delete() fails and the old file still exists, renameTo() may also fail on some platforms. The rotation is marked complete regardless of actual success, meaning getLastFile() could return stale profile data from a previous session instead of the correct ANR profile. This could cause incorrect profiling data to be attached to ANR events.


📜 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.
💚 How did you test it?
📝 Checklist
sendDefaultPIIis enabled.🔮 Next steps