Skip to content
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

View Hierarchy #33

Merged
merged 29 commits into from
Nov 28, 2022
Merged

View Hierarchy #33

merged 29 commits into from
Nov 28, 2022

Conversation

brustolin
Copy link
Contributor

@brustolin brustolin commented Nov 2, 2022

Summary

Allow SDKs to send View hierarchy of the application UI.

Motivation

Being able to see what the user was looking at is a good way to found potential problems in a mobile application.
This RFC proposes a way to send information from the UI state of an application, known as "view hierarchy".

Rendered RFC

@marandaneto
Copy link
Contributor

Add RFC ID to the README, See example https://github.com/getsentry/rfcs/pull/22/files

Comment on lines 29 to 30
[
{
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd make proper indentation here.
Maybe the JSON should have a property to define which screen is the left/right or main one.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Depending on the environment we may have the window position in coordinates, but left/right I would say its not relevant.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's just an example, if the x, y, z solves the issue, all good.
We might need a state field tho, for example, on Android, its possible to be
half_open.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Im proposing the minimum required properties. Each platform may add more if needed.

Co-authored-by: Manoel Aranda Neto <5731772+marandaneto@users.noreply.github.com>
@brustolin brustolin changed the title View Hierarch View Hierarchy Nov 3, 2022
README.md Outdated Show resolved Hide resolved
Co-authored-by: Abhijeet Prasad <aprasad@sentry.io>
@ueman
Copy link

ueman commented Nov 3, 2022

What about code (de)obfuscation?

@marandaneto
Copy link
Contributor

What about code (de)obfuscation?

Good point, on Android at least, native widgets such as EditText, etc are not obfuscated, Activities also not, but custom widgets and Fragments are, those will be obfuscated, @brustolin is worth mentioning this on the RFC.
Its a good point to start thinking about obfuscation in all the features, breadcrumbs etc are also obfuscated.

@JoshFerge
Copy link
Member

Our Replay product feature uses the DOM. I think it's worth checking which representation they use.

Do we know what we want to do with the view hierarchy data in Sentry? The data format depends on the answer to the previous question. Maybe we can use an existing solution, which requires a specific data format.

We use rrweb's json format, and their player library. for our hackweek we attempted to translate the iOS view hierarchy into rrweb JSON. you could try something similar for this -- I'm sure instead of a player you could just have it render HTML as well.

@brustolin
Copy link
Contributor Author

@JoshFerge @philipphofmann The plan now is to have a textual tree representation of the View hierarchy. I assume that the RRWeb json format is more complex than what we are proposing and therefore would delay the release of this feature.

@brustolin brustolin requested review from jjbayer and removed request for ueman November 21, 2022 14:47
@brustolin
Copy link
Contributor Author

@jjbayer do you believe we have enough information regarding what we need from ingestion?
Is it possible to approve this proposal?

text/0033-view-hierarchy.md Outdated Show resolved Hide resolved

# Proposal

Capture the view hierarchy in our mobile SDKs and convert it to a common JSON representation. Add it as an [attachment](https://develop.sentry.dev/sdk/envelopes/#attachment) to the envelope. The `attachment_type` is set to `"event.viewhierarchy"` and the `content_type` is set to the `"application/json"` mime-type.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jan-auer since we're not forward compatible in regards to attachment types, meaning that outdated external Relays will fail to parse the envelope when there's a new attachment type, should we just use AttachmentType::Attachment here instead?

I noticed that for screenshots, there is no special attachment type either.

@brustolin
Copy link
Contributor Author

I noticed that for screenshots, there is no special attachment type either.

And this is a problem. Front-end needs to check for attachment file name which IMO is not good.

@jjbayer
Copy link
Member

jjbayer commented Nov 22, 2022

I noticed that for screenshots, there is no special attachment type either.

And this is a problem. Front-end needs to check for attachment file name which IMO is not good.

Discussed this with @jan-auer and agreed that we should introduce a new attachment type. We will add forward compatibility to the attachment type in Relay. Because external Relays could still be outdated when we launch this feature, we should warn users who enable view hierarchies that they have to update their Relays first (if they have any).

@brustolin
Copy link
Contributor Author

Discussed this with @jan-auer and agreed that we should introduce a new attachment type. We will add forward compatibility to the attachment type in Relay. Because external Relays could still be outdated when we launch this feature, we should warn users who enable view hierarchies that they have to update their Relays first (if they have any).

Make sense. Thanks!!

Copy link
Member

@AbhiPrasad AbhiPrasad left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just some grammar clean up, and one final ask about event.view_hierarchy vs. event.viewhierarchy. Overall proposal is 👍 for me.

text/0033-view-hierarchy.md Outdated Show resolved Hide resolved
text/0033-view-hierarchy.md Outdated Show resolved Hide resolved
text/0033-view-hierarchy.md Outdated Show resolved Hide resolved
text/0033-view-hierarchy.md Outdated Show resolved Hide resolved
text/0033-view-hierarchy.md Show resolved Hide resolved
text/0033-view-hierarchy.md Outdated Show resolved Hide resolved
text/0033-view-hierarchy.md Outdated Show resolved Hide resolved
Co-authored-by: Abhijeet Prasad <aprasad@sentry.io>
@brustolin
Copy link
Contributor Author

Thanks @AbhiPrasad!

Copy link
Member

@philipphofmann philipphofmann left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great job 👏 . Let's go ahead and iterate over it if it doesn't work.

text/0033-view-hierarchy.md Outdated Show resolved Hide resolved
text/0033-view-hierarchy.md Show resolved Hide resolved
Copy link
Member

@narsaynorath narsaynorath left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great, thanks for the detailed example for the JSON structure and the expected sizes of view hierarchies. That information became handy in a product meeting with MDX 🙏

@brustolin brustolin merged commit eddd1c6 into main Nov 28, 2022
jjbayer added a commit to getsentry/relay that referenced this pull request Nov 28, 2022
As we did in #1246 for
`ItemType`, add an `Unknown(String)` variant to `AttachmentType`. When
`accept_unknown_items` is set to false, items with this attachment type
will be dropped.

This will allow outdated external Relays to forward new attachment types
instead of dropping the entire envelope.

This defect was discovered in getsentry/rfcs#33,
which introduces a new attachment type.
@chadwhitacre chadwhitacre deleted the feat/view-hierarch branch January 19, 2023 15:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants