[synacormedia] Add synacormedia adapter #1029

coreykress · 2019-09-09T16:35:33Z

No description provided.

stale · 2019-09-17T18:23:03Z

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

guscarreon

Appart from the comments made for adapters/synacormedia/synacormedia.go, can we increase code coverage to 95% or so?

╰─➤  go test -coverprofile=c.out
PASS
coverage: 87.8% of statements
ok      github.com/prebid/prebid-server/adapters/synacormedia   0.023s
╭─gcarreongutierrez@Guss-MacBook-Pro.local ~/go/src/github.com/prebid/prebid-server/adapters/synacormedia  ‹PR1029*›
╰─➤  go tool cover -func=c.out
github.com/prebid/prebid-server/adapters/synacormedia/synacormedia.go:29:       NewSynacorMediaBidder   50.0%
github.com/prebid/prebid-server/adapters/synacormedia/synacormedia.go:39:       MakeRequests            100.0%
github.com/prebid/prebid-server/adapters/synacormedia/synacormedia.go:52:       makeRequest             87.5%
github.com/prebid/prebid-server/adapters/synacormedia/synacormedia.go:116:      buildEndpointURL        100.0%
github.com/prebid/prebid-server/adapters/synacormedia/synacormedia.go:120:      getExtImpObj            85.7%
github.com/prebid/prebid-server/adapters/synacormedia/synacormedia.go:139:      MakeBids                100.0%
github.com/prebid/prebid-server/adapters/synacormedia/synacormedia.go:173:      getMediaTypeForImp      85.7%
github.com/prebid/prebid-server/adapters/synacormedia/usersync.go:10:           NewSynacorMediaSyncer   100.0%
total:                                                                          (statements)            87.8%

adapters/synacormedia/synacormedia.go

guscarreon · 2019-09-17T20:07:29Z

adapters/synacormedia/synacormedia.go

+		}
+	}
+	return mediaType
+}


Function getMediaTypeForImp(impId string, imps []openrtb.Imp) might return openrtb_ext.BidTypeBanner even if imp.Banner == nil and imp.Video == nil. Other adapters overcome this issue by filtering out non-Banner and non-Video types somewhere inside MakeRequests(request *openrtb.BidRequest, reqInfo *adapters.ExtraRequestInfo). I believe either filtering out non-Banner and non-Video imps, or modifying this function to return an error or something so we don't append this bid to the bidResponse array in MakeBids may solve this issue.

173 func getMediaTypeForImp(impId string, imps []openrtb.Imp) openrtb_ext.BidType { 174 mediaType := openrtb_ext.BidTypeBanner 175 for _, imp := range imps { 176 if imp.ID == impId { 177 if imp.Banner == nil && imp.Video != nil { 178 mediaType = openrtb_ext.BidTypeVideo 179 } 180 return mediaType //<- may return openrtb_ext.BidTypeBanner on an imp with imp.Banner==nil && imp.Video==nil element 181 } 182 } 183 return mediaType //<- may return openrtb_ext.BidTypeBanner even if imp wasn't found in the imp list 184 } synacormedia.go

Added filter.

// filter out non banner and non video imps var validRequestImps []openrtb.Imp for _, imp := range internalRequest.Imp { if imp.Banner != nil || imp.Video != nil { validRequestImps = append(validRequestImps, imp) } }

I don't think this for loop is exactly filtering out non banner and non video imps because if getMediaTypeForImp(sb.Bid[i].ImpID, validRequestImps) doesn't find a match in validRequestImps it will still assign the value openrtb_ext.BidTypeBanner to something that probably isn't. Do you think a better approach is this?

138 func (a *SynacorMediaAdapter) MakeBids(internalRequest *openrtb.BidRequest, externalRequest *adapters.RequestData, response *adapters.ResponseData) (*adapters.BidderResponse, []error) { 139 const errorMessage string = "Unexpected status code: %d. Run with request.debug = 1 for more info" 140 switch { 141 case response.StatusCode == http.StatusNoContent: 142 return nil, nil 143 case response.StatusCode == http.StatusBadRequest: 144 return nil, []error{&errortypes.BadInput{ 145 Message: fmt.Sprintf(errorMessage, response.StatusCode), 146 }} 147 case response.StatusCode != http.StatusOK: 148 return nil, []error{&errortypes.BadServerResponse{ 149 Message: fmt.Sprintf(errorMessage, response.StatusCode), 150 }} 151 } 152 153 var bidResp openrtb.BidResponse 154 155 if err := json.Unmarshal(response.Body, &bidResp); err != nil { 156 return nil, []error{err} 157 } 158 159 bidResponse := adapters.NewBidderResponseWithBidsCapacity(1) 160 161 - // filter out non banner and non video imps 162 - var validRequestImps []openrtb.Imp 163 - for _, imp := range internalRequest.Imp { 164 - if imp.Banner != nil || imp.Video != nil { 165 - validRequestImps = append(validRequestImps, imp) 166 - } 167 - } 168 - 169 for _, sb := range bidResp.SeatBid { 170 for i := range sb.Bid { + mediaType = getMediaTypeForImp(sb.Bid[i].ImpID, validRequestImps) + if mediaType != openrtb_ext.BidTypeBanner && mediaType != openrtb_ext.BidTypeVideo { + continue + } 171 bidResponse.Bids = append(bidResponse.Bids, &adapters.TypedBid{ 172 Bid: &sb.Bid[i], 173 - BidType: getMediaTypeForImp(sb.Bid[i].ImpID, validRequestImps), 173 BidType: mediaType, 174 }) 175 } 176 } 177 return bidResponse, nil 178 } 179 180 func getMediaTypeForImp(impId string, imps []openrtb.Imp) openrtb_ext.BidType { 181 mediaType := openrtb_ext.BidTypeBanner 182 for _, imp := range imps { 183 if imp.ID == impId { 184 - if imp.Banner == nil && imp.Video != nil { 185 - mediaType = openrtb_ext.BidTypeVideo + if imp.Banner != nil { + break + } + if imp.Video != nil { + mediaType = openrtb_ext.BidTypeVideo + break + } + if imp.Native != nil { + mediaType = openrtb_ext.BidTypeNative + break + } + if imp.Audio != nil { + mediaType = openrtb_ext.BidTypeAudio + break 186 } 187 - return mediaType 188 } 189 } 190 return mediaType 191 } adapters/synacormedia/synacormedia.go

stale · 2019-09-24T21:06:00Z

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

coreykress · 2019-09-25T15:11:07Z

please don't close, currently in progress

stale · 2019-10-02T15:45:43Z

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

coreykress · 2019-10-02T19:24:38Z

still not stale

…ormedia-updates * commit '36bd18f75c15a1c3d13315ec9c6f083e13792e57': CAP-1467 - updates for github comments

guscarreon · 2019-10-09T16:45:33Z

adapters/synacormedia/synacormedia.go

+
+	for _, imp := range request.Imp {
+		_, err := getExtImpObj(&imp)
+		validImp, err := getExtImpObj(&imp)


Why does getExtImp(&imp) get executed twice in a row?

guscarreon · 2019-10-09T18:43:18Z

adapters/synacormedia/synacormedia.go

+		}
+	}
+	return mediaType
+}


I don't think this for loop is exactly filtering out non banner and non video imps because if getMediaTypeForImp(sb.Bid[i].ImpID, validRequestImps) doesn't find a match in validRequestImps it will still assign the value openrtb_ext.BidTypeBanner to something that probably isn't. Do you think a better approach is this?

138 func (a *SynacorMediaAdapter) MakeBids(internalRequest *openrtb.BidRequest, externalRequest *adapters.RequestData, response *adapters.ResponseData) (*adapters.BidderResponse, []error) { 139 const errorMessage string = "Unexpected status code: %d. Run with request.debug = 1 for more info" 140 switch { 141 case response.StatusCode == http.StatusNoContent: 142 return nil, nil 143 case response.StatusCode == http.StatusBadRequest: 144 return nil, []error{&errortypes.BadInput{ 145 Message: fmt.Sprintf(errorMessage, response.StatusCode), 146 }} 147 case response.StatusCode != http.StatusOK: 148 return nil, []error{&errortypes.BadServerResponse{ 149 Message: fmt.Sprintf(errorMessage, response.StatusCode), 150 }} 151 } 152 153 var bidResp openrtb.BidResponse 154 155 if err := json.Unmarshal(response.Body, &bidResp); err != nil { 156 return nil, []error{err} 157 } 158 159 bidResponse := adapters.NewBidderResponseWithBidsCapacity(1) 160 161 - // filter out non banner and non video imps 162 - var validRequestImps []openrtb.Imp 163 - for _, imp := range internalRequest.Imp { 164 - if imp.Banner != nil || imp.Video != nil { 165 - validRequestImps = append(validRequestImps, imp) 166 - } 167 - } 168 - 169 for _, sb := range bidResp.SeatBid { 170 for i := range sb.Bid { + mediaType = getMediaTypeForImp(sb.Bid[i].ImpID, validRequestImps) + if mediaType != openrtb_ext.BidTypeBanner && mediaType != openrtb_ext.BidTypeVideo { + continue + } 171 bidResponse.Bids = append(bidResponse.Bids, &adapters.TypedBid{ 172 Bid: &sb.Bid[i], 173 - BidType: getMediaTypeForImp(sb.Bid[i].ImpID, validRequestImps), 173 BidType: mediaType, 174 }) 175 } 176 } 177 return bidResponse, nil 178 } 179 180 func getMediaTypeForImp(impId string, imps []openrtb.Imp) openrtb_ext.BidType { 181 mediaType := openrtb_ext.BidTypeBanner 182 for _, imp := range imps { 183 if imp.ID == impId { 184 - if imp.Banner == nil && imp.Video != nil { 185 - mediaType = openrtb_ext.BidTypeVideo + if imp.Banner != nil { + break + } + if imp.Video != nil { + mediaType = openrtb_ext.BidTypeVideo + break + } + if imp.Native != nil { + mediaType = openrtb_ext.BidTypeNative + break + } + if imp.Audio != nil { + mediaType = openrtb_ext.BidTypeAudio + break 186 } 187 - return mediaType 188 } 189 } 190 return mediaType 191 } adapters/synacormedia/synacormedia.go

… synacormedia-updates * commit 'b26ddff10d5a9808cb0af47f5bc486de3bf86ca6': CAP-1467 - additional updates for github review

…prebid-server into synacormedia-updates

guscarreon · 2019-10-11T15:44:47Z

adapters/synacormedia/synacormedia.go

+	// filter out non banner and non video imps
+	var validRequestImps []openrtb.Imp
+	for _, imp := range internalRequest.Imp {
+		if imp.Banner != nil || imp.Video != nil {
+			validRequestImps = append(validRequestImps, imp)
+		}
+	}


Lines 160 to 166 are no longer needed with the additions made to the for loop that follows. Lines 170 to 173 are already filtering out non banner and non video bids.

guscarreon · 2019-10-11T18:23:19Z

config/config.go

@@ -648,6 +649,7 @@ func SetupViper(v *viper.Viper, filename string) {
 	v.SetDefault("adapters.rtbhouse.endpoint", "http://prebidserver-s2s-ams.creativecdn.com/bidder/prebidserver/bids")
 	v.SetDefault("adapters.somoaudience.endpoint", "http://publisher-east.mobileadtrading.com/rtb/bid")
 	v.SetDefault("adapters.sovrn.endpoint", "http://ap.lijit.com/rtb/bid?src=prebid_server")
+	v.SetDefault("adapters.synacormedia.endpoint", "http://{{.Host}}.technoratimedia.com/openrtb/bids/{{.Host}}")


Do we expect .technoratimedia.com after the host value parses? Or did we mean http://{{.Host}}/openrtb/bids/{{.Host}}?

The .technoratimedia.com is correct

guscarreon · 2019-10-11T18:34:28Z

Can we add a couple of test cases to get to 93~95% coverage?

╰─➤  go test -coverprofile=c.out
PASS
coverage: 85.2% of statements
ok      github.com/prebid/prebid-server/adapters/synacormedia   0.033s
╰─➤  go tool cover -func=c.out
github.com/prebid/prebid-server/adapters/synacormedia/synacormedia.go:29:       NewSynacorMediaBidder   60.0%
github.com/prebid/prebid-server/adapters/synacormedia/synacormedia.go:38:       MakeRequests            100.0%
github.com/prebid/prebid-server/adapters/synacormedia/synacormedia.go:51:       makeRequest             90.6%
github.com/prebid/prebid-server/adapters/synacormedia/synacormedia.go:114:      buildEndpointURL        100.0%
github.com/prebid/prebid-server/adapters/synacormedia/synacormedia.go:118:      getExtImpObj            85.7%
github.com/prebid/prebid-server/adapters/synacormedia/synacormedia.go:137:      MakeBids                95.0%
github.com/prebid/prebid-server/adapters/synacormedia/synacormedia.go:183:      getMediaTypeForImp      60.0%
github.com/prebid/prebid-server/adapters/synacormedia/usersync.go:10:           NewSynacorMediaSyncer   100.0%
total:                                                                          (statements)            85.2%

… synacormedia-updates * commit 'ceffd229b57c69a03b6cc13917ece7db08ccebd1': CAP-1467 - additional github comments

guscarreon

LGTM. Thank you for your patience

* CAP-1394 - move synacormedia adapter to correct repo * CAP-1467 - updates for github comments * CAP-1467 - additional updates for github review * CAP-1467 - additional github comments

CAP-1394 - move synacormedia adapter to correct repo

e17042b

mansinahar assigned guscarreon Sep 10, 2019

stale bot added the stale label Sep 17, 2019

guscarreon requested changes Sep 17, 2019

View reviewed changes

stale bot removed the stale label Sep 17, 2019

stale bot added the stale label Sep 24, 2019

stale bot removed the stale label Sep 25, 2019

stale bot added the stale label Oct 2, 2019

stale bot removed the stale label Oct 2, 2019

CAP-1467 - updates for github comments

36bd18f

mansinahar added the in progress label Oct 3, 2019

Corey Kress and others added 2 commits October 7, 2019 14:02

Merge pull request #1 in SA/sync-prebid-server from CAP-1467 to synac…

0ce2f35

…ormedia-updates * commit '36bd18f75c15a1c3d13315ec9c6f083e13792e57': CAP-1467 - updates for github comments

Merge branch 'master' into synacormedia-updates

e5eae19

coreykress requested a review from guscarreon October 7, 2019 20:06

guscarreon requested changes Oct 9, 2019

View reviewed changes

Corey Kress and others added 3 commits October 9, 2019 15:58

CAP-1467 - additional updates for github review

b26ddff

Merge pull request prebid#2 in SA/sync-prebid-server from CAP-1467 to…

35d1060

… synacormedia-updates * commit 'b26ddff10d5a9808cb0af47f5bc486de3bf86ca6': CAP-1467 - additional updates for github review

Merge branch 'synacormedia-updates' of https://github.com/coreykress/…

37ecb9d

…prebid-server into synacormedia-updates

guscarreon requested changes Oct 11, 2019

View reviewed changes

Corey Kress and others added 2 commits October 14, 2019 15:01

CAP-1467 - additional github comments

ceffd22

Merge pull request prebid#3 in SA/sync-prebid-server from CAP-1467 to…

ed8508c

… synacormedia-updates * commit 'ceffd229b57c69a03b6cc13917ece7db08ccebd1': CAP-1467 - additional github comments

coreykress requested a review from guscarreon October 14, 2019 20:27

guscarreon approved these changes Oct 16, 2019

View reviewed changes

guscarreon requested a review from hhhjort October 16, 2019 21:00

guscarreon assigned hhhjort Oct 16, 2019

hhhjort approved these changes Oct 16, 2019

View reviewed changes

hhhjort merged commit fe0aa4d into prebid:master Oct 16, 2019

coreykress deleted the synacormedia-updates branch October 21, 2019 17:17

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[synacormedia] Add synacormedia adapter #1029

[synacormedia] Add synacormedia adapter #1029

coreykress commented Sep 9, 2019

stale bot commented Sep 17, 2019

guscarreon left a comment

guscarreon Sep 17, 2019

coreykress Oct 7, 2019

guscarreon Oct 9, 2019

stale bot commented Sep 24, 2019

coreykress commented Sep 25, 2019

stale bot commented Oct 2, 2019

coreykress commented Oct 2, 2019

guscarreon Oct 9, 2019

guscarreon Oct 9, 2019

guscarreon Oct 11, 2019

coreykress Oct 14, 2019

guscarreon Oct 11, 2019

coreykress Oct 14, 2019

guscarreon Oct 16, 2019

guscarreon commented Oct 11, 2019

guscarreon left a comment

[synacormedia] Add synacormedia adapter #1029

[synacormedia] Add synacormedia adapter #1029

Conversation

coreykress commented Sep 9, 2019

stale bot commented Sep 17, 2019

guscarreon 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

stale bot commented Sep 24, 2019

coreykress commented Sep 25, 2019

stale bot commented Oct 2, 2019

coreykress commented Oct 2, 2019

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

guscarreon commented Oct 11, 2019

guscarreon left a comment

Choose a reason for hiding this comment