-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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
Rubicon analytics v2 #5698
Rubicon analytics v2 #5698
Conversation
@@ -126,10 +135,11 @@ function sendMessage(auctionId, bidWonId) { | |||
}); | |||
} | |||
let auctionCache = cache.auctions[auctionId]; | |||
let referrer = config.getConfig('pageUrl') || auctionCache.referrer; | |||
let referrer = config.getConfig('pageUrl') || (auctionCache && auctionCache.referrer); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
handle if this is undefinied in some cases
if (auctionCache.session.fpkvs && Object.keys(auctionCache.session.fpkvs).length) { | ||
message.fpkvs = Object.keys(auctionCache.session.fpkvs).map(key => { | ||
return { key, value: auctionCache.session.fpkvs[key] }; | ||
}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
mapping into array of objects of { key, value } requested by internal team
@@ -376,12 +492,18 @@ let rubiconAdapter = Object.assign({}, baseAdapter, { | |||
]); | |||
cacheEntry.bids = {}; | |||
cacheEntry.bidsWon = {}; | |||
cacheEntry.referrer = args.bidderRequests[0].refererInfo.referer; | |||
cacheEntry.referrer = utils.deepAccess(args, 'bidderRequests.0.refererInfo.referer'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
deep acess so we do not throw runtime undefined errors
'creativeId', creativeId => utils.isNumber(creativeId) ? creativeId : undefined, | ||
'lineItemId', lineItemId => utils.isNumber(lineItemId) ? lineItemId : undefined, | ||
'adSlot', () => event.slot.getAdUnitPath(), | ||
'isSlotEmpty', () => event.isEmpty || undefined |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
WE only want to send this if slot.isEmpty is true.
If the slot is NOT empty, then there will be the advertiserIds etc, so not necessary to send.
…iconAnalytics-v2
new param `channel` tests
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code changes look good. Just had 1 question on the test schema file
@@ -293,7 +368,8 @@ | |||
"success", | |||
"no-bid", | |||
"error", | |||
"rejected" | |||
"rejected-gdpr", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will the status ever be rejected-gdpr
? I don't see this string used anywhere else in the code
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not yet, but that is to come in another phase.
I can remove it though
…iconAnalytics-v2
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
few questions
cache.auctions[args.auctionId] = cacheEntry; | ||
// register to listen to gpt events if not done yet | ||
if (!cache.gpt.registered && utils.isGptPubadsDefined()) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
where is cache.gpt.registered
set to true
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it is not, must have left it out or removed it accidentally! Thanks, good catch!
modules/rubiconAnalyticsAdapter.js
Outdated
|
||
function setRpaCookie(decodedCookie) { | ||
try { | ||
storage.setCookie(COOKIE_NAME, window.btoa(JSON.stringify(decodedCookie))); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why using a cookie
rather than local storage
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
updated to localStorage
if (!utils.deepAccess(bid, 'adUnit.adSlot') && utils.deepAccess(args, 'floorData.matchedFields.gptSlot')) { | ||
bid.adUnit.adSlot = args.floorData.matchedFields.gptSlot; | ||
if (!utils.deepAccess(bid, 'adUnit.gam.adSlot') && utils.deepAccess(args, 'floorData.matchedFields.gptSlot')) { | ||
utils.deepSetValue(bid, 'adUnit.gam.adSlot', args.floorData.matchedFields.gptSlot); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not sending pbAdSlot
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
https://github.com/prebid/Prebid.js/pull/5698/files#diff-7eda04fa0d03f759128e98228a4d9ed5R583
Set just above. 11 lines
modules/rubiconAnalyticsAdapter.js
Outdated
@@ -479,7 +608,7 @@ let rubiconAdapter = Object.assign({}, baseAdapter, { | |||
delete bid.error; // it's possible for this to be set by a previous timeout | |||
break; | |||
case NO_BID: | |||
bid.status = args.status === BID_REJECTED ? 'rejected' : 'no-bid'; | |||
bid.status = args.status === BID_REJECTED ? 'rejected-ipf' : 'no-bid'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why -ipf
? Is this the only reason a bid could be rejected? Maybe we use a status called BID_REJECTED_IPF
to make this more explicit?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There will be future rejected reasons, so updating with -ipf now.
But I'll make it a constant.
@@ -608,7 +612,7 @@ let rubiconAdapter = Object.assign({}, baseAdapter, { | |||
delete bid.error; // it's possible for this to be set by a previous timeout | |||
break; | |||
case NO_BID: | |||
bid.status = args.status === BID_REJECTED ? 'rejected-ipf' : 'no-bid'; | |||
bid.status = args.status === BID_REJECTED ? BID_REJECTED_IPF : 'no-bid'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was thinking args.status === BID_REJECTED_IPF
as the constant. Cause what if new code starts rejecting bids for a reason other than ipf? But this would require updates in other places.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
but we can fix this later. for now this is ok
Type of change
Description of change
Adding functionality to the Rubicon Analytics adapter to work with session and googletag level data sets.