feat: Attach screenshots for errors#1751
Conversation
|
Codecov Report
@@ 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
Continue to review full report at Codecov.
|
|
|
|
Does it work even if the event isn't called by the main thread? |
Yes |
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
left a comment
philipphofmann
left a comment
There was a problem hiding this comment.
I know it's a draft, but we are still missing tests. I'm surprised by how simple the solution is. I like it 👏
|
|
||
| @implementation SentryScreenshot | ||
|
|
||
| - (NSArray<NSData *> *)appScreenshots |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
It is a little expensive.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Writing these kind of comments can drive users away from the feature.
💯
The thing is, using |
philipphofmann
left a comment
philipphofmann
left a comment
There was a problem hiding this comment.
I like the new proposal; it decouples the client from attaching screenshots. Good job. We are only missing tests, I guess.
Well, it works, but gives a lot of warnings, which are annoying. |
philipphofmann
left a comment
philipphofmann
left a comment
There was a problem hiding this comment.
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.
brustolin
changed the title
Co-authored-by: Philipp Hofmann <[email protected]>
philipphofmann
left a comment
philipphofmann
left a comment
There was a problem hiding this comment.
We are only missing the UITest. Thanks.
6 participants
📜 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
🔮 Next steps