Vrtcal: Added Video(VAST) Support #1965

vrtcal-dev · 2021-08-17T13:42:34Z

adapters/vrtcal/vrtcal.go

mansinahar · 2021-08-17T14:45:24Z

Can you please put a doc PR to include the new supported video type: https://github.com/prebid/prebid.github.io/blob/master/dev-docs/bidders/vrtcal.md ?

vrtcal-dev · 2021-08-17T16:05:10Z

-----Mansinahar: This is actually considered as an anti-pattern. Consider a request with multiple formats, that is video and banner both set. Is your bidder service confirmed to always return a banner bid? If not necessarily then you might be assigning the wrong bid type here and that will cause the ad to not be properly rendered. Does your bidder service perhaps communicate the bid type in the bid extension? If so, we highly recommend you to use that to set the bid type.--------------

Thanks for the feedback mansinahar. Our backend RTB platform only accepts single impression requests and responds with a single impression response 100% of the time so there is no possible ambiguity of bid type returned. So, this won't be an issue with our platform. Also, I've updated https://github.com/prebid/prebid.github.io/blob/master/dev-docs/bidders/vrtcal.md to include video in a pull request. Thanks again.

mansinahar · 2021-08-17T16:20:55Z

@vrtcal-dev Thanks for that clarification. However, a single impression could have both video and banner set, making it a multi-format request. This is different from multi-impression requests. Also, just looking at your adapter, code I don't see the requests to your backend being broken down for each impression which means if a publisher sends Prebid Server a request with multiple impressions, that gets forwarded as is to your backend.. Would your backend consider this as an invalid request and drop it or just bid for the first impression in the list and ignore others? In either case, if your backend only accepts single impression requests, you might wanna add logic in your Prebid Server adapter code to make multiple requests to your backend server for every impression in a request or else you might be losing on publisher inventory. You can do this as a separate PR if you want to but I just wanted to bring this to your attention.

vrtcal-dev · 2021-08-17T16:26:51Z

@vrtcal-dev Thanks for that clarification. However, a single impression could have both video and banner set, making it a multi-format request. This is different from multi-impression requests. Also, just looking at your adapter, code I don't see the requests to your backend being broken down for each impression which means if a publisher sends Prebid Server a request with multiple impressions, that gets forwarded as is to your backend.. Would your backend consider this as an invalid request and drop it or just bid for the first impression in the list and ignore others? In either case, if your backend only accepts single impression requests, you might wanna add logic in your Prebid Server adapter code to make multiple requests to your backend server for every impression in a request or else you might be losing on publisher inventory. You can do this as a separate PR if you want to but I just wanted to bring this to your attention.

Yes, our platform will only bid on the first impressions as we've always put this controlled spec on our backend servers...Yes possibly in the future we will support multiple impressions in a single request, but this is not in our plans at the moment. If, we do decide on such an enhancement later, I'll definitely add in another PR...Thanks again...

guscarreon

.

Wrong pr

mansinahar · 2021-08-18T14:54:48Z

@vrtcal-dev Sounds good! Just want to bring this to your attention that Prebid Server has the capability to make multiple requests to a bidder for multiple imps in a single incoming request. So say, if a request to Prebid Server comes in with multiple imps, your adapter's MakeRequests function can split that request into multiple requests to your backend service. This way you won't lose out on publisher inventory. You can refer to the Impression Splitting example here: https://docs.prebid.org/prebid-server/developers/add-new-bidder-go.html#makerequests if you want to add that support in the future.

vrtcal-dev · 2021-08-18T14:56:35Z

@vrtcal-dev Sounds good! Just want to bring this to your attention that Prebid Server has the capability to make multiple requests to a bidder for multiple imps in a single incoming request. So say, if a request to Prebid Server comes in with multiple imps, your adapter's MakeRequests function can split that request into multiple requests to your backend service. This way you won't lose out on publisher inventory. You can refer to the Impression Splitting example here: https://docs.prebid.org/prebid-server/developers/add-new-bidder-go.html#makerequests if you want to add that support in the future.

Thank you. I've noted for future consideration.

mansinahar

Looks pretty straightforward to me. Just have one comment to add more test coverage

mansinahar · 2021-08-18T14:59:15Z

adapters/vrtcal/vrtcal.go

+	}
+
+	//Failsafe default in case impression ID is not found
+	return "", &errortypes.BadServerResponse{


Can you please add test coverage for this case here?

Can you please add test coverage for this case here?

I've added supplemental test coverage for the failsafe unmatched impression ID. Thanks.

hhhjort · 2021-08-18T18:29:56Z

@vrtcal-dev Thanks for that clarification. However, a single impression could have both video and banner set, making it a multi-format request. This is different from multi-impression requests. Also, just looking at your adapter, code I don't see the requests to your backend being broken down for each impression which means if a publisher sends Prebid Server a request with multiple impressions, that gets forwarded as is to your backend.. Would your backend consider this as an invalid request and drop it or just bid for the first impression in the list and ignore others? In either case, if your backend only accepts single impression requests, you might wanna add logic in your Prebid Server adapter code to make multiple requests to your backend server for every impression in a request or else you might be losing on publisher inventory. You can do this as a separate PR if you want to but I just wanted to bring this to your attention.

Yes, our platform will only bid on the first impressions as we've always put this controlled spec on our backend servers...Yes possibly in the future we will support multiple impressions in a single request, but this is not in our plans at the moment. If, we do decide on such an enhancement later, I'll definitely add in another PR...Thanks again...

This does not address the comment about multiformat impressions. If you receive a request for a single imp, with both the banner and video objects filled in, this is a request that allows for both banner and video bids. According to your logic in choosing the bid type, you will always bid with a banner bid even if both banners and videos are valid. Some other adapters specify they will always choose video as video typically involves higher CPMs. We just want to confirm that, given a choice between the two, your bidder will always choose banner.

vrtcal-dev · 2021-08-18T18:47:26Z

given a choice between the two, your bidder will always choose banner.

Correct, our bidder will always choose banner if a video object is also specified within an impression request.

mansinahar · 2021-08-19T14:40:44Z

adapters/vrtcal/vrtcal.go

@@ -70,9 +70,14 @@ func (a *VrtcalAdapter) MakeBids(internalRequest *openrtb2.BidRequest, externalR

 	for _, sb := range bidResp.SeatBid {
 		for i := range sb.Bid {
+			bidType, err := getReturnTypeForImp(sb.Bid[i].ImpID, internalRequest.Imp)
+			if err != nil {
+				return nil, []error{err}


Sorry, I missed this in my earlier review. In case there's an error getting the type of one of the bids, are you sure you want to return an error and discard the rest of the bids? It might be better to rather append the error in an error slice and move on to the next bid by adding a continue here.

Thinking more about this, I realize that you mentioned your backend service only bids on a single impression for a single request so the response likely is gonna have a single bid as well but I'd still recommend adding a continue here and appending the error to an error slice as it is easy to do and this way you won't have to update this logic in the future even when your backend service does start supporting multiple impressions and returning multiple bids

Sorry, I missed this in my earlier review. In case there's an error getting the type of one of the bids, are you sure you want to return an error and discard the rest of the bids? It might be better to rather append the error in an error slice and move on to the next bid by adding a continue here.

Thinking more about this, I realize that you mentioned your backend service only bids on a single impression for a single request so the response likely is gonna have a single bid as well but I'd still recommend adding a continue here and appending the error to an error slice as it is easy to do and this way you won't have to update this logic in the future even when your backend service does start supporting multiple impressions and returning multiple bids

Thanks Mansinahar. I've added a patch to allow the continuance of processing of further bids after ones with an unmatched Impression ID...

mansinahar · 2021-08-23T17:59:03Z

@vrtcal-dev Looks like there are some conflicts with the recent stuff that got merged in. Can you please rebase your PR on top of master and resolve the conflicts?

mansinahar · 2021-08-23T18:10:03Z

adapters/vrtcal/vrtcal.go

-				BidType: "banner",
-			})
+			bidType, err := getReturnTypeForImp(sb.Bid[i].ImpID, internalRequest.Imp)
+			if err == nil {


@vrtcal-dev This check would just ignore the error. I'd recommend changing this to the following so that the errors are properly communicated to the user.

if (err != nil) { errs = append(errs, err) continue } bidResponse.Bids = append(bidResponse.Bids, &adapters.TypedBid{ Bid: &sb.Bid[i], BidType: bidType, })

You'll just. have to instantiate and errs slice either at the start of this function or before the for loop

This check would just ignore the error. I'd recommend changing this to the following so that the errors are properly communicated to the user.

Thanks mansinahar, I've udpated to properly report errored bids while continuing to proces further bids.

…elated test case update.

Added Video(VAST) Support

48da8c9

mansinahar reviewed Aug 17, 2021

View reviewed changes

adapters/vrtcal/vrtcal.go Show resolved Hide resolved

vrtcal-dev closed this Aug 17, 2021

vrtcal-dev reopened this Aug 17, 2021

mansinahar requested review from mansinahar and hhhjort August 17, 2021 18:15

mansinahar assigned mansinahar and hhhjort Aug 17, 2021

SyntaxNode changed the title ~~Added Video(VAST) Support~~ Vrtcal: Added Video(VAST) Support Aug 17, 2021

guscarreon previously requested changes Aug 18, 2021

View reviewed changes

mansinahar reviewed Aug 18, 2021

View reviewed changes

Added supplemental unmatched impression ID test coverage

fc72d74

mansinahar reviewed Aug 19, 2021

View reviewed changes

Ubuntu added 2 commits August 19, 2021 19:47

Update allowing successive bid use after those with unmatched imp IDs

dda79e4

Fix for PR #1879 Conflict

40a71fe

mansinahar reviewed Aug 23, 2021

View reviewed changes

Update to fix unreporting errored bids with unmatched imp IDs. Also r…

25a44da

…elated test case update.

vrtcal-dev closed this Aug 24, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Vrtcal: Added Video(VAST) Support #1965

Vrtcal: Added Video(VAST) Support #1965

vrtcal-dev commented Aug 17, 2021 •

edited by mansinahar

Loading

mansinahar commented Aug 17, 2021 •

edited

Loading

vrtcal-dev commented Aug 17, 2021 •

edited

Loading

mansinahar commented Aug 17, 2021

vrtcal-dev commented Aug 17, 2021

guscarreon left a comment •

edited

Loading

mansinahar commented Aug 18, 2021

vrtcal-dev commented Aug 18, 2021

mansinahar left a comment

mansinahar Aug 18, 2021

vrtcal-dev Aug 18, 2021 •

edited

Loading

hhhjort commented Aug 18, 2021

vrtcal-dev commented Aug 18, 2021

mansinahar Aug 19, 2021

vrtcal-dev Aug 19, 2021

mansinahar commented Aug 23, 2021

mansinahar Aug 23, 2021

vrtcal-dev Aug 24, 2021

Vrtcal: Added Video(VAST) Support #1965

Vrtcal: Added Video(VAST) Support #1965

Conversation

vrtcal-dev commented Aug 17, 2021 • edited by mansinahar Loading

mansinahar commented Aug 17, 2021 • edited Loading

vrtcal-dev commented Aug 17, 2021 • edited Loading

mansinahar commented Aug 17, 2021

vrtcal-dev commented Aug 17, 2021

guscarreon left a comment • edited Loading

Choose a reason for hiding this comment

mansinahar commented Aug 18, 2021

vrtcal-dev commented Aug 18, 2021

mansinahar left a comment

Choose a reason for hiding this comment

mansinahar Aug 18, 2021

Choose a reason for hiding this comment

vrtcal-dev Aug 18, 2021 • edited Loading

Choose a reason for hiding this comment

hhhjort commented Aug 18, 2021

vrtcal-dev commented Aug 18, 2021

mansinahar Aug 19, 2021

Choose a reason for hiding this comment

vrtcal-dev Aug 19, 2021

Choose a reason for hiding this comment

mansinahar commented Aug 23, 2021

mansinahar Aug 23, 2021

Choose a reason for hiding this comment

vrtcal-dev Aug 24, 2021

Choose a reason for hiding this comment

vrtcal-dev commented Aug 17, 2021 •

edited by mansinahar

Loading

mansinahar commented Aug 17, 2021 •

edited

Loading

vrtcal-dev commented Aug 17, 2021 •

edited

Loading

guscarreon left a comment •

edited

Loading

vrtcal-dev Aug 18, 2021 •

edited

Loading