draft about distinct id implementation #342
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,90 @@ | ||
| package io.sentry.android.core; | ||
|
|
||
| import android.content.SharedPreferences; | ||
| import io.sentry.core.IUserCache; | ||
| import io.sentry.core.protocol.User; | ||
| import io.sentry.core.util.Objects; | ||
| import org.jetbrains.annotations.NotNull; | ||
| import org.jetbrains.annotations.Nullable; | ||
|
|
||
| // TODO: to think, this could be a SentryOptionsCache that could be expanded, does it make sense? | ||
|
There was a problem hiding this comment. to discuss, related to the naming and scope of this class (IUserCache/AndroidUserCache) There was a problem hiding this comment. Makes sense to me to keep this focused on user caching, specially the abstraction There was a problem hiding this comment. I'm not sure if Cache is the right naming here, because we actually persist the data to the There was a problem hiding this comment. not entirely, I borrowed the idea from https://developer.android.com/reference/android/content/Context.html#getCacheDir() I see the Caching term as something temporary, being in memory or in the disk.
I'm fine changing it as well, but it might make it inconsistent with other SDKs. @bruno-garcia opnions? |
||
|
|
||
| /** the Android User Cache class. It caches the user data in the Android SharedPreferences */ | ||
| public final class AndroidUserCache implements IUserCache { | ||
|
|
||
| // TODO: only these 3 fields make sense to me, otherwise caching a raw json would be better, but I | ||
| // don't want to do that. | ||
| private static final String EMAIL = "email"; | ||
|
There was a problem hiding this comment. Shouldn't we prefix those constants here? Could it be that the users of our SDK also save email to their shared preferences? What about 'sentry_user_cache_email' or something similar? There was a problem hiding this comment. its a dedicated SharedPreferences, see: |
||
| private static final String ID = "id"; | ||
| private static final String USERNAME = "user_name"; | ||
|
Comment on lines
+15
to
+19
There was a problem hiding this comment. to discuss, for sessions, we only need these 3 fields |
||
|
|
||
| private final @NotNull SharedPreferences sharedPreferences; | ||
| private final @NotNull SentryAndroidOptions options; | ||
| private final @Nullable String sentryDeviceId; | ||
|
|
||
| public AndroidUserCache( | ||
| final @NotNull SentryAndroidOptions options, | ||
| final @NotNull SharedPreferences sharedPreferences, | ||
| final @Nullable String sentryDeviceId) { | ||
| this.options = Objects.requireNonNull(options, "SentryAndroidOptions is required."); | ||
| this.sharedPreferences = | ||
| Objects.requireNonNull(sharedPreferences, "SharedPreferences is required."); | ||
| this.sentryDeviceId = sentryDeviceId; | ||
| } | ||
|
|
||
| // TODO: I believe this makes sense only for global hub SDKs, but if not, | ||
| // how do we cache multiple users (because we have multiple scopes)? | ||
|
Comment on lines
+35
to
+36
There was a problem hiding this comment. For me it makes sense to keep this class only specific to Android. I guess we are not going to run into the problem of caching multiple users. There was a problem hiding this comment. yep, this one is Android-specific, but we can't forget that |
||
|
|
||
| @Override | ||
| public void setUser(final @Nullable User user) { | ||
| if (options.isCacheUserForSessions()) { | ||
|
There was a problem hiding this comment. I think we could separate the logic of whether the user data should be saved or nor from the actual Android code. I would imagine two classes. One called something like There was a problem hiding this comment. yep, we could do that, just not sure how deep this abstraction should be, do we really see this user caching thing going forward with other SDKs? like java-backend, java-desktop etc... otherwise, it's just more classes for a single block of code, which is an There was a problem hiding this comment. It could be pulled out from the Android code but I believe it would only make sense then in desktop apps, right? Since this User in server apps are usually one per request and doesn't really make sense to cache. The proposal for |
||
| final SharedPreferences.Editor edit = sharedPreferences.edit(); | ||
| if (user != null) { | ||
| edit.putString(ID, user.getId()) | ||
|
There was a problem hiding this comment. Could we settle for only caching the If we're expecting to store the actual User, we'd need to consider the protocol can change (versioning/new fields added), user define data and makes it trickier:
If we're storing just a single value to be used for session tracking only, it would minimize these edge cases.
There was a problem hiding this comment. yep, a single value would be ok, makes sense. |
||
| .putString(EMAIL, user.getEmail()) | ||
| .putString(USERNAME, user.getUsername()) | ||
|
Comment on lines
+43
to
+45
There was a problem hiding this comment. For me it is a bit confusing why we pass the whole user to There was a problem hiding this comment. we could, but our There was a problem hiding this comment. Maybe then we should create an extra class for this and name it something like There was a problem hiding this comment. I believe the Caching class should not be aware of what There was a problem hiding this comment.
I prefer this too. It says what it does. So when setting the user, if this option is enabled, we select the data from the user object (id, username, email) to become the distinctId and cache that so the caching itself isn't aware of this logic and we don't have to deal with caching the actual user object and possible user-defined fields. |
||
| .apply(); // it does in-memory cache and flush to the disk async. | ||
| } else { | ||
| cleanUserFields(edit); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| @Override | ||
| public @Nullable User getUser() { | ||
| // sharedPreferences has in-memory cache and flush to the disk async., so it's not expensive | ||
| if (options.isCacheUserForSessions()) { | ||
| final String id = sharedPreferences.getString(ID, null); | ||
| final String email = sharedPreferences.getString(EMAIL, null); | ||
| final String userName = sharedPreferences.getString(USERNAME, null); | ||
|
|
||
| boolean hasCachedUser = true; | ||
| if (id == null && email == null && userName == null) { | ||
| hasCachedUser = false; | ||
| } | ||
|
|
||
| final User user = new User(); | ||
|
|
||
| if (hasCachedUser) { | ||
| // returns previous set user, even if it has no id | ||
| user.setId(id); | ||
| user.setEmail(email); | ||
| user.setUsername(userName); | ||
| } else { | ||
| user.setId(sentryDeviceId); | ||
| } | ||
| return user; | ||
| } else { | ||
| final SharedPreferences.Editor edit = sharedPreferences.edit(); | ||
| // is it a good idea to do it here? this guarantees that it cleans up on restart if the flag | ||
| // has been swapped | ||
| cleanUserFields(edit); | ||
|
Comment on lines
+79
to
+81
There was a problem hiding this comment. I think it makes sense, because otherwise you could receive an old user. Imagine the following scenario:
There was a problem hiding this comment. yep, that was exactly the use-case There was a problem hiding this comment. I just came up with another scenario:
In this scenario, even with calling There was a problem hiding this comment. yeah, I'd say on SDK. init, if the flag is |
||
| return null; | ||
| } | ||
| } | ||
|
|
||
| private void cleanUserFields(final @NotNull SharedPreferences.Editor edit) { | ||
| // do not clean SENTRY_DEVICE_ID | ||
| edit.remove(ID).remove(EMAIL).remove(USERNAME).apply(); | ||
| } | ||
| } | ||
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.
Is the place to have this code on some package private static class? If so we could just call it from here.
I believe this
containscall could be replace with thesharedPreferences.getString(SENTRY_DEVICE_ID, null) != nullto avoid two I/O calls.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.
SharedPreferencesdoes in-memory caching sync and it flushes to the disk async., so unless its value has changed, it's not an issue.I'd separate it in a method or class, yes.