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

fix: User ID fallback on mobile #1710

merged 10 commits into from
Jul 9, 2024

Conversation

bitsandfoxes
Copy link
Contributor

User reported events without any user ID coming from the C# layer. This should work out of the box. In case of user.Id being null the enricher would set the installation ID.
https://github.com/getsentry/sentry-dotnet/blob/82dcbcdadaf37670729b394bf47489cc73fc7aa1/src/Sentry/Internal/Enricher.cs#L83

But if the ID was an empty string, it would not get overwritten by the enricher, it would not get serialized, and it would not show up on the issue.

To prevent those edge-cases I added fallbacks to both Android and iOS, relying on the same mechanism as we do on desktop to create a new ID and pass it down.

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'

// 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;
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

@bitsandfoxes
Copy link
Contributor Author

The user confirmed that the build from this PR fixed the issue for them.

Copy link
Member

@bruno-garcia bruno-garcia left a comment

Choose a reason for hiding this comment

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

Approving to unblock, strongly suggest the comment about how the user-id

@bitsandfoxes bitsandfoxes merged commit 2f75734 into main Jul 9, 2024
14 checks passed
@bitsandfoxes bitsandfoxes deleted the fix/default-user-id branch July 9, 2024 12:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants