Fix invalid log file being attached to crash events on Mac/iOS #873
Conversation
| isScreenshotAttachmentEnabled = settings->AttachScreenshot; | ||
| isGameLogAttachmentEnabled = settings->EnableAutoLogAttachment; |
There was a problem hiding this comment.
Should these go on the actual options? We're starting to mix settings. with options. here. I'd really prefer the options to be the source of truth of:
- What options exist?
- What options does the SDK use.
This will also make it easier for users to browse and explore and for us to reason through the SDK's behaviour.
There was a problem hiding this comment.
These two flags are cached separately here since they don't necessarily have a direct representation in Cocoa SDK options:
isGameLogAttachmentEnabledis Unreal-specific featureisScreenshotAttachmentEnabledcan't be just set viaoptions.attachScreenshotas it's an iOS-only thing insentry-cocoa
Accessing them using FSentryModule::Get().GetSettings() could cause unwanted ensures when called from a non-game thread.
There was a problem hiding this comment.
attachScreenshot
This option (with this name) also exists on Android, .NET, Godot, Unity.
There was a problem hiding this comment.
I might be missing the concern here. settings properties are passed to the Cocoa SDK’s options within the startWithConfigureOptions block right after we make copies of these flags here (mainly for convenience as we use them in a few other places beyond initialization). Or is this about the naming?
There was a problem hiding this comment.
I guess I never realized we call it 'settings', where everywhere else we call options in Sentry .. options.
Is this a UE thing to call stuff settings?
There was a problem hiding this comment.
Yes, it's a common naming convention for Unreal - ISettingsModule::RegisterSettings
bruno-garcia
left a comment
bruno-garcia
left a comment
There was a problem hiding this comment.
I'm concerned that we have no tests for this. Do we have iOS device tests we could verify this change and avoid regressions?
| ### Fixes | ||
|
|
||
| - Fix warnings caused by deprecated Cocoa SDK API usages ([#868](https://github.com/getsentry/sentry-unreal/pull/868)) | ||
| - Fix invalid log file being attached to crash events on Mac/iOS ([#873](https://github.com/getsentry/sentry-unreal/pull/873)) |
There was a problem hiding this comment.
The section on the changelog is already "Fixes" so we could (going forward, from the next release perhaps) avoid starting the message with Fix.
Personally I'd be more interested in which platform the fix relates to. So this is a iOS/Mac, could be:
- iOS/macOS: Correct log file now attached to crashes. Since crashes are sent on restart, the SDK now looks at the previous run's log files to attach. Leaving the current run's log file untouched
Or something like that.
| isScreenshotAttachmentEnabled = settings->AttachScreenshot; | ||
| isGameLogAttachmentEnabled = settings->EnableAutoLogAttachment; |
There was a problem hiding this comment.
attachScreenshot
This option (with this name) also exists on Android, .NET, Godot, Unity.
plugin-dev/Source/Sentry/Private/Apple/AppleSentrySubsystem.cpp
Outdated
Show resolved
Hide resolved
At the moment we don’t have a way to automate testing on mobile so manual testing is our only option for now. |
bruno-garcia
left a comment
bruno-garcia
left a comment
There was a problem hiding this comment.
Lets ship this to unblock folks, and follow up with device tests if we can
| isScreenshotAttachmentEnabled = settings->AttachScreenshot; | ||
| isGameLogAttachmentEnabled = settings->EnableAutoLogAttachment; |
There was a problem hiding this comment.
I guess I never realized we call it 'settings', where everywhere else we call options in Sentry .. options.
Is this a UE thing to call stuff settings?
This PR fixes an issue where the game log file from the current session was being attached to a captured crash event before sending it to Sentry instead of attaching the log file from the previous session in which the crash occurred.
It leverages the same approach used for screenshots (#849) by sending the log file in a separate envelope within the
onCrashedLastRunhandler.Closes #302
Also, supersedes #732