Sync telemetry #1090
Sync telemetry #1090
Conversation
|
@eliserichards here is some docs I'll review this today as well. |
There was a problem hiding this comment.
How difficult would it be to generate a random "sync_id" for each sync and include that in all sync-related events? would make analyzing success/failure on a per-sync basis easier.
barring that, i believe we would still have at most one sync end or sync timeout event per sync start event, correct?
| case .reset: | ||
| return .reset | ||
| case .sync: | ||
| return .sync |
There was a problem hiding this comment.
given that there is also sync timeout end and error, should this be sync start or similar?
| return id | ||
| case .touch(let id): | ||
| return "ID: \(id)" | ||
| case .syncError(let error): |
There was a problem hiding this comment.
according the telemetry library there is a max string length of 50 https://github.com/mozilla-mobile/telemetry-ios/blob/aac1e1873134ea69f992e3bbc45b431d6564d64c/Telemetry/TelemetryEvent.swift#L8 its unclear to me what would happen if we pass it something longer,e.g. if an error message is really long
There was a problem hiding this comment.
I understand that you're using Actions in a way we haven't done previously, in order to leverage the existing Telemetry machinery.
This is okay, but feels somewhat hacky. The right way to do this would be to make the telemetry machinery listen to the datastore sync state and log something there — we already have a TimedOut sync state.
Given this may be additionally machinery to add another alternative would be to make the actions change the state of the datastore: check the sync status isn't .Synced, dispatch a .Timeout action, then make the datastore respond to that action by syncStatus.accept(.Timeout).
Incidentally, the android version appears to use exactly your implementation.
For exediency, I'm going to raise a tech-debt issue for this, but approve this.
Shared/Action/DataStoreAction.swift
Outdated
| @@ -16,6 +16,59 @@ enum DataStoreAction: Action { | |||
| case sync | |||
| case touch(id: String) | |||
| case delete(id: String) | |||
| case syncEnd | |||
| case syncTimeout(error: String) | |||
There was a problem hiding this comment.
Nit: Do we need to have an error message for syncTimeout?
| return id | ||
| case .touch(let id): | ||
| return "ID: \(id)" | ||
| case .syncError(let error): |
Shared/Store/BaseDataStore.swift
Outdated
| @@ -257,17 +259,20 @@ extension BaseDataStore { | |||
| if (self.syncSubject.value != .Synced) { | |||
| self.syncSubject.accept(.TimedOut) | |||
| self.dispatcher.dispatch(action: SentryAction(title: "Sync timeout without error", error: nil, line: "")) | |||
| self.dispatcher.dispatch(action: DataStoreAction.syncTimeout(error: "")) | |||
There was a problem hiding this comment.
Oh, I see what is happening. Hmm. I don't think I like this. I'll expand further.
…ncTimeout DataStoreAction
iOS version of mozilla-lockwise/lockwise-android#835