-
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
Magnite Analytics Adapter: New Adapter #9068
Magnite Analytics Adapter: New Adapter #9068
Conversation
…niteAnalyticsAdapter
fix clashing rubicon configs in tests
…into magniteAnalyticsAdapter
so use "requestId"
…niteAnalyticsAdapter
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.
Apart from the comment in the code, remeber to add entry in prebid docs
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.
added a typo and a question, but in general LGTM, after next responses I can approve
modules/rubiconAnalyticsAdapter.js
Outdated
@@ -799,7 +804,7 @@ let rubiconAdapter = Object.assign({}, baseAdapter, { | |||
} | |||
|
|||
let bid = auctionEntry.bids[args.requestId]; | |||
// If floor resolved gptSlot but we have not yet, then update the adUnit to have the adSlot name | |||
// If floor rot besolved gptSlut we have not yet, then update the adUnit to have the adSlot name |
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 think there's an error here
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.
@robertrmartinez don't forget about this one
} | ||
}; | ||
} | ||
resetRubiConf(); |
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.
can yoy explain why there was the need for this addition?
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 for tests to be able to start clean, and I figured instead of defaulting and then having a function reset those defaults, better to just have a function to set defaults and call it at load time.
But yeah, tests use it to clean the conf between tests.
} | ||
|
||
const subscribeToGamSlots = () => { | ||
window.googletag.pubads().addEventListener('slotRenderEnded', event => { |
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 not use slotResponseReceived; not a change request, just curious
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 actually do not know! This is what we have been using in the old Rubicon Analytics Adapter.
What are the differences between the two? I will go look at documentation but curious as to your thoughts.
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.
Looks like slotResponseReceived
does not have the gam ID's included. At least from the documentation standpoint.
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 does though, they are just undocumented
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.
also, annoyingly, yieldgroupid and companyid are not documented
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.
does it really?
I looked and it did not pass it, will double check, if so the slotResponseReceived is def better as it fires earlier!
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.
@patmmccann On my test page I get the following:
Add delay processing to Bid Won
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.
LGTM
Hi @patmmccann the review is ok on my side but since you are also listed in reviewers I'd wait for your approval too. |
@ChrisHuie Want to give this a once over and merge please? @patmmccann as well if you want to! |
* stash * mgni analytics done? * test not finished * remove log * get rid of console log * pass render transaction id * add bid Id * fix bidder done * fix tests * Handle PPI elementids case * added lots of test + minor fixes * implement Michele review comments fix clashing rubicon configs in tests * michele fixes * sometimes bidWons do not have "bidId" so use "requestId" * add logs so we can debug on page * handle currency conversion errors better * bug fixes * time since page load * Magnite MD * Move session to auction init * Change adUnitMap to transactionId Add delay processing to Bid Won * oops
* stash * mgni analytics done? * test not finished * remove log * get rid of console log * pass render transaction id * add bid Id * fix bidder done * fix tests * Handle PPI elementids case * added lots of test + minor fixes * implement Michele review comments fix clashing rubicon configs in tests * michele fixes * sometimes bidWons do not have "bidId" so use "requestId" * add logs so we can debug on page * handle currency conversion errors better * bug fixes * time since page load * Magnite MD * Move session to auction init * Change adUnitMap to transactionId Add delay processing to Bid Won * oops
Type of change
Description of change
New Analytics Adapter for MAGNITE.
This new analytics adapter will sunset the Rubicon Analytics Adapter. Possible in 8.X or 9.X depending on timing. For now we will leave both in.
DOCS PR: prebid/prebid.github.io#4056