feat: add identity module and file storage #8

qingzhuozhen · 2022-02-28T06:17:11Z

Summary

add file storage
move identity module to this PR
in core and android, initialize identity with different storage
adjust storage interface a bit for file storage and potential other keys

Checklist

Does your PR title have the correct title format?
Does your PR have a breaking change?: no

bgiori · 2022-03-01T00:15:34Z

    val amplitudeDispatcher: CoroutineDispatcher = Executors.newCachedThreadPool().asCoroutineDispatcher(),
    val networkIODispatcher: CoroutineDispatcher = Executors.newSingleThreadExecutor().asCoroutineDispatcher(),
    val storageIODispatcher: CoroutineDispatcher = Executors.newFixedThreadPool(2).asCoroutineDispatcher()

Somewhat off topic, I'll review the ID stuff separately. Just a question for you:

Any reason you're creating dedicated threads instead of using Dispatchers.Default and Dispatchers.IO?

I still appreciate the DI aspect of allowing for custom dispatchers, just feel that using the default would be more effective and optimized in most cases.

qingzhuozhen · 2022-03-01T00:47:01Z

@bgiori Thanks for the question. I guess the benefits for having custom dispatchers is we have more controls on our end. While Default and IO could be used and shared with other programs and that may cause some unexpected happen. I see segment is doing that as well so use this as a practice for us. We can do more testing and discuss further if we need to adjust this.

bgiori

I'll come back to review more again another day. Just want to get my initial thoughts out.

Also we could have a meeting to discuss details. Might be more efficient than over the github PR.

android/src/main/java/com/amplitude/android/utilities/AndroidStorage.kt

bgiori · 2022-03-01T00:21:47Z

id/src/main/java/com/amplitude/id/IdentityStore.kt

+data class Identity(
+    val userId: String? = null,
+    val deviceId: String? = null,
+    val userProperties: Map<String, Any?> = mapOf(),
+)


IMO conflating userProperties with identity was a mistake on my part initially. I would not consider user properties as a part of a user's Identity. I.e. I think we should handle this separately. This also means we can remove the setUserProperties and updateUserProperties from the Editor.

Interesting, if not handling user properties here then it may be easier for identity. Curious how we should handle this separately.

bgiori · 2022-03-01T00:32:44Z

id/src/main/java/com/amplitude/id/IdentityStore.kt

+    fun addIdentityListener(listener: IdentityListener)
+    fun removeIdentityListener(listener: IdentityListener)


If we want to go full on coroutines we could make this into a StateFlow<IdentityState> where IdentityState is an class of Identity and IdentityUpdateType

If we do this, though, then the consumer must also be using kotlin. E.g. accessing coroutine objects from java is a PITA. If this is only an internal library called from other kotlin libraries then we should be able to go crazy with the coroutine api.

This module would be an internal module though. Would there be any use case customer would like to access the data directly without analytics or experiment? If not I guess we can definitely consider this.

bgiori · 2022-03-01T00:38:27Z

core/src/main/java/com/amplitude/core/utilities/EventsFileManager.kt

+
+    private var curFile: File? = null
+
+    private val semaphore = Semaphore(1)


You could use a Mutex which comes built in with a withLock extension function.

Let me take a look at Mutex

android/src/main/java/com/amplitude/android/Amplitude.kt

bgiori · 2022-03-01T00:43:44Z

id/src/main/java/com/amplitude/id/IdentityStore.kt

+    fun onUserIdChange(userId: String?)
+
+    fun onDeviceIdChange(deviceId: String?)


Whats the use case for these two callbacks when onIdentityChanged could handle both?

just to separate out for analytics end to tell which has been updated. We can discuss more about this

id/src/main/java/com/amplitude/id/utilities/PropertiesFile.kt

core/src/main/java/com/amplitude/core/Amplitude.kt

bgiori · 2022-03-01T01:10:47Z

core/src/main/java/com/amplitude/core/Storage.kt

+    enum class Constants(val rawVal: String) {
+        LAST_EVENT_ID("last_event_id"),
+        PREVIOUS_SESSION_ID("previous_session_id"),
+        LAST_EVENT_TIME("last_event_time"),
+        OPT_OUT("opt_out"),
+        Events("events")
+    }
+
+    suspend fun writeEvent(event: BaseEvent)
+
+    suspend fun write(key: Constants, value: String)
+
+    suspend fun rollover()
+
+    fun read(key: Constants): String?


Can this be called EventStorage to disambiguate?

Do you mean the interface? I thought about that once. Then kind of debate if I would need another storage to handle event id, session id, etc.

core/src/main/java/com/amplitude/core/Amplitude.kt

qingzhuozhen

I setup a time for us to sync tomorrow. Feel free to review more in the meantime.

android/src/main/java/com/amplitude/android/Amplitude.kt

android/src/main/java/com/amplitude/android/utilities/AndroidStorage.kt

core/src/main/java/com/amplitude/core/Amplitude.kt

qingzhuozhen · 2022-03-01T19:16:47Z

core/src/main/java/com/amplitude/core/Storage.kt

+    enum class Constants(val rawVal: String) {
+        LAST_EVENT_ID("last_event_id"),
+        PREVIOUS_SESSION_ID("previous_session_id"),
+        LAST_EVENT_TIME("last_event_time"),
+        OPT_OUT("opt_out"),
+        Events("events")
+    }
+
+    suspend fun writeEvent(event: BaseEvent)
+
+    suspend fun write(key: Constants, value: String)
+
+    suspend fun rollover()
+
+    fun read(key: Constants): String?


Do you mean the interface? I thought about that once. Then kind of debate if I would need another storage to handle event id, session id, etc.

qingzhuozhen · 2022-03-01T19:17:46Z

core/src/main/java/com/amplitude/core/utilities/EventsFileManager.kt

+
+    private var curFile: File? = null
+
+    private val semaphore = Semaphore(1)


Let me take a look at Mutex

qingzhuozhen · 2022-03-01T19:19:17Z

id/src/main/java/com/amplitude/id/IdentityStore.kt

+    fun onUserIdChange(userId: String?)
+
+    fun onDeviceIdChange(deviceId: String?)


just to separate out for analytics end to tell which has been updated. We can discuss more about this

qingzhuozhen · 2022-03-01T19:20:48Z

id/src/main/java/com/amplitude/id/IdentityStore.kt

+data class Identity(
+    val userId: String? = null,
+    val deviceId: String? = null,
+    val userProperties: Map<String, Any?> = mapOf(),
+)


Interesting, if not handling user properties here then it may be easier for identity. Curious how we should handle this separately.

qingzhuozhen · 2022-03-01T19:22:47Z

id/src/main/java/com/amplitude/id/IdentityStore.kt

+    fun addIdentityListener(listener: IdentityListener)
+    fun removeIdentityListener(listener: IdentityListener)


This module would be an internal module though. Would there be any use case customer would like to access the data directly without analytics or experiment? If not I guess we can definitely consider this.

id/src/main/java/com/amplitude/id/utilities/PropertiesFile.kt

justin-fiedler · 2022-03-01T02:26:58Z

core/src/main/java/com/amplitude/core/platform/Plugin.kt

@@ -100,3 +100,14 @@ abstract class DestinationPlugin: EventPlugin {
    }
 }

+abstract class ObservePlugin: Plugin {


Maybe IdObserverPlugin instead? Or do you see adding more non-id callbacks to this class?

android/src/main/java/com/amplitude/android/utilities/AndroidStorage.kt

id/src/main/java/com/amplitude/id/utilities/PropertiesFile.kt

justin-fiedler · 2022-03-01T20:51:18Z

id/src/main/java/com/amplitude/id/IdentityStorage.kt

+
+    fun setup(identityStore: IdentityStore)
+
+    fun saveUserId(userId: String?)


Is there a way to name the methods in Storage & IdentityStorage interface to be more consistent? Storage.saveEvents() vs Storage.writeEvents() etc?

id/src/main/java/com/amplitude/id/IdentityManager.kt

id/src/main/java/com/amplitude/id/IdConfiguration.kt

bgiori

WIP

id/src/main/java/com/amplitude/id/IdentityManager.kt

id/src/main/java/com/amplitude/id/IdentityContainer.kt

id/src/main/java/com/amplitude/id/IdentityManager.kt

bgiori

🎉

# 1.0.0 (2022-06-28) ### Bug Fixes * fix android aar issue ([#32](#32)) ([69f5fe7](69f5fe7)) * fix group call ([#42](#42)) ([f84d143](f84d143)) ### Features * add amplitude destination plugin draft ([#3](#3)) ([7ee9408](7ee9408)) * add android context plugin ([#14](#14)) ([25b6da3](25b6da3)) * add basic test action and issue templates ([#9](#9)) ([e72c608](e72c608)) * add button to kotlin example to send custom event ([#41](#41)) ([5c1e573](5c1e573)) * add configuration java support ([#35](#35)) ([a4e0801](a4e0801)) * add error handling and retry ([#13](#13)) ([689b114](689b114)) * add eu and batch support ([#17](#17)) ([6402b2a](6402b2a)) * add event bridge module ([#12](#12)) ([49229ac](49229ac)) * add event id support ([#25](#25)) ([94aaa6b](94aaa6b)) * add github action about docs and release ([#19](#19)) ([3aa868e](3aa868e)) * add identify support ([#18](#18)) ([2f86adc](2f86adc)) * add identity module and file storage ([#8](#8)) ([8b7bada](8b7bada)) * add Java android example ([#24](#24)) ([2670a2b](2670a2b)) * add Kotlin android sample ([#23](#23)) ([3b948c8](3b948c8)) * add parner_id ([#16](#16)) ([5e46cb9](5e46cb9)) * add plan in configuration and export ([#34](#34)) ([d84a84e](d84a84e)) * add plan versionId support ([#22](#22)) ([ae84619](ae84619)) * add revenue helper class ([#6](#6)) ([341fcf1](341fcf1)) * add unit tests for common jvm module ([#29](#29)) ([dcc0e9d](dcc0e9d)) * add unit tests in core module ([#30](#30)) ([4283148](4283148)) * add unit tests in event bridge module ([#28](#28)) ([8d63c28](8d63c28)) * add unit tests in id module ([#27](#27)) ([dc78bc6](dc78bc6)) * android local sync update ([#20](#20)) ([e25ecd4](e25ecd4)) * enable to pass the event options for all track call ([#36](#36)) ([8dd1ab6](8dd1ab6)) * expose more api for support ([#33](#33)) ([312fd85](312fd85)) * make configuration open ([#38](#38)) ([d0dfb15](d0dfb15)) * remove json object from public interface ([#26](#26)) ([0840b5d](0840b5d)) * set the client userId and deviceId in identify call ([#37](#37)) ([2c8b3f1](2c8b3f1)) * set up android module bare bone ([#7](#7)) ([a8a8dc9](a8a8dc9)) * Set up jvm example ([#4](#4)) ([ae825d6](ae825d6)) * setup basic barebone ([#2](#2)) ([8e4b635](8e4b635)) * setup basic doc and workflow ([#1](#1)) ([31bd223](31bd223)) * setup common modules ([#11](#11)) ([4a4ff66](4a4ff66)) * update android storage ([#15](#15)) ([56c1d6a](56c1d6a)) * update library name ([#21](#21)) ([309976d](309976d))

github-actions · 2022-06-28T20:41:00Z

🎉 This PR is included in version 1.0.0 🎉

The release is available on:

GitHub release
v1.0.0

Your semantic-release bot 📦🚀

qingzhuozhen requested review from justin-fiedler, bgiori and kevinpagtakhan February 28, 2022 06:24

qingzhuozhen mentioned this pull request Feb 28, 2022

feat: Stab to integrate current connector and extract identity storage #5

Closed

1 task

bgiori reviewed Mar 1, 2022

View reviewed changes

qingzhuozhen commented Mar 1, 2022

View reviewed changes

justin-fiedler reviewed Mar 1, 2022

View reviewed changes

qingzhuozhen requested review from bgiori and justin-fiedler March 6, 2022 20:09

bgiori reviewed Mar 8, 2022

View reviewed changes

id/src/main/java/com/amplitude/id/IdentityManager.kt Outdated Show resolved Hide resolved

id/src/main/java/com/amplitude/id/IdConfiguration.kt Outdated Show resolved Hide resolved

id/src/main/java/com/amplitude/id/IdConfiguration.kt Outdated Show resolved Hide resolved

qingzhuozhen force-pushed the add-file-storage branch from 68e20c7 to 0b240ef Compare March 12, 2022 22:41

qingzhuozhen requested a review from bgiori March 13, 2022 06:19

bgiori reviewed Mar 15, 2022

View reviewed changes

id/src/main/java/com/amplitude/id/IdentityManager.kt Outdated Show resolved Hide resolved

id/src/main/java/com/amplitude/id/IdentityContainer.kt Outdated Show resolved Hide resolved

id/src/main/java/com/amplitude/id/IdentityManager.kt Outdated Show resolved Hide resolved

qingzhuozhen added 9 commits March 16, 2022 22:58

feat: add identity module and file storage

8af0897

patch: clean up

ee1a11b

patch: fix compile

23490ac

patch: rename

623fd55

patch: add instace name and rename identity store

303332a

patch: adjust to use mutex

2231f06

patch: rename

35a9bef

patch: rename and move the storage to idenity manager

147eb46

patch: resolve comments

2ec5db8

qingzhuozhen force-pushed the add-file-storage branch from 0b240ef to 2ec5db8 Compare March 17, 2022 05:59

qingzhuozhen requested a review from bgiori March 17, 2022 06:02

patch: fix typo

60e7691

qingzhuozhen mentioned this pull request Mar 18, 2022

feat: add error handling and retry #13

Merged

1 task

bgiori approved these changes Mar 18, 2022

View reviewed changes

qingzhuozhen merged commit 8b7bada into main Mar 18, 2022

qingzhuozhen deleted the add-file-storage branch March 31, 2022 22:58

github-actions bot added the released label Jun 28, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: add identity module and file storage #8

feat: add identity module and file storage #8

qingzhuozhen commented Feb 28, 2022

bgiori commented Mar 1, 2022

qingzhuozhen commented Mar 1, 2022

bgiori left a comment

bgiori Mar 1, 2022

qingzhuozhen Mar 1, 2022

bgiori Mar 1, 2022

qingzhuozhen Mar 1, 2022

bgiori Mar 1, 2022

qingzhuozhen Mar 1, 2022

bgiori Mar 1, 2022

qingzhuozhen Mar 1, 2022

bgiori Mar 1, 2022

qingzhuozhen Mar 1, 2022

qingzhuozhen left a comment

qingzhuozhen Mar 1, 2022

qingzhuozhen Mar 1, 2022

qingzhuozhen Mar 1, 2022

qingzhuozhen Mar 1, 2022

qingzhuozhen Mar 1, 2022

justin-fiedler Mar 1, 2022

justin-fiedler Mar 1, 2022

bgiori left a comment

bgiori left a comment

github-actions bot commented Jun 28, 2022

		fun addIdentityListener(listener: IdentityListener)
		fun removeIdentityListener(listener: IdentityListener)


		private var curFile: File? = null

		private val semaphore = Semaphore(1)

		fun onUserIdChange(userId: String?)

		fun onDeviceIdChange(deviceId: String?)


		fun setup(identityStore: IdentityStore)

		fun saveUserId(userId: String?)

feat: add identity module and file storage #8

feat: add identity module and file storage #8

Conversation

qingzhuozhen commented Feb 28, 2022

Summary

Checklist

bgiori commented Mar 1, 2022

qingzhuozhen commented Mar 1, 2022

bgiori left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

qingzhuozhen left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bgiori left a comment

Choose a reason for hiding this comment

bgiori left a comment

Choose a reason for hiding this comment

github-actions bot commented Jun 28, 2022