jkrall / analytical

Gem for managing multiple analytics services in your rails app.
MIT License
381 stars 93 forks source link

Mixpanel: track page views as a single event, and allow optionally disabling of page view tracking #70

Closed oshoma closed 11 years ago

oshoma commented 11 years ago

This change breaks compatibility for page view tracking via Mixpanel.

Previously, each page view was logged as a distinct event, with the URL as the name of the event. This can result in many events, if you track many different URLs.

As of this commit the Mixpanel module logs each page view as an instance of the "page viewed" event.

This change confirms with a recommendation in the Mixpanel docs. See here: https://mixpanel.com/docs/getting-started/events-vs-page-views

To revert to the old behavior, use track(url, event: url) to specify the URL as the event name.

To disable page view tracking altogether in Mixpanel, initialize analytical with a { track: false } option for the mixpanel module.

Lastly, the method signature for #track in this module now matches the method signature used elsewhere in the analytical gem.

sfsekaran commented 11 years ago

I see a few test failures in travis: https://travis-ci.org/jkrall/analytical/builds/6141393

oshoma commented 11 years ago

Thanks Sathya.

Looks like the order in which strings are converted to JSON is not necessarily the same on all machines.

I'll push a fix for this later today.

osh

On Mon, Apr 8, 2013 at 2:39 PM, Sathya Sekaran notifications@github.comwrote:

I see a few test failures in travis: https://travis-ci.org/jkrall/analytical/builds/6141393

— Reply to this email directly or view it on GitHubhttps://github.com/jkrall/analytical/pull/70#issuecomment-16069715 .

nirvdrum commented 11 years ago

Yeah. Ruby is the only system I'm aware of that imposes an ordering on dictionary keys by default. Most others will not, including anything you pass in and out of a JSON engine. You're better off parsing the JSON and checking for logical equivalence.

sfsekaran commented 11 years ago

Thanks for fixing the failure. :+1:

I tend to think we should actually start a v3.1.x branch and start accumulating some of these backwards-incompatible changes that we still want to merge somewhere.

Thoughts?

Other than that, it should be noted that Mixpanel just plain doesn't advocate doing your analysis via pageviews, or at least not on every single page. I'm sure you all know this, but for anyone who stumbles upon this, it's just an FYI.

oshoma commented 11 years ago

Sathya, I'd be much more comfortable pushing breaking changes to a branch other than master.

One important question here is who is the "we" in "we should actually start a v3.1.x branch"? :) Joshua notes in his README that the gem needs a committed maintainer.

And yes, what you say about Mixpanel and pageviews is true.

On Wed, Apr 10, 2013 at 3:46 AM, Sathya Sekaran notifications@github.comwrote:

Thanks for fixing the failure. [image: :+1:]

I tend to think we should actually start a v3.1.x branch and start accumulating some of these backwards-incompatible changes that we still want to merge somewhere.

Thoughts?

Other than that, it should be noted that Mixpanel just plain doesn't advocate doing your analysis via pageviews, or at least not on every single page. I'm sure you all know this, but for anyone who stumbles upon this, it's just an FYI.

— Reply to this email directly or view it on GitHubhttps://github.com/jkrall/analytical/pull/70#issuecomment-16159969 .

sfsekaran commented 11 years ago

Agreed.

And "we" includes me, since I'm a maintainer now. That messaging in the README should be changed a bit I suppose, in the very least to not alarm people.

My only issue with the separate branch is that it's the nature of analytical to make backwards-incompatible changes since client-side tracking libs do it all the time. After giving it some more thought, I feel as though we could reasonably merge this into master and then release v3.1.0 afterward.

I'd love another maintainer's stamp of approval on this, but if we get no response from the others for a while, I'm going to go with my gut and merge and try to get @jkrall to give me access to release the next gem version. (It's been a long time since the last gem version anyway.)