-
Notifications
You must be signed in to change notification settings - Fork 753
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
New Adapter: smartx #3109
New Adapter: smartx #3109
Conversation
Code coverage summaryNote:
smartxRefer here for heat map coverage report
|
Code coverage summaryNote:
smartxRefer here for heat map coverage report
|
adapters/smartx/smartx.go
Outdated
//println("Builder smartx", config.Endpoint) | ||
// initialize the adapter and return it |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nitpick - I don't think you need to have this comment here. Please remove them
adapters/smartx/smartx.go
Outdated
"github.com/prebid/prebid-server/openrtb_ext" | ||
) | ||
|
||
// adapter is a implementation of the adapters.Bidder interface. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit - please remove this comment.
adapters/smartx/smartx.go
Outdated
// MakeRequests prepares the HTTP requests which should be made to fetch bids. | ||
func (a *adapter) MakeRequests(openRTBRequest *openrtb2.BidRequest, requestInfo *adapters.ExtraRequestInfo) (requestsToBidder []*adapters.RequestData, errs []error) { | ||
// parse the requests answer | ||
openRTBRequestJSON, err := json.MarshalIndent(openRTBRequest, "", " ") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a reason to choose MarshalIndent instead of MarshalJson? I think - adding an indentation would slightly take performance hit. Requesting you to please use MarshalJson if possible.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thank you, debug leftover. adjusted
adapters/smartx/smartx.go
Outdated
|
||
// MakeRequests prepares the HTTP requests which should be made to fetch bids. | ||
func (a *adapter) MakeRequests(openRTBRequest *openrtb2.BidRequest, requestInfo *adapters.ExtraRequestInfo) (requestsToBidder []*adapters.RequestData, errs []error) { | ||
// parse the requests answer |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit - please remove this comment.
adapters/smartx/smartx.go
Outdated
}, nil | ||
} | ||
|
||
// MakeRequests prepares the HTTP requests which should be made to fetch bids. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit - please remove this comment.
adapters/smartx/smartx.go
Outdated
// parse the requests answer | ||
openRTBRequestJSON, err := json.MarshalIndent(openRTBRequest, "", " ") | ||
if err != nil { | ||
// can't parse the request, this is an critical error |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit - please remove this comment.
adapters/smartx/smartx.go
Outdated
return nil, errs | ||
} | ||
|
||
// create the HEADER of the request |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit - please remove this comment.
adapters/smartx/smartx.go
Outdated
}), nil | ||
} | ||
|
||
// MakeBids unpacks the server's response into Bids. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit - please remove this comment.
adapters/smartx/smartx.go
Outdated
} | ||
} | ||
|
||
// add the new request to the list |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit - please remove this comment.
adapters/smartx/smartx.go
Outdated
// check HTTP status code 400 - Bad Request | ||
// check HTTP status code NOT 200 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit - please remove this comment.
adapters/smartx/smartx.go
Outdated
return nil, []error{err} | ||
} | ||
|
||
// check HTTP status code 204 - No Content |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit - please remove this comment.
adapters/smartx/smartx.go
Outdated
|
||
var errs []error | ||
|
||
// loop the SeatBids |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit - please remove this comment.
adapters/smartx/smartx.go
Outdated
|
||
// loop the SeatBids | ||
for _, seatBid := range response.SeatBid { | ||
// loop the Bids |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit - please remove this comment.
adapters/smartx/smartx.go
Outdated
for _, seatBid := range response.SeatBid { | ||
// loop the Bids | ||
for i, bid := range seatBid.Bid { | ||
// get the MType of the bid |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit - please remove this comment.
adapters/smartx/smartx.go
Outdated
bidType, err := getMediaTypeForBid(bid) | ||
|
||
if err != nil { | ||
// if an error occures add it to the errors list and continue |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit - please remove this comment.
adapters/smartx/smartx.go
Outdated
continue | ||
} | ||
|
||
// add the response to the list |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit - please remove this comment.
adapters/smartx/smartx.go
Outdated
} | ||
|
||
func getMediaTypeForBid(bid openrtb2.Bid) (openrtb_ext.BidType, error) { | ||
return openrtb_ext.BidTypeVideo, nil |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the smartx adapter supports both video and native. This value should be dervied from openrtb2.Bid.MType
field.
static/bidder-info/smartx.yaml
Outdated
@@ -0,0 +1,14 @@ | |||
endpoint: "https://bid.smartclip.net/bid/1005" | |||
maintainer: | |||
email: "[email protected]" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sent verification email. Please reply with received.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mail received, thanks -- but my colleagues pointed out that this mailbox only has left a limited time of life. They are in the process of setting up a new mailbox to be used here as well as with Smartx's Prebid JS. The change with the new mailbox will be committed on Monday.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lots of unnecessary comment in code. Please remove it. Also suggested few changes.
- removed comments - removed native as its not used currently
Code coverage summaryNote:
smartxRefer here for heat map coverage report
|
Updated email address
endpoint: "https://bid.smartclip.net/bid/1005" | ||
maintainer: | ||
email: "[email protected]" | ||
gvlVendorID: 115 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
verified gvl id
~ % curl https://vendor-list.consensu.org/v2/vendor-list.json | jq '.vendors."115"'
% Total % Received % Xferd Average Speed Time Time Time Current
Dload Upload Total Spent Left Speed
100 425k 100 425k 0 0 114k 0 0:00:03 0:00:03 --:--:-- 114k
{
"id": 115,
"name": "smartclip Europe GmbH",
"purposes": [
1,
2,
3,
4,
7,
10
],
"legIntPurposes": [],
"flexiblePurposes": [
2,
7,
10
],
"specialPurposes": [
1,
2
],
"features": [
2,
3
],
"specialFeatures": [],
"policyUrl": "https://privacy-portal.smartclip.net/en/privacy-policy",
"cookieMaxAgeSeconds": 31536000,
"usesCookies": true,
"cookieRefresh": true,
"usesNonCookieAccess": true,
"deviceStorageDisclosureUrl": "https://cdn.smartclip.net/iab/deviceStorageDisclosure.json"
}
@@ -0,0 +1,12 @@ | |||
endpoint: "https://bid.smartclip.net/bid/1005" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
endpoint is reachable
~ % curl -i https://bid.smartclip.net/bid/1005
HTTP/2 405
server: nginx/1.17.6
date: Thu, 28 Sep 2023 10:30:07 GMT
content-length: 0
via: 1.1 google
alt-svc: h3=":443"; ma=2592000,h3-29=":443"; ma=2592000
@fkoch-sc, can you provide details on endpoint specifically what does 1005
corresponds to?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
1005 is the dedicated endpoint for prebid server in our system. prebid js uses 1000 instead. Helps to keep services separate and tidy.
adapters/smartx/smartx.go
Outdated
func getMediaTypeForBid(bid openrtb2.Bid) (openrtb_ext.BidType, error) { | ||
return openrtb_ext.BidTypeVideo, nil |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As of now, getMediaTypeForBid()
only returns openrtb_ext.BidTypeVideo
. Instead skimpily assign openrtb_ext.BidTypeVideo
as BidType
bidResponse.Bids = append(bidResponse.Bids, &adapters.TypedBid{
Bid: &seatBid.Bid[i],
BidType: openrtb_ext.BidTypeVideo,
})
@fkoch-sc should create docs PR in https://github.com/prebid/prebid.github.io repo. Note that this docs will be used as guideline for publisher to work with smartx adapter. |
Code coverage summaryNote:
smartxRefer here for heat map coverage report
|
- safeguard currency - removed mediaType function as we only support video for now
Code coverage summaryNote:
smartxRefer here for heat map coverage report
|
Tests are failing with following error
|
Code coverage summaryNote:
smartxRefer here for heat map coverage report
|
- sorted smartx alpabetically into bidders - ran gofmt on the smartx adapter - adjusted test url
Code coverage summaryNote:
smartxRefer here for heat map coverage report
|
PR created: prebid/prebid.github.io#4912 |
adapters/smartx/smartx.go
Outdated
if err := adapters.CheckResponseStatusCodeForErrors(responseData); err != nil { | ||
return nil, []error{err} | ||
} | ||
|
||
if adapters.IsResponseStatusCodeNoContent(responseData) { | ||
return nil, nil | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you might wanna reverse this. You should check for no content first before you check CheckResponseStatusCodeForErrors
because CheckResponseStatusCodeForErrors
function will return an error even if response from bidder is 204
which is StatusNoContent. So -
if adapters.IsResponseStatusCodeNoContent(responseData) {
return nil, nil
}
if err := adapters.CheckResponseStatusCodeForErrors(responseData); err != nil {
return nil, []error{err}
}
Code coverage summaryNote:
smartxRefer here for heat map coverage report
|
@fkoch-sc, docs PR is failing please take a look - prebid/prebid.github.io#4912 (comment) |
Co-authored-by: Andreas Schubert <[email protected]> Co-authored-by: schubert-sc <[email protected]>
New adapter for smartx ad server