Skip to content
This repository was archived by the owner on Aug 14, 2024. It is now read-only.

sdk built-in screenshot feature #422

Merged
merged 6 commits into from
Mar 11, 2022
Merged

sdk built-in screenshot feature #422

Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
389395b
sdk built-in screenshot feature
bruno-garcia Aug 31, 2021
33ed6ec
Update src/docs/sdk/features.mdx
bruno-garcia Aug 31, 2021
49976bd
Update src/docs/sdk/features.mdx
bruno-garcia Aug 31, 2021
a96199a
Update src/docs/sdk/features.mdx
bruno-garcia Dec 14, 2021
11d87c6
Update src/docs/sdk/features.mdx
bruno-garcia Dec 14, 2021
4e5ecc6
Apply suggestions from code review
bruno-garcia Dec 14, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 16 additions & 0 deletions src/docs/sdk/features.mdx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -141,6 +141,22 @@ envelope item, the SDK must discard items larger than the `maxAttachmentSize`. E
useful because attachments could quickly eat up the users' disk space. Furthermore, Relay has a
[maximum size for attachments](https://docs.sentry.io/product/relay/options/), and we want to reduce unnecessary requests.

## Screenshots

When the user opts-in, if technically possible, take a screenshot of the application and include as an [attachment](#attachments) to the [envelope](/sdk/envelopes/) with the event.

This feature only applies to SDKs with a user interface, such as Mobile and Desktop.
In some environments such as native iOS, taking a screenshot requires the UI thread and in the event of a crash, that might not be available. So inherently this feature will be a best effort solution.
It's advised to provide this feature through a single option called `attachScreenshot`. That's the preferred way but in platforms such as Flutter, a wrapping widget is required so documentation can point users to that instead of the suggested option name.

Copy link
Contributor

@lucas-zimerman lucas-zimerman Sep 1, 2021

Choose a reason for hiding this comment

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

Suggested change
Furthermore, if `attachScreenshot` is enabled, the SDK should attach screenshots to events with levels higher or equal to error, to avoid attaching screenshots to debug/info events (the same could be configurable).

My suggestion.

Copy link
Contributor

@marandaneto marandaneto Sep 1, 2021

Choose a reason for hiding this comment

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

instead of checking the event level, I'd rather consider if its an errored or crashed event via exception list/mechanism, that's how it works for session, maybe we do the same? https://github.com/getsentry/sentry-java/blob/main/sentry/src/main/java/io/sentry/SentryEvent.java#L223-L249

Copy link
Member Author

@bruno-garcia bruno-garcia Sep 1, 2021

Choose a reason for hiding this comment

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

Similar to attachStacktrace I was hoping we'd attach the screenshot on all events.
Perhaps we should consider a shouldAttachScreenshot callback that receives the event as an argument?
If the callback is null we always attach, but it gives the user a chance to configure things like: only attach on unhandled exception or other use cases

Copy link
Contributor

@marandaneto marandaneto Sep 9, 2021

Choose a reason for hiding this comment

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

I guess we have to give the user a chance to not add the screenshot (either via callback or only for more important events) since we also detect events automatically and taking SS is visible by the user (the screen kinda flashes out for a few millis)

Copy link
Contributor

@rhcarvalho rhcarvalho Sep 9, 2021

Choose a reason for hiding this comment

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

Seems reasonable to make it very clear and intentional when screenshots (in the order of megabytes) go out of the SDK to avoid excessive data consumption.

Copy link
Member Author

@bruno-garcia bruno-garcia Dec 14, 2021

Choose a reason for hiding this comment

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

Agree with this. Lets consider this a second iteration.

The feature is achieved by adding an attachment with:

* File name `screenshot.jpg` or `screenshot.png`
Copy link
Contributor

@lucas-zimerman lucas-zimerman Sep 1, 2021

Choose a reason for hiding this comment

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

should we mention that it should overwrite any existing attachment with that given name during the process of attaching the screenshot?

Copy link
Contributor

@marandaneto marandaneto Sep 1, 2021

Choose a reason for hiding this comment

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

its a good point, or doing a sort of send it once and drop it, so the same attachment won't be attached to every following event

Copy link
Member Author

@bruno-garcia bruno-garcia Sep 1, 2021

Choose a reason for hiding this comment

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

I think we should just add one and if there's always one added, the front end will just pick one of them. It's an edge case I rather the SDK doesn't have custom code to look for stuff in the attachments list (it can also be added after our check though so it would anwyay be best effort)

Copy link
Contributor

@marandaneto marandaneto Sep 9, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

so if there were 3 events before, the 4th event contains the 3 previous attachments + the new one? I'd say it's burning users' data and it's not that nice.

Copy link
Member Author

@bruno-garcia bruno-garcia Dec 14, 2021

Choose a reason for hiding this comment

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

The SDK should add a single attachment when capturing the event. It'll not keep adding to the scope. Each capture includes a fresh screenshot. If the user had already added one manually with the same name, that's just an edge case I don't think worth spending too many cycles on.

* Image size, if possible should stay below 2 MB but quality/size could be configurable
* `ContentType: image/jpg` or `ContentType: image/png`
Copy link
Member

@AbhiPrasad AbhiPrasad Dec 14, 2021

Choose a reason for hiding this comment

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

Any particular reason why we need the ContentType? Does relay/the sentry product treat this differently>

Copy link
Member Author

@bruno-garcia bruno-garcia Dec 15, 2021

Choose a reason for hiding this comment

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

At the end we didn't rely on it at all for the preview. It's just file extension and file name. ContentType was @mitsuhiko's idea so maybe he can help us with a direction here

Copy link
Contributor

@mitsuhiko mitsuhiko Dec 15, 2021

Choose a reason for hiding this comment

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

If no content type is set then sentry will try to guess it. I would generally prefer we send the content type explicitly.


Whenever possible, avoid adding the attachment altogether if taking the screenshot fails. Alternatively, when streaming, it's possible the envelope header was already flushed through before the attempt to take the screenshot happens. In this case, a 0 byte attachment will be included. In that case, Sentry will not show a screenshot preview.

## Before-Send Hook

Hook called with the event (and on some platforms the hint) that allow the user to decide whether an event should be sent or not. This can also be used to further modify the event.
Expand Down