RP adapter: use video placement parameter to set size ID #1607
Conversation
adapters/rubicon/rubicon_test.go
Outdated
| @@ -524,6 +530,36 @@ func TestAppendTracker(t *testing.T) { | |||
| } | |||
| } | |||
|
|
|||
| func TestResolveVideoSizeId(t *testing.T) { | |||
| testScenarios := []rubiResolveVideoSizeIdTestScenario{ | |||
There was a problem hiding this comment.
You can define the test struct right here as:
testScenarios := []struct {
placement openrtb.VideoPlacementType
instl int8
expected int
} {
{
placement: 1,
instl: 1,
expected: 201,
},
{
placement: 3,
instl: 1,
expected: 203,
},
}
adapters/rubicon/rubicon.go
Outdated
| func resolveVideoSizeId(placement openrtb.VideoPlacementType, instl int8) (sizeID int) { | ||
| if placement != 0 { | ||
| if placement == 1 { | ||
| return 201 |
There was a problem hiding this comment.
Please add JSON test cases to cover this part of the code as well where the SizeID is determined based on imp.Video.Placement
adapters/rubicon/rubicon.go
Outdated
| if instl == 1 { | ||
| return 202 | ||
| } | ||
| return 0 |
There was a problem hiding this comment.
In case an Imp element comes in still written in the "the old" way without instl and a placement and with a valid rubiconExt.Video.VideoSizeID, do we want to use the latter? In other words, if resolveVideoSizeId returns zero, do we want to try rubiconExt.Video.VideoSizeID, set the size to zero, or return an error?
adapters/rubicon/rubicon.go
Outdated
| @@ -785,7 +802,8 @@ func (a *RubiconAdapter) MakeRequests(request *openrtb.BidRequest, reqInfo *adap | |||
| } | |||
|
|
|||
| videoCopy := *thisImp.Video | |||
| videoExt := rubiconVideoExt{Skip: rubiconExt.Video.Skip, SkipDelay: rubiconExt.Video.SkipDelay, VideoType: videoType, RP: rubiconVideoExtRP{SizeID: rubiconExt.Video.VideoSizeID}} | |||
| sizeId := resolveVideoSizeId(thisImp.Video.Placement, thisImp.Instl) | |||
| videoExt := rubiconVideoExt{Skip: rubiconExt.Video.Skip, SkipDelay: rubiconExt.Video.SkipDelay, VideoType: videoType, RP: rubiconVideoExtRP{SizeID: sizeId}} | |||
There was a problem hiding this comment.
Given that the rubiconExt.Video.VideoSizeID field will no longer be used here nor elsewhere, should we remove the check on lines 791 to 796 above?
790 if isVideo {
791 - if rubiconExt.Video.VideoSizeID == 0 {
792 - errs = append(errs, &errortypes.BadInput{
793 - Message: fmt.Sprintf("imp[%d].ext.bidder.rubicon.video.size_id must be defined for video impression", i),
794 - })
795 - continue
796 - }
797
798 // if imp.ext.is_rewarded_inventory = 1, set imp.video.ext.videotype = "rewarded"
799 var videoType = ""
800 if bidderExt.Prebid != nil && bidderExt.Prebid.IsRewardedInventory == 1 {
801 videoType = "rewarded"
802 }
803
804 videoCopy := *thisImp.Video
805 sizeId := resolveVideoSizeId(thisImp.Video.Placement, thisImp.Instl)
806 videoExt := rubiconVideoExt{Skip: rubiconExt.Video.Skip, SkipDelay: rubiconExt.Video.SkipDelay, VideoType: videoType, RP: rubiconVideoExtRP{SizeID: sizeId}}
807 videoCopy.Ext, err = json.Marshal(&videoExt)
adapters/rubicon/rubicon.go
If future imps come with an empty VideSizeID (JSON field size_id) field, we might risk wrongfully discarding impressions.
| if videoSizeId == 0 { | ||
| resolvedSizeId, err := resolveVideoSizeId(thisImp.Video.Placement, thisImp.Instl, thisImp.ID) | ||
| if err == nil { | ||
| videoSizeId = resolvedSizeId |
There was a problem hiding this comment.
Just curious, why is the error behavior for the legacy auction endpoint different from that of the openrtb endpoint i.e. in this case, even if you get an error while resolving the video size ID, you just let it be 0 and move on with processing as usual but in case of openrtb, you actually discard that imp
There was a problem hiding this comment.
if rubiconExt.Video.VideoSizeID == 0 { errs = append(errs, &errortypes.BadInput{ Message: fmt.Sprintf("imp[%d].ext.bidder.rubicon.video.size_id must be defined for video impression", i), }) continue
this was previous logic in non legacy, haven't found similar in legacy
There was a problem hiding this comment.
I see but if your bidder expects a non-zero sizeID and will return an error for requests with an empty/invalid video sizeID then I'd suggest returning an error here and failing fast as you do for non-legacy rather than sending the request to the bidder and the bidder then returning an error back. That will be inefficient use of resources.
Also, I'd assume the behavior to be the same as both in legacy and non-legacy endpoints unless your system has a way to differentiate between the two.
There was a problem hiding this comment.
Ooops! Looks like I missed your update to continue in an error situation for the legacy endpoint as well. Thanks!
| resolvedSizeId, err := resolveVideoSizeId(thisImp.Video.Placement, thisImp.Instl, thisImp.ID) | ||
| if err == nil { | ||
| videoSizeId = resolvedSizeId | ||
| } |
There was a problem hiding this comment.
In line 496, if the error turns out to not be nil, do we want to continue as we do in the check in line 505 below?
491 if thisImp.Video != nil {
492
493 videoSizeId := params.Video.VideoSizeID
494 if videoSizeId == 0 {
495 resolvedSizeId, err := resolveVideoSizeId(thisImp.Video.Placement, thisImp.Instl, thisImp.ID)
496 if err == nil {
497 videoSizeId = resolvedSizeId
498 - }
+ } else { // err != nil
+ continue
+ }
499 }
500
501 videoExt := rubiconVideoExt{Skip: params.Video.Skip, SkipDelay: params.Video.SkipDelay, RP: rubiconVideoExtRP{SizeID: videoSizeId}}
502 thisImp.Video.Ext, err = json.Marshal(&videoExt)
503 } else {
504 primarySizeID, altSizeIDs, err := parseRubiconSizes(unit.Sizes)
505 if err != nil {
506 continue
507 }
adapters/rubicon/rubicon.go
No description provided.