sdk built-in screenshot feature #422

bruno-garcia · 2021-08-31T20:01:10Z

No description provided.

vercel · 2021-08-31T20:01:13Z

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://vercel.com/sentry/develop/7UU9SLzvChtrqDCgMYDxU9eWJFj4
✅ Preview: https://develop-git-sdk-screenshot-built-in-feature.sentry.dev

src/docs/sdk/features.mdx

lucas-zimerman · 2021-09-01T01:17:26Z

src/docs/sdk/features.mdx

+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.
+It's advised to provide this feature through a single option called `attachScreenshot`. That's a 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.
+


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.

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

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

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)

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.

Agree with this. Lets consider this a second iteration.

lucas-zimerman · 2021-09-01T01:20:21Z

src/docs/sdk/features.mdx

+
+The feature is achieved by adding an attachment with:
+
+* File name `screenshot.jpg` or `screenshot.png`


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

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

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)

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.

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.

rhcarvalho

SGTM

Seems that clarification around when to send (always/once, all events/only errors or crashes, ...) and how to avoid excessive data consumption are still WIP?

src/docs/sdk/features.mdx

rhcarvalho · 2021-09-09T12:37:23Z

src/docs/sdk/features.mdx

+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.
+It's advised to provide this feature through a single option called `attachScreenshot`. That's a 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.
+


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.

AbhiPrasad · 2021-09-09T12:39:41Z

src/docs/sdk/features.mdx


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


Suggested change

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

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

Make sure to add a note on PII.

src/docs/sdk/features.mdx

Co-authored-by: Rodolfo Carvalho <[email protected]>

Co-authored-by: Abhijeet Prasad <[email protected]> Co-authored-by: Rodolfo Carvalho <[email protected]>

AbhiPrasad · 2021-12-14T18:23:36Z

src/docs/sdk/features.mdx

+
+* File name `screenshot.jpg` or `screenshot.png`
+* Image size, if possible should stay below 2 MB but quality/size could be configurable
+* `ContentType: image/jpg` or `ContentType: image/png`


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

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

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

sdk built-in screenshot feature

389395b

vercel bot deployed to Preview August 31, 2021 20:01 View deployment

bruno-garcia commented Aug 31, 2021

View reviewed changes

src/docs/sdk/features.mdx Outdated Show resolved Hide resolved

Update src/docs/sdk/features.mdx

33ed6ec

vercel bot deployed to Preview August 31, 2021 20:59 View deployment

bruno-garcia commented Aug 31, 2021

View reviewed changes

src/docs/sdk/features.mdx Show resolved Hide resolved

Update src/docs/sdk/features.mdx

49976bd

vercel bot deployed to Preview August 31, 2021 21:10 View deployment

lucas-zimerman reviewed Sep 1, 2021

View reviewed changes

priscilawebdev mentioned this pull request Sep 3, 2021

feat(tests): Add mobile screenshot tests getsentry/sentry#28391

Merged

bruno-garcia marked this pull request as ready for review September 9, 2021 12:25

bruno-garcia mentioned this pull request Sep 9, 2021

Support screenshot feature (via attachments) getsentry/sentry-dart#556

Closed

rhcarvalho approved these changes Sep 9, 2021

View reviewed changes

AbhiPrasad reviewed Sep 9, 2021

View reviewed changes

bruno-garcia mentioned this pull request Sep 27, 2021

JavaFX integration getsentry/sentry-java#1743

Closed

bruno-garcia mentioned this pull request Dec 14, 2021

Add AttachScreenshot option getsentry/sentry-unity#284

Closed

bruno-garcia commented Dec 14, 2021

View reviewed changes

src/docs/sdk/features.mdx Show resolved Hide resolved

Update src/docs/sdk/features.mdx

a96199a

vercel bot deployed to Preview December 14, 2021 17:44 View deployment

Update src/docs/sdk/features.mdx

11d87c6

Co-authored-by: Rodolfo Carvalho <[email protected]>

vercel bot deployed to Preview December 14, 2021 17:47 View deployment

Apply suggestions from code review

4e5ecc6

Co-authored-by: Abhijeet Prasad <[email protected]> Co-authored-by: Rodolfo Carvalho <[email protected]>

vercel bot deployed to Preview December 14, 2021 17:50 View deployment

AbhiPrasad approved these changes Dec 14, 2021

View reviewed changes

bruno-garcia merged commit 2cf5e65 into master Mar 11, 2022

bruno-garcia deleted the sdk/screenshot-built-in-feature branch March 11, 2022 00:41

bruno-garcia mentioned this pull request Aug 3, 2022

Add screenshot context #659

Closed


	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).


		The feature is achieved by adding an attachment with:

		* File name `screenshot.jpg` or `screenshot.png`


		## Screenshots

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

Uh oh!

sdk built-in screenshot feature #422

sdk built-in screenshot feature #422

Uh oh!

Conversation

bruno-garcia commented Aug 31, 2021

Uh oh!

vercel bot commented Aug 31, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

marandaneto Sep 9, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rhcarvalho left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

vercel bot commented Aug 31, 2021 •

edited

Loading

marandaneto Sep 9, 2021 •

edited

Loading