Skip to content
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

Adds keyvalue hb_format support #1414

Merged
merged 4 commits into from
Jul 30, 2020
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions docs/endpoints/openrtb2/auction.md
Original file line number Diff line number Diff line change
Expand Up @@ -173,6 +173,7 @@ to set these params on the response at `response.seatbid[i].bid[j].ext.prebid.ta
},
"includewinners": false, // Optional param defaulting to true
"includebidderkeys": false // Optional param defaulting to true
"includeformat": false //Optional param defaulting to false
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nitpick: Please add a space before the comment for consistency.

}
}
}
Expand All @@ -184,6 +185,8 @@ For backwards compatibility the following strings will also be allowed as price

One of "includewinners" or "includebidderkeys" must be true (both default to true if unset). If both were false, then no targeting keys would be set, which is better configured by omitting targeting altogether.

The parameter "includeformat" is for multiformat support. It will add the key `hb_format` and/or `hb_format_{bidderName}` as per "includewinners" and "includebidderkeys" above.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What do you think about modifying this a little bit to say:

The parameter "includeformat" indicates the type of the bid (banner, video, etc) for multiformat requests. It will add the key ....


MediaType PriceGranularity (PBS-Java only) - when a single OpenRTB request contains multiple impressions with different mediatypes, or a single impression supports multiple formats, the different mediatypes may need different price granularities. If `mediatypepricegranularity` is present, `pricegranularity` would only be used for any mediatypes not specified.

```
Expand Down
1 change: 1 addition & 0 deletions exchange/exchange.go
Original file line number Diff line number Diff line change
Expand Up @@ -135,6 +135,7 @@ func (e *exchange) HoldAuction(ctx context.Context, bidRequest *openrtb.BidReque
includeBidderKeys: requestExt.Prebid.Targeting.IncludeBidderKeys,
includeCacheBids: shouldCacheBids,
includeCacheVast: shouldCacheVAST,
includeFormat: requestExt.Prebid.Targeting.IncludeFormat,
}
targData.cacheHost, targData.cachePath = e.cache.GetExtCacheData()
}
Expand Down
4 changes: 4 additions & 0 deletions exchange/targeting.go
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@ type targetData struct {
includeBidderKeys bool
includeCacheBids bool
includeCacheVast bool
includeFormat bool
// cacheHost and cachePath exist to supply cache host and path as targeting parameters
cacheHost string
cachePath string
Expand Down Expand Up @@ -53,6 +54,9 @@ func (targData *targetData) setTargeting(auc *auction, isApp bool, categoryMappi
if vastID, ok := auc.vastCacheIds[topBidPerBidder.bid]; ok {
targData.addKeys(targets, openrtb_ext.HbVastCacheKey, vastID, bidderName, isOverallWinner)
}
if targData.includeFormat {
targData.addKeys(targets, openrtb_ext.HbFormatKey, string(topBidPerBidder.bidType), bidderName, isOverallWinner)
}

if targData.cacheHost != "" {
targData.addKeys(targets, openrtb_ext.HbConstantCacheHostKey, targData.cacheHost, bidderName, isOverallWinner)
Expand Down
163 changes: 163 additions & 0 deletions exchange/targeting_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -247,3 +247,166 @@ func (f *mockFetcher) GetId(bidder openrtb_ext.BidderName) (string, bool) {
func mockServer(w http.ResponseWriter, req *http.Request) {
w.Write([]byte("{}"))
}

type TargetingTestData struct {
Description string
TargetData targetData
Auction auction
IsApp bool
CategoryMapping map[string]string
ExpectedBidTargetsByBidder map[string]map[openrtb_ext.BidderName]map[string]string
}

func TestSetTargeting(t *testing.T) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please put test fixture after the test cases.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What do you mean by "test fixture"?

for _, test := range TargetingTests {
auc := &test.Auction
// Set rounded prices from the auction data
auc.setRoundedPrices(test.TargetData.priceGranularity)
winningBids := make(map[string]*pbsOrtbBid)
// Set winning bids from the auction data
for imp, bidsByBidder := range auc.winningBidsByBidder {
for _, bid := range bidsByBidder {
if winningBid, ok := winningBids[imp]; ok {
if winningBid.bid.Price < bid.bid.Price {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do we need to filter by price in a unit test?

winningBids[imp] = bid
}
} else {
winningBids[imp] = bid
}
}
}
auc.winningBids = winningBids
targData := test.TargetData
targData.setTargeting(auc, test.IsApp, test.CategoryMapping)
for imp, targetsByBidder := range test.ExpectedBidTargetsByBidder {
for bidder, expected := range targetsByBidder {
assert.Equal(t,
expected,
auc.winningBidsByBidder[imp][bidder].bidTargets,
"Test: %s\nTargeting failed for bidder %s on imp %s.",
test.Description,
string(bidder),
imp)
}
}
}

}

var TargetingTests []TargetingTestData = []TargetingTestData{
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some of these tests aren't related to hb_format. Did we have testing gaps before that you're addressing?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe so. We had no testing on the setTargeting() level. I believe the higher level tests would end up covering some cases, but they are rather far removed, so would be a lot of work to confirm which cases are covered at that level. As you can see, setTargeting() requires a lot of data to be set up, which is probably why it was not done before. But now with this framework in place we can easily expand it next time we touch targeting or find a specific hole in our testing.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks a lot @hhhjort for adding these tests. They are definitely very helpful. Could we maybe also add some test cases around the other targeting keys like the cache ones and the deal ones? I understand that this PR doesn't make any code changes for those but since we have this framework in place now, I am hoping that it wouldn't be too much of an effort to add that. I will leave this up to you though.

{
Description: "Targeting winners only (most basic targeting example)",
TargetData: targetData{
priceGranularity: openrtb_ext.PriceGranularityFromString("med"),
includeWinners: true,
},
Auction: auction{
winningBidsByBidder: map[string]map[openrtb_ext.BidderName]*pbsOrtbBid{
"ImpId-1": {
openrtb_ext.BidderAppnexus: {
bid: &openrtb.Bid{
Price: 1.23,
},
bidType: openrtb_ext.BidTypeBanner,
},
openrtb_ext.BidderRubicon: {
bid: &openrtb.Bid{
Price: 0.84,
},
bidType: openrtb_ext.BidTypeBanner,
},
},
},
},
ExpectedBidTargetsByBidder: map[string]map[openrtb_ext.BidderName]map[string]string{
"ImpId-1": {
openrtb_ext.BidderAppnexus: {
"hb_bidder": "appnexus",
"hb_pb": "1.20",
},
openrtb_ext.BidderRubicon: {},
},
},
},
{
Description: "Targeting on bidders only",
TargetData: targetData{
priceGranularity: openrtb_ext.PriceGranularityFromString("med"),
includeBidderKeys: true,
},
Auction: auction{
winningBidsByBidder: map[string]map[openrtb_ext.BidderName]*pbsOrtbBid{
"ImpId-1": {
openrtb_ext.BidderAppnexus: {
bid: &openrtb.Bid{
Price: 1.23,
},
bidType: openrtb_ext.BidTypeBanner,
},
openrtb_ext.BidderRubicon: {
bid: &openrtb.Bid{
Price: 0.84,
},
bidType: openrtb_ext.BidTypeBanner,
},
},
},
},
ExpectedBidTargetsByBidder: map[string]map[openrtb_ext.BidderName]map[string]string{
"ImpId-1": {
openrtb_ext.BidderAppnexus: {
"hb_bidder_appnexus": "appnexus",
"hb_pb_appnexus": "1.20",
},
openrtb_ext.BidderRubicon: {
"hb_bidder_rubicon": "rubicon",
"hb_pb_rubicon": "0.80",
},
},
},
},
{
Description: "Full basic targeting with hd_format",
TargetData: targetData{
priceGranularity: openrtb_ext.PriceGranularityFromString("med"),
includeWinners: true,
includeBidderKeys: true,
includeFormat: true,
},
Auction: auction{
winningBidsByBidder: map[string]map[openrtb_ext.BidderName]*pbsOrtbBid{
"ImpId-1": {
openrtb_ext.BidderAppnexus: {
bid: &openrtb.Bid{
Price: 1.23,
},
bidType: openrtb_ext.BidTypeBanner,
},
openrtb_ext.BidderRubicon: {
bid: &openrtb.Bid{
Price: 0.84,
},
bidType: openrtb_ext.BidTypeBanner,
},
},
},
},
ExpectedBidTargetsByBidder: map[string]map[openrtb_ext.BidderName]map[string]string{
"ImpId-1": {
openrtb_ext.BidderAppnexus: {
"hb_bidder": "appnexus",
"hb_bidder_appnexus": "appnexus",
"hb_pb": "1.20",
"hb_pb_appnexus": "1.20",
"hb_format": "banner",
"hb_format_appnexus": "banner",
},
openrtb_ext.BidderRubicon: {
"hb_bidder_rubicon": "rubicon",
"hb_pb_rubicon": "0.80",
"hb_format_rubicon": "banner",
},
},
},
},
}
3 changes: 3 additions & 0 deletions openrtb_ext/bid.go
Original file line number Diff line number Diff line change
Expand Up @@ -99,6 +99,9 @@ const (
HbSizeConstantKey TargetingKey = "hb_size"
HbDealIDConstantKey TargetingKey = "hb_deal"

// HbFormatKey is the format of the bid. For example, "video", "banner"
HbFormatKey TargetingKey = "hb_format"

// HbCacheKey and HbVastCacheKey store UUIDs which can be used to fetch things from prebid cache.
// Callers should *never* assume that either of these exist, since the call to the cache may always fail.
//
Expand Down
1 change: 1 addition & 0 deletions openrtb_ext/request.go
Original file line number Diff line number Diff line change
Expand Up @@ -85,6 +85,7 @@ type ExtRequestTargeting struct {
IncludeWinners bool `json:"includewinners"`
IncludeBidderKeys bool `json:"includebidderkeys"`
IncludeBrandCategory *ExtIncludeBrandCategory `json:"includebrandcategory"`
IncludeFormat bool `json:"includeformat"`
DurationRangeSec []int `json:"durationrangesec"`
}

Expand Down