Add screenshot context

[timfish]

After adding screenshot capture to the Electron SDK, we realised that with multiple windows (and therefore multiple screenshots) it would be great to have some more context to help determine which screenshot is most relevant to the captured event.

Primarily it would be nice to know which window was the source of the error but it's also handy to have other context here such as window focus to have a better idea of which windows might be getting user interaction.

[vercel]

@timfish is attempting to deploy a commit to the Sentry Team on Vercel.

A member of the Team first needs to authorize it.

[AbhiPrasad]

Sharing with the team so we can get more feedback!

[AbhiPrasad]

Can we introduce a more opinionated schema for now?

type ScreenshotContext = {
  [key: string]: {
    focus?: boolean;
    errored?: boolean;
  } & Record<string, unknown>,
};

[bruno-garcia]

How about this info going in the attachment envelope header?
Somehow being made available to the attachment, the data is used by the thumbnail component in the UI

[brustolin]

We have some specs for screenshot.

Probably the more relevant one would be the first screenshot, all others are just additional attachments. Front only displays the first one for now.

[dcramer]

Is there a reason this isnt handled through the attachments API? For example adding metadata to the attachment itself? It feels like an anti-pattern to disconnect the data in this way.

I dont really see a reason we can't take all images and just render them in a gallery, and if we really need one to pull it out, we could just allow the ability to mark a primary (again, via attachment metadata).

Otherwise this feels like we're bleeding internals into contexts, and contexts are supposed to just be additional contextual information.

[AbhiPrasad]

We chatted about attaching metadata to attachments in a previous TSC: https://www.notion.so/sentry/Screenshots-TXN-Heuristics-Mutability-357f4b9365cc41228e36ed6286cb5b24#42b3c1f74b2a462fb3307ed1404a8d06. Linking notes below as this is a private notion doc.

- aprasad: we’re almost done with adding screenshots as attachments in sdks
- different attachments mean different things, where to put metadata about an attachment?
- extra fields in attachment envelope item?
- info in attachment name?
- what metadata?
    - annotate the screenshot of the focussed view
    - annotate the screenshot of the errored view
- could attach as event context
    - ideally it would be part of the attachment though
- jan: generally attachments can have metadata (the name is one), but the way we store that metadata probably doesn’t scale for custom attributes
- attachment types: add screenshots?

We decided against the attachment metadata because of the difficulty with custom attributes on the indexing side - perhaps @jan-auer can clarify here.

[AbhiPrasad]

A quick idea I had. As per https://develop.sentry.dev/sdk/envelopes/#attachment, we do have the attachment_type field, which we do index. We can add event.screenshot as an attachment type - that'll give us the freedom to no longer restrict the filenames to be called screenshot-.png or screenshot-1.png. Then we can just attach the metadata as filename, for example call it errored.png or focused.png, and the product is aware of these differences because they are all considered screenshots as per the attachment_type of the attachment.

@bruno-garcia @timfish thoughts on the above?

[dcramer]

Why do we care about filenames? I feel like im missing something or we're overthinking this problem. Are we trying to build a UI that shows explicitly labeled states? Do we really need that? is it really superior to just showing a gallery? The simpler we can make the MVP here the less re-work we may have in the future if we need to augment things for different paradigms? Like, why cant we just use the attachment alt text/title/label to indicate what it is, rather than creating additional complexity here?

Keep in mind we can basically never delete SDK schema.

[bruno-garcia]

Is there a reason this isnt handled through the attachments API? For example adding metadata to the attachment itself? It feels like an anti-pattern to disconnect the data in this way.

This makes sense to me. I think the solution in this PR isn't the way to go.

A quick idea I had. As per develop.sentry.dev/sdk/envelopes/#attachment, we do have the attachment_type field, which we do index. We can add event.screenshot as an attachment type - that'll give us the freedom to no longer restrict the filenames to be called screenshot-.png or screenshot-1.png. Then we can just attach the metadata as filename, for example call it errored.png or focused.png, and the product is aware of these differences because they are all considered screenshots as per the attachment_type of the attachment.

This also makes sense to me. But it requires some context to the reader to understand why it is a good way forward.

Why do we care about filenames? I feel like im missing something or we're overthinking this problem.

The screenshot thumbnail component relies on convention to detect a screenshot.

It's basically "attachment with file name screenshot\.png|jpg. More details here: https://develop.sentry.dev/sdk/features/#screenshots

We document for customers here: https://docs.sentry.io/platforms/android/enriching-events/screenshots/

Are we trying to build a UI that shows explicitly labeled states? Do we really need that? is it really superior to just showing a gallery? The simpler we can make the MVP here the less re-work we may have in the future if we need to augment things for different paradigms? Like, why cant we just use the attachment alt text/title/label to indicate what it is, rather than creating additional complexity here?

The UI is super simple right now and only shows 1 screenshot. We understood the limitation of devices with multiple screens but that wasn't a concern for us to ship the first version. In particular for mobile that multiple screen devices are super rare, for now.

Adding the new attachment type as @AbhiPrasad suggested would help us expand the screenshots feature without concern to impact in the overall attachments type. I believe @jan-auer also had thoughts on this matter and might want to chime in.

Keep in mind we can basically never delete SDK schema.

👍

[dcramer]

@bruno-garcia would it be bad to not just throw any screenshots up there? make it so you can cycle through them, and make it work beyond mobile? you could still mimetype detect. i get its not 100% the same, but i dont really think its worse. you could also aspect-ratio detect to find a desired primary photo

[bruno-garcia]

@bruno-garcia would it be bad to not just throw any screenshots up there? make it so you can cycle through them, and make it work beyond mobile? you could still mimetype detect. i get its not 100% the same, but i dont really think its worse. you could also aspect-ratio detect to find a desired primary photo

We can definitely do that. I'm just not sure then if the imagine in that case is "worthy" of being put on top of the issues detail page. The idea for screenshots was that it was indeed very important, hence the thumbnail was focused on screenshot.

Some additional context:

• sdk built-in screenshot feature #422
• feat: Update screenshot feature detail #563
• GA screenshots team-mobile#17

[AbhiPrasad]

We've decided we're gonna take another look at attaching metadata to the attachment itself - will update this PR with a link to a GH issue/new develop PR once that has been fleshed out. Thanks for raising this discussion @timfish!