Skip to content
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

fix: User ID fallback on mobile #1710

Merged
merged 10 commits into from
Jul 9, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@

### Fixes

- Added a fallback for user.id on Android and iOS in case none could be extracted from the native layer ([#1710](https://github.com/getsentry/sentry-unity/pull/1710))
- The IL2CPP exception processor no longer fails when the native support has been disabled ([#1708](https://github.com/getsentry/sentry-unity/pull/1708))

### Dependencies
Expand Down
6 changes: 3 additions & 3 deletions src/Sentry.Unity.Android/AndroidJavaScopeObserver.cs
Original file line number Diff line number Diff line change
Expand Up @@ -6,11 +6,11 @@ namespace Sentry.Unity.Android
/// Scope Observer for Android through Java (JNI).
/// </summary>
/// <see href="https://github.com/getsentry/sentry-java"/>
public class AndroidJavaScopeObserver : ScopeObserver
internal class AndroidJavaScopeObserver : ScopeObserver
{
private readonly JniExecutor _jniExecutor;
private readonly IJniExecutor _jniExecutor;

public AndroidJavaScopeObserver(SentryOptions options, JniExecutor jniExecutor) : base("Android", options)
public AndroidJavaScopeObserver(SentryOptions options, IJniExecutor jniExecutor) : base("Android", options)
{
_jniExecutor = jniExecutor;
}
Expand Down
10 changes: 10 additions & 0 deletions src/Sentry.Unity.Android/IJniExecutor.cs
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
using System;

namespace Sentry.Unity.Android
{
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can u use top level namespace here already?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I might, but everything in that .csproj is inside the Sentry.Unity.Android namespace.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just mean 'top level namespace'

internal interface IJniExecutor : IDisposable
{
public TResult? Run<TResult>(Func<TResult?> jniOperation);
public void Run(Action jniOperation);
}
}
2 changes: 1 addition & 1 deletion src/Sentry.Unity.Android/JniExecutor.cs
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@

namespace Sentry.Unity.Android
{
public class JniExecutor
internal class JniExecutor : IJniExecutor
{
private readonly CancellationTokenSource _shutdownSource;
private readonly AutoResetEvent _taskEvent;
Expand Down
8 changes: 5 additions & 3 deletions src/Sentry.Unity.Android/NativeContextWriter.cs
Original file line number Diff line number Diff line change
Expand Up @@ -4,11 +4,13 @@ namespace Sentry.Unity.Android
{
internal class NativeContextWriter : ContextWriter
{
private readonly JniExecutor _jniExecutor;
private readonly IJniExecutor _jniExecutor;
private readonly ISentryJava _sentryJava;

public NativeContextWriter(JniExecutor jniExecutor)
public NativeContextWriter(IJniExecutor jniExecutor, ISentryJava sentryJava)
{
_jniExecutor = jniExecutor;
_sentryJava = sentryJava;
}

protected override void WriteScope(
Expand Down Expand Up @@ -50,7 +52,7 @@ protected override void WriteScope(
// We're only setting the missing contexts, the rest is configured by sentry-java. We could also sync
// the "unity" context, but it doesn't seem so useful and the effort to do is larger because there's no
// class for it in Java - not sure how we could add a generic context object in Java...
SentryJava.WriteScope(
_sentryJava.WriteScope(
_jniExecutor,
GpuId,
GpuName,
Expand Down
83 changes: 55 additions & 28 deletions src/Sentry.Unity.Android/SentryJava.cs
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,30 @@

namespace Sentry.Unity.Android
{
internal interface ISentryJava
{
public string? GetInstallationId(IJniExecutor jniExecutor);
public bool? CrashedLastRun(IJniExecutor jniExecutor);
public void Close(IJniExecutor jniExecutor);
public void WriteScope(
IJniExecutor jniExecutor,
int? GpuId,
string? GpuName,
string? GpuVendorName,
int? GpuMemorySize,
string? GpuNpotSupport,
string? GpuVersion,
string? GpuApiType,
int? GpuMaxTextureSize,
bool? GpuSupportsDrawCallInstancing,
bool? GpuSupportsRayTracing,
bool? GpuSupportsComputeShaders,
bool? GpuSupportsGeometryShaders,
string? GpuVendorId,
bool? GpuMultiThreadedRendering,
string? GpuGraphicsShaderLevel);
}

/// <summary>
/// JNI access to `sentry-java` methods.
/// </summary>
Expand All @@ -12,9 +36,9 @@ namespace Sentry.Unity.Android
/// and `sentry-java` maven packages.
/// </remarks>
/// <see href="https://github.com/getsentry/sentry-java"/>
internal static class SentryJava
internal class SentryJava : ISentryJava
{
internal static string? GetInstallationId(JniExecutor jniExecutor)
public string? GetInstallationId(IJniExecutor jniExecutor)
{
return jniExecutor.Run(() =>
{
Expand All @@ -35,7 +59,7 @@ internal static class SentryJava
/// True if the last run terminated in a crash. No otherwise.
/// If the SDK wasn't able to find this information, null is returned.
/// </returns>
public static bool? CrashedLastRun(JniExecutor jniExecutor)
public bool? CrashedLastRun(IJniExecutor jniExecutor)
{
return jniExecutor.Run(() =>
{
Expand All @@ -45,7 +69,7 @@ internal static class SentryJava
});
}

public static void Close(JniExecutor jniExecutor)
public void Close(IJniExecutor jniExecutor)
{
jniExecutor.Run(() =>
{
Expand All @@ -56,8 +80,8 @@ public static void Close(JniExecutor jniExecutor)

private static AndroidJavaObject GetSentryJava() => new AndroidJavaClass("io.sentry.Sentry");

public static void WriteScope(
JniExecutor jniExecutor,
public void WriteScope(
IJniExecutor jniExecutor,
int? GpuId,
string? GpuName,
string? GpuVendorName,
Expand Down Expand Up @@ -101,28 +125,6 @@ public static void WriteScope(
});
}

private static void SetIfNotNull<T>(this AndroidJavaObject javaObject, string property, T? value, string? valueClass = null)
{
if (value is not null)
{
if (valueClass is null)
{
javaObject.Set(property, value!);
}
else
{
using var valueObject = new AndroidJavaObject(valueClass, value!);
javaObject.Set(property, valueObject);
}
}
}

private static void SetIfNotNull(this AndroidJavaObject javaObject, string property, int? value) =>
SetIfNotNull(javaObject, property, value, "java.lang.Integer");

private static void SetIfNotNull(this AndroidJavaObject javaObject, string property, bool? value) =>
SetIfNotNull(javaObject, property, value, "java.lang.Boolean");

// Implements the io.sentry.ScopeCallback interface.
internal class ScopeCallback : AndroidJavaProxy
{
Expand Down Expand Up @@ -156,4 +158,29 @@ public ScopeCallback(Action<AndroidJavaObject> callback) : base("io.sentry.Scope
}
}
}

internal static class AndroidJavaObjectExtension
{
public static void SetIfNotNull<T>(this AndroidJavaObject javaObject, string property, T? value, string? valueClass = null)
{
if (value is not null)
{
if (valueClass is null)
{
javaObject.Set(property, value!);
}
else
{
using var valueObject = new AndroidJavaObject(valueClass, value!);
javaObject.Set(property, valueObject);
}
}
}

public static void SetIfNotNull(this AndroidJavaObject javaObject, string property, int? value) =>
SetIfNotNull(javaObject, property, value, "java.lang.Integer");

public static void SetIfNotNull(this AndroidJavaObject javaObject, string property, bool? value) =>
SetIfNotNull(javaObject, property, value, "java.lang.Boolean");
}
}
30 changes: 26 additions & 4 deletions src/Sentry.Unity.Android/SentryNativeAndroid.cs
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
using System;
using Sentry.Extensibility;
using UnityEngine;
using UnityEngine.Analytics;

namespace Sentry.Unity.Android
{
Expand All @@ -9,7 +10,8 @@ namespace Sentry.Unity.Android
/// </summary>
public static class SentryNativeAndroid
{
private static JniExecutor? JniExecutor;
internal static IJniExecutor? JniExecutor;
internal static ISentryJava? SentryJava;

/// <summary>
/// Configures the native Android support.
Expand All @@ -25,9 +27,10 @@ public static void Configure(SentryUnityOptions options, ISentryUnityInfo sentry
return;
}

JniExecutor = new JniExecutor();
JniExecutor ??= new JniExecutor();
SentryJava ??= new SentryJava();

options.NativeContextWriter = new NativeContextWriter(JniExecutor);
options.NativeContextWriter = new NativeContextWriter(JniExecutor, SentryJava);
options.ScopeObserver = new AndroidJavaScopeObserver(options, JniExecutor);
options.EnableScopeSync = true;
options.CrashedLastRun = () =>
Expand Down Expand Up @@ -64,7 +67,26 @@ public static void Configure(SentryUnityOptions options, ISentryUnityInfo sentry
}

options.NativeSupportCloseCallback = () => Close(options.DiagnosticLogger);

options.DefaultUserId = SentryJava.GetInstallationId(JniExecutor);
if (string.IsNullOrEmpty(options.DefaultUserId))
{
// In case we can't get an installation ID we create one and sync that down to the native layer
options.DiagnosticLogger?.LogDebug("Failed to fetch 'Installation ID' from the native SDK. Creating new 'Default User ID'.");

// We fall back to Unity's Analytics Session Info: https://docs.unity3d.com/ScriptReference/Analytics.AnalyticsSessionInfo-userId.html
// It's a randomly generated GUID that gets created immediately after installation helping
// to identify the same instance of the game
options.DefaultUserId = AnalyticsSessionInfo.userId;
if (options.DefaultUserId is not null)
{
options.ScopeObserver.SetUser(new SentryUser { Id = options.DefaultUserId });
}
else
{
options.DiagnosticLogger?.LogDebug("Failed to create new 'Default User ID'.");
}
}
}

/// <summary>
Expand All @@ -78,7 +100,7 @@ public static void Close(IDiagnosticLogger? logger = null)
// This is an edge-case where the Android SDK has been enabled and setup during build-time but is being
// shut down at runtime. In this case Configure() has not been called and there is no JniExecutor yet
JniExecutor ??= new JniExecutor();
SentryJava.Close(JniExecutor);
SentryJava?.Close(JniExecutor);
JniExecutor.Dispose();
}
}
Expand Down
19 changes: 19 additions & 0 deletions src/Sentry.Unity.iOS/SentryNativeCocoa.cs
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
using Sentry.PlatformAbstractions;
using Sentry.Unity.Integrations;
using UnityEngine;
using UnityEngine.Analytics;

namespace Sentry.Unity.iOS
{
Expand Down Expand Up @@ -58,6 +59,24 @@ internal static void Configure(SentryUnityOptions options, ISentryUnityInfo sent
if (sentryUnityInfo.IL2CPP)
{
options.DefaultUserId = SentryCocoaBridgeProxy.GetInstallationId();
if (string.IsNullOrEmpty(options.DefaultUserId))
{
// In case we can't get an installation ID we create one and sync that down to the native layer
options.DiagnosticLogger?.LogDebug("Failed to fetch 'Installation ID' from the native SDK. Creating new 'Default User ID'.");

// We fall back to Unity's Analytics Session Info: https://docs.unity3d.com/ScriptReference/Analytics.AnalyticsSessionInfo-userId.html
// It's a randomly generated GUID that gets created immediately after installation helping
// to identify the same instance of the game
options.DefaultUserId = AnalyticsSessionInfo.userId;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

AnalyticsSessionInfo.userId

Is this something devs would need to add to their app manifest now? Might be a problem to use something that can be used to track users across apps. That's why we use our own guid everywhere

Copy link
Contributor Author

@bitsandfoxes bitsandfoxes Jul 4, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I picked AnalyticsSessionInfo.userId as we currently use it for the desktop platforms. It's a GUID generated during installation. Based on the Unity Docs it only gets regenerated during a re-install.

For the Unity SDK to use the .NET SDKs installation ID we'd need to make that public.
https://github.com/getsentry/sentry-dotnet/blob/d88635187ba02b4e47906a38b5bc0ebb19548355/src/Sentry/SentryOptions.cs#L46

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oh OK, so it uses the same principal as we do.

Worth adding a big block of comment clarifying that btw for anyone reasing the code and being worried about tracking and privacy

if (options.DefaultUserId is not null)
{
options.ScopeObserver.SetUser(new SentryUser { Id = options.DefaultUserId });
}
else
{
options.DiagnosticLogger?.LogDebug("Failed to create new 'Default User ID'.");
}
}
}
}

Expand Down
17 changes: 15 additions & 2 deletions test/Sentry.Unity.Android.Tests/SentryNativeAndroidTests.cs
Original file line number Diff line number Diff line change
@@ -1,8 +1,6 @@
using System;
using System.Threading;
using NUnit.Framework;
using Sentry.Unity;
using UnityEngine;

namespace Sentry.Unity.Android.Tests
{
Expand All @@ -24,6 +22,9 @@ public void SetUp()
_fakeReinstallSentryNativeBackendStrategy);
_reinstallCalled = false;
_sentryUnityInfo = new TestUnityInfo { IL2CPP = false };

SentryNativeAndroid.JniExecutor = new TestJniExecutor();
SentryNativeAndroid.SentryJava = new TestSentryJava();
}

[TearDown]
Expand Down Expand Up @@ -92,5 +93,17 @@ public void Configure_NativeAndroidSupportDisabled_DoesNotReInitializeNativeBack
SentryNativeAndroid.Configure(options, _sentryUnityInfo);
Assert.False(_reinstallCalled);
}

[Test]
public void Configure_NoInstallationIdReturned_SetsNewDefaultUserId()
{
var options = new SentryUnityOptions();
var sentryJava = SentryNativeAndroid.SentryJava as TestSentryJava;
Assert.NotNull(sentryJava);
sentryJava!.InstallationId = string.Empty;

SentryNativeAndroid.Configure(options, _sentryUnityInfo);
Assert.False(string.IsNullOrEmpty(options.DefaultUserId));
}
}
}
21 changes: 21 additions & 0 deletions test/Sentry.Unity.Android.Tests/TestJniExecutor.cs
Original file line number Diff line number Diff line change
@@ -0,0 +1,21 @@
using System;

namespace Sentry.Unity.Android.Tests
{
public class TestJniExecutor : IJniExecutor
{
public TResult? Run<TResult>(Func<TResult?> jniOperation)
{
return default;
}

public void Run(Action jniOperation)
{
}

public void Dispose()
{
// TODO release managed resources here
}
}
}
39 changes: 39 additions & 0 deletions test/Sentry.Unity.Android.Tests/TestSentryJava.cs
Original file line number Diff line number Diff line change
@@ -0,0 +1,39 @@
namespace Sentry.Unity.Android.Tests
{
internal class TestSentryJava : ISentryJava
{
public string? InstallationId { get; set; }
public bool? IsCrashedLastRun { get; set; }

public string? GetInstallationId(IJniExecutor jniExecutor)
{
return InstallationId;
}

public bool? CrashedLastRun(IJniExecutor jniExecutor)
{
return IsCrashedLastRun;
}

public void Close(IJniExecutor jniExecutor) { }

public void WriteScope(
IJniExecutor jniExecutor,
int? GpuId,
string? GpuName,
string? GpuVendorName,
int? GpuMemorySize,
string? GpuNpotSupport,
string? GpuVersion,
string? GpuApiType,
int? GpuMaxTextureSize,
bool? GpuSupportsDrawCallInstancing,
bool? GpuSupportsRayTracing,
bool? GpuSupportsComputeShaders,
bool? GpuSupportsGeometryShaders,
string? GpuVendorId,
bool? GpuMultiThreadedRendering,
string? GpuGraphicsShaderLevel)
{ }
}
}
Loading