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

Set Scope.User.Id to the InstallationId by default #3425

Merged
merged 9 commits into from
Jun 20, 2024

Conversation

jamescrosswell
Copy link
Collaborator

Resolves #3302

Copy link
Contributor

@bitsandfoxes bitsandfoxes left a comment

Choose a reason for hiding this comment

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

I think this is doing it the wrong way around. Now the client and the session manager each hold their own version of an installationIdHelper. And the client tries to fix a missing ID after the fact?

The ID is part of the state and should sit on the hub if it's getting used in multiple places, which, if it's getting resolved in the GlobalSessionManager, is already sort of happening. In that case, it should be set on the user where applicable.
The client should not need go around trying to cover gaps in the setup.

src/Sentry/GlobalSessionManager.cs Outdated Show resolved Hide resolved
@jamescrosswell
Copy link
Collaborator Author

jamescrosswell commented Jun 17, 2024

I think this is doing it the wrong way around. Now the client and the session manager each hold their own version of an installationIdHelper. And the client tries to fix a missing ID after the fact?

The ID is part of the state and should sit on the hub if it's getting used in multiple places, which, if it's getting resolved in the GlobalSessionManager, is already sort of happening. In that case, it should be set on the user where applicable. The client should not need go around trying to cover gaps in the setup.

That sounds good in principal. In practice, I had to delay setting the User.Id until the client since setting it earlier impacts various other functionality.

Here, for example, the client runs an enricher:

_enricher.Apply(transaction);

The enricher does this:

if (_options is { SendDefaultPii: true, IsEnvironmentUser: true } && !eventLike.HasUser())
{
eventLike.User.Username = Environment.UserName;
}

Setting the User.Id before this point would break the enricher then (as it changes the result of the call to eventLike.HasUser().

The MainSentryEventProcessor does something similar for events.

I think utlimately the InstallationId is not state that's held by the hub at all... rather it's state that gets initialised the first time the SDK runs for a particular project on a particular device. It sets a unique ID identifying that installation and stores it on the file system. Logically, it's a piece of context that stays the same for the duration of the application's execution so it's similar to some of the other stuff we new up when initialising the SDK/Options. It's accessed by the Global Session Manager and also by the Sentry Client - but neither of these creates/owns/stores that state. I'm thinking maybe we could move it to be a Lazy property on the SentryOptions then (SentryOptions.InstallationId). That seems like a more natural place for it to live.

Copy link
Contributor

@bitsandfoxes bitsandfoxes left a comment

Choose a reason for hiding this comment

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

Ahh, I like this a lot better! That makes a lot of sense, putting it on the options! Thanks for that!

src/Sentry/SentryClient.cs Outdated Show resolved Hide resolved
src/Sentry/SentryClient.cs Outdated Show resolved Hide resolved
@jamescrosswell jamescrosswell merged commit 009241d into main Jun 20, 2024
21 checks passed
@jamescrosswell jamescrosswell deleted the default-user-id branch June 20, 2024 04:05
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.

User ID is left blank on Android
3 participants