-
-
Notifications
You must be signed in to change notification settings - Fork 50
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
Changes from 9 commits
b88060c
f4cca63
7e7ed03
885ab9b
44e9861
6d9841c
ce80bd4
aede97e
ffd942d
48d840d
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,10 @@ | ||
using System; | ||
|
||
namespace Sentry.Unity.Android | ||
{ | ||
internal interface IJniExecutor : IDisposable | ||
{ | ||
public TResult? Run<TResult>(Func<TResult?> jniOperation); | ||
public void Run(Action jniOperation); | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -3,6 +3,7 @@ | |
using Sentry.PlatformAbstractions; | ||
using Sentry.Unity.Integrations; | ||
using UnityEngine; | ||
using UnityEngine.Analytics; | ||
|
||
namespace Sentry.Unity.iOS | ||
{ | ||
|
@@ -58,6 +59,21 @@ 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'."); | ||
|
||
options.DefaultUserId = AnalyticsSessionInfo.userId; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I picked For the Unity SDK to use the .NET SDKs installation ID we'd need to make that public. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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'."); | ||
} | ||
} | ||
} | ||
} | ||
|
||
|
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 | ||
} | ||
} | ||
} |
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) | ||
{ } | ||
} | ||
} |
There was a problem hiding this comment.
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?There was a problem hiding this comment.
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 theSentry.Unity.Android
namespace.There was a problem hiding this comment.
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'