Adding engage endpoint

[GBH]

So now it's possible to manage people via /engage endpoint. I added engage_set and engage_add methods, also the generic engage one to keep things dry.

Looks like this:

  @mixpanel.engage_set(@user.id, {:username => @user.username, :email => @user.email})
  @mixpanel.engage_add(@user.id, {:monkeys_punched => 12})

I reshuffled some code inside tracker.rb. The only adjustment I needed to do is remove endpoints from @url attribute as I wanted to reuse initialized @mixpanel tracker. Otherwise you need to spin up a separate one if yo want to track events and people.

Moved special person properties parser into it's own method. It won't mutate passed in hash either. Wanted to do similar thing for event tracking, but can't find any documentation. I know $signup is a thing, can't find others.

Added all necessary tests.

[Emerson]

This is an exciting addition to the codebase.

[dombort]

+1

[zevarito]

Hi @GBH,

Yesterday I receive another pull request which adds engage endpoint.

https://github.com/zevarito/mixpanel/pull/45

I will take my time to analyze both before merge. Thoughts and comments are appreciated.

[sturgill]

There are only so many ways to skin a cat, and both approaches are similar enough. There are some stylistic differences, but I don't think one is necessarily better than the other. That said, this pull request does take into account special properties whereas I didn't.

I don't think it really matters which pull you merge: this one should work as is, and my approach can be amended to include special properties.

Coin toss as far as I can see.

[GBH]

I think the only major difference is that in my patch you can reuse same tracker instance:

@mixpanel = Mixpanel::Tracker.new(MIXPANEL_API_KEY, { }, :async => true)
@mixpanel.track_event('Signup')
@mixpanel.engage_set(@user.id, :email => @user.email)

Also it doesn't change original tracker initialization, no need for :action param.

[sturgill]

Yeah, my personal use case (which obviously influenced the direction I took) doesn't have a need to reuse the tracker instance. Every call is basically a once-off called through Resque: there's a single and specific job the tracker is being asked to do.

I also don't completely see why reusing the same tracker instance is advantageous. The format of the request is different enough in the engage API that I actually heavily considered creating a separate class completely and having the Engage and Tracker objects inherit from the same parent class to reuse the Base64 stuff.

I would actually love to see two distinct classes, e.g.

@track = Mixpanel::Tracker.new(TOKEN)
@engage = Mixpanel::Engage.new(TOKEN)

Tracker and Engage would then both be based on a common parent:

module Mixpanel
  class Tracker << Mixpanel
    ...
  end

  class Engage << Mixpanel
    ...
  end
end

and have the Mixpanel::Mixpanel class have the common methods.

Again, it's a matter of style, but I would actually argue that splitting the two makes more sense than keeping them merged. I tried to keep the new code as non-intrusive to the original as possible, but I would actually prefer to see a refactoring of sorts.

But I still think it's just a style thing. And while style is important, I'd rather have this feature part of the official gem sooner than later.

[GBH]

Yeah, I'd prefer doing this:

@mixpanel = Mixpanel.new(TOKEN)
@mixpanel.track('Event')
@mixpanel.engage(:set, @user_id, :email => 'test@example.com')
@mixpanel.import('Event', :foo => 'bar')

Don't know why do you want to initialize new objects for different endpoints though. But yeah, Mixpanel::Tracker currently does too many things. Right now I just need engage method in.

Cheers.

[sturgill]

I think we're pretty much on the same page: neither of us is thrilled with how tracker works, but all we really care about is being able to send something to the engage API.

I would be completely down with your suggested approach: one object and sensibly named methods. Again, my use case only calls one method per object, so my approach above is pretty jaded.

[sturgill]

Had some spare time tonight so I worked through some of this rewrite. See this commit. I made it a new branch on my fork, see here.

I didn't work through async or middleware in case this isn't of interest. But I think it's a lot cleaner... ;)

Sample implementation:

@mixpanel = Mixpanel.new TOKEN
@mixpanel.track 'Event Name', { hash_of_properties }
@mixpanel.import API_KEY, 'Event Name', { hash_of_properties }
@mixpanel.set distinct_id, { hash_of_properties }
@mixpanel.increment distinct_id, { hash_of_properties }

All of these will take an options hash as a final parameter. One of those keys would be :async allowing to set async in the initializer, but change that for a specific action. The track, import, set and increment methods also look for a key called :url to overwrite the default endpoint for each of these methods.

[zevarito]

@sturgill, @GBH,

Overall I like how different API usages are exposed, but there is a small things that we should keep in mind.

• Get rid of Mixpanel module and go back with Mixpanel class is not acceptable because the gem will conflict with other libs using Mixpanel namespace as well, please take a look at this commit: https://github.com/zevarito/mixpanel/commit/9631c51313e01de90863d9e01cdcd4376b36d4f1 That was a feature request made by other users from this gem in the past. This makes me thing that if you want to keep all API calls inside the same class (say Tracker) it doesn't seems to be the right name, I'd say let's find a properly name or just split it in Track, Import etc. sub-classes.
• If changes will not be backward compatible (api change) we should push a Mayor version of the Gem instead a Minor update to avoid break users code.
• Test should pass in every supported platform.

I think you both have reached a point where this gem needs to evolve and the discussion you are having here is great, let's polish this work a little bit to complete the refactor and finally deliver a new version.

[sturgill]

• Changing the class to be Mixpanel::Tracker is easy enough to do - I'll work on that on Monday. I rather like having the various methods accessible within a single class - @GBH converted me...
• My opinion: you should merge @GBH's work as is as a minor point release so that current users have access to this API immediately, and if the refactor work is advantageous, you release that as a major update (and not backwards compatible).
  • I might be biased, but I think the refactor is quite a bit cleaner and easier to extend in the future with additional Mixpanel features that are released.
  • That said, I don't see why you should wait to expose the engage endpoint. Regardless of the refactor decision, you should merge this pull request as soon as you can. That's just my opinion, of course, but this pull request gets the job done within the constraints of the current Tracker workflow. We should continue to improve the refactor, but current users might as well benefit from this work right away.
• I'll fire up rvm and test in other environments as I can, but would love some help with that as well. Especially with JRuby.

[zevarito]

@GBH I've merged your changes and pushed 2.2.0 version of the gem, thanks!

@sturgill Looking forward on your changes, please re-open a new issue to handle that, thanks!

[GBH]

Thanks @zevarito . Looking forward to improvements in 3.0 :)

[zevarito]

Hi @GBH, take a look here https://github.com/zevarito/mixpanel/pull/47