-
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
Added bid pool and fixed getAllWinningBids function #2328
Conversation
…bids # Conflicts: # src/prebid.js
src/targeting.js
Outdated
|
||
const MAX_DFP_KEYLENGTH = 20; | ||
const TTL_BUFFER = 200; |
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.
let's make the buffer 1000
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 good. Can you up the buffer amount
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.
Some names look like they could be updated, but overall looks good
src/auction.js
Outdated
@@ -197,8 +197,8 @@ export function newAuction({adUnits, adUnitCodes, callback, cbTimeout, labels}) | |||
executeCallback, | |||
callBids, | |||
bidsBackAll, | |||
setWinningBid: (winningBid) => { _winningBid = winningBid }, | |||
getWinningBid: () => _winningBid, | |||
setWinningBid: (winningBid) => { _winningBids = _winningBids.concat(winningBid) }, |
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.
Seems addWinningBid
is a more apt name now.
src/auction.js
Outdated
setWinningBid: (winningBid) => { _winningBid = winningBid }, | ||
getWinningBid: () => _winningBid, | ||
setWinningBid: (winningBid) => { _winningBids = _winningBids.concat(winningBid) }, | ||
getWinningBid: () => _winningBids, |
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.
and getWinningBids
here.
Docs PR here prebid/prebid.github.io#700 |
* Created bid pool, fixed getAllWinningBids and added new api getAllPrebidWinningBids * Updated ttl buffer to 1000 * updated function names
Type of change
Description of change
This PR adds bid pool concept to prebid.js
In case of concurrent auctions, auctionManager can have multiple bids from same bidder for a particular adUnit. While selecting bids to do auction, prebid.js uses following criteria if such multiple bids are found.
ttl
is not expired. Here buffer of2001000 ms is included in check.Also added new api function
getAllPrebidWinningBids
which will return the bids won in prebid auction but not rendered by dfp.Also fixed the bug in
getAllWinningBids
described here #2238