feat: Attach screenshots for errors

[brustolin]

📜 Description

Feat: screenshots
https://develop.sentry.dev/sdk/features/#screenshots=

💡 Motivation and Context

Users may want to see what's in the screen when there's an error or crash.

close #1721

💚 How did you test it?

Unit Tests and Samples

📝 Checklist

• I reviewed the submitted code
• I added tests to verify the changes
• I updated the docs if needed
• Review from the native team if needed
• No breaking changes

🔮 Next steps

[github-actions]

	Messages
📖	Do not forget to update Sentry-docs with your feature once the pull request gets approved.

Generated by 🚫 dangerJS against 0cc8284

[codecov-commenter]

Codecov Report

Merging #1751 (0cc8284) into master (da18470) will decrease coverage by 0.15%.
The diff coverage is 78.49%.

@@            Coverage Diff             @@
##           master    #1751      +/-   ##
==========================================
- Coverage   91.82%   91.66%   -0.16%     
==========================================
  Files         195      198       +3     
  Lines        8914     9001      +87     
==========================================
+ Hits         8185     8251      +66     
- Misses        729      750      +21     

Impacted Files	Coverage Δ
Sources/Sentry/SentryUIApplication.m	0.00% <0.00%> (ø)
Sources/Sentry/SentryDependencyContainer.m	96.29% <80.00%> (-3.71%)	⬇️
Sources/Sentry/Public/SentryOptions.h	100.00% <100.00%> (ø)
Sources/Sentry/SentryClient.m	99.14% <100.00%> (+0.01%)	⬆️
Sources/Sentry/SentryOptions.m	99.56% <100.00%> (+<0.01%)	⬆️
Sources/Sentry/SentryScreenshot.m	100.00% <100.00%> (ø)
Sources/Sentry/SentryScreenshotIntegration.m	100.00% <100.00%> (ø)
Sources/Sentry/include/SentryDependencyContainer.h	42.85% <100.00%> (+22.85%)	⬆️
Sources/Sentry/SentryThreadHandle.cpp	68.34% <0.00%> (-0.72%)	⬇️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update da18470...0cc8284. Read the comment docs.

[marandaneto]

captureCrashEvent and captureEvent are not calling buildErrorEvent, does that mean it won't attach screenshots?

[marandaneto]

Does it work even if the event isn't called by the main thread?

[brustolin]

Does it work even if the event isn't called by the main thread?

Yes

[brustolin]

captureCrashEvent and captureEvent are not calling buildErrorEvent, does that mean it won't attach screenshots?

Should all events, other then transactions, have a screenshot? I thought only errors and exceptions.

Crash events cannot be created by the user, so I also chose not to include a screenshot.

[marandaneto]

captureCrashEvent and captureEvent are not calling buildErrorEvent, does that mean it won't attach screenshots?

Should all events, other then transactions, have a screenshot? I thought only errors and exceptions.

Crash events cannot be created by the user, so I also chose not to include a screenshot.

Every event that has an exception should attach the screenshot, an Event created by the user might have an exception, e.g. a synthetic exception, See https://github.com/getsentry/sentry-java/blob/main/sentry/src/main/java/io/sentry/SentryEvent.java#L242-L249=

[philipphofmann]

I know it's a draft, but we are still missing tests. I'm surprised by how simple the solution is. I like it 👏

[philipphofmann]

m: How much overhead is it to take a screenshot? If it's expensive, I think we should add it to the docs and the code docs of SentryOptions

[brustolin]

It is a little expensive.

[brustolin]

I don't think we should point this out on documentation. Developers should have a little notion on how expensive is to draw the entire screen.
And anyway, this captures are only done on-demand, is not like this would be harmful to the app execution.

Writing these kind of comments can drive users away from the feature.

[bruno-garcia]

Writing these kind of comments can drive users away from the feature.

💯

[brustolin]

I think it should also be possible to send a screenshot with the naming screenshot-0.png. This would make the logic simpler here. Otherwise, it would be

The thing is, using screenshot.png is the way frontend knows how to show it. Appending 0 would be problematic. Im checking with @priscilawebdev how to proceed.

[philipphofmann]

I like the new proposal; it decouples the client from attaching screenshots. Good job. We are only missing tests, I guess.

[brustolin]

Does it work even if the event isn't called by the main thread?

Yes

Well, it works, but gives a lot of warnings, which are annoying.
This may break in the future.
I'll synchronize this with the main thread.

[philipphofmann]

I like the solution. We have to update the docs after releasing this PR, which should be easy as Android already has them https://docs.sentry.io/platforms/android/configuration/options/#attach-screenshot

Running the SentryScreenShotTests on an iPhone 7 simulator fails with

Terminating app due to uncaught exception 'NSInvalidArgumentException', reason: '*** -[__NSArrayM insertObject:atIndex:]: object cannot be nil

I think https://github.com/getsentry/sentry-cocoa/runs/6092111176?check_suite_focus=true fails for the same reason.

I think we should add a UI test that clicks the error button for the UI tests of the iOS-Swift sample app, just to be safe we are not crashing.

[philipphofmann]

We are only missing the UITest. Thanks.

[philipphofmann]

LGTM 🚀