-
Notifications
You must be signed in to change notification settings - Fork 17
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[#16] user based cross device tracking #17
Conversation
* adding an optional user_id attribute to experiments * when this attribute is present, variant selection will be based on the user_id as part of the seed for a hash function (along with experiment name and the type of caller) * tracking uuid for Gimel and Keen will also change to be based on the user_id as seed, so the same user will only get tracked once from different devices. Unless the goal is not-unique. * updated tests and added adapter tests for Gimel and Keen.io
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks in general very good. Only a few comments
CHANGES
Outdated
* 0.15.0-rc.1 BREAKING changes (only if you wrote your own tracking adapter) | ||
- User-based / Cross-device tracking | ||
- If you use your own tracking adapter, you'll have to change it. | ||
* `experiment_start` now accepts the `experiment` as the first parameter |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would use the word 'expects' since you really have to pass this parameter. 'accepts' sounds quite optional.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, great suggestion!
}, | ||
goal_complete: function(experiment_name, variant, event_name) { | ||
keen_client.addEvent(experiment_name, {variant: variant, event: event_name}); | ||
goal_complete: function(experiment, variant, event_name, _props) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why _props
when you don't use it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
by convention variables that are passed over but not used are marked with _
prefix. I think it's useful to know that it's being passed over from the experiment, even if it's not used.
callback = @_remove_uuid(item.properties.uuid) | ||
@_ajax_get(@url, item.properties, callback) | ||
callback = @_remove_quuid(item.properties._quuid) | ||
@_ajax_get(@url, utils.omit(item.properties, '_quuid'), callback) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do you have to remove the _quuid
here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
there's no need to send it over the wire.
@experiment_start: (experiment_name, variant) => | ||
@_track(@namespace, "#{experiment_name} | #{variant}", 'Visitors') | ||
@experiment_start: (experiment, variant) => | ||
@_track(@namespace, "#{experiment.name} | #{variant}", 'Visitors') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the third parameter should be an object, no?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not in this tracking adapter... the tracking adapters don't share any code and can implement internally differently.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In general I think you should try to keep the interfaces of the adapters similar.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The interface is the same. This is internal implementation.
src/adapters.coffee
Outdated
|
||
@goal_complete: (experiment_name, variant, goal) => | ||
@_track(@namespace, "#{experiment_name} | #{variant}", goal) | ||
@goal_complete: (experiment, variant, goal, _props) => |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In PersistentQueueKeenAdapter
the third parameter is the goal_name
and here it is just goal
. I think you should stay consistent with it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for spotting. Will change this.
_random: (salt) -> | ||
return utils.random() unless @user_id | ||
seed = "#{@name}.#{salt}.#{@user_id}" | ||
utils.random(seed) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do you need this seed for the user_id
? Can't you just always use utils.random("#{@name}.#{salt}.#{@user_id}")
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are you asking why I'm assigning it to a variable? not strictly necessary. I can pass it directly as you suggested. But might be a bit more readable to understand what this string is used for?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No that wasn't my question. My question is why you can't use utils.random("#{@name}.#{salt}.#{@user_id}")
even when there is no @user_id
? Simply writing
_random: ->
utils.random("#{@name}.#{@user_id}")
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That won't be random though...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you quickly explain how does it work with the salt?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Take a look at the implementation. It's super small and simple. It uses a hash function with the seed and generates a random number based on the seed. If the seed is different for different users, the result hash will produce a different hash, which would get "translated" to a different number between 0 and 1. So for a given experiment, salt and user_id, we'll get a pseudo-random number. If you send the same user_id (or null
), then you'll always get the same result, which won't be (pseudo)random...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When we don't have a user_id, using a "real" random number is preferable.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok, thanks
@gingerlime Somehow I missed it. Why do I need to specifically add the |
@joker-777 I wrote about it on this wiki page https://github.com/Alephbet/alephbet/wiki/User-based-and-Cross-device-tracking
I don't think you can mix not-logged-in and logged-in on the same experiment, without getting unreliable results unfortunately. I tried to cover a couple of cases where this might cause inconsistent results on the wiki page.
It might be possible to do this internally, but this definitely complicates things. You'd need to store/cache the user_id. You'd need to know when the user_id is missing, vs when it wasn't meant to be used at all... It's probably a good idea, but need to think carefully and design it to work seamlessly without creating unexpected behaviour. For now, I think it's ok to expect the caller to make sure the user_id is available, and it's not too difficult to cache it externally if you want to "remember" who the user was initially. |
The only way to achieve this is to add another attribute, in addition to |
two attributes for one purpose makes the Alephbet API ugly. If the intention is to make it transparent, then this should be handled internally with only the |
be based on the user_id as part of the seed for a hash
function (along with experiment name and the type of caller)
based on the user_id as seed, so the same user will only
get tracked once from different devices. Unless the goal
is not-unique.