Skip to content
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

Merged
merged 10 commits into from
Oct 9, 2023
Merged

New Adapter: smartx #3109

merged 10 commits into from
Oct 9, 2023

Conversation

fkoch-sc
Copy link
Contributor

New adapter for smartx ad server

@github-actions
Copy link

Code coverage summary

Note:

  • Prebid team doesn't anticipate tests covering code paths that might result in marshal and unmarshal errors
  • Coverage summary encompasses all commits leading up to the latest one, bc6f73f

smartx

Refer here for heat map coverage report

github.com/prebid/prebid-server/adapters/smartx/smartx.go:20:	Builder			100.0%
github.com/prebid/prebid-server/adapters/smartx/smartx.go:29:	MakeRequests		85.7%
github.com/prebid/prebid-server/adapters/smartx/smartx.go:64:	MakeBids		80.0%
github.com/prebid/prebid-server/adapters/smartx/smartx.go:114:	getMediaTypeForBid	100.0%
total:								(statements)		83.3%

@github-actions
Copy link

Code coverage summary

Note:

  • Prebid team doesn't anticipate tests covering code paths that might result in marshal and unmarshal errors
  • Coverage summary encompasses all commits leading up to the latest one, 9bc44e2

smartx

Refer here for heat map coverage report

github.com/prebid/prebid-server/adapters/smartx/smartx.go:20:	Builder			100.0%
github.com/prebid/prebid-server/adapters/smartx/smartx.go:29:	MakeRequests		85.7%
github.com/prebid/prebid-server/adapters/smartx/smartx.go:64:	MakeBids		80.0%
github.com/prebid/prebid-server/adapters/smartx/smartx.go:114:	getMediaTypeForBid	100.0%
total:								(statements)		83.3%

Comment on lines 21 to 22
//println("Builder smartx", config.Endpoint)
// initialize the adapter and return it
Copy link
Contributor

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

"github.com/prebid/prebid-server/openrtb_ext"
)

// adapter is a implementation of the adapters.Bidder interface.
Copy link
Contributor

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.

// 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, "", " ")
Copy link
Contributor

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.

Copy link
Contributor

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


// 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
Copy link
Contributor

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.

}, nil
}

// MakeRequests prepares the HTTP requests which should be made to fetch bids.
Copy link
Contributor

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.

// parse the requests answer
openRTBRequestJSON, err := json.MarshalIndent(openRTBRequest, "", " ")
if err != nil {
// can't parse the request, this is an critical error
Copy link
Contributor

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.

return nil, errs
}

// create the HEADER of the request
Copy link
Contributor

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.

}), nil
}

// MakeBids unpacks the server's response into Bids.
Copy link
Contributor

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.

}
}

// add the new request to the list
Copy link
Contributor

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.

Comment on lines 65 to 66
// check HTTP status code 400 - Bad Request
// check HTTP status code NOT 200
Copy link
Contributor

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.

return nil, []error{err}
}

// check HTTP status code 204 - No Content
Copy link
Contributor

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.


var errs []error

// loop the SeatBids
Copy link
Contributor

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.


// loop the SeatBids
for _, seatBid := range response.SeatBid {
// loop the Bids
Copy link
Contributor

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.

for _, seatBid := range response.SeatBid {
// loop the Bids
for i, bid := range seatBid.Bid {
// get the MType of the bid
Copy link
Contributor

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.

bidType, err := getMediaTypeForBid(bid)

if err != nil {
// if an error occures add it to the errors list and continue
Copy link
Contributor

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.

continue
}

// add the response to the list
Copy link
Contributor

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.

}

func getMediaTypeForBid(bid openrtb2.Bid) (openrtb_ext.BidType, error) {
return openrtb_ext.BidTypeVideo, nil
Copy link
Contributor

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.

@@ -0,0 +1,14 @@
endpoint: "https://bid.smartclip.net/bid/1005"
maintainer:
email: "adtech@smartclip.tv"
Copy link
Contributor

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.

Copy link
Contributor Author

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.

Copy link
Contributor

@gargcreation1992 gargcreation1992 left a 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.

@bsardo bsardo changed the title new adapter smartx New Adapter: smartx Sep 20, 2023
- removed comments
- removed native as its not used currently
@github-actions
Copy link

Code coverage summary

Note:

  • Prebid team doesn't anticipate tests covering code paths that might result in marshal and unmarshal errors
  • Coverage summary encompasses all commits leading up to the latest one, d15a8f6

smartx

Refer here for heat map coverage report

github.com/prebid/prebid-server/adapters/smartx/smartx.go:19:	Builder			100.0%
github.com/prebid/prebid-server/adapters/smartx/smartx.go:25:	MakeRequests		85.7%
github.com/prebid/prebid-server/adapters/smartx/smartx.go:55:	MakeBids		80.0%
github.com/prebid/prebid-server/adapters/smartx/smartx.go:97:	getMediaTypeForBid	100.0%
total:								(statements)		83.3%

Updated email address
endpoint: "https://bid.smartclip.net/bid/1005"
maintainer:
email: "bidding@smartclip.tv"
gvlVendorID: 115
Copy link
Contributor

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"
Copy link
Contributor

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?

Copy link
Contributor Author

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.

Comment on lines 97 to 98
func getMediaTypeForBid(bid openrtb2.Bid) (openrtb_ext.BidType, error) {
return openrtb_ext.BidTypeVideo, nil
Copy link
Contributor

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,
			})

@onkarvhanumante
Copy link
Contributor

@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.

@github-actions
Copy link

Code coverage summary

Note:

  • Prebid team doesn't anticipate tests covering code paths that might result in marshal and unmarshal errors
  • Coverage summary encompasses all commits leading up to the latest one, 0616f85

smartx

Refer here for heat map coverage report

github.com/prebid/prebid-server/adapters/smartx/smartx.go:19:	Builder			100.0%
github.com/prebid/prebid-server/adapters/smartx/smartx.go:25:	MakeRequests		85.7%
github.com/prebid/prebid-server/adapters/smartx/smartx.go:55:	MakeBids		80.0%
github.com/prebid/prebid-server/adapters/smartx/smartx.go:97:	getMediaTypeForBid	100.0%
total:								(statements)		83.3%

- safeguard currency
- removed mediaType function as we only support video for now
@github-actions
Copy link

Code coverage summary

Note:

  • Prebid team doesn't anticipate tests covering code paths that might result in marshal and unmarshal errors
  • Coverage summary encompasses all commits leading up to the latest one, 7cd6376

smartx

Refer here for heat map coverage report

github.com/prebid/prebid-server/adapters/smartx/smartx.go:19:	Builder		100.0%
github.com/prebid/prebid-server/adapters/smartx/smartx.go:25:	MakeRequests	85.7%
github.com/prebid/prebid-server/adapters/smartx/smartx.go:55:	MakeBids	88.2%
total:								(statements)	87.5%

@onkarvhanumante
Copy link
Contributor

@fkoch-sc

Tests are failing with following error

Run ./validate.sh --nofmt --cov --race 10
gofmt needs to be run, 1 files have issues.  Below is a list of files to review:
adapters/smartx/smartx.go
Error: Process completed with exit code 1.

@github-actions
Copy link

Code coverage summary

Note:

  • Prebid team doesn't anticipate tests covering code paths that might result in marshal and unmarshal errors
  • Coverage summary encompasses all commits leading up to the latest one, 1a66d09

smartx

Refer here for heat map coverage report

github.com/prebid/prebid-server/adapters/smartx/smartx.go:19:	Builder		100.0%
github.com/prebid/prebid-server/adapters/smartx/smartx.go:25:	MakeRequests	85.7%
github.com/prebid/prebid-server/adapters/smartx/smartx.go:55:	MakeBids	88.2%
total:								(statements)	87.5%

- sorted smartx alpabetically into bidders
- ran gofmt on the smartx adapter
- adjusted test url
@github-actions
Copy link

Code coverage summary

Note:

  • Prebid team doesn't anticipate tests covering code paths that might result in marshal and unmarshal errors
  • Coverage summary encompasses all commits leading up to the latest one, 5763e95

smartx

Refer here for heat map coverage report

github.com/prebid/prebid-server/adapters/smartx/smartx.go:19:	Builder		100.0%
github.com/prebid/prebid-server/adapters/smartx/smartx.go:25:	MakeRequests	85.7%
github.com/prebid/prebid-server/adapters/smartx/smartx.go:55:	MakeBids	88.2%
total:								(statements)	87.5%

onkarvhanumante
onkarvhanumante previously approved these changes Oct 3, 2023
@fkoch-sc
Copy link
Contributor Author

fkoch-sc commented Oct 5, 2023

@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.

PR created: prebid/prebid.github.io#4912

Comment on lines 56 to 62
if err := adapters.CheckResponseStatusCodeForErrors(responseData); err != nil {
return nil, []error{err}
}

if adapters.IsResponseStatusCodeNoContent(responseData) {
return nil, nil
}
Copy link
Contributor

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}
}

@github-actions
Copy link

github-actions bot commented Oct 6, 2023

Code coverage summary

Note:

  • Prebid team doesn't anticipate tests covering code paths that might result in marshal and unmarshal errors
  • Coverage summary encompasses all commits leading up to the latest one, 4fb973a

smartx

Refer here for heat map coverage report

github.com/prebid/prebid-server/adapters/smartx/smartx.go:19:	Builder		100.0%
github.com/prebid/prebid-server/adapters/smartx/smartx.go:25:	MakeRequests	85.7%
github.com/prebid/prebid-server/adapters/smartx/smartx.go:55:	MakeBids	88.2%
total:								(statements)	87.5%

@onkarvhanumante
Copy link
Contributor

@fkoch-sc, docs PR is failing please take a look - prebid/prebid.github.io#4912 (comment)

@gargcreation1992 gargcreation1992 merged commit 23bc394 into prebid:master Oct 9, 2023
5 checks passed
svamiftah pushed a commit to sovrn/prebid-server that referenced this pull request Nov 21, 2023
Co-authored-by: Andreas Schubert <as@burgeins.de>
Co-authored-by: schubert-sc <144821265+schubert-sc@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants