-
Notifications
You must be signed in to change notification settings - Fork 765
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
OpenRTB Prebid Cache support #276
Changes from 21 commits
0adbe76
c5f02c0
167624b
ccdf9e7
466b291
3ed291f
8c576d9
785dc47
6ac6765
dbfab9e
578c1e5
80caedd
795d706
672cbf9
f3584f5
272b4c4
28d73e4
13ebd07
ae04b53
2095cfd
910c56f
b04779e
d16ef60
0a0a446
1bb3724
251efc7
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -33,7 +33,6 @@ build | |
|
||
# config files | ||
pbs.* | ||
cache.* | ||
inventory_url.yaml | ||
|
||
# autogenerated version file | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,84 @@ | ||
package exchange | ||
|
||
import ( | ||
"github.com/mxmCherry/openrtb" | ||
"github.com/prebid/prebid-server/openrtb_ext" | ||
) | ||
|
||
// auction stores the Bids for a single call to Exchange.HoldAuction(). | ||
// Construct these with the newAuction() function. | ||
type auction struct { | ||
// winningBids is a map from imp.id to the highest overall CPM bid in that imp. | ||
winningBids map[string]*openrtb.Bid | ||
// winningBidders is a map from imp.id to the BidderName which made the winning Bid. | ||
winningBidders map[string]openrtb_ext.BidderName | ||
// winningBidsFromBidder stores the highest bid on each imp by each bidder. | ||
winningBidsByBidder map[string]map[openrtb_ext.BidderName]*openrtb.Bid | ||
// cachedBids stores the cache ID for each bid, if it exists. | ||
// This is set by cacheBids() in cache.go, and is nil beforehand. | ||
cachedBids map[*openrtb.Bid]string | ||
} | ||
|
||
func newAuction(numImps int) *auction { | ||
return &auction{ | ||
winningBids: make(map[string]*openrtb.Bid, numImps), | ||
winningBidders: make(map[string]openrtb_ext.BidderName, numImps), | ||
winningBidsByBidder: make(map[string]map[openrtb_ext.BidderName]*openrtb.Bid, numImps), | ||
} | ||
} | ||
|
||
// addBid should be called for each bid which is "officially" valid for the auction. | ||
func (auction *auction) addBid(name openrtb_ext.BidderName, bid *openrtb.Bid) { | ||
if auction == nil { | ||
return | ||
} | ||
|
||
cpm := bid.Price | ||
wbid, ok := auction.winningBids[bid.ImpID] | ||
if !ok || cpm > wbid.Price { | ||
auction.winningBidders[bid.ImpID] = name | ||
auction.winningBids[bid.ImpID] = bid | ||
} | ||
if bidMap, ok := auction.winningBidsByBidder[bid.ImpID]; ok { | ||
bestSoFar, ok := bidMap[name] | ||
if !ok || cpm > bestSoFar.Price { | ||
bidMap[name] = bid | ||
} | ||
} else { | ||
auction.winningBidsByBidder[bid.ImpID] = make(map[openrtb_ext.BidderName]*openrtb.Bid) | ||
auction.winningBidsByBidder[bid.ImpID][name] = bid | ||
} | ||
} | ||
|
||
func (auction *auction) numImps() int { | ||
if auction == nil { | ||
return 0 | ||
} else { | ||
return len(auction.winningBids) | ||
} | ||
} | ||
|
||
func (auction *auction) forEachCachedBid(callback func(bid *openrtb.Bid, id string)) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You don't seem to be using forEachCacheBid() at all? Are you planning an interface definition that auction will need to implement, and if so shouldn't these be public functions? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. No plans... just sloppy of me. I'll delete it. |
||
for bid, id := range auction.cachedBids { | ||
callback(bid, id) | ||
} | ||
} | ||
|
||
func (auction *auction) cacheId(bid *openrtb.Bid) (id string, exists bool) { | ||
id, exists = auction.cachedBids[bid] | ||
return | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this returns values for id and exists? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||
} | ||
|
||
// forEachBestBid runs the callback function on every bid which is the highest one for each Bidder on each Imp. | ||
func (auction *auction) forEachBestBid(callback func(impID string, bidder openrtb_ext.BidderName, bid *openrtb.Bid, winner bool)) { | ||
if auction == nil { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. is it necessary to check this? can auction be nil if it was able to call this function? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. no, you're right, it's not necessary. I removed it. |
||
return | ||
} | ||
|
||
for impId, bidderMap := range auction.winningBidsByBidder { | ||
overallWinner, _ := auction.winningBids[impId] | ||
for bidderName, bid := range bidderMap { | ||
callback(impId, bidderName, bid, bid == overallWinner) | ||
} | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,90 @@ | ||
package exchange | ||
|
||
import ( | ||
"github.com/mxmCherry/openrtb" | ||
"github.com/prebid/prebid-server/openrtb_ext" | ||
"testing" | ||
) | ||
|
||
func TestImpCount(t *testing.T) { | ||
a := newAuction(2) | ||
a.addBid(openrtb_ext.BidderAppnexus, &openrtb.Bid{ | ||
ImpID: "imp-1", | ||
}) | ||
a.addBid(openrtb_ext.BidderRubicon, &openrtb.Bid{ | ||
ImpID: "imp-1", | ||
}) | ||
a.addBid(openrtb_ext.BidderIndex, &openrtb.Bid{ | ||
ImpID: "imp-2", | ||
}) | ||
if a.numImps() != 2 { | ||
t.Errorf("Expected 2 imps. Got %d", a.numImps()) | ||
} | ||
} | ||
|
||
func TestAuctionIntegrity(t *testing.T) { | ||
a := newAuction(2) | ||
oneImpId := "imp-1" | ||
otherImpId := "imp-2" | ||
|
||
apnWinner := &openrtb.Bid{ | ||
ImpID: oneImpId, | ||
Price: 3, | ||
} | ||
apnLoser := &openrtb.Bid{ | ||
ImpID: oneImpId, | ||
Price: 2, | ||
} | ||
apnCompetitor := &openrtb.Bid{ | ||
ImpID: otherImpId, | ||
Price: 1, | ||
} | ||
rubiWinner := &openrtb.Bid{ | ||
ImpID: otherImpId, | ||
Price: 2, | ||
} | ||
a.addBid(openrtb_ext.BidderAppnexus, apnWinner) | ||
a.addBid(openrtb_ext.BidderAppnexus, apnLoser) | ||
a.addBid(openrtb_ext.BidderRubicon, rubiWinner) | ||
a.addBid(openrtb_ext.BidderAppnexus, apnCompetitor) | ||
|
||
seenWinnerImp1 := false | ||
seenWinnerImp2 := false | ||
seenLoserImp1 := false | ||
seenLoserImp2 := false | ||
|
||
numBestBids := 0 | ||
a.forEachBestBid(func(impId string, bidderName openrtb_ext.BidderName, bid *openrtb.Bid, winner bool) { | ||
numBestBids++ | ||
|
||
if bid == apnWinner { | ||
seenWinnerImp1 = true | ||
} | ||
if bid == apnLoser { | ||
seenLoserImp1 = true | ||
} | ||
if bid == rubiWinner { | ||
seenWinnerImp2 = true | ||
} | ||
if bid == apnCompetitor { | ||
seenLoserImp2 = true | ||
} | ||
}) | ||
|
||
if !seenWinnerImp1 { | ||
t.Errorf("foreachBestBid did not execute on apn winning bid.") | ||
} | ||
if seenLoserImp1 { | ||
t.Errorf("foreachBestBid should not execute on apn backup bid.") | ||
} | ||
if !seenWinnerImp2 { | ||
t.Errorf("foreachBestBid did not execute on rubicon winning bid.") | ||
} | ||
if !seenLoserImp2 { | ||
t.Errorf("foreachBestBid did not execute on apn best-effort losing bid.") | ||
} | ||
|
||
if numBestBids != 3 { | ||
t.Errorf("expected 3 best-effort bids. Got %d", numBestBids) | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,49 @@ | ||
package exchange | ||
|
||
import ( | ||
"context" | ||
"encoding/json" | ||
"github.com/golang/glog" | ||
"github.com/mxmCherry/openrtb" | ||
"github.com/prebid/prebid-server/openrtb_ext" | ||
"github.com/prebid/prebid-server/pbs/buckets" | ||
"github.com/prebid/prebid-server/prebid_cache_client" | ||
"strings" | ||
) | ||
|
||
// cacheBids stores the given Bids in prebid cache, and saves the generated UUIDs in the auction. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This caches all the best bids from each bidder from my reading. In any case you don't feed this function a list of bids to cache. Might want to clean up the comment, especially in light of the potential that we will eventually implement multiple different bid caching strategies, and this implements one specific strategy. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. agreed, thanks. I updated the comments to match. |
||
// If any cache calls fail, then there's not much anyone can do about it. This function will just log | ||
// the error and save IDs to any bids which are cached successfully. | ||
func cacheBids(ctx context.Context, cache prebid_cache_client.Client, auction *auction, granularity openrtb_ext.PriceGranularity) { | ||
bids := make([]*openrtb.Bid, 0, 30) // Arbitrary initial capacity | ||
nextBidIndex := 0 | ||
auction.forEachBestBid(func(impID string, bidder openrtb_ext.BidderName, bid *openrtb.Bid, winner bool) { | ||
// Fixes #199 | ||
granularityStr, err := buckets.GetPriceBucketString(bid.Price, granularity) | ||
if err == nil && strings.ContainsAny(granularityStr, "123456789") { | ||
bids = append(bids, bid) | ||
nextBidIndex++ | ||
} | ||
}) | ||
|
||
// Marshal the bids into JSON payloads. If any errors occur during marshalling, eject that bid from the array. | ||
// After this block, we expect "bids" and "jsonValues" to have the same number of elements in the same order. | ||
jsonValues := make([]json.RawMessage, 0, len(bids)) | ||
for i := 0; i < len(bids); i++ { | ||
if jsonBytes, err := json.Marshal(bids[i]); err != nil { | ||
glog.Errorf("Error marshalling OpenRTB Bid for Prebid Cache: %v", err) | ||
bids = append(bids[:i], bids[i+1:]...) | ||
i-- | ||
} else { | ||
jsonValues = append(jsonValues, jsonBytes) | ||
} | ||
} | ||
|
||
ids := cache.PutJson(ctx, jsonValues) | ||
auction.cachedBids = make(map[*openrtb.Bid]string, len(bids)) | ||
for i := 0; i < len(bids); i++ { | ||
if ids[i] != "" { | ||
auction.cachedBids[bids[i]] = ids[i] | ||
} | ||
} | ||
} |
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.
Is this function supposed to have a purpose outside of the one test case?
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 makes
auction
nil-safe. The idea was to keep the "do we need to track winning bids?" logic in one place. If not, theexchange
can initialize the variable to anil
. If so, the exchange can initialize a realauction
object.Then the rest of the
exchange
logic can call these functions as if it needed to track the winning bids without much performance impact.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.
Wait I'm sorry.... no, you're right. It's not called anywhere real.
I'll delete it.