Skip to content
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

Allow configuring ga tracker name when enabling ga analytics for Prebid #292

Closed
wants to merge 1 commit into from

Conversation

bborn
Copy link
Contributor

@bborn bborn commented Mar 31, 2016

See issue #291

@mkendall07
Copy link
Member

Thanks for the PR @bborn
Do you know the way to get the tracker name from GTM? Since they append a timestamp to their object name, it would be hard to know it ahead of time (to pass through the ga.config object). Any thoughts there?

@bborn
Copy link
Contributor Author

bborn commented Apr 1, 2016

Yeah, docs are here: https://developers.google.com/analytics/devguides/collection/analyticsjs/accessing-trackers#getall

ga(function(tracker) {
  // Logs the trackers name.
  // (Note: default trackers are given the name "t0")
  console.log(tracker.get('name'));

  // Logs the client ID for the current user.
  console.log(tracker.get('clientId'));

  // Logs the URL of the referring site (if available).
  console.log(tracker.get('referrer'));
});

I'm pretty sure GTM also logs the timestamp to the dataLayer, but I didn't figure out exactly where/when.

I don't think Prebid should do that though - there are cases where a uses might have set there own custom name, or where there are multiple trackers on the page (so how would Prebid know which one to use?). I think it makes more sense to just allow setting the name and then let the user pass it in (and maybe give an example of usage with GTM).

@mkendall07 mkendall07 self-assigned this Apr 1, 2016
if (typeof gaOptions.trackerName !== 'undefined') {
_trackerName = gaOptions.trackerName;
} else {
_trackerName = null;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This else isn't necessary, since it's already defined as null

@mkendall07
Copy link
Member

@bborn
Thanks for the commits and PR. I've updated with my feedback, can you please address those, and squash your commits into a single commit (http://gitready.com/advanced/2009/02/10/squashing-commits-with-rebase.html) then force commit

@bborn
Copy link
Contributor Author

bborn commented Apr 1, 2016

@mkendall07 ok, does this look better? I'm a little out of my element here (JS/node), but I think setting a _trackerPrefix variable makes more sense, since the prefix is composed of the trackerName plus a .

@@ -175,11 +182,16 @@ function getCpmDistribution(cpm) {
return distribution;
}

exports.prefixedSend = function(){
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This doesn't need to be a function. Just do exports.prefixedSend = _trackerPrefix + 'send';

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does that exports.prefixedSend need to be inside the enableAnalytics function?

@bborn
Copy link
Contributor Author

bborn commented Apr 6, 2016

@mkendall07 updated to use trackerSend. It also seems like all those window[_gaGlobal]( trackerSend, 'event' calls should be refactored to use a function, but I didn't want to mess with it.

@protonate
Copy link
Collaborator

Thanks, could you flatten these two commits? And then we'll be ready to merge.

@bborn
Copy link
Contributor Author

bborn commented Apr 6, 2016

Done.

@@ -179,7 +187,7 @@ function sendBidRequestToGa(bid) {
if (bid && bid.bidderCode) {
_analyticsQueue.push(function () {
_eventCount++;
window[_gaGlobal]('send', 'event', _category, 'Requests', bid.bidderCode, 1, _disibleInteraction);
window[_gaGlobal]( trackerSend, 'event', _category, 'Requests', bid.bidderCode, 1, _disibleInteraction);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

trackerSend is undefined when this function is called, you probably want a local variable for it. Also the test needs to be updated with the exported trackerSend -- though I believe that is the only case where the export is needed?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@protonate not sure how that happens. Shouldn't enableAnalytics always get called before this function, therefore setting trackerSend?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

trackerSend is defined on the options object at line 42, it's not accessible any other way at the moment. The window[_gaGlobal] function is called when the _analyticsQueue is iterated in the checkAnalytics function, so scope has changed a few times over by this point. I've attached two screenshots that illustrate this.

screen shot 2016-04-06 at 10 57 12 pm

screen shot 2016-04-06 at 11 03 53 pm

This could be made to work by declaring trackerSend as a local variable in the ga module's scope, as you do with _trackerPrefix. Then it will be accessible to enableAnalytics and all the sendToGa functions.

allow configuring ga tracker name and add some tests

add ga spec

coding style updates

update prefixedSend usage

use trackerSend instead of prefixedSend

fix tests
@bborn
Copy link
Contributor Author

bborn commented Apr 7, 2016

Sorry - fixed specs.

@protonate
Copy link
Collaborator

Fixed undefined error and merged manually with this commit: 3551d9e

Thanks for contributing to Prebid.js.

@protonate protonate closed this Apr 8, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants