Fix invalid assertion issues grouping #537
Conversation
# Conflicts: # CHANGELOG.md # plugin-dev/Source/Sentry/Private/Android/SentrySubsystemAndroid.h # plugin-dev/Source/Sentry/Private/Apple/SentrySubsystemApple.h # plugin-dev/Source/Sentry/Private/Desktop/SentrySubsystemDesktop.h # plugin-dev/Source/Sentry/Private/Interface/SentrySubsystemInterface.h # plugin-dev/Source/Sentry/Private/SentrySubsystem.cpp # plugin-dev/Source/Sentry/Public/SentrySubsystem.h
|
On Android assertions are intercepted by |
|
@tustanivsky is there an update on this change? |
There was a problem hiding this comment.
Thanks for all the effort put into this! It'll be a huge QOL improvement!
There are just a couple of things that concern me:
- Addition to
CaptureException/Assertion/Ensureto the public API. As far as I can tell they are getting used by theOutputDevice? But since they are public and user facing they don't make much sense to me - especially with theskip framesin the signature. - I'm not a huge fan of modifying the stacktrace on the client to "clean it up" as this gets messy easily. Updates and changes to the engine might require changes to the SDK which will require users to update their project e.t.c. So ideally, this would be done by the server. Since this works and we don't have an alternative in place I'm fine with adding this to the SDK but it should be done in a way that's easy for us to remove/replace. And it should be done away from anything
public.
| @@ -89,6 +94,24 @@ public Double sample(SamplingContext samplingContext) { | |||
| }); | |||
| } | |||
|
|
|||
| private static void checkForUnrealException(SentryEvent event) { | |||
There was a problem hiding this comment.
This does a whole lot more than just check for an Unreal exception as it returns nothing but modifies an event.
There was a problem hiding this comment.
Yeah, the naming could be better here - changed it to a more generic preProcessEvent yet I'm open to considering other options.
| @@ -169,6 +169,32 @@ USentryId* SentrySubsystemAndroid::CaptureEventWithScope(USentryEvent* event, co | |||
| return SentryConvertorsAndroid::SentryIdToUnreal(*id); | |||
| } | |||
|
|
|||
| USentryId* SentrySubsystemAndroid::CaptureException(const FString& type, const FString& message, int32 framesToSkip) | |||
There was a problem hiding this comment.
I don't follow this. CaptureException does not actually capture an exception. Instead it adds a whole bunch of markers that are later getting picked up by the bridge?
plugin-dev/Source/Sentry/Private/Android/SentrySubsystemAndroid.cpp
Outdated
Show resolved
Hide resolved
| @@ -153,6 +153,7 @@ void SentrySubsystemDesktop::InitWithSettings(const USentrySettings* settings, U | |||
| sentry_options_set_max_breadcrumbs(options, settings->MaxBreadcrumbs); | |||
| sentry_options_set_before_send(options, HandleBeforeSend, this); | |||
| sentry_options_set_on_crash(options, HandleBeforeCrash, this); | |||
| sentry_options_set_shutdown_timeout(options, 3000); | |||
There was a problem hiding this comment.
Where is this coming from?
There was a problem hiding this comment.
Previously, UE caused the app to crash whenever an assertion failed and the Crashpad captured these crashes for us. We've changed how assertions are handled so now when it fails a timeout is used to ensure a corresponding event is sent to Sentry within the same session before shutting down the app manually.
Co-authored-by: Stefan Jandl <reg@bitfox.at>
|
One more thing that comes to mind: We can mark those engine frames as |
|
Since there are quite a few questions regarding the assertions/ensures capturing on Android I'll try to address these with the following explanation: The general idea of this PR is to hook into Unreal's error handling process using a custom The problem with this approach on Android is that Java event's callstack doesn't contain any C++ frames and thus it's not informative at all. There's no straightforward way to convert a native callstack which we can obtain on Unreal's side to a corresponding Java structure to override the default one either. However, instead of reporting assert in a described way within a single session we can set some flags identifying that it happened and force the app to crash. This allows to pass things over to The assertion-specific flags are set to an event via tags and not context since |
This PR adds a custom
FOutputDeviceErrorimplementation which should allow to hook into Unreal's error handling flow and override assert reporting logic.By default, an exception is raised whenever assert is fired, which is then captured by
crashpad/breakpadbackend. The topmost callstack frames for such events are always the same preventing them from being grouped properly and they all appear under a single issue at Sentry.To avoid this situation instead of crashing the app a corresponding event can be constructed manually and reported via
CaptureEvent. Once it's done the app is forced to shut down which somewhat repeats the original flow.The current problem with the approach suggested here is that the engine's error handling is mixed up with some platform-specific logic (i.e. logging, restoring UI, etc.) which leads to skipping some parts of it when using the new
FSentryOutputDeviceError.Closes #489
Closes #514
Closes #1