Video endpoint bid selection enhancements #1419

VeronikaSolovei9 · 2020-07-28T08:33:47Z

Video endpoint bid selection enhancement: return all winning bids from all bidders per impression instead of one overall winning bid

jmaynardxandr · 2020-07-28T20:00:41Z

exchange/auction_test.go

 		t.Errorf("%s:  [CACHE_ERROR] More elements were cached than expected \n", fileDisplayName)
-	} else { // len(specData.ExpectedCacheables) == len(cache.items)
+	} else if len(specData.ExpectedCacheables) == len(cache.items) && isVideo {


I think you can just leave this as an 'else' here (leaving the condition commented out like it was before). In the event that isVideo and more bids than expected were cached it looks to me like the below code will still work (which just checks that all of the expected cachables were indeed cached, and that should still be the case).

Right, fixed.

hhhjort · 2020-07-29T13:49:12Z

exchange/targeting.go

 	for impId, topBidsPerImp := range auc.winningBidsByBidder {
 		overallWinner := auc.winningBids[impId]
 		for bidderName, topBidPerBidder := range topBidsPerImp {
-			isOverallWinner := overallWinner == topBidPerBidder
+			isOverallWinner := overallWinner == topBidPerBidder || isVideo


Won't setting all bids as winning bids cause key collisions between bids from different bidders? Why is setting includeBidderKeys to true not a solution to the problem?

@hhhjort Hi Hans, just tested it with includeBidderKeys = true. In this case every parameter in targeting data has _biddername at the end, for example: hb_pb_appnexus -> "12.00", hb_uuid_appnexus -> "d81d1315-6060-4c42-bfa7-430f26f8d7d8" instead of hb_pb and hb_uuid. With this keys we filter out such bids in video endpoint as "loosing" bids, because they don't have hb_uuid key:
if tempRespBidExt.Prebid.Targeting[string(openrtb_ext.HbVastCacheKey)] == "" { continue }
Do you mean collisions in hb_pb_cat_dur keys? We run applyCategoryMapping function and deduplicate bids before auction and caching logic.

Ahh, yes, the keys are segregated by bids initially, which keeps them separate. Still it seems inelegant to push the isVideo flag down from the top in order to change the targeting key generation behavior. We should probably consider putting the flag somewhere in the request itself, the reason for the original behavior (only counting the "winning" bids) was that as the number of bidders increased, the demands on cache increased. Our initial estimates were that the cost of caching so many video bids would render the prebid video service uneconomical. So if we have a flag inside the request, we can have it on for requests that only invoke 2-3 bidders, and then turn it off for the larger requests.

Also, filtering out the "loosing" bids is exactly what this PR is trying to turn off, but by adding a workaround rather than removing the filtering. So that seems another count against this approach.

I am also not really happy with this flag. We need to distinguish video and non-video requests in exchange somehow and we deal with openRtb Bid request only, so we cannot add any new flags in it.
We can add this flag in targeting data only, do you want me to process with this?
Cache usage will increase in this case, true. For bidders limiting - do we want to have max number of bidders in config?
In case we have bids back from 5 bidders. Should we select 3 bids per imp with better prices and filter out other 2 bids?

Update. Me and @jmaynardxandr just had a discussion on this and here are some points we came up with.

Modify "applyCategoryMapping", deduplication part of it:
-sort bids by pod
-for the same pod deduplicate bids by category only, not by hb_pb_cat_dur
-instead of removing random bid, find bid with better price (or price per second)

Implement "altering auction" where we will return only 3 (max bids per imp, value from config) bids per imp and they all will be winning. In case we have 5 bids back per impression - we will return best 3 of them (by price only or by price per second).
If we have new "alteringAuction" function, we can rely on labels and don't create any new ugly variables:
if labels.RType == pbsmetrics.ReqTypeVideo{ alteringAuction }else{ auc = newAuction(adapterBids, len(bidRequest.Imp)) }
@hhhjort does this make sense to you?

I think we want to configure the number of bids we want to cache and return, likely in the PBS config rather than the request. We don't know what the proper balance will be between number of bids and cost ... each additional bid gives us a diminishing return, and at some point the cost in caching will outweigh the benefit gained from the extra bid. In addition, this balance point could easily be different for different PBS hosts.

Perhaps a config for the maximum amount of bidding/caching the host wants to do, and an in request parameter from the publisher for the level they would like to see. That would allow for publishers to tune the requests per the performance they see, but not let them dial it up past the level the host is willing to support.

@hhhjort Agreed, Eventually we would like to find the right balance between bid caching and the value of choosing it, however in the phase where demand and inventory are just ramping up, this limit would act as a negative. Later, we can make a decision based on data (win rates based on various factors such as category and creative duration, etc) to figure out what the number would look. For now, the idea is to limit 1 winning bid per imp for each bidder/adapter. Ideally we need this only for long form video. Other inventory types may not need this. Please suggest, if and how we can apply this to only long-form video.

The straightforward way would be to have the video endpoint request includebidderkeys in the request it passes to the openrtb code. Then it can read out the bidder specific versions of the targeting keys (such as hb_pb_<bidder> in place of hb_pb). Another option is to not have the openrtb susbsystem generate the keys, but instead have the video endpoint read the bids and execute its own code to extract the key values. This might be best if you suspect that video needs will diverge significantly from what the openrtb endpoint does.

VeronikaSolovei9 · 2020-08-10T08:01:59Z

@hhhjort Ready for review

hhhjort · 2020-08-10T17:50:23Z

endpoints/openrtb2/video_auction.go

@@ -626,6 +634,7 @@ func createBidExtension(videoRequest *openrtb_ext.BidRequestVideo) ([]byte, erro
 		IncludeWinners:       true,


You may not need IncludeWinners now.

hhhjort · 2020-08-10T17:54:12Z

exchange/targeting.go

@@ -7,7 +7,7 @@ import (
 	"github.com/prebid/prebid-server/openrtb_ext"
 )

-const maxKeyLength = 20
+const MaxKeyLength = 20


If you want, you can make this a config option rather than being hardcoded. There has been some talk about making this configurable, but no one has gotten around to it yet. Not required in this PR.

@hhhjort I agree this should be a config option. Can we leave it for later, because this PR has first priority now?

We already have a story for this internally. Not required for this PR.

hhhjort · 2020-08-10T17:56:05Z

Two comments on things you may change, but otherwise looking very good now. Waiting on your feedback on the two points before I approve.

… tests modified accordingly

VeronikaSolovei9 · 2020-08-11T00:27:14Z

Thank you for your comments, @hhhjort!
Removed 'includeWinners:true' from default video request config, unit tests modified accordingly.
thank you for this suggestion, code looks cleaner with this approach.
The only thing we need to have in unit tests to test old functionality works the same as before without this option:

File: multiImpVideoNoIncludeBidderKeys.json has
"targetDataIncludeWinners": true,
"targetDataIncludeBidderKeys": false

otherwise doCache test fails due to nil cached data because of this check:
if !((bids || vast) && (includeBidderKeys || includeWinners)) { return nil }

About moving MaxKeyLength to config - can we have it in a separate PR?

hhhjort

Making the cache key length configurable can certainly wait.

SyntaxNode · 2020-08-12T16:44:52Z

exchange/targeting.go

@@ -7,7 +7,7 @@ import (
 	"github.com/prebid/prebid-server/openrtb_ext"
 )

-const maxKeyLength = 20
+const MaxKeyLength = 20


We already have a story for this internally. Not required for this PR.

…ter-video * origin/master: (26 commits) Fix bid dedup (prebid#1456) moving docs to website repo (prebid#1443) Fixing comment for usage of deal priority field (prebid#1451) Fix minor error message spelling mistake "vastml" -> "vastxml" (prebid#1455) Adform adapter: additional targeting params added (prebid#1424) Added adpod_id to request extension (prebid#1444) Fixes bug (prebid#1448) Validate External Cache Host (prebid#1422) Enable geo activation of GDPR flag (prebid#1427) Update the fallback GVL to last version (prebid#1440) Fix no bid debug log (prebid#1375) Eplanning adapter: Get domain from page (prebid#1434) Fix TCF1 Fetcher Fallback (prebid#1438) Refactor rate converter separating scheduler from converter logic to improve testability (prebid#1394) [WIP] Bid deduplication enhancement (prebid#1430) Video endpoint bid selection enhancements (prebid#1419) update to the latest go-gdpr release (prebid#1436) Default TCF1 GVL in anticipation of IAB no longer hosting the v1 GVL (prebid#1433) Tcf2 id support (prebid#1420) Remove redundad struct (prebid#1432) ...

jmaynardxandr reviewed Jul 28, 2020

View reviewed changes

SyntaxNode requested review from SyntaxNode and hhhjort July 29, 2020 04:48

SyntaxNode assigned SyntaxNode and hhhjort Jul 29, 2020

hhhjort reviewed Jul 29, 2020

View reviewed changes

VeronikaSolovei9 changed the title ~~Video endpoint bid selection enhancemen~~ [WIP]Video endpoint bid selection enhancemen Aug 4, 2020

VeronikaSolovei9 force-pushed the video_bids_selection_enhancement branch from 35ddd9c to c89fc29 Compare August 7, 2020 20:56

VeronikaSolovei9 closed this Aug 7, 2020

VeronikaSolovei9 force-pushed the video_bids_selection_enhancement branch from c89fc29 to f1582a4 Compare August 7, 2020 20:56

Veronika Solovei added 3 commits August 7, 2020 14:01

Code refactoring, unit tests need to be fixed

5b9f471

Code refactoring, unit tests need to be fixed

5dcd325

Code refactoring

3151fcc

VeronikaSolovei9 changed the title ~~[WIP]Video endpoint bid selection enhancemen~~ [WIP]Video endpoint bid selection enhancements Aug 7, 2020

VeronikaSolovei9 reopened this Aug 7, 2020

Unit tests

1b50c04

hhhjort reviewed Aug 10, 2020

View reviewed changes

Removed 'includeWinners:true' from default video request config, unit…

0910ca1

… tests modified accordingly

VeronikaSolovei9 changed the title ~~[WIP]Video endpoint bid selection enhancements~~ Video endpoint bid selection enhancements Aug 11, 2020

hhhjort approved these changes Aug 11, 2020

View reviewed changes

SyntaxNode approved these changes Aug 12, 2020

View reviewed changes

SyntaxNode merged commit 48c865c into prebid:master Aug 12, 2020

VeronikaSolovei9 deleted the video_bids_selection_enhancement branch March 22, 2021 05:45

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Video endpoint bid selection enhancements #1419

Video endpoint bid selection enhancements #1419

VeronikaSolovei9 commented Jul 28, 2020 •

edited

Loading

jmaynardxandr Jul 28, 2020

VeronikaSolovei9 Jul 29, 2020

hhhjort Jul 29, 2020

VeronikaSolovei9 Jul 29, 2020 •

edited

Loading

hhhjort Jul 29, 2020

hhhjort Jul 29, 2020

VeronikaSolovei9 Jul 29, 2020

VeronikaSolovei9 Jul 29, 2020 •

edited

Loading

hhhjort Jul 30, 2020

rshivapurkar Aug 5, 2020 •

edited

Loading

hhhjort Aug 6, 2020

VeronikaSolovei9 commented Aug 10, 2020

hhhjort Aug 10, 2020

VeronikaSolovei9 Aug 11, 2020

hhhjort Aug 10, 2020

VeronikaSolovei9 Aug 11, 2020

SyntaxNode Aug 12, 2020

hhhjort commented Aug 10, 2020

VeronikaSolovei9 commented Aug 11, 2020 •

edited

Loading

hhhjort left a comment

SyntaxNode Aug 12, 2020

		@@ -626,6 +634,7 @@ func createBidExtension(videoRequest *openrtb_ext.BidRequestVideo) ([]byte, erro
		IncludeWinners: true,

Video endpoint bid selection enhancements #1419

Video endpoint bid selection enhancements #1419

Conversation

VeronikaSolovei9 commented Jul 28, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

VeronikaSolovei9 Jul 29, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

VeronikaSolovei9 Jul 29, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rshivapurkar Aug 5, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

VeronikaSolovei9 commented Aug 10, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hhhjort commented Aug 10, 2020

VeronikaSolovei9 commented Aug 11, 2020 • edited Loading

hhhjort left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

VeronikaSolovei9 commented Jul 28, 2020 •

edited

Loading

VeronikaSolovei9 Jul 29, 2020 •

edited

Loading

VeronikaSolovei9 Jul 29, 2020 •

edited

Loading

rshivapurkar Aug 5, 2020 •

edited

Loading

VeronikaSolovei9 commented Aug 11, 2020 •

edited

Loading