Telaria: Add pod support #2171
Telaria: Add pod support #2171
Conversation
Merging with master
Keeping the fork updated with master
Keeping the fork up to date with master
…red but Bid.SeatCode is required
Keeping our fork updated with master
Merging with master
Keeping the fork up to date with master
- added more test cases
Keeping the fork up to date with master
Keeping the fork up to date with master
Keeping the fork up to date with master
adapters/telaria/telaria.go
Outdated
| @@ -205,6 +205,9 @@ func (a *TelariaAdapter) MakeRequests(requestIn *openrtb2.BidRequest, reqInfo *a | |||
| // Swap the tagID with adCode | |||
| request.Imp[i].TagID = telariaImpExt.AdCode | |||
| } | |||
| if len(request.Imp) > 1 { | |||
There was a problem hiding this comment.
If you are going to drop all but the first imp, do we need to loop over all the imps above?
Also, if there is only 1 imp that goes to the bidder, you know all your bids are for that imp, and don't need to loop over all the imps in MakeBids()
There was a problem hiding this comment.
Thank you for the input. That's a good point; there's no need to loop over all the imps above. I will make changes to that right away.
It's still necessary to loop over all the imps inmakeBids() because not all responses from the bidder is for the 1 imp that's sent. Our bidder takes in ext information such as pod duration to calculate the number of imps that's most suitable for the pod and return multiple bids for each slot in the pod. Therefore, I need to loop over them and make sure they are for different imp.
There was a problem hiding this comment.
It looks like you only send the first imp to your bidder. How does your bidder know what the imp IDs for the other imps are in order to bid on them?
There was a problem hiding this comment.
Similar to the RTB 2.6 spec, our bidder calculates the appropriate pod size based on poddur and maxseq parameters in addition to the configured parameters in the ad unit. Since we cannot really get the poddur information from what's passed to our adapter, we ask it to be present in the video ext. And In the end, we match the bid responses with the imp ids.
There was a problem hiding this comment.
Do you only support long format pods, or should you verify that the request is such a request before throwing the rest of the imps away? I just want to make sure you can still support the old general video case unless you explicitly are not supporting it. :) Otherwise the PR looks good.
There was a problem hiding this comment.
Thank you so much for the review and inputs. And this is all we support at the moment :)
There was a problem hiding this comment.
Looks pretty good. I see the exemplary directory comes with a couple of tests where the incoming request holds only one imp. Given that the current version of adapters/telaria/telaria.go dealt with more than one imp, should we include a test case with multiple imps too? Is a multi-imp scenario feasible or worth considering anymore?
adapters/telaria/telaria.go
Outdated
| }) | ||
| for i := range sb.Bid { | ||
| bid := sb.Bid[i] | ||
| if i < len(internalRequest.Imp) { |
There was a problem hiding this comment.
If len(internalRequest.Imp) is less than len(sb.Bid) should we break the for loop to optimize a bit?
278
279 for i := range sb.Bid {
280 bid := sb.Bid[i]
281 - if i < len(internalRequest.Imp) {
+ if i >= len(internalRequest.Imp) {
+ break
+ }
282 bid.ImpID = internalRequest.Imp[i].ID
283 bidResponse.Bids = append(bidResponse.Bids, &adapters.TypedBid{
284 Bid: &bid,
285 BidType: openrtb_ext.BidTypeVideo,
286 })
287 - }
288 }
289 return bidResponse, nil
290 }
291
adapters/telaria/telaria.go
There was a problem hiding this comment.
Thank you so much for your inputs. They are really good points. I made the suggested changes and added the test cases as well.
also break response loop early to optimize performance further
There was a problem hiding this comment.
@xxu-telaria thank you for addressing our feedback. Your changes look good to me
add ability to handle multiple bid responses