Add eventID's to time and track methods for more reliable durations. #625
Conversation
… accurate to the types stored in the dictionary.
cmriboldi
changed the title
| typealias TimedEventID = String | ||
| typealias TimedEvents = [TimedEventID: TimeInterval] |
There was a problem hiding this comment.
I added these two typealiases for additional type-safely and clarity. Ideally TimedEventID is it's own type that wraps a string but I didn't want to change the call site too drastically from what it already is.
| @@ -1017,17 +1020,34 @@ extension MixpanelInstance { | |||
| - parameter properties: properties dictionary | |||
| */ | |||
| public func track(event: String?, properties: Properties? = nil) { | |||
| track(event: event, withID: nil, properties: properties) | |||
There was a problem hiding this comment.
The call site for all of these methods remains the same for backwards compatibility, and under the hood they just call the other method that uses the event id.
| return timedEvents | ||
| } | ||
| let timedEventID = eventID ?? eventName |
There was a problem hiding this comment.
If no eventID is provided we fall back to using whatever the eventName is which means we default back to the original behavior.
| @@ -139,32 +141,32 @@ class Track { | |||
| update(&superProperties) | |||
| } | |||
|
|
|||
| func time(event: String?, timedEvents: InternalProperties, startTime: Double) -> InternalProperties { | |||
| func time(eventID: TimedEventID, timedEvents: TimedEvents, startTime: TimeInterval) -> TimedEvents { | |||
There was a problem hiding this comment.
I renamed the property name for clarity, and changed the types for consistency/correctness and removed the optionality of the eventID because it didn't need to be optional.
|
@zihejia and @jaredmixpanel Is there any chance we can add this? I can go ahead and fix the merge conflict but this is a great enhancement we would love to get into your SDK so that we don't have to live off of a fork of the repo. |
|
@cmriboldi Thanks for the PR. I think we can add it, just a few things though...
|
|
Awesome! thank you @jaredmixpanel. All of those are reasonable. I'll make those changes and then update this MR. We actually use a UUID in our code base for the eventID which was helpful. The main reason I went with a String at least internally was so that eventName can be backwards compatible. But I'll come up with something. |
As mentioned in this issue there is no way to time events independent of their name. This is a HUGE issue for any serious user of Mixpanel events because there is no way to follow the Best Practice Naming Guidelines and be certain your durations are correct.
For example, if I were to have a generic System event that I want to time as long as I have one of those events I'm fine, but if I ever need to time multiple system events there is no way for me to organize them or distinguish using Mixpanel which
.track(event:)call matches with the originaltime(event:)call. This means that durations will be completely missing or inaccurate.The that I'm adding in this MR is to link the tracking of timed events to an EventID instead of to the event name. That means callers of the SDK can provide their own unique id's for id's of events and be guaranteed that when the duration completes for that generic event the duration is completely accurate.
I added a new function signature for these timed and tracked events that allow users to provide a String eventID to link the two events. I would have liked to create a more type safe ID so that it's not just any string but because of the way the SDK works with events I didn't want to change too much besides separating the idea of an eventName with the idea of an eventID. I also separated out the idea of a
TimedEventsdictionary from theInternalPropertiesdictionary that existed previously, this adds some small type safety around the dictionary and adds purpose clarity in the code.The original method signatures still exist and work just as they did before they simply call the eventID versions of these methods so that there aren't two places that the logic needs to be maintained.