New Feature: Save Undispatched Events For Offline Tracking #377
Conversation
save events for possible offline tracking
|
Hey @bobunmeng, thanks a lot for your PR. Really cool that you are taking on this task. I think it's something that can improve the Matomo SDK quite a bit. I've got a counter proposal for the technical implementation though: In the SDK the |
|
Thank you @brototyp . There is always a greater idea out there. I will try to make this work more smoothly and please help check it when it is done. This is very important for the organizaion I am working for as well. |
|
@brototyp The update is complete. I hope it is better than the previous implementation. Thank you. |
There was a problem hiding this comment.
Looks really nice to me. I've got a few small comments in the code.
I was also wondering, if we should limit the number of events somehow. Imagine a device that is offline for a long time. That could mean we would be collecting tons of events which could slow down tracking operations quite a bit (adding another event to the queue) and such.
What do you think about adding another optional parameter to the initializer, with which we can limit the number of events that will be queued with a reasonable default value. And every time we queue the next events, we make sure to ensure these limits. The user of the SDK can then decide if they want to change this value.
| @objc convenience public init(siteId: String, baseURL: URL, userAgent: String? = nil) { | ||
| /// - useBackedQueue: An optional parameter for using UserDefaultsBackedQueue instead of the default memory queue. | ||
| @objc convenience public init(siteId: String, baseURL: URL, userAgent: String? = nil, useBackedQueue: Bool = false) { | ||
| let validSuffix = baseURL.absoluteString.hasSuffix("piwik.php") || | ||
| baseURL.absoluteString.hasSuffix("matomo.php") | ||
| assert(validSuffix, "The baseURL is expected to end in piwik.php or matomo.php") | ||
|
|
||
| let queue = MemoryQueue() | ||
| let dispatcher = URLSessionDispatcher(baseURL: baseURL, userAgent: userAgent) | ||
| let queue: Queue = useBackedQueue ? UserDefaultsBackedQueue(suiteName: "\(siteId)\(dispatcher.baseURL.absoluteString)") : MemoryQueue() | ||
| self.init(siteId: siteId, queue: queue, dispatcher: dispatcher) | ||
| } |
There was a problem hiding this comment.
I think we would not even need any changes to this initializer here. The user of the SDK could use the init(siteId: String, queue: Queue, dispatcher: Dispatcher) directly and use the UserDefaultsBackedQueue as a queue there. What do you think?
|
|
||
| extension UserDefaultsBackedQueue { | ||
| internal struct Key { | ||
| static let queueEvents = "PiwikBackedQueueEvents" |
There was a problem hiding this comment.
What do you think about this?
| static let queueEvents = "PiwikBackedQueueEvents" | |
| static let queuedEvents = "MatomoQueuedEvents" |
| init(suiteName: String?) { | ||
| userDefaults = UserDefaults(suiteName: suiteName)! | ||
| } |
There was a problem hiding this comment.
What do you think about passing in an instance of UserDefaults directly?
| init(suiteName: String?) { | |
| userDefaults = UserDefaults(suiteName: suiteName)! | |
| } | |
| init(userDefaults: UserDefaults) { | |
| userDefaults = userDefaults | |
| } |
|
Hey @bobunmeng, thanks for the changes. Looks really nice. I left a few comments from my side. Can you also do mea favor an rebase this pr onto develop (or merge develop into here)? That should also fix the build issues here. |
|
@brototyp I have merged with the current develop branch. I will check your comments soon. Thanks :) |
|
Hi @bobunmeng, I took the liberty to fork your PR and polish it a little bit to make it mergeable. Do you mind having a look at it to give me your feedback if you have some? It's here: #408 |
…into feature/support_offline_tracking
|
#408 got merged. Closing this PR |
Matomo dispatches event every 30seconds.
Issues:
Solutions:
** This is also great for offline tracking. Because the events are saved no matter in which scenario.
Consequence: