-
-
Notifications
You must be signed in to change notification settings - Fork 444
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
Make possible screenshots on hybrid sdks (react-native) #2360
Conversation
…rrentActivity public
Performance metrics 🚀
|
Revision | Plain | With Sentry | Diff |
---|---|---|---|
a04f788 | 321.78 ms | 354.12 ms | 32.35 ms |
38e4f11 | 358.20 ms | 433.73 ms | 75.53 ms |
4a9c176 | 320.62 ms | 334.68 ms | 14.06 ms |
507f924 | 342.51 ms | 402.65 ms | 60.14 ms |
90e9745 | 314.68 ms | 357.28 ms | 42.60 ms |
4a9c176 | 319.77 ms | 363.20 ms | 43.43 ms |
f809aac | 301.51 ms | 346.60 ms | 45.09 ms |
7597ded | 289.60 ms | 339.69 ms | 50.09 ms |
4a9c176 | 336.33 ms | 384.73 ms | 48.41 ms |
App size
Revision | Plain | With Sentry | Diff |
---|---|---|---|
a04f788 | 1.73 MiB | 2.32 MiB | 609.88 KiB |
38e4f11 | 1.73 MiB | 2.32 MiB | 609.82 KiB |
4a9c176 | 1.73 MiB | 2.33 MiB | 612.69 KiB |
507f924 | 1.73 MiB | 2.32 MiB | 609.95 KiB |
90e9745 | 1.73 MiB | 2.32 MiB | 608.63 KiB |
4a9c176 | 1.73 MiB | 2.33 MiB | 612.69 KiB |
f809aac | 1.73 MiB | 2.32 MiB | 608.63 KiB |
7597ded | 1.73 MiB | 2.32 MiB | 609.88 KiB |
4a9c176 | 1.73 MiB | 2.33 MiB | 612.69 KiB |
Previous results on branch: feat-screenshots-for-hybrid-sdks
Startup times
Revision | Plain | With Sentry | Diff |
---|---|---|---|
3cf8df7 | 305.09 ms | 353.15 ms | 48.05 ms |
bb49b91 | 359.93 ms | 359.96 ms | 0.03 ms |
b8cac16 | 328.96 ms | 365.33 ms | 36.37 ms |
87377ea | 285.15 ms | 297.33 ms | 12.18 ms |
e28984f | 336.61 ms | 382.63 ms | 46.02 ms |
7b65fcc | 302.75 ms | 358.90 ms | 56.15 ms |
49cc55d | 402.14 ms | 487.92 ms | 85.78 ms |
d1da770 | 390.72 ms | 453.61 ms | 62.89 ms |
87da5a1 | 318.52 ms | 371.67 ms | 53.15 ms |
c768b19 | 297.19 ms | 328.81 ms | 31.62 ms |
App size
Revision | Plain | With Sentry | Diff |
---|---|---|---|
3cf8df7 | 1.73 MiB | 2.32 MiB | 610.11 KiB |
bb49b91 | 1.73 MiB | 2.32 MiB | 609.17 KiB |
b8cac16 | 1.73 MiB | 2.32 MiB | 610.14 KiB |
87377ea | 1.73 MiB | 2.32 MiB | 610.11 KiB |
e28984f | 1.73 MiB | 2.32 MiB | 609.17 KiB |
7b65fcc | 1.73 MiB | 2.32 MiB | 611.62 KiB |
49cc55d | 1.73 MiB | 2.32 MiB | 609.82 KiB |
d1da770 | 1.73 MiB | 2.32 MiB | 610.13 KiB |
87da5a1 | 1.73 MiB | 2.32 MiB | 610.13 KiB |
c768b19 | 1.73 MiB | 2.32 MiB | 609.17 KiB |
sentry-android-core/src/main/java/io/sentry/android/core/ScreenshotEventProcessor.java
Outdated
Show resolved
Hide resolved
sentry-android-core/src/main/java/io/sentry/android/core/ScreenshotEventProcessor.java
Outdated
Show resolved
Hide resolved
…' into feat-screenshots-for-hybrid-sdks
sentry-android-core/src/main/java/io/sentry/android/core/ScreenshotEventProcessor.java
Outdated
Show resolved
Hide resolved
sentry-android-core/src/main/java/io/sentry/android/core/ScreenshotEventProcessor.java
Outdated
Show resolved
Hide resolved
sentry-android-core/src/main/java/io/sentry/android/core/ScreenshotEventProcessor.java
Outdated
Show resolved
Hide resolved
sentry-android-core/src/main/java/io/sentry/android/core/ScreenshotEventProcessor.java
Outdated
Show resolved
Hide resolved
sentry-android-core/src/main/java/io/sentry/android/core/ScreenshotEventProcessor.java
Outdated
Show resolved
Hide resolved
Missing a few things, nullability annotations, naming convention, and tests. |
sentry-android-core/src/main/java/io/sentry/android/core/AndroidLogger.java
Outdated
Show resolved
Hide resolved
sentry-android-core/src/main/java/io/sentry/android/core/ScreenshotEventProcessor.java
Outdated
Show resolved
Hide resolved
sentry-android-core/src/main/java/io/sentry/android/core/ScreenshotEventProcessor.java
Outdated
Show resolved
Hide resolved
sentry-android-core/src/main/java/io/sentry/android/core/ScreenshotEventProcessor.java
Outdated
Show resolved
Hide resolved
sentry-android-core/src/main/java/io/sentry/android/core/internal/util/ScreenshotUtils.java
Outdated
Show resolved
Hide resolved
sentry-android-core/src/main/java/io/sentry/android/core/internal/util/ScreenshotUtils.java
Outdated
Show resolved
Hide resolved
Co-authored-by: Manoel Aranda Neto <[email protected]>
sentry-android-core/src/main/java/io/sentry/android/core/internal/util/ScreenshotUtils.java
Outdated
Show resolved
Hide resolved
Left the last comment but all is good on my side. |
I've noticed the Benchmark |
it's just flaky |
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 👍 can you also please add a changelog entry - I understand that it's more-or-less an internal change, but if something goes wrong and this PR causes bugs, we can quickly look at the past releases and see if there was something suspicious in the changelog.
sentry-android-core/src/main/java/io/sentry/android/core/CurrentActivityHolder.java
Show resolved
Hide resolved
Changelog is meant for user-facing changes, I'd not add it but your call :) |
This reverts commit 3bb055f.
📜 Description
To make screenshots work on RN we need to set the current activity for the ScreenshotEventProcessor when initializing the Android SDK otherwise it will be null.
We will be sending the event with a screenshot therefore we need to ensure another screenshot won't be added by the Android SDK.
💡 Motivation and Context
💚 How did you test it?
📝 Checklist
🔮 Next steps
#skip-changelog