-
-
Notifications
You must be signed in to change notification settings - Fork 435
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
Add Android View Hierarchy support #2440
Conversation
|
* @param viewHierarchy the View Hierarchy | ||
* @return the Attachment | ||
*/ | ||
public static @Nullable Attachment fromViewHierarchy(final ViewHierarchy viewHierarchy) { |
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.
@romtsn I really don't like this part
- this will do the serialization on the main thread
- it's accessing
Sentry.getCurrentHub().getOptions().getSerializer()
- which it probably shouldn't do, and which makes it hard to test
Now I'm wondering: Would it make sense to refactor the Attachment
class and provide an option for having serialize-able payload?
e.g.
abstract class Attachment {}
class RawAttachment extends Attachment {
private final byte[] bytes;
}
class JsonAttachment extends Attachment {
private final JsonSerializable serializable;
}
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.
re. options - I think we can just pass them as a parameter from the event processor, right? then it'd be testable.
re. serialization on main thread - I think we can offload it to another thread, but should still block and wait for it, because we're doing it only for errored events and the app is gonna crash anyway? That's what we do for screenshots as well. But I'm wondering how other SDKs did that? @marandaneto @brustolin
Imo we should block here, because the app may crash, and if we don't wait for it, then we'll lose the attachment
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.
@markushi I do the serialization on the background, you can do the same on Android, attachments already have a callback that is only called during the event sending (and reads the bytes async).
You can check the Dart PR or the Android code.
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 moved the serialization part to theViewHierarchyProcessor
, looking way better now in terms of testability.
re. serialization on main thread: You're right, view hierarchies are only generated when the app crashes, thus it shouldn't be an issue. (On a another note: The Android -> Native scope sync is performing serialization on the main thread as well, maybe that's something we should fix).
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.
For cocoa, if it's a crash we serialize it in the crash thread, which is not the main. And if is an error capture we serialize in the main thread otherwise there is a bunch of warnings that we are calling UI methods in a background thread.
Performance metrics 🚀
|
Revision | Plain | With Sentry | Diff |
---|---|---|---|
4a9c176 | 320.62 ms | 334.68 ms | 14.06 ms |
a04f788 | 321.78 ms | 354.12 ms | 32.35 ms |
f1150bc | 306.15 ms | 306.58 ms | 0.44 ms |
d00c464 | 337.43 ms | 387.57 ms | 50.15 ms |
f55020d | 338.00 ms | 370.74 ms | 32.74 ms |
4a9c176 | 319.77 ms | 363.20 ms | 43.43 ms |
703d523 | 275.51 ms | 323.02 ms | 47.51 ms |
ecf9680 | 321.55 ms | 385.52 ms | 63.97 ms |
b85d8aa | 289.35 ms | 335.92 ms | 46.56 ms |
90e9745 | 314.68 ms | 357.28 ms | 42.60 ms |
App size
Revision | Plain | With Sentry | Diff |
---|---|---|---|
4a9c176 | 1.73 MiB | 2.33 MiB | 612.69 KiB |
a04f788 | 1.73 MiB | 2.32 MiB | 609.88 KiB |
f1150bc | 1.73 MiB | 2.33 MiB | 615.66 KiB |
d00c464 | 1.73 MiB | 2.33 MiB | 613.02 KiB |
f55020d | 1.73 MiB | 2.33 MiB | 616.54 KiB |
4a9c176 | 1.73 MiB | 2.33 MiB | 612.69 KiB |
703d523 | 1.73 MiB | 2.33 MiB | 613.23 KiB |
ecf9680 | 1.73 MiB | 2.32 MiB | 612.39 KiB |
b85d8aa | 1.73 MiB | 2.32 MiB | 611.62 KiB |
90e9745 | 1.73 MiB | 2.32 MiB | 608.63 KiB |
Previous results on branch: feat/view-hierarchy
Startup times
Revision | Plain | With Sentry | Diff |
---|---|---|---|
f194df4 | 333.84 ms | 375.43 ms | 41.59 ms |
8a345b2 | 419.88 ms | 479.98 ms | 60.10 ms |
660db01 | 351.49 ms | 375.48 ms | 23.99 ms |
8120979 | 323.25 ms | 370.59 ms | 47.34 ms |
f96fd31 | 333.66 ms | 396.88 ms | 63.22 ms |
8d532d2 | 302.78 ms | 358.23 ms | 55.45 ms |
4f4ea04 | 326.26 ms | 373.66 ms | 47.40 ms |
4a4964e | 454.85 ms | 474.06 ms | 19.21 ms |
2733cd6 | 325.02 ms | 382.14 ms | 57.12 ms |
a35c0bf | 290.51 ms | 367.54 ms | 77.03 ms |
App size
Revision | Plain | With Sentry | Diff |
---|---|---|---|
f194df4 | 1.73 MiB | 2.33 MiB | 618.12 KiB |
8a345b2 | 1.73 MiB | 2.33 MiB | 618.02 KiB |
660db01 | 1.73 MiB | 2.33 MiB | 618.12 KiB |
8120979 | 1.73 MiB | 2.33 MiB | 618.10 KiB |
f96fd31 | 1.73 MiB | 2.33 MiB | 618.12 KiB |
8d532d2 | 1.73 MiB | 2.33 MiB | 619.13 KiB |
4f4ea04 | 1.73 MiB | 2.33 MiB | 618.12 KiB |
4a4964e | 1.73 MiB | 2.33 MiB | 618.37 KiB |
2733cd6 | 1.73 MiB | 2.33 MiB | 619.22 KiB |
a35c0bf | 1.73 MiB | 2.33 MiB | 619.24 KiB |
Codecov ReportBase: 80.13% // Head: 80.14% // Increases project coverage by
Additional details and impacted files@@ Coverage Diff @@
## main #2440 +/- ##
==========================================
Coverage 80.13% 80.14%
- Complexity 3834 3870 +36
==========================================
Files 310 312 +2
Lines 14469 14664 +195
Branches 1915 1939 +24
==========================================
+ Hits 11595 11752 +157
- Misses 2124 2153 +29
- Partials 750 759 +9
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report at Codecov. |
Fixes resolve errors when performing Android Studio gradle sync
This way the CurrentActivityHolder can provide the activity to both the screenshot and viewhierarchy integration
sentry-android-core/src/main/java/io/sentry/android/core/ViewHierarchyEventProcessor.java
Outdated
Show resolved
Hide resolved
sentry-android-core/src/main/java/io/sentry/android/core/ViewHierarchyEventProcessor.java
Outdated
Show resolved
Hide resolved
sentry-android-core/src/main/java/io/sentry/android/core/ViewHierarchyEventProcessor.java
Outdated
Show resolved
Hide resolved
sentry-android-core/src/main/java/io/sentry/android/core/internal/gestures/ViewUtils.java
Outdated
Show resolved
Hide resolved
M: A few methods/params/fields are missing the nullability annotations or |
Based on PR comments
Co-authored-by: Roman Zavarnitsyn <[email protected]>
Co-authored-by: Roman Zavarnitsyn <[email protected]>
Useful to postphone serialization of attachment payload to the point when it's needed
sentry-android-core/src/main/java/io/sentry/android/core/AndroidOptionsInitializer.java
Outdated
Show resolved
Hide resolved
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.
Very nice, I like how straightforward the ViewHierarchyProcessor looks 👍 🚀
sentry-android-core/src/main/java/io/sentry/android/core/ViewHierarchyEventProcessor.java
Outdated
Show resolved
Hide resolved
I've tried the snapshotViewHierarchy on RN. It works but I get a lot "invalid id" error logs. Any ideas about what it could cause? The whole implementation is here getsentry/sentry-react-native@ logger.log(SentryLevel.INFO, "snapshot viewHierarchy");
final @NotNull ViewHierarchy viewHierarchy = ViewHierarchyEventProcessor.snapshotViewHierarchy(decorView);
|
Gets rid of "Invalid ID 0x" logcat output spam
@krystofwoldrich I think I've found the culprit which was happening when converting view IDs to their resource names (e.g. button_login), a fix is already pushed, could you give it another try? |
Thanks. I will try. Update: @markushi the |
Gets rid of "Invalid ID" logcat error output
@krystofwoldrich I just pushed another fix for that. Apparently RN creates view IDs on the fly using |
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. Works great with RN. Thanks @markushi
📜 Description
Add an event processor which captures the View Hierarchy.
💡 Motivation and Context
getsentry/team-mobile#64
Some refactoring was required:
Created at
CurrentActivityIntegration
which watches for activity lifecycle updates and updates theCurrentActivityHolder
singleton - this was previously managed by theScreenshotEventProcessor
.Now both
ScreenshotEventProcessor
andViewHierarchyEventProcessor
use theCurrentActivityHolder
to retrieve the current activity.Open issues:
event.view_hierarchy
type isn't shown in the Issues page relay#1704 needs fixed first before we can use the properattachment_type
💚 How did you test it?
Unit tests
📝 Checklist
🔮 Next steps