sovrn: Add video support #2232

cpabst · 2022-04-22T19:46:10Z

No description provided.

sort bids by price so multiple sovrn tags in a unit can function properly

sovrn adapter unit tests

VeronikaSolovei9 · 2022-05-02T04:23:23Z

adapters/sovrn/sovrn.go

-	}
-
-	return imp.TagID, nil
+	return response, nil
 }

 func getTagid(sovrnExt openrtb_ext.ExtImpSovrn) string {


I'm curious what is the purpose of this function? I know this is not in scope of this PR, but do you think you need it?

I have inherited this code from someone who is no longer at Sovrn. But it appears that our original work with Prebid Server used a spelling on our tagid parameter that we quickly decided was wrong. 😄

So the old version is deprecated, but not removed yet. Not sure how we will go forward with the actual removal, but we will figure that out.

VeronikaSolovei9 · 2022-05-02T04:23:59Z

.devcontainer/devcontainer.json

@@ -10,6 +10,7 @@
 			// Options
 			"INSTALL_NODE": "false",
 			"NODE_VERSION": "lts/*",
+			"TEST": "false"


Please remove this line, you probably pushed it accidentally.

VeronikaSolovei9 · 2022-05-02T04:31:00Z

adapters/sovrn/sovrn.go

+		if video != nil {
+			if video.MIMEs == nil ||
+				video.MinDuration == 0 ||
+				video.MaxDuration == 0 ||


Please review this logic again. For video endpoint there is a possibility to have only maxDuration when requireExactDuration if false or not specified. In this case the entire request will be rejected.

I'll need to review this with product. I'll let you know how that conversation goes.

@cpabst Were you able to check about this with your team?

He has been very busy. 😄
I'd like to keep it this way, and look at this idea as a future enhancement. I see others doing it the same way.

VeronikaSolovei9 · 2022-05-02T04:39:31Z

adapters/sovrn/sovrn.go

-	}
+				if request.Imp[0].Video != nil {
+					bidType = openrtb_ext.BidTypeVideo
+				}


In case you return bid type in bid.ext we recommend to use this approach:

prebid-server/adapters/algorix/algorix.go

Line 190 in c93a036

func getBidType(bid openrtb2.Bid, imps []openrtb2.Imp) (openrtb_ext.BidType, error) {

It will also help to determine bid type correctly in case if req.imp is multi typed.

We do not support multi-typed imps yet.

You may not support it, but request may have it. In your code you take imp type from request and match it with response: request.Imp[impIdx]

VeronikaSolovei9 · 2022-05-02T04:42:55Z

adapters/sovrn/sovrn.go

+		imp.TagID = getTagid(sovrnExt)
+
+		if imp.BidFloor == 0 && sovrnExt.BidFloor > 0 {
+			imp.BidFloor = sovrnExt.BidFloor


Can you please add coverage for this branch?

Done. Fixed a bunch of other tests as well.

mansinahar · 2022-05-04T16:42:42Z

adapters/sovrn/sovrn.go

+				errs = append(errs, &errortypes.BadInput{
+					Message: "Missing required video parameter",
+				})
+				return nil, errs


Are you sure you wanna throw away the entire request only if a single video imp isn't valid? You could just continue here and process the imps that are valid.

Good point. Thanks.

mansinahar · 2022-05-04T16:50:01Z

adapters/sovrn/sovrn.go

-		}
-	}
+				if request.Imp[0].Video != nil {
+					bidType = openrtb_ext.BidTypeVideo


As mentioned by @VeronikaSolovei9, this can return the wrong bid type in case where the imp is a multi-format imp. But also, since you're only looking at the first imp in the request, this might also return the wrong bid types if not all imps in the request are of same type. For example, say the first imp in the request is of type video but the second one is of type banner, then you'll be returning wrong bid type for the bid corresponding to the second imp.

We highly recommend using bid.Ext.MediaType to send back this information to Prebid Server.

I'm solving this presently with using the Bid.impid value to set bid type. We will look into using the recommended value in the future.

mansinahar · 2022-05-04T16:54:00Z

adapters/sovrn/sovrntest/exemplary/no-bid.json

@@ -20,7 +20,8 @@
        "bidder": {
          "tagid": "123456"
        }
-      }
+      },


Wondering why was imp.tagid added to this and a few other test requests in this PR? Won't it be added by your adapter code based on what you have in your tagid bidder param?

Yep. Figured that out today. Those all should be fixed now.

mansinahar · 2022-05-04T16:58:17Z

adapters/sovrn/sovrntest/supplemental/with-only-custom-bidfloor.json

+            }
+          ]
+        },
+        "bidfloor": 4.2,


The test name indicates that this test case is where there's only custom bid floor present, which as I understand is the one in the bidder ext. In that case shouldn't this be not present in the original request? This will then be filled by your adapter

Yes, you are correct. This has been fixed.

mansinahar · 2022-05-04T17:01:51Z

adapters/sovrn/sovrntest/video/multi-banner-video.json

+          "format": [
+            {
+              "w": 300,
+              "h": 250


Consider the imp order reversed in this test case where the first imp is a video imp and the second one is the banner imp, in that case the bid type in the response will be incorrectly set to video.. Now also consider the imps in the same order as you have in this test, if your bidder service were to send back a bid for the video imp as well, in this case the video bid would have the bid type incorrectly set to banner

I believe this has been solved by using the response Bid.impid value.

cpabst · 2022-05-04T22:54:09Z

@mansinahar @VeronikaSolovei9 I have made quite a few updates based on these comments. This PR is ready for another review.

mansinahar · 2022-05-09T13:21:29Z

adapters/sovrn/sovrn.go

+			continue
+		}
+
+		imp.TagID = getTagid(sovrnExt)


You've already gotten the tagId value from the getTagid function on L63 so this line should be just:

imp.TagID = tagId

Also, can you please rename the getTagid function to getTagId?

mansinahar · 2022-05-09T13:27:06Z

adapters/sovrn/sovrn.go

+		return nil, errs
+	}
+
+	reqJSON, err := json.Marshal(request)


At this point, this request can have both valid and invalid imps because on L92 you're just reassigning the valid imps to the same place in the imps array of the request. You'd actually have to remove the invalid ones as you find them in the for loop so that you're only left with the valid ones or a much easier way would be to just create a new validImps array and keep appending each valid imp you find to that array and then outside the for loop, you can just replace request.Imp with that new validImps array before JSON marshalling

Good point. Done.

mansinahar · 2022-05-09T13:29:51Z

adapters/sovrn/sovrn.go

@@ -75,71 +116,54 @@ func addHeaderIfNonEmpty(headers http.Header, headerName string, headerValue str
 	}
 }

-func (s *SovrnAdapter) MakeBids(internalRequest *openrtb2.BidRequest, externalRequest *adapters.RequestData, response *adapters.ResponseData) (*adapters.BidderResponse, []error) {
-	if response.StatusCode == http.StatusNoContent {
+func (s *SovrnAdapter) MakeBids(request *openrtb2.BidRequest, requestData *adapters.RequestData, responseData *adapters.ResponseData) (*adapters.BidderResponse, []error) {


Nitpick: The names requestData and responseData don't really convey the proper context here IMHO. How about using, bidderRequest and bidderResponse, respectively?

mansinahar · 2022-05-09T13:34:17Z

adapters/sovrn/sovrn.go

+		}
+	}
+
+	return -1 // hopefully this never happens


We should be returning an error if this does happen.. This would mean that for some reason your bidder bid for an imp that doesn't match an ID with the imps in the request. By returning an explicit error, it will be much easier to debug and fix the issue rather than silently just setting the bid type to banner

I see your point. I believe I have fixed this correctly now. Please let me know if you feel otherwise.

mansinahar · 2022-05-11T16:51:23Z

adapters/sovrn/sovrn.go

 		}
 	}

-	return -1 // hopefully this never happens
+	return -1, &errortypes.BadInput{
+		Message: fmt.Sprintf("Could not find imp ID '%s' in bid, dropping bid", impId),


Could you please rephrase this as something like:

Imp ID %s in bid didn't match with any imp in the original request

The above indicates we're trying to find the imp ID in a list of bids whereas the other way round is true. We're trying to match the impID in the bid with the list of imps in the request

Works for me. Done.

mansinahar · 2022-05-11T16:55:46Z

adapters/sovrn/sovrntest/video/simple-video.json

+{
+  "mockBidRequest": {
+  "id": "test-request-id",
+  "imp": [


Is this test case testing anything different from the full video test case you have above?

This one just has the required parameters, and the other one has all parameters. I like that it shows the minimum viable request, but don't feel strongly if you'd like it to go.

mansinahar · 2022-05-11T17:00:14Z

adapters/sovrn/sovrntest/video/multi-banner-video.json

+          }
+        }
+      },
+      "mockResponse": {


Please consider adding a response for video as well in this test case. This will allow this test case to verify that responses with multiple bids for various imps are processed correctly and will also verify the getBidType logic to make sure it matches bids to imps properly based on the imp ID

Good point. Done.

mansinahar · 2022-05-11T17:01:10Z

adapters/sovrn/sovrn.go

+
+				impIdx, impIdErr := getImpIdx(bid.ImpID, request)
+				if impIdErr != nil {
+					errs = append(errs, impIdErr)


Please add a test case for this error condition

mansinahar · 2022-05-11T17:02:26Z

@cpabst Thanks for addressing the comments. This is looking to be in good shape now. Just have a couple minor comments.

kevinKalmbach and others added 30 commits July 28, 2017 12:53

initial checkin of sovrn adapter

dd18694

Merge remote-tracking branch 'upstream/master'

f619601

Merge remote-tracking branch 'upstream/master'

563787f

Merge remote-tracking branch 'upstream/master'

cd70e4a

some adapter updates from testing

446c8d8

sort bids by price so multiple sovrn tags in a unit can function properly

Merge remote-tracking branch 'upstream/master'

572725e

User sync url/params updates

4d20c54

merge upstream/master

c61126a

Merge branch 'master' of https://github.com/prebid/prebid-server

058319c

Merge branch 'master' of https://github.com/prebid/prebid-server

0bd1577

sovrn adapter unit tests

cd0de9b

Merge remote-tracking branch 'prebid/master' into PSERV-19_sovrn_adapter

5d707f9

Updates to pass validation

12005e8

Merge pull request #1 from sovrn/PSERV-19_sovrn_adapter

1ecb04e

sovrn adapter unit tests

Merge branch 'master' of https://github.com/prebid/prebid-server

1dc8299

Add Headers

d284e95

secure

20e2b1f

Tests for headers

c576243

fix pixel endpoint params

2a721ba

Fix test for new pixel ep

48f2f32

Merge branch 'master' of https://github.com/prebid/prebid-server

bf1cd4f

Merge branch 'master' of https://github.com/prebid/prebid-server

a3b0275

fix test after os change

06a29b2

add info.yaml

deaf51a

Merge branch 'master' of https://github.com/prebid/prebid-server

16dcca8

test no cookies behavior & adapter names

43cc6dc

Add test for nocontent & notfound responses

4595b2d

Merge branch 'master' of https://github.com/prebid/prebid-server

a1f4cb6

remove semicolon

8647533

Merge branch 'master' of https://github.com/prebid/prebid-server

b91d1c4

mansinahar requested review from bsardo and VeronikaSolovei9 April 25, 2022 18:00

mansinahar assigned bsardo and VeronikaSolovei9 Apr 25, 2022

VeronikaSolovei9 reviewed May 2, 2022

View reviewed changes

Merge branch 'prebid:master' into master

7d3c80f

cpabst force-pushed the EX-3550 branch from 2020c52 to ebf45ca Compare May 3, 2022 23:16

mansinahar requested review from mansinahar and removed request for bsardo May 4, 2022 17:17

mansinahar assigned mansinahar and unassigned bsardo May 4, 2022

mansinahar reviewed May 4, 2022

View reviewed changes

mansinahar reviewed May 9, 2022

View reviewed changes

mansinahar reviewed May 11, 2022

View reviewed changes

cpabst and others added 2 commits May 11, 2022 18:31

Merge branch 'prebid:master' into master

b1a5d48

SOVRN: Add support for video requests

ad6ac9b

cpabst force-pushed the EX-3550 branch from ad110c0 to ad6ac9b Compare May 12, 2022 00:33

More PR fixes

edd2ca2

mansinahar approved these changes May 16, 2022

View reviewed changes

VeronikaSolovei9 approved these changes May 16, 2022

View reviewed changes

mansinahar merged commit bd471be into prebid:master May 16, 2022

marki1an mentioned this pull request May 19, 2022

Sovrn: Add video support prebid/prebid-server-java#1874

Merged

jorgeluisrocha pushed a commit to jwplayer/prebid-server that referenced this pull request Sep 28, 2022

sovrn: Add video support (prebid#2232)

abf12b5

shunj-nb pushed a commit to ParticleMedia/prebid-server that referenced this pull request Nov 8, 2022

sovrn: Add video support (prebid#2232)

96ce168

sovrn: Add video support #2232

sovrn: Add video support #2232

Conversation

cpabst commented Apr 22, 2022

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 May 2, 2022 • 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

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

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

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

cpabst commented May 4, 2022

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

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mansinahar commented May 11, 2022

VeronikaSolovei9 May 2, 2022 •

edited

Loading