-
Notifications
You must be signed in to change notification settings - Fork 22
fix: queue user and device ID changes for proper event ordering #349
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
Changes from all commits
f3ac02f
5523406
a6499e4
5f87fa0
02f312f
6d2f6e6
e5b49c0
5259267
9b897f0
134b9c5
5ea7b9c
bac5645
d18d353
4203557
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 |
|---|---|---|
|
|
@@ -7,6 +7,7 @@ import com.amplitude.android.migration.MigrationManager | |
| import com.amplitude.android.plugins.AnalyticsConnectorIdentityPlugin | ||
| import com.amplitude.android.plugins.AnalyticsConnectorPlugin | ||
| import com.amplitude.android.plugins.AndroidContextPlugin | ||
| import com.amplitude.android.plugins.AndroidContextPlugin.Companion.validDeviceId | ||
| import com.amplitude.android.plugins.AndroidLifecyclePlugin | ||
| import com.amplitude.android.plugins.AndroidNetworkConnectivityCheckerPlugin | ||
| import com.amplitude.android.storage.AndroidStorageContextV3 | ||
|
|
@@ -97,14 +98,15 @@ open class Amplitude internal constructor( | |
| if (this.configuration.offline != AndroidNetworkConnectivityCheckerPlugin.Disabled) { | ||
| add(AndroidNetworkConnectivityCheckerPlugin()) | ||
| } | ||
| androidContextPlugin = | ||
| object : AndroidContextPlugin() { | ||
| override fun setDeviceId(deviceId: String) { | ||
| // call internal method to set deviceId immediately i.e. dont' wait for build() to complete | ||
| this@Amplitude.setDeviceIdInternal(deviceId) | ||
| } | ||
| } | ||
| androidContextPlugin = AndroidContextPlugin() | ||
| add(androidContextPlugin) | ||
| val deviceId = | ||
| configuration.deviceId | ||
| ?: store.deviceId?.takeIf { validDeviceId(it, allowAppSetId = false) } | ||
| ?: androidContextPlugin.createDeviceId() | ||
| // Persist device ID synchronously during build (not via coroutine dispatch) | ||
| store.deviceId = deviceId | ||
| idContainer.identityManager.editIdentity().setDeviceId(deviceId).commit() | ||
| add(GetAmpliExtrasPlugin()) | ||
| add(AndroidLifecyclePlugin(activityLifecycleCallbacks)) | ||
| add(AnalyticsConnectorIdentityPlugin()) | ||
|
|
@@ -126,20 +128,29 @@ open class Amplitude internal constructor( | |
|
|
||
| /** | ||
| * Reset identity: | ||
| * - reset userId to "null" | ||
| * - reset deviceId via AndroidContextPlugin | ||
| * - reset userId to null | ||
| * - generate and set a new deviceId | ||
| * @return the Amplitude instance | ||
| */ | ||
| override fun reset(): Amplitude { | ||
| this.setUserId(null) | ||
| amplitudeScope.launch(amplitudeDispatcher) { | ||
| isBuilt.await() | ||
| idContainer.identityManager.editIdentity().setDeviceId(null).commit() | ||
| androidContextPlugin.initializeDeviceId(configuration as Configuration) | ||
polbins marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| setUserId(null) | ||
| setDeviceId(androidContextPlugin.createDeviceId()) | ||
| } | ||
| return this | ||
| } | ||
cursor[bot] marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
|
||
| override fun setUserId(userId: String?): Amplitude { | ||
polbins marked this conversation as resolved.
Show resolved
Hide resolved
polbins marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| super.setUserId(userId) | ||
| return this | ||
polbins marked this conversation as resolved.
Show resolved
Hide resolved
polbins marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| } | ||
|
|
||
| override fun setDeviceId(deviceId: String): Amplitude { | ||
| super.setDeviceId(deviceId) | ||
|
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. Deferred identity callbacks can overwrite newer immediate stateMedium Severity The dual-write architecture introduces a race: the Android Additional Locations (1)
Contributor
Author
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. It's a pre-existing race in core's coroutine dispatch + listener pattern, not introduced by this PR. Noted, in case it becomes an issue later. 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 think this is valid - state would be temporarily overwritten by
Contributor
Author
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. It looks to me that the onUserIdChange/onDeviceIdChange is the one flagged here. 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. Can experiment edit the identity too? The callbacks on I do see a race condition here specifically in the init(deviceId: A) // sets state deviceId: A, async kicks off AnalyticsIdentityListener.init |
||
| return this | ||
| } | ||
|
|
||
| @GuardedAmplitudeFeature | ||
| fun onEnterForeground(timestamp: Long) { | ||
| (timeline as Timeline).onEnterForeground(timestamp) | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -7,13 +7,11 @@ import com.amplitude.common.android.AndroidContextProvider | |
| import com.amplitude.core.Amplitude | ||
| import com.amplitude.core.RestrictedAmplitudeFeature | ||
| import com.amplitude.core.events.BaseEvent | ||
| import com.amplitude.core.platform.Plugin | ||
| import com.amplitude.core.platform.plugins.ContextPlugin | ||
| import java.util.UUID | ||
|
|
||
| @OptIn(RestrictedAmplitudeFeature::class) | ||
| open class AndroidContextPlugin : Plugin { | ||
| override val type: Plugin.Type = Plugin.Type.Before | ||
| override lateinit var amplitude: Amplitude | ||
| open class AndroidContextPlugin : ContextPlugin() { | ||
| private lateinit var contextProvider: AndroidContextProvider | ||
|
|
||
| override fun setup(amplitude: Amplitude) { | ||
|
|
@@ -26,8 +24,6 @@ open class AndroidContextPlugin : Plugin { | |
| configuration.trackingOptions.shouldTrackAdid(), | ||
| configuration.trackingOptions.shouldTrackAppSetId(), | ||
| ) | ||
| initializeDeviceId(configuration) | ||
|
|
||
| amplitude.diagnosticsClient.setTag( | ||
| name = "sdk.${SDK_LIBRARY}.version", | ||
| value = SDK_VERSION, | ||
|
|
@@ -39,45 +35,56 @@ open class AndroidContextPlugin : Plugin { | |
| return event | ||
| } | ||
|
|
||
| fun initializeDeviceId(configuration: Configuration) { | ||
| // Check configuration | ||
| var deviceId = configuration.deviceId | ||
| if (deviceId != null) { | ||
| setDeviceId(deviceId) | ||
| return | ||
| } | ||
|
|
||
| // Check store | ||
| deviceId = amplitude.store.deviceId | ||
| if (deviceId != null && validDeviceId(deviceId) && !deviceId.endsWith("S")) { | ||
| return | ||
| } | ||
| /** | ||
| * Create a device ID from available sources. | ||
| * Priority: advertising ID -> app set ID -> random UUID | ||
| */ | ||
| internal fun createDeviceId(): String { | ||
| val configuration = amplitude.configuration as Configuration | ||
|
|
||
| // Check new device id per install | ||
| // Check advertising ID (if enabled and not per-install) | ||
| if (!configuration.newDeviceIdPerInstall && | ||
| configuration.useAdvertisingIdForDeviceId && | ||
| !contextProvider.isLimitAdTrackingEnabled() | ||
| ) { | ||
| val advertisingId = contextProvider.advertisingId | ||
| if (advertisingId != null && validDeviceId(advertisingId)) { | ||
| setDeviceId(advertisingId) | ||
| return | ||
| contextProvider.advertisingId?.let { advertisingId -> | ||
| if (validDeviceId(advertisingId)) { | ||
| return advertisingId | ||
| } | ||
| } | ||
| } | ||
|
|
||
| // Check app set id | ||
| // Check app set ID (if enabled) | ||
| if (configuration.useAppSetIdForDeviceId) { | ||
| val appSetId = contextProvider.appSetId | ||
| if (appSetId != null && validDeviceId(appSetId)) { | ||
| setDeviceId("${appSetId}S") | ||
| return | ||
| contextProvider.appSetId?.let { appSetId -> | ||
| if (validDeviceId(appSetId)) { | ||
| return "$appSetId$DEVICE_ID_SUFFIX_APP_SET_ID" | ||
| } | ||
| } | ||
| } | ||
|
|
||
| // Generate random id | ||
| val generatedUuid = UUID.randomUUID().toString() | ||
| val randomId = generatedUuid + "R" | ||
| setDeviceId(randomId) | ||
| // Generate random ID | ||
| return UUID.randomUUID().toString() + DEVICE_ID_SUFFIX_RANDOM | ||
| } | ||
|
|
||
| @Deprecated( | ||
| message = "Use createDeviceId() instead. Amplitude now handles setting the deviceId.", | ||
| replaceWith = ReplaceWith("createDeviceId()"), | ||
| ) | ||
| fun initializeDeviceId(configuration: Configuration) { | ||
| val deviceId = | ||
| configuration.deviceId | ||
| ?: amplitude.store.deviceId?.takeIf { validDeviceId(it, allowAppSetId = false) } | ||
| ?: createDeviceId() | ||
| amplitude.setDeviceId(deviceId) | ||
|
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. this saved to storage synchronously in the previous version - could you please maintain that behavior?
Contributor
Author
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. The Now the actual init path is in 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. Previously all the calls here went from |
||
| } | ||
polbins marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
|
||
| @Deprecated( | ||
| message = "Use Amplitude.setDeviceId() instead. The plugin should not set identity directly.", | ||
| replaceWith = ReplaceWith("amplitude.setDeviceId(deviceId)"), | ||
| ) | ||
| protected open fun setDeviceId(deviceId: String) { | ||
| amplitude.setDeviceId(deviceId) | ||
| } | ||
|
|
||
| private fun applyContextData(event: BaseEvent) { | ||
|
|
@@ -171,19 +178,37 @@ open class AndroidContextPlugin : Plugin { | |
| } | ||
| } | ||
|
|
||
| protected open fun setDeviceId(deviceId: String) { | ||
| amplitude.setDeviceId(deviceId) | ||
| } | ||
|
|
||
| companion object { | ||
| const val PLATFORM = "Android" | ||
| const val SDK_LIBRARY = "amplitude-analytics-android" | ||
| const val SDK_VERSION = BuildConfig.AMPLITUDE_VERSION | ||
|
|
||
| /** | ||
| * Device ID suffix indicating the ID was derived from App Set ID. | ||
| * | ||
| * When a stored device ID ends with this suffix, [createDeviceId] will attempt | ||
| * to regenerate it to potentially upgrade to a better identifier (e.g., advertising ID | ||
| * if the user later grants permission). | ||
| */ | ||
| private const val DEVICE_ID_SUFFIX_APP_SET_ID = "S" | ||
|
|
||
| /** | ||
| * Device ID suffix indicating the ID is a randomly generated UUID. | ||
| */ | ||
| private const val DEVICE_ID_SUFFIX_RANDOM = "R" | ||
|
|
||
| private val INVALID_DEVICE_IDS = | ||
| setOf("", "9774d56d682e549c", "unknown", "000000000000000", "Android", "DEFACE", "00000000-0000-0000-0000-000000000000") | ||
|
|
||
| fun validDeviceId(deviceId: String): Boolean { | ||
| return !(deviceId.isEmpty() || INVALID_DEVICE_IDS.contains(deviceId)) | ||
| fun validDeviceId( | ||
| deviceId: String, | ||
| allowAppSetId: Boolean = true, | ||
| ): Boolean { | ||
| return when { | ||
| deviceId.isEmpty() || INVALID_DEVICE_IDS.contains(deviceId) -> false | ||
| !allowAppSetId && deviceId.endsWith(DEVICE_ID_SUFFIX_APP_SET_ID) -> false | ||
| else -> true | ||
| } | ||
| } | ||
| } | ||
| } | ||


Uh oh!
There was an error while loading. Please reload this page.