-
-
Notifications
You must be signed in to change notification settings - Fork 463
fix(anr): Always report stacktraces for ANRv1 #4918
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
Conversation
|
@sentry review |
sentry-android-core/src/main/java/io/sentry/android/core/AnrIntegrationFactory.java
Outdated
Show resolved
Hide resolved
sentry-android-core/src/main/java/io/sentry/android/core/AnrIntegrationFactory.java
Outdated
Show resolved
Hide resolved
Performance metrics 🚀
|
| Revision | Plain | With Sentry | Diff |
|---|---|---|---|
| 604a261 | 380.65 ms | 451.27 ms | 70.62 ms |
| a416a65 | 295.53 ms | 373.74 ms | 78.21 ms |
| 889ecea | 367.58 ms | 437.52 ms | 69.94 ms |
| a5ab36f | 320.47 ms | 389.77 ms | 69.30 ms |
| 27d7cf8 | 397.90 ms | 498.65 ms | 100.75 ms |
| ee747ae | 400.46 ms | 423.61 ms | 23.15 ms |
| d217708 | 375.27 ms | 415.68 ms | 40.41 ms |
| fcec2f2 | 357.47 ms | 447.32 ms | 89.85 ms |
| 23d6b12 | 354.10 ms | 408.38 ms | 54.28 ms |
| d5a29b6 | 298.62 ms | 391.78 ms | 93.16 ms |
App size
| Revision | Plain | With Sentry | Diff |
|---|---|---|---|
| 604a261 | 1.58 MiB | 2.10 MiB | 533.42 KiB |
| a416a65 | 1.58 MiB | 2.12 MiB | 555.26 KiB |
| 889ecea | 1.58 MiB | 2.11 MiB | 539.75 KiB |
| a5ab36f | 1.58 MiB | 2.12 MiB | 555.26 KiB |
| 27d7cf8 | 1.58 MiB | 2.12 MiB | 549.42 KiB |
| ee747ae | 1.58 MiB | 2.10 MiB | 530.95 KiB |
| d217708 | 1.58 MiB | 2.10 MiB | 532.97 KiB |
| fcec2f2 | 1.58 MiB | 2.12 MiB | 551.50 KiB |
| 23d6b12 | 1.58 MiB | 2.10 MiB | 532.31 KiB |
| d5a29b6 | 1.58 MiB | 2.12 MiB | 549.37 KiB |
Previous results on branch: refactor/decouple-sentry-thread-factory
Startup times
| Revision | Plain | With Sentry | Diff |
|---|---|---|---|
| bb4a325 | 312.60 ms | 365.22 ms | 52.62 ms |
| fa4b1c9 | 327.00 ms | 369.06 ms | 42.06 ms |
| 985e853 | 300.61 ms | 353.67 ms | 53.06 ms |
App size
| Revision | Plain | With Sentry | Diff |
|---|---|---|---|
| bb4a325 | 1.58 MiB | 2.13 MiB | 557.41 KiB |
| fa4b1c9 | 1.58 MiB | 2.13 MiB | 557.45 KiB |
| 985e853 | 1.58 MiB | 2.13 MiB | 557.40 KiB |
adinauer
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.
LGTM, left a question about some tests
| @Test | ||
| fun `when getCurrentThreads is called, not empty result`() { | ||
| val sut = fixture.getSut() | ||
| val threads = sut.getCurrentThreads(null) |
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.
l some tests here used to implicitly have attachStacktrace=true, now we're explicitly passing false. Does that matter?
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 that should be fine, we have some tests for extra covering the attachStacktrace flag
romtsn
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.
LGTM!
Description
Refactored
SentryThreadFactoryto remove its dependency onSentryOptions. Instead of passing the entire options object, the relevant configuration (whether to attach stack traces) is now passed as a boolean parameter through the method calls, this allows us to attach ANR (v1) stacktraces, to events, regardless of the SentryOptions.Motivation
Fixes #4904
Testing