getsentry / sentry-unity

Development of Sentry SDK for Unity
https://docs.sentry.io/platforms/unity/
MIT License
200 stars 51 forks source link

Auto Session Tracking should only be enabled by default on Mobile until total process isolation is supported #264

Open bruno-garcia opened 2 years ago

bruno-garcia commented 2 years ago

The session support currently expects exclusive access to the directory it caches data. This works well in sandboxed apps like on mobile but not on Desktop when multiple instances can be executed concurrently.

For this reason, until https://github.com/getsentry/sentry-dotnet/issues/1067 is fixed, we can only opt-in to auto session tracking safely if we are sure there's only 1 instance of the app (mobile, or a player on desktop that only allows one instance running).

When PlayerSettings.forceSingleInstance is set we're safe to enable auto session tracking. Also if the player is Android or iOS and console.

semuserable commented 2 years ago

PlayerSettings.forceSingleInstance is present in UnityEditor.dll and not available in Sentry.Unity (references UnityEngine) where logic for this is executing (SentryOptionsUtility.SetDefaults).

There are 2 possible approaches to determine the platform: Application.platform or Platform dependent compilation.

Suggested way is to use Platform dependent compilation approach. But the problem is still in PlayerSettings.forceSingleInstance not available at the place of calling.

Possible implementation could look something like this

scriptableOptions.AutoSessionTracking = IsAutoSessionTracking(options.AutoSessionTracking);

private bool IsAutoSessionTracking(bool autoSessionTracking)
{
#if UNITY_IOS || UNITY_ANDROID || UNITY_PS4 || UNITY_XBOXONE
    return autoSessionTracking;
#elif UNITY_STANDALONE || UNITY_EDITOR
    return PlayerSettings.forceSingleInstance;
#else
    return autoSessionTracking;
#endif
}
bruno-garcia commented 2 years ago

SentryWindow runs in the editor (Sentry.Unity.Editor), we could read PlayerSettings.forceSingleInstance there and set it to the scriptable object and rely on that value at runtime to disable auto session tracking if the Application.platform is standalone and forceSingleInstance is not true.

Or we live with this until we address getsentry/sentry-dotnet#1067

bitsandfoxes commented 2 years ago

The SentryWindow would not get notified about a change to the player settings. But we could read them during pre-build and set it accordingly?

vaind commented 2 years ago

As per a discord discussion:

vaind commented 2 years ago

Updating impact, priority & status after the workaround has been implemented. The final solution is blocked by the dotnet issue.