method-inc / nodealytics

Server-side google analytics collection for node.js
MIT License
69 stars 16 forks source link

Tracked events not associated with page #8

Open marks opened 11 years ago

marks commented 11 years ago

My understand of Google Analytics is that events should be associated with a page url/title. When I run the examples in this repo for event tracking after tracking a pageview, the event is not associated with the page like I think it should be.

Thanks,

Mark

marks commented 11 years ago

My commit at https://github.com/marks/nodealytics/commit/b6eaa6e89e11cd3bc8ca3dcb2c34ce74e813db7d and the one before it (https://github.com/marks/nodealytics/commit/08d0db3f36ddfbad2afd0fd0943eccd820e93f33) seem to do it for me but is this the correct way?

NOTE: I have not tested it to ensure it works when not all params are passed to NA.trackEvent

iamdustan commented 11 years ago

The method you want is NA.trackPage https://github.com/Skookum/nodealytics#track-page

Event tracking is an entirely different piece of the GA puzzle. https://developers.google.com/analytics/devguides/collection/gajs/eventTrackerGuide

iamdustan commented 11 years ago

I just realized you may be asking for a way to tie a page to a custom event. Mah bad.

That eventTracker method is asking for a ridiculous number of arguments. A pull request to refactor that is more than welcome https://github.com/Skookum/nodealytics/blob/master/lib/main.js#L103

Perhaps something like:

exports.trackEvent = function (event, options, callback) {
  if (arguments.length === 7) return doBackwardsCompatabilityWay.apply(NA, arguments);

  // do new thing with event and options object
}

This would also open up the flexibility for you to easily add that data to your event tracking.

marks commented 11 years ago

Yeah - I definitely need to refactor it but I'm more looking for guidance if this is correct and why it wasn't done like this before. I want to make sure that this is the proper way to do it and the fact that the page name/url not populating for events wasnt due to a cookie or other issue.

iamdustan commented 11 years ago

@rockbot – any knowledge to share on this front?

iamdustan commented 11 years ago

@marks that’s a great question. I’m not really sure; I’ve just inherited responsibility for this. I invited the original author back in to provide feedback.

rockbot commented 11 years ago

Hi @marks - thanks for the feedback! The only reason it wasn't done before was probably because we didn't need the functionality at the time.

So, looking at the code now, I'm not liking having so many parameters in the function call in the first place. Instead, I'd recommend changing it to an object: function (eventParams, callback), where eventParams takes in all of those extra parameters :-)

That way you can get rid of all the if (typeof blah === function) business (which is good for a shorter number of arguments, but now it's just getting silly) and simply update the eventParams object before sending it off to runQuery. It'll be simpler, shorter, etc.

Does that make sense? If you can put that together as a pull request, I think that'd make an excellent upgrade to the current codebase :-)

Hope that's helpful!

marks commented 11 years ago

Yeah - no big deal... I havent refactored the arguments yet just because I wasn't sure if it was a logical change in the first place. Will try to contribute later in the week.

Mark Silverberg @Skramhttp://twitter.com/skram | @MappyHealthhttp://twitter.com/mappyhealth http://www.socialhealthinsights.comhttp://www.socialhealthinsights.com/ http://www.linkedin.com/in/silverbergmark

Coming late summer 2013: CheckQMhttp://checkqm.com/ "Simplifying the process of building clinical quality measures into your EHR" [cid:0855C261-8F74-418E-9E3A-5D36710D579D]

From: Raquel Vélez notifications@github.com<mailto:notifications@github.com> Reply-To: Skookum/nodealytics reply@reply.github.com<mailto:reply@reply.github.com> Date: Wednesday, July 10, 2013 1:09 PM To: Skookum/nodealytics nodealytics@noreply.github.com<mailto:nodealytics@noreply.github.com> Cc: Mark Silverberg mark@socialhealthinsights.com<mailto:mark@socialhealthinsights.com> Subject: Re: [nodealytics] Tracked events not associated with page (#8)

Hi @markshttps://github.com/marks - thanks for the feedback! The only reason it wasn't done before was probably because we didn't need the functionality at the time.

So, looking at the code now, I'm not liking having so many parameters in the function call in the first place. Instead, I'd recommend changing it to an object: function (eventParams, callback), where eventParams takes in all of those extra parameters :-)

That way you can get rid of all the if (typeof blah === function) business (which is good for a shorter number of arguments, but now it's just getting silly) and simply update the eventParams object before sending it off to runQuery. It'll be simpler, shorter, etc.

Does that make sense? If you can put that together as a pull request, I think that'd make an excellent upgrade to the current codebase :-)

Hope that's helpful!

Reply to this email directly or view it on GitHubhttps://github.com/Skookum/nodealytics/issues/8#issuecomment-20757493.

iamdustan commented 11 years ago

Thanks @rockbot!

Yeah, I would like to keep the old signature around for at least one version, too. Whenever you get around to it could you throw something like this into the old signature? Thanks, @marks

console.warn('Deprecated: GA.trackEvent signature has been deprecated and will be removed soon.\n See https://github.com/Skookum/nodealytics for updated docs")