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?
| @@ -9,6 +9,7 @@ | |||
| ### 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.
| } | ||
|
|
||
| TSharedPtr<ISentryId> FAppleSentrySubsystem::CaptureEventWithScope(TSharedPtr<ISentryEvent> event, const FSentryScopeDelegate& onConfigureScope) | ||
| { | ||
| TSharedPtr<SentryEventApple> eventIOS = StaticCastSharedPtr<SentryEventApple>(event); | ||
|
|
||
| SentryId* id = [SENTRY_APPLE_CLASS(SentrySDK) captureEvent:eventIOS->GetNativeObject() withScopeBlock:^(SentryScope* scope) { | ||
| if(isGameLogAttachmentEnabled) { | ||
| const FString logFilePath = GetGameLogPath(); | ||
| SentryAttachment* logAttachment = [[SENTRY_APPLE_CLASS(SentryAttachment) alloc] initWithPath:logFilePath.GetNSString()]; |
There was a problem hiding this comment.
I'm trying to diff the 3 versions of this snippet in this PR but they all seem the same?
If we're changing things would you want them all to be consistent?
I think it's better to throw this into a function, add some debug logging in there (central point) for example
At the moment we don’t have a way to automate testing on mobile so manual testing is our only option for now. |
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