zevarito / mixpanel

Simple lib to track events in Mixpanel service. It can be used in any rack based framework.
MIT License
273 stars 84 forks source link

When setting people properties, cannot set $ip for proper geolocation #59

Closed jochu closed 11 years ago

jochu commented 11 years ago

When calling mixpanel.set distinct_id, {:status => "spiffy", :$ip => "0.0.0.0"}

It generates a person like:

{"$set": {"status": "spiffy", "$ip": "0.0.0.0"},
 "$token": "...",
 "$distinct_id": "..."
}

When I want a request like:

{"$set": {"status": "spiffy"},
 "$token": "...",
 "$distinct_id": "...",
 "$ip": "0.0.0.0"
}

My current workaround is patching build_person to take special action on :$ip, but that seems less than ideal.

zevarito commented 11 years ago

Please tell me which version are you using.

I'll take a look tomorrow.

On Sat, Dec 8, 2012 at 1:59 AM, Jeffrey Chu notifications@github.comwrote:

When calling mixpanel.set distinct_id, {:status => "spiffy", :$ip => "0.0.0.0"}

It generates a person like:

{"$set": {"status": "spiffy", "$ip": "0.0.0.0"}, "$token": "...", "$distinct_id": "..." }

When I want a request like:

{"$set": {"status": "spiffy"}, "$token": "...", "$distinct_id": "...", "$ip": "0.0.0.0" }

My current workaround is patching build_person to take special action on :$ip, but that seems less than ideal.

— Reply to this email directly or view it on GitHubhttps://github.com/zevarito/mixpanel/issues/59.

Alvaro

jochu commented 11 years ago

I am using version 3.1.0.

Looking at the code, it looks like it doesn't support it in master either. It's specifically in

  def build_person(action, distinct_id, properties)
    { "$#{action}".to_sym => properties_hash(properties, PERSON_PROPERTIES), :$token => @token, :$distinct_id => distinct_id }
  end

Which predefines the only things that appear outside of the $set (or other actions) is going to only be $token and $distinct_id

zevarito commented 11 years ago

@jochu I didn't meant it was on master, I just want to know which version you were using :)

Ok, I need to get time to add it, I'll try to do it this week, but seems like you will still need to continue with your workaround for a few days, it is hard to get a short period of time these weeks. If you'd like to submit a pull request before that, you are welcome, just let me know so I don't do it. Thanks.

jeremybdk commented 11 years ago

I'd love to get that too as right now all my user on mixpanel get the same ip ...

mixellent commented 11 years ago

+1

etagwerker commented 11 years ago

I am seeing an issue related to this.

This is part of my code:

...
def self.mixpanel(env)
  Mixpanel::Tracker.new MIXPANEL_API_KEY, { :env => env }
end

def self.perform(name, seller_id, env)
  seller = Seller.find(seller_id)
  m = mixpanel(env)

  begin
    m.set(seller_id, seller.to_mixpanel)
    ..

I've made sure I followed the instructions for Resque jobs and I've triple checked that I am setting REMOTE_ADDR correctly.

Here is a sample of env:

{"REMOTE_ADDR"=>"186.137.140.183", "HTTP_X_FORWARDED_FOR"=>"186.137.140.183", ... }

That IP is from Argentina, but it's still setting the location using the IP from the server (USA)

Here is a sample of seller.to_mixpanel:

{:distinct_id=>1068, :name=>"mixxxxxpanl", :created=>Thu, 20 Dec 2012 21:25:02 UTC +00:00, :url=>"http://mixxxxxpanl.ombushop.com", :active=>false, :account_type=>"Plus", :email=>"ernesto+mixxxxxpanl@ombushop.com", :first_name=>nil, :ip=>"186.137.140.183", :last_name=>nil, :last_login=>nil, :phone=>""}

I guess I should try the approach suggested by @jochu

zevarito commented 11 years ago

Guys, I am keeping an eye on this thread, I wish to have more time to fix it for but with holidays and stuff I haven't. As soon as I get a bit of time I'll fix it, meanwhile pull requests for this bug are very welcome.

jochu commented 11 years ago

I've written a patch that I'll submit a pull request shortly.

It sets the geolocation ip automatically based on the provided env.

For those who won't necessarily have a sane env around, I've also updated it so you can set the geolocation ip by doing:

mixpanel.set {:distinct_id => "...", :ip => "127.0.0.1"}, {:status => "spiffy"}

This change is backward compatible - simply providing a distinct_id as the first argument will still work. This change does not apply to append_ series of methods because those will automatically use the javascript behavior.

zevarito commented 11 years ago

Ops. I guess we should have coordinated first, I got 3 different pull requests for the same issue, looking at those now.

I'll answer them in a moment, also I'll be working a few hours Today and Tomorrow so we can discuss about implementations, I hope to be able to push a fix before week starts.

Pull requests are...

62

63

64

zevarito commented 11 years ago

I've updated all PR with comments. So far I think #62 is good since reuses ip method in tracker. #64 is adding more supported params of Mixpanel API call.

Thoughts?

jochu commented 11 years ago

One of the goals when making my update was to make it pretty easy to add additional fields that I didn't consider. I believe you should be able to simply add ignore_time to a constant.

I also wanted to avoid muddling what went into the options argument. I may be wrong, but to me it looked like options was largely used for modifying how requests were handled - not what requests contained.

Written on my phone, so please excuse me for being brief or other similar quirks.

zevarito commented 11 years ago

I've merged #62 PR to fix this issue. Also pushed version 3.4.0 of the Gem.

Please start from there for improvements, fixes and new features.