refactor: cdp migration - processing pending tasks in bgQ #424

ami-aman · 2023-12-06T16:42:39Z

closes: https://github.com/customerio/issues/issues/11649

What does this PR do?

This pull request introduces a method that analyzes the Journey's SDK background queue to determine if any tasks from the Journeys SDK remain unexecuted. Should such tasks exist, it then fetches each individual task and executes them one by one. This ensures that all pending tasks are executed prior to transitioning to the CDP and all the pending tasks are sent to the CDP. Once a task is successfully executed, it is removed from the background queue.

This PR currently handles :

fetching pending tasks
executing them
sending data to workspace via (migrated) cdp module
added TODO note to the methods that are unavailable currently in segment and have to be added in our CDP SDK
Some edge cases like sending data of non-current identifier
Code cleanup
Tested with large data (> 300)
Automated test with large data (3000takss)

Pending:

NA

Complete each step to get your pull request merged in. Learn more about the workflow this project uses.

Assign members of your team to review the pull request.
Wait for pull request status checks to complete. If there are problems, fix them until you see that all status checks are passing.
Wait until the pull request has been reviewed and approved by a teammate
After code reviews are approved and you determine this PR is ready to merge, select Squash and Merge button on this screen, leave the title and description to the default values, then merge the PR.

levibostian

Could you share some more information about this PR? What work has been done and what has not been done?

Without this context, I am unsure what to review.

Quickly reviewing it now, tests need to be added, implementation is incomplete, and there are performance issues in this implementation.

I understand if more PRs will be made to address these concerns but with the information I have right now, I am unsure if that's true.

github-actions · 2023-12-07T13:59:50Z

Sample app builds 📱

Below you will find the list of the latest versions of the sample apps. It's recommended to always download the latest builds of the sample apps to accurately test the pull request.

CocoaPods-FCM: Build failed. See CI job logs to determine the issue and try re-building.
APN-UIKit: fate-of-tracking (1703847519)

ami-aman · 2023-12-07T17:11:44Z

Could you share some more information about this PR? What work has been done and what has not been done?

Without this context, I am unsure what to review.

Quickly reviewing it now, tests need to be added, implementation is incomplete, and there are performance issues in this implementation.

I understand if more PRs will be made to address these concerns but with the information I have right now, I am unsure if that's true.

@levibostian Thanks for your feedback. I have updated the description with some detail. I hope it helps. Also to address the issue you highlighted with profileAttributes, I have considered that and updated the code. Let me know if you have any questions.

levibostian

Leaving some suggestions to start with. I will probably have more suggestions once more of the implementation is written for this feature.

Sources/Common/Util/JsonAdapter.swift

levibostian · 2023-12-11T20:53:45Z

Sources/Common/Background Queue/Queue.swift

+    public func deleteProcessedTask(_ task: QueueTaskMetadata) {
+        let storageId = task.taskPersistedId
+        if !storage.delete(storageId: storageId) {
+            logger.error("Failed to delete task with storage id: \(storageId).")
+        }
+    }


Instead of deleting BQ tasks after they have been migrated, I think it would be more performant that we tell the OS to delete the entire directory where all BQ inventory tasks are stored.

That is one option I did think of but this does not cover a scenario where the task isn't processed for some reason but then deleting the entire directory would remove those tasks too. Does this make sense?

levibostian · 2023-12-11T20:57:03Z

Sources/Tracking/CustomerIO.swift

+        // Check if any unprocessed tasks are pending in the background queue.
+        // If so, iterate over them and process each one.
+        if let allStoredTasks = implementation.getAllStoredTasks(), !allStoredTasks.isEmpty {
+            allStoredTasks.forEach { task in
+                implementation.getStoredTask(for: task)
+            }
+        }


I think the SDK will experience a lot of performance problems if we have the BQ migration code existing where it is here. Instead, I think the BQ migration should run on a different thread.

Here is an example of where we have done background thread processing in the past. Could we use similar logic?

I also want to suggest that we encapsulate all of this BQ migration code into it's own separate file. So we can write tests against it easier instead of having the logic inside of CustomerIO.initialize

I agree with you, Levi ! I do believe that this could create performance issues for bigger queues and it is a good idea to move this processing to a separate background thread. FYI, as mentioned in the description, it is a TODO to test this implementation with large data !

Shahroz16 · 2023-12-21T14:35:30Z

Sources/DataPipeline/DataPipelineImplementation.swift

+
+// To process pending tasks in background queue
+extension DataPipelineImplementation {
+    func processIdentifyFromBGQ(identifier: String, body: [String: Any]?) {


should we be adding timestamp as well?

identify does not have a timestamp in JSON that we get from the queue.
{"attributes_json_string":"null","identifier":"[email protected]"}

All event and screen tracks have timestamp and have been added to the methods (Refer this and this)

These timestamps are from the metric model, we can have timestamp for events from TaskMetaData. I added the comment where i think we can probably get and return that information.

That is createdAt which I believe might differ in terms of the actual timestamp. If that works for us then I can utilise and make updates in all the methods.

Updated the code with timestamp as available, if not then createdAt from QueueTaskMetadata !!!

ami-aman · 2023-12-26T12:53:04Z

Apps/APN-UIKit/APN UIKit/View/LoginViewController.swift

+            CustomerIO.shared.identify(identifier: emailId)
+            return
+        }
+        CustomerIO.shared.identify(identifier: emailId, body: data)


This fixes a crash that happens when this method sends a nil data to identify that CDP method doesn't expect.

we should fix the identify method as well, even if it gets nil there shouldn't be a crash. thanks for this workaround

Right. We must fix the identify method too in our SDK. This can be done in a separate PR as this is unrelated to this ticket.

Shahroz16 · 2023-12-27T12:49:56Z

Apps/APN-UIKit/APN UIKit/View/LoginViewController.swift

+            CustomerIO.shared.identify(identifier: emailId)
+            return
+        }
+        CustomerIO.shared.identify(identifier: emailId, body: data)


we should fix the identify method as well, even if it gets nil there shouldn't be a crash. thanks for this workaround

Shahroz16 · 2023-12-27T13:02:56Z

Sources/Tracking/CustomerIOImplementation.swift

+        case .trackDeliveryMetric:
+            // TODO: Segment doesn't provide this method by default needs to get added
+            // Remove isProcessed when the method is added
+            print("Track Delivery Metrics for in-app - Needs discussion")


its the same method as track metric for push, we just need to exclude recipient

let properties: [String: Any] = metaData.mergeWith([ "metric": event.rawValue, "deliveryId": deliveryId, ])

The calls are automatically going to trackMetric. What I have been thinking about is the use case of trackDeliveryMetric as I could not figure out a way to test this case. I added a "need discussion" comment because this is one of the possible cases in QueueTaskType but I had a hard time trying to reproduce this one.

Shahroz16 · 2023-12-27T13:21:32Z

Sources/DataPipeline/DataPipeline.swift

+    func processIdentifyFromBGQ(identifier: String, body: [String: Any]?)
+    func processScreenEventFromBGQ(identifier: String, name: String, timestamp: String?, properties: [String: Any])
+    func processEventFromBGQ(identifier: String, name: String, timestamp: String?, properties: [String: Any])
+    func processDeleteTokenFromBGQ(identifier: String, token: String)
+    func processRegisterDeviceFromBGQ(identifier: String, token: String, attributes: [String: Any]?)
+    func processPushMetricsFromBGQ(token: String, event: Metric, deliveryId: String, timestamp: String, metaData: [String: Any])


we should have timestamps in here, commenting in the method i think we can get that value from.

Shahroz16 · 2023-12-27T13:22:37Z

Sources/Common/Background Queue/Queue.swift

+    }
+
+    // TODO: Write test case
+    public func getTaskDetail(_ task: QueueTaskMetadata) -> (data: Data, taskType: QueueTaskType)? {


we can probably get the timestamp from here?

let createdAt = task.createdAt

Shahroz16 · 2023-12-27T13:24:42Z

Sources/DataPipeline/DataPipelineImplementation.swift

+
+// To process pending tasks in background queue
+extension DataPipelineImplementation {
+    func processIdentifyFromBGQ(identifier: String, body: [String: Any]?) {


These timestamps are from the metric model, we can have timestamp for events from TaskMetaData. I added the comment where i think we can probably get and return that information.

Shahroz16

Looks good, some suggestions to increase confidence in migration tasks.

Shahroz16 · 2023-12-29T10:20:28Z

Sources/DataPipeline/DataPipelineImplementation.swift

+extension DataPipelineImplementation {
+    func processIdentifyFromBGQ(identifier: String, timestamp: String, body: [String: Any]?) {
+        var identifyEvent = IdentifyEvent(userId: identifier, traits: nil)
+        identifyEvent.timestamp = timestamp


just make sure we have verified the format of timestamp that we are adding from BQ is same as one Analytics expect

Updated all timestamps to ISO format.

Sources/DataPipeline/DataPipelineImplementation.swift

Sources/Tracking/CustomerIOImplementation.swift

Shahroz16 · 2023-12-29T10:30:43Z

Tests/Tracking/CustomerIOImplementationTest.swift

+    func test_givenBacklog_expectTaskProcessed() {
+        var inventory: [QueueTaskMetadata] = []
+        let givenType = QueueTaskType.identifyProfile
+        let givenTask = IdentifyProfileQueueTaskData(identifier: String.random, attributesJsonString: "null")


we are only appending the same kind of task, it would have been great if we could have an additional test case where we verify each type of task and make sure its values are being received accordingly.

I do not think that will change the functionality in any way., Since it is a test case and not actual implementation so I do not believe it will make any change!

ami-aman · 2023-12-29T11:00:13Z

Looks good, some suggestions to increase confidence in migration tasks.

I would still suggest to do a peer and group testing of this feature since this is the major one. I am confident in it's working as far as I have tested but I do agree that dev testing is not enough at times specially when the feature is an important one like this ! We can do a test run once the team is back next week of all the modules that have been done. It can be a kind of UAT !

ami-aman added 13 commits December 5, 2023 16:41

getallstoredtasks

0a407c5

autogenerated file

4bcbf43

testing

f0a5290

gettasketail

715d6e7

iterations & passing on to cdp

13ef47a

tracking event and screen

0c906c1

added todo notes

da1b3e7

device attributes using cdp

4d58356

run each task

21d5cfc

todo question

ecdc2ec

undo queue change to reproduce a use case

df11e42

delete processed tasks

9647cfa

comment and todo

6b74a80

ami-aman requested a review from a team December 6, 2023 16:46

levibostian reviewed Dec 6, 2023

View reviewed changes

ami-aman added 3 commits December 7, 2023 19:24

saving work

cbc2b2b

autommockable

6f43ae8

Merge branch 'main-replica-for-cdp' into fate-of-tracking

c68cb72

clean code

1f19206

ami-aman requested review from a team and levibostian December 7, 2023 17:11

mrehan27 added 2 commits December 11, 2023 19:42

cdp branch update

0b6d957

updated swift package

dbcf3a8

levibostian reviewed Dec 11, 2023

View reviewed changes

mrehan27 added 4 commits December 12, 2023 11:26

added MetricEvent

acdc2f7

DeviceAttributes plugin

a3d777b

datapipeline implementation updates

f93c2b1

revert analytics branch

6de138b

ami-aman added 3 commits December 21, 2023 00:10

simplified register token & device attributes

0055238

minor fix

6dc4ba0

push metric

bab7b54

Shahroz16 reviewed Dec 21, 2023

View reviewed changes

ami-aman added 4 commits December 26, 2023 16:46

push metric with timestamp

bb497af

time stamp for track events

2b86e03

timestamp optional

82c6f77

minor fix

e126d7c

ami-aman commented Dec 26, 2023

View reviewed changes

ami-aman added 3 commits December 26, 2023 19:12

comments

dc75441

comment

4b5bf5a

more comments

498ad3a

ami-aman requested review from a team and Shahroz16 December 26, 2023 15:15

ami-aman marked this pull request as ready for review December 26, 2023 15:17

getAllStoredTasks test case

2ba29ca

Shahroz16 reviewed Dec 27, 2023

View reviewed changes

ami-aman added 5 commits December 29, 2023 01:16

all test cases

7cc58c5

more test cases

20c8533

timestamp

fc13cfd

DI & mockables

023eaaf

updated test case

6995cee

ami-aman requested a review from Shahroz16 December 29, 2023 09:29

Shahroz16 approved these changes Dec 29, 2023

View reviewed changes

ami-aman added 3 commits December 29, 2023 16:08

pr suggestion

6cbf33b

minor fix

9920ef4

iso format

dc2eeef

ami-aman merged commit 4ee9553 into main-replica-for-cdp Dec 29, 2023

ami-aman deleted the fate-of-tracking branch December 29, 2023 11:01

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor: cdp migration - processing pending tasks in bgQ #424

refactor: cdp migration - processing pending tasks in bgQ #424

ami-aman commented Dec 6, 2023 •

edited

Loading

levibostian left a comment

github-actions bot commented Dec 7, 2023 •

edited

Loading

ami-aman commented Dec 7, 2023

levibostian left a comment

levibostian Dec 11, 2023

ami-aman Dec 12, 2023

levibostian Dec 11, 2023

levibostian Dec 11, 2023

ami-aman Dec 12, 2023

ami-aman Dec 29, 2023

Shahroz16 Dec 21, 2023

ami-aman Dec 26, 2023

Shahroz16 Dec 27, 2023

ami-aman Dec 28, 2023

ami-aman Dec 29, 2023

ami-aman Dec 26, 2023 •

edited

Loading

Shahroz16 Dec 27, 2023

ami-aman Dec 28, 2023 •

edited

Loading

Shahroz16 Dec 27, 2023

Shahroz16 Dec 27, 2023

ami-aman Dec 28, 2023

Shahroz16 Dec 27, 2023

ami-aman Dec 28, 2023

Shahroz16 Dec 27, 2023

ami-aman Dec 28, 2023

Shahroz16 Dec 27, 2023

Shahroz16 left a comment

Shahroz16 Dec 29, 2023

ami-aman Dec 29, 2023

Shahroz16 Dec 29, 2023

ami-aman Dec 29, 2023

ami-aman commented Dec 29, 2023

refactor: cdp migration - processing pending tasks in bgQ #424

refactor: cdp migration - processing pending tasks in bgQ #424

Conversation

ami-aman commented Dec 6, 2023 • edited Loading

What does this PR do?

levibostian left a comment

Choose a reason for hiding this comment

github-actions bot commented Dec 7, 2023 • edited Loading

Sample app builds 📱

ami-aman commented Dec 7, 2023

levibostian 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

Choose a reason for hiding this comment

ami-aman Dec 26, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ami-aman Dec 28, 2023 • edited Loading

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

Shahroz16 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

ami-aman commented Dec 29, 2023

ami-aman commented Dec 6, 2023 •

edited

Loading

github-actions bot commented Dec 7, 2023 •

edited

Loading

ami-aman Dec 26, 2023 •

edited

Loading

ami-aman Dec 28, 2023 •

edited

Loading