Wap 4485 adaptor #2

mgloystein · 2019-06-10T22:50:14Z

Adding the adaptor and tests

bluedreamer

I have more to look at but maybe you can get started on current comments

bluedreamer · 2019-06-11T21:18:05Z

adapters/spotx/spotx.go

+	}
+
+	timeDuration := string(dur)
+	timeCodeComponents := strings.Split(timeDuration, ":")


You might find these useful - these are from the C++ vast unit tests

static std::vectorstd::string invalid_time_strings = {"invalid",
"0s",
"00:00",
"00:00:0",
"00:0:00",
"00:00:000",
"00:000:00",
"00:00:00.0",
"00:00:00.00",
"00:00:00.0000",
"00:00:00PM"};

static std::vectorstd::string valid_time_strings =
{"0:00:00", "00:00:00", "00:00:00.000", "000:00:00.000", "123:45:56.789", "9999:99:99.999"};

I'll change it to a float64 and increase my slice capacity to 16.

bluedreamer · 2019-06-11T21:23:22Z

adapters/spotx/spotx.go

+	httpReq.Header.Add("Content-Type", "application/json;charset=utf-8")
+	httpReq.Header.Add("Accept", "application/json")
+
+	resp, err := ctxhttp.Do(ctx, a.http.Client, httpReq)


This is the request you are sending to spotx right? So a lot of our content is going to be refined on geo/device type - so I wonder if you need this section

"device":{
"h":736,
"w":414,
"lmt":0,
"connectionType":2,
"make":"Apple",
"model":"iPhone9,4",
"os":"iOS",
"osv":"11.2.6",
"ua":"Mozilla/5.0 (iPhone; CPU iPhone OS 11_2_6 like Mac OS X) AppleWebKit/604.5.6 (KHTML,like Gecko) Mobile/15D100",
"ifa":"EEBBF4EE-3B08-4556-BC3E-CCD3EBB8E4B4",
"geo":{
"country":"NZ",
"city":"Auckland",
"zip":"1150"
},

Hopefully all that information will already be in the request and correctly massaged into the Open RTB request we are making to ourselves. Call calls this method after calling adapters.MakeOpenRTBGeneric, which is meant to transform the prebid request into an ortb request. Device is part of both specs so I assume that translation is done in that call.

bluedreamer · 2019-06-11T21:28:22Z

adapters/spotx/spotx.go

+
+	httpReq, err := http.NewRequest("POST", uri, bytes.NewBuffer(reqJSON))
+	httpReq.Header.Add("Content-Type", "application/json;charset=utf-8")
+	httpReq.Header.Add("Accept", "application/json")


I wonder if we should set a user agent so we can id our own calls
"User-Agent: spotx-prebidderwidget_do_dad"

Do you have an idea of what a legitimate value should be for this?

It might be nice to identify, so how about "spotx_prebid_server 1.0" or similar

SpotMarket needs the user agent accurately reflect the device that the ad will run on. We use it for audience identification, device targeting, etc. I would suggest another method for indicating prebid server calls (maybe supply chain?)

strider820 · 2019-07-19T17:24:06Z

I'll let others leave comments for things I might have missed, but from my perspective, it seems like it'd do what I'd expect for prebid server

thomasrmielke

It seems like this adapter is sending a lot of fields that are not currently supported by SpotX. It will not cause an error in the request, but the extra fields will be ignored.

Also, it was a little difficult to trace the flow of the code. I expected to see the bids constructed hierarchically, but it didn't seem to be organized that way. That being said, I'm not familiar with a lot of the syntax in Go, so that may be contributing to it.

Have you tried connecting the requests generated here with a test SpotX server to validate the responses?

thomasrmielke · 2019-08-02T19:31:56Z

adapters/spotx/spotx.go

+		return
+	}
+
+	if config.ChannelID == 0 {


Are you checking for '0' here specifically because it's a default value?

thomasrmielke · 2019-08-02T19:36:46Z

adapters/spotx/spotx.go

+
+	httpReq, err := http.NewRequest("POST", uri, bytes.NewBuffer(reqJSON))
+	httpReq.Header.Add("Content-Type", "application/json;charset=utf-8")
+	httpReq.Header.Add("Accept", "application/json")


SpotMarket needs the user agent accurately reflect the device that the ad will run on. We use it for audience identification, device targeting, etc. I would suggest another method for indicating prebid server calls (maybe supply chain?)

thomasrmielke · 2019-08-02T19:38:54Z

adapters/spotx/spotx.go

+		if imp.ID == bid.ImpID {
+			if imp.Video != nil {
+				if len(bid.Cat) > 0 {
+					cat = bid.Cat[0]


Why only the first element in bid.Cat?

I wasn't able to find a primary category in our result but the object passed to the decision point requires there be a primary.

thomasrmielke · 2019-08-02T19:42:19Z

adapters/spotx/spotx.go

+	)
+
+vastXmlSearch:
+	for i := 0; i < len(vastResponse); i++ { // Iterate over every character


Why are you writing an xml parser here? Are there no available libs to parse XML?

I'm not fully parsing the xml, I only care about a single value and this has to be fast.

thomasrmielke · 2019-08-02T19:47:05Z

adapters/spotx/spotx.go

+
+func getMediaTypeForImp(impId string, imps []openrtb.Imp) openrtb_ext.BidType {
+	for _, imp := range imps {
+		if imp.ID == impId {


I don't know if this is a realistic request, but what if there are multiple objects specified? The spec doesn't indicate that the values are mutually exclusive. This might be a question for product.

Also, there is an implicit precedence for choosing the media type. Is that intentionally? E.g., video will always be choosen over audio?

I think an impression can only have one type and the requester would have to specify another impression if they wanted a bid for a different type.

Ok, any reason for the implicit precedence chosen here? Video over audio?

thomasrmielke · 2019-08-02T19:52:32Z

adapters/spotx/spotx.go

+		ortbReq.Imp[i].BidFloorCur = param.Currency
+
+		if param.Boxing.Valid && param.Boxing.Bool {
+			ortbReq.Imp[1].Video.BoxingAllowed = int8(1)


Not supported.

Will it break things? Might it be supported in the future?

Will not break things. Just suggesting that you could limit the scope of your work to just the supported fields (reduces our bandwidth too).

thomasrmielke · 2019-08-02T19:55:52Z

adapters/spotx/spotx.go

+		}
+
+		if len(param.KVP) > 0 {
+			ortbReq.Imp[i].Ext = kvpToExt(param.KVP)


Looking at the code and docs, I don't believe KVPs are supported this way either.

How are they supported?

Per our docs, in the video ext field (not the imp.ext). See https://confluence.spotxchange.com/display/byndocspotx/SpotMarket+Publisher+OpenRTB+Endpoint+Support

thomasrmielke · 2019-08-02T19:57:00Z

adapters/spotx/spotx.go

+	}
+
+	ortbReq.BAdv = param.BlackList.Advertiser
+	ortbReq.BCat = param.BlackList.Category


Not supported.

thomasrmielke · 2019-08-02T19:57:50Z

adapters/spotx/spotx.go

+
+	ortbReq.WLang = param.WhiteList.Language
+
+	if len(param.WhiteList.Seat) > 0 {


Not supported.

thomasrmielke · 2019-08-02T20:00:57Z

adapters/spotx/spotxtests/exemplary/ortb_2.3_video.json

+    "tmax": 9000,
+    "ext": {
+      "spotx":{
+        "ortb_version": "2.3",


This isn't currently supported.

thomasrmielke · 2019-08-02T20:07:42Z

There are several internal SpotX confluence pages outlining what is supported on the pORTB endpoint and what isn't. Let me know if you'd like those pages.

thomasrmielke · 2019-08-30T19:04:49Z

adapters/spotx/spotx.go

+		}
+
+		if len(param.KVP) > 0 {
+			ortbReq.Imp[i].Ext = kvpToExt(param.KVP)


Per our docs, in the video ext field (not the imp.ext). See https://confluence.spotxchange.com/display/byndocspotx/SpotMarket+Publisher+OpenRTB+Endpoint+Support

thomasrmielke

Just a few minor questions remain. Outstanding issues would be the custom kvps and user agent.

mgloystein requested review from bluedreamer, jbaldwin, mattslocum and mooyah June 11, 2019 14:11

bluedreamer reviewed Jun 12, 2019

View reviewed changes

thomasrmielke reviewed Aug 2, 2019

View reviewed changes

thomasrmielke approved these changes Aug 30, 2019

View reviewed changes

thomasrmielke suggested changes Aug 30, 2019

View reviewed changes

mgloystein force-pushed the spotx_bid_adaptor branch from 77a974f to e1ed5a6 Compare September 3, 2019 14:30

Michael Gloystein added 10 commits September 3, 2019 08:40

Add new user syncer

a9027c0

Updated package and branding name

bd197ce

Adding Spotx Bid adaptor : initial

ae2a506

Adding params to auction request

e929e70

Adaptor and test skeleton

4fb8ac3

Parse VAST response for duration

9cd7c88

Tests and fixes

edb9870

Address comment on valid time strings

d270927

Refactor for readability

c4d307b

Comments

ed0b189

mgloystein force-pushed the wap-4485_adaptor branch from 696ea51 to ed0b189 Compare September 3, 2019 14:52

Michael Gloystein added 2 commits September 4, 2019 14:07

KVP location change

2bdb16e

Fix bad merge artifacts

fd68db8

thomasrmielke approved these changes Sep 4, 2019

View reviewed changes

More bad artifacts from merge

d230f82


		ortbReq.WLang = param.WhiteList.Language

		if len(param.WhiteList.Seat) > 0 {

Wap 4485 adaptor #2

Are you sure you want to change the base?

Wap 4485 adaptor #2

Conversation

mgloystein commented Jun 10, 2019 • edited Loading

bluedreamer left a 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

strider820 commented Jul 19, 2019

thomasrmielke left a 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

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

thomasrmielke commented Aug 2, 2019

Choose a reason for hiding this comment

thomasrmielke left a comment

Choose a reason for hiding this comment

mgloystein commented Jun 10, 2019 •

edited

Loading