-
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
Conversant Analytics Adapter: initial release #8981
Conversant Analytics Adapter: initial release #8981
Conversation
Sync'ing from upstream
…alytics adapter - commits squashed to 1 checkin - testing - documentation - throttling
@robertrmartinez I'm curious if you have an ETA on the review for this. My team is trying to figure out when we can announce some marketing material and I'm not sure what I can tell them. |
Will give it a thorough review tonight! |
@robertrmartinez great thanks so much. Didn't mean to rush you, I know you are busy. Just trying to figure out what to tell my product manager :) |
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 fine, just a couple minor things + a question.
sampleRate: 100 | ||
} | ||
}) | ||
}); |
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.
think we can remove this from the example page.
import CONSTANTS from '../src/constants.json'; | ||
import {getGlobal} from '../src/prebidGlobal.js'; | ||
import adapterManager from '../src/adapterManager.js'; | ||
import * as utils from '../src/utils.js'; |
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.
Better to just import the needed utils instead of all 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.
Looks to just be a couple log utils, deepAccess, and isInteger. So very small amount of the util lib! So just import those specifically.
const cnvrAdUnit = cnvrHelper.createAdUnit(); | ||
// Initialize bids with bidderCode | ||
adUnit.bids.forEach(bid => { | ||
cnvrAdUnit.bids[bid.bidder] = cnvrHelper.initializeBidDefaults(); |
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 possible that a publisher has more than one bid for a given bidder within an adUnit.
{
code: 'some-ad-unit-code',
mediaTypes: {
banner: {
sizes: [300,250, 728,90]
}
},
bids: [
{ bidder: 'ix', params: { siteId: 1234, size: [300,250] } },
{ bidder: 'ix', params: { siteId: 1234, size: [728,90] } }
]
}
I think in this case, this code will always just set it to whatever the last ix bid is in here.
Same for lower in the nobids
and bidsrecieved
parts, it will add an event to eventCodes array, but overwrite the bidPayload
with the last entry in it.
I guess if you do not care and do not want to track such scenario, or just want the last bid in the array to send it's data it is fine.
But maybe something to look into. I know with my experience I see many pubs having several bids in the bids array for the same bidders.
…m 1 bidder for 1 adslot
@robertrmartinez fixed all issues noted |
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.
Nice! Looks good
* cnvr_analytics-adapter - First implementation of Conversant Prebid Analytics adapter - commits squashed to 1 checkin - testing - documentation - throttling * cnvr_analytics-adapter - cicircle fix * cnvr_analytics-adapter - code review tweaks, handle multiple bids from 1 bidder for 1 adslot
* cnvr_analytics-adapter - First implementation of Conversant Prebid Analytics adapter - commits squashed to 1 checkin - testing - documentation - throttling * cnvr_analytics-adapter - cicircle fix * cnvr_analytics-adapter - code review tweaks, handle multiple bids from 1 bidder for 1 adslot
Type of change
Bugfix
[x ] Feature
New bidder adapter
Code style update (formatting, local variables)
Refactoring (no functional changes, no api changes)
Build related changes
CI related changes
Does this change affect user-facing APIs or examples documented on http://prebid.org?
Other
Description of change
Initial release of Conversant Prebid Analytics Adapter
Documentation: prebid/prebid.github.io#4011
Other information