getsentry / team-mobile

Meta issues for the Mobile team
4 stars 1 forks source link

Persist user information across app launches #120

Open markushi opened 1 year ago

markushi commented 1 year ago

Right now Sentry.setUser(user) is not persisted over app starts. Thus any captured events which occur at the app start (but before .setUser() is called) will result in an anonymous user.

Ideally the user data can be persisted over app starts to better reflect the state of the app.

See https://github.com/getsentry/team-mobile/issues/119 for some community input.

### Tasks
- [ ] https://github.com/getsentry/sentry-cocoa/issues/3057
- [ ] https://github.com/getsentry/sentry-java/issues/2717
- [ ] https://github.com/getsentry/sentry-react-native/issues/3065
- [ ] https://github.com/getsentry/sentry-dart/issues/1439
- [ ] .NET
- [ ] Unity
philipphofmann commented 1 year ago

This seems like to be a breaking change. Currently, apps assume that users don't get persisted across application launches. Apps not calling setUser(null) when no user is logged in when the app launches would now get a different behavior by the SDK. They would suddenly get the user from the previous app launch, for example, when the session expires for a banking app and the app doesn't unset the user when it's terminated.

marandaneto commented 1 year ago

@philipphofmann it's also fine to add a flag to enable/disable this behavior, I believe a minor is fine but we can do it in the next major as well if we all agree on that.

krystofwoldrich commented 1 year ago

I think using a flag that will be disabled for now and can be enabled by default with the next major version makes sense as we are not blocking this from being developed and also not blocking the users who would like to use this already.

If adding the flag would be an unreasonable extra amount of work then I agree with the breaking change and the next major.

philipphofmann commented 1 year ago

I need help understanding what a good use case for that feature is, justifying the effort of building it, and forcing users to unset the user explicitly. Is it so important to have user information for fixing specific bugs?

marandaneto commented 1 year ago

@philipphofmann If you open the app, init the SDK, and call captureException manually or automatically, the user is the default one with installationId, the app didn't have time to call setUser yet, maybe the user is logged out or something yet, expired. If we persist the user and retrieve it during SDK init, every exception will contain the last user even if the app restarted, so the number of users affected by the X issue will be more accurate. This is one of the use cases.

philipphofmann commented 1 year ago

Ah, the main point for me here is that it messes up release health data, which is enough for adding the feature. Thanks for explaining, @marandaneto.

armcknight commented 1 year ago

I wonder about the way we're counting users for release health purposes. What you're describing here counts the number of accounts, not users because a user (ie, device owner) might have multiple accounts. For instance, say I'm using a Reddit client app, and I have multiple handles. If the app experiences an issue right after log in, and I log in and out of multiple handles throughout the day, then the release health looks like an issue is affecting more people than it really is. Or I may have a personal and business bank account in an app, or mail client with multiple email accounts. Shouldn't we just use installation IDs for these situations instead of user accounts?

marandaneto commented 1 year ago

Customer-defined user data has precedence over installation ID, installation ID is just the fallback in case there's nothing there. As an SDK, we don't know if a person is using multiple accounts in the same app, yes. For example: The customer sets user A, triggering an error. Logs out The customer sets user B, triggering an error. There will be 2 users affected, but still the same device. If the customer doesn't set anything, and fallback to the installation ID, there will be 3 users affected.

On the other hand, it could be a shared device where the 3 users are actually different people.

So the best we can do is what we already do, the ability to let them set a user, and fallback to something else (installation ID) if nothing is provided.

Also, how common is it to have multiple accounts within the same app? in some cases, most likely, but not the majority for sure.

There could be a mismatched number of affected users, but it's a matter of setting the user and unsettling the user whenever makes sense in their app.

armcknight commented 1 year ago

Yes, I guess there are some use cases where devices really are shared between multiple people, especially publicly-shared devices like at kiosks. But those may also not always have knowledge of actual separate users.

Do we also report the number of devices affected? Like, "affecting X users across Y devices"?

marandaneto commented 1 year ago

AFAIK not really.

kvenn commented 7 months ago

I'd like to put my emphatic +1 on this and provide my use case (which I imagine is similar to most others).

  1. A user authenticates once and is logged in
  2. If they log out or change users, I call Sentry.setUser

I think it's more rare that a user signs in every time the app is opened. But I agree backwards compatibility is important, and a flag would suffice.

We ran into an issue with this current behavior in that errors occurring early in our apps lifecycle were missing the user data. We persist the user id to disk, but we want to make sure Sentry is initialized as early as possible.

It would be ideal if Sentry could send the id along with the error once it has pulled the id from it's own storage - so that the consumer doesn't need to await initialization of their app based on crash reporting.

Better yet, it's handled on the backend (if the flag is set). I'm pretty sure that is how Amplitude does it. You only need to set the user property once (this is the same with MixPanel and Localytics client SDKs).