-
-
Notifications
You must be signed in to change notification settings - Fork 51
Add support for unreal's additional crash context scopes #1052
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
...ev/Source/Sentry/Private/GenericPlatform/CrashReporter/GenericPlatformSentryCrashContext.cpp
Show resolved
Hide resolved
...ev/Source/Sentry/Private/GenericPlatform/CrashReporter/GenericPlatformSentryCrashContext.cpp
Outdated
Show resolved
Hide resolved
Hey @redxdev, thank you for submitting this PR! We'll be looking forward to Epics accepting you're changes to the engine and also explore if there's more context data we can add like that. |
Looks like epic grabbed the required commit on their side: https://github.com/EpicGames/UnrealEngine/commit/8d79c7e26943f5c3c3ad1291db25488215fa8b09 I've updated the review to check for a minimum version of 5.7. |
We've noticed an issue where this can get into an infinite loop specifically when sending an event that is not a crash (seems to be an issue with some graphics code applying a context scope that probably doesn't expect to be called into when not in an actual crashing state), so this shouldn't be committed as-is - I need to move the application of the engine's context to a location only used when sending an actual crash. |
@redxdev Thanks for following up on this! Could you share a few examples of how you’re using it? |
@tustanivsky we actually aren't using this much at the moment, though the general pattern is something like this:
If a crash happens within the scope the macro is in, then the lambda inside will execute and add whatever data you want to the crash context. So this PR is just exposing that additional context to sentry. The engine itself only seems to use this in two places - one for the The breadcrumbs specifically seem to be what hangs the engine if called for an ensure or any other non-crash event (probably because it's accessing data from the RHI thread that isn't thread-safe unless everything is stopped due to a crash) so the current PR isn't safe - applying these scopes needs to be done only when there's an actual crash and not for other events. |
Hm, unfortunately we've now seen the hang caused by the breadcrumbs context even when crashing. So I guess this isn't safe after all. |
Requires Epic to accept https://github.com/EpicGames/UnrealEngine/pull/13686 first as one required method is not exported from
Core
.Unreal has a little-known macro
UE_ADD_CRASH_CONTEXT_SCOPE
which allows running code to add additional information to a crash if the crash happens within the scope the macro is in. This adds support to sentry to retrieve this information and add it as an additional scope. Some other systems may also add data into this viaOnAdditionalCrashContextDelegate()
.