Targeting: Add alwaysincludedeals flag

[AlexBVolcy]

Addresses this issue #2214

The goal is to add a new flag on the request at ext.prebid.targeting.alwaysincludedeals, that if true: PBS-core would generate targeting key-value pairs for all bids with a dealid. This is independent of which other ext.prebid.targeting flags are present.

[guscarreon]

As written, targData.setTargeting() call in line 461 of exchange/exchange.go might not get all the bids with a bid.Bid.DealID if targData.preferDeals is set to false.

Notice the third parameter of newAuction in line 437 below:

433         if targData != nil {
434             multiBidMap := buildMultiBidMap(requestExtPrebid)
435
436             // A non-nil auction is only needed if targeting is active. (It is used below this block to extract cache keys)
437             auc = newAuction(adapterBids, len(r.BidRequestWrapper.Imp), targData.preferDeals)
438             auc.validateAndUpdateMultiBid(adapterBids, targData.preferDeals, r.Account.DefaultBidLimit)
439             auc.setRoundedPrices(*targData)
440
441 *-- 19 lines: if requestExtPrebid.SupportDeals {--------------------------------------------------------
460
461             targData.setTargeting(auc, r.BidRequestWrapper.BidRequest.App != nil, bidCategory, r.Account.TruncateTargetAttribute, multiBidMap)
462         }
exchange/exchange.go

If targData.preferDeals, is set to true, bids with a len(bid.Bid.DealID) > 0 will be added to auc.winningBidsByBidder but, conversely, when targData.preferDeals is false, even if a bid comes a with a populated bid.Bid.DealID field, it won't be added to auc.winningBidsByBidder as depicted inside isNewWinningBid()'s if statement in line 140 below:

138 // isNewWinningBid calculates if the new bid (nbid) will win against the current winning bid (wbid) given preferDeals.
139 func isNewWinningBid(bid, wbid *openrtb2.Bid, preferDeals bool) bool {
140     if preferDeals {  // <-- A BID WITH len(bid.DealID) GREATER THAN ZERO, IS ONLY ADDED IF targData.preferDeals IS TRUE
141         if len(wbid.DealID) > 0 && len(bid.DealID) == 0 {
142             return false
143         }
144         if len(wbid.DealID) == 0 && len(bid.DealID) > 0 {
145             return true
146         }
147     }
148     return bid.Price > wbid.Price
149 }
exchange/auction.go

According to the PR specs above: "..flag ext.prebid.targeting.alwaysincludedeals if true, should generate targeting key-value pairs for all bids with a DealId, independent of which other ext.prebid.targeting flags are present". That should include, the targData.preferDeals flag, right? In other words, when ext.prebid.targeting.alwaysincludedeals is true, any bid with a non-empty DealID field should end up inside the targData.setTargeting() call regardless of the value of targData.preferDeals. Right? Or am I misunderstanding the specs?

[AlexBVolcy]

So, after looking into this more, newAuction() which calls isNewWinningBid, does two things, it creates a winningBids map, which contains all of the winning bids per impID, and then a winningBidsByBidder map, which maps an impID to another map that maps the bidderName to the bid, and you would think the bids present would only be the winning bids based off the name.

However, this naming of this map is misleading, as it actually contains all bids that were being evaluated, not just the winning ones. And it's this map that's being looped over within setTargeting(), so that means all bids with dealIDs are still present to be evaluated, and alwaysincludedeals can be applied correctly.

Here are lines 121-127 of newAuction()

if bidMap, ok := winningBidsByBidder[bid.Bid.ImpID]; ok {
	bidMap[bidderName] = append(bidMap[bidderName], bid)
} else {
	winningBidsByBidder[bid.Bid.ImpID] = map[openrtb_ext.BidderName][]*entities.PbsOrtbBid{
		bidderName: {bid},
	}
}

These lines are where winningsBidsByBidder are actually populated. If an impID exists in the map, grab the associated value (which is another map), and update it so that it maps the bidderName to the current bid. And the important point is this bid is not necessarily the winning bid, as the bid was grabbed from all the bids returned from getAllBids().

So in conclusion, a more appropriate name from winningsBidsByBidder would be allBidsByBidder, and the concern about preferDeals removing bids with dealIDs, and thus contradicting alwaysincludedeals purpose isn't an issue since it's really the allBidsByBidder map that's being considered within setTargeting().

[guscarreon]

LGTM