-
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
Bidder Settings: Support for option to apply adapter bid adjustment to unknown bidder codes #9609
Conversation
src/utils/cpm.js
Outdated
const bidCpmAdjustment = bs.get(bidResponse?.bidderCode || bidRequest?.bidder, 'bidCpmAdjustment'); | ||
|
||
const adapterAllowAlternateBidderCodes = bs.get(bidResponse?.adapterCode || bidRequest?.bidder, 'allowAlternateBidderCodes'); | ||
const catchUnknownBidderCodesWithAdapterBidAdjustment = bs.get(bidResponse?.adapterCode || bidRequest?.bidder, 'catchUnknownBidderCodesWithAdapterBidAdjustment'); |
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.
- this setting (and variable) name is way too long, past the point of ridiculous. I'll offer
adjustAlternateBids
, oradjustAllBids
. - what's the reason for looking at
allowAlternateBidderCodes
? If you have a bid from an alternate code (that is, one whereadapterCode
is different frombidderCode
), it should mean that it was allowed already. - I can't make sense of the fallback
bidResponse?.adapterCode || bidRequest?.bidder
. If you are usingbidRequest?.bidder
, are you not just using the "normal" bid adjustment rules?
src/utils/cpm.js
Outdated
@@ -2,9 +2,15 @@ import {auctionManager} from '../auctionManager.js'; | |||
import {bidderSettings} from '../bidderSettings.js'; | |||
import {logError} from '../utils.js'; | |||
|
|||
export function adjustCpm(cpm, bidResponse, bidRequest, {index = auctionManager.index, bs = bidderSettings} = {}) { | |||
export function adjustCpm(cpm, bidResponse, bidRequest, {index = auctionManager.index, bs = bidderSettings} = {}, gba = getBidAdjustment) { |
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.
gba
is a very thin wrapper around bs
. it gets confusing if you can "decouple" them; if I pass in a particular instance of bidderSettings
, I would expect it to be used throughout. to me this looks like you're asking for the same thing twice.
@dgirardi thanks for the feedback. I addressed what you brought up, could you take another look when you have the time? |
src/utils/cpm.js
Outdated
@@ -4,7 +4,10 @@ import {logError} from '../utils.js'; | |||
|
|||
export function adjustCpm(cpm, bidResponse, bidRequest, {index = auctionManager.index, bs = bidderSettings} = {}) { | |||
bidRequest = bidRequest || index.getBidRequest(bidResponse); | |||
const bidCpmAdjustment = bs.get(bidResponse?.bidderCode || bidRequest?.bidder, 'bidCpmAdjustment'); | |||
|
|||
const adjustForAlternateBids = bs.get(bidResponse?.adapterCode, 'adjustForAlternateBids'); |
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.
adjustFor
reads weird; we're adjusting the bids, not adjusting for the bids.
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.
changed the var name to be just adjustAlternateBids
src/utils/cpm.js
Outdated
const bidCpmAdjustment = bs.get(bidResponse?.bidderCode || bidRequest?.bidder, 'bidCpmAdjustment'); | ||
|
||
const adjustForAlternateBids = bs.get(bidResponse?.adapterCode, 'adjustForAlternateBids'); | ||
const scope = adjustForAlternateBids ? bidResponse?.adapterCode : bidResponse?.bidderCode || bidRequest?.bidder; |
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.
this means something else now - if I have a bid with {adapterCode: "adapter', bidderCode: 'bidder'}
, and this config:
bidderSettings = {
bidder: {
bidCpmAdjustment: bidderAdjustment,
},
adapter: {
bidCpmAdjustment: adapterAdjustment,
adjustForAlternateBids: true
}
}
then we are picking adapterAdjustment
over bidderAdjustment
; is that the intent? I think it's surprising behavior.
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.
Yes, believe that is the intent from #8312 (or my interpretation at least), to provide Publisher's with a config option to handle a bidAdjustment by adapter. Do you think this would be a feature that we would want to merge in? Or would we want to pass on this one?
cc @patmmccann
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.
discussed offline - we agreed that it makes sense to pick the more specific adjustment funcition; e.g., if we find an adjustment for bidder
, we should pick it over the adapter's adjustment funcition even if the adapter says adjustForAlternateBids
.
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.
discussed briefly, if a publisher has a specific adjustment for 'friend_of_pubmatic' than it should be used regardless of flag adjustForAlternateBids, and we should fall back to that flag applying when there is no named bidder adjustment function match
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.
thanks guys! makes sense with what you both are saying. Adjusted the logic just now with a new commit.
src/adapters/bidderFactory.js
Outdated
if (!!responseBidder && !!requestBidder && requestBidder !== responseBidder) { | ||
alternateBiddersList = isArray(alternateBiddersList) ? alternateBiddersList.map(val => val.trim().toLowerCase()).filter(val => !!val).filter(uniques) : alternateBiddersList; | ||
if (!allowAlternateBidderCodes || (isArray(alternateBiddersList) && (alternateBiddersList[0] !== '*' && !alternateBiddersList.includes(responseBidder)))) { | ||
if ((!adjustForAlternateBids && !allowAlternateBidderCodes) || !allowAlternateBidderCodes || (isArray(alternateBiddersList) && (alternateBiddersList[0] !== '*' && !alternateBiddersList.includes(responseBidder)))) { |
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.
what's this for - why would isInvalidAlternateBidder
care about adjustments?
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.
See what you mean, reverted my change for this line.
src/utils/cpm.js
Outdated
const adapterCode = bidResponse?.adapterCode; | ||
const bidderCode = bidResponse?.bidderCode || bidRequest?.bidder; | ||
const adjustAlternateBids = bs.get(bidResponse?.adapterCode, 'adjustAlternateBids'); | ||
const scope = adjustAlternateBids && !globalBidderSettings.hasOwnProperty(bidderCode) ? adapterCode : bidderCode; |
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.
This should be equilvalent but avoids using getGlobal().bidderSettings
(which defeats the purpose of injecting in bs
as a dependency):
const bidCpmAdjustment = bs.getOwn(bidderCode, 'bidCpmAdjustment') || bs.get(adjustAlternateBids ? adapterCode : bidderCode, 'bidCpmAdjustment')`
Hey @patmmccann, could you also review this one when you have a chance? |
…o unknown bidder codes (prebid#9609) * support to default on adapter bidderSetting if config option is true * addressed review feedback * changed var name to adjustAlternateBids and refactored isInvalidAlternateBidder func * adjusted logic around adapter and bidder code * improvements * consolidate tests in single spec file * fix lint --------- Co-authored-by: Demetrio Girardi <[email protected]>
Type of change
Description of change
Other information
#8312