-
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
Adhese: Switch to openrtb2 endpoint #3864
base: master
Are you sure you want to change the base?
Conversation
if err != nil { | ||
return nil, []error{err, WrapServerError(fmt.Sprintf("Could not parse Price %v as float ", string(adheseBid.Extension.Prebid.Cpm.Amount)))} | ||
func inferBidTypeFromImp(i openrtb2.Imp) (openrtb_ext.BidType, []error) { | ||
if i.Banner != 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.
Consider this as a suggestion. The current implementation follows an anti-pattern, assumes that if there is a multi-format request, the media type defaults to openrtb_ext.BidTypeBanner, nil. Prebid server expects the media type to be explicitly set in the adapter response. Therefore, we strongly recommend implementing a pattern where the adapter server sets the MType field in the response to accurately determine the media type for the impression.
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.
@mefjush any feedback on this
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.
We've opened a PR where we document it as a limitation that multiple formats aren't supported.
https://github.com/prebid/prebid.github.io/pull/5618/files
Code coverage summaryNote:
adheseRefer here for heat map coverage report
|
@SyntaxNode It got delayed a bit because of some internal testing with the new adapter, the techlead from Adhese has re-opened the PR. Sorry for the late reply! |
@@ -1,4 +1,4 @@ | |||
endpoint: "https://ads-{{.AccountID}}.adhese.com/json" | |||
endpoint: "https://ads-{{.AccountID}}.adhese.com/openrtb2" |
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.
@mefjush endpoint is not reachable
curl -i --location --request POST 'https://ads-1.adhese.com/openrtb2'
curl: (60) SSL certificate problem: self signed certificate
More details here: https://curl.se/docs/sslcerts.html
curl failed to verify the legitimacy of the server and therefore could not
establish a secure connection to it. To learn more about this situation and
how to fix it, please visit the web page mentioned above.
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.
This is related to the other concern you already shared. We require the users of the adapter to specify the AccountID
so it matches their account name in Adhese. An example endpoint that could be reached is https://ads-demo.adhese.com/openrtb2
- that's coupled with demo
account in our system.
Can we do any better to make this requirement more obvious to the user?
Account string `json:"account"` | ||
Location string `json:"location"` | ||
Format string `json:"format"` | ||
Targets map[string][]string `json:"targets,omitempty"` |
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.
@mefjush should update bidder docs to add description for Targets
ext param
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 field is called data
for pbjs, but it got renamed to targets
in both the server-side adapters. I've added a column in the documentation where we specify the pbs ext name next to the pbjs configuration param name.
https://github.com/prebid/prebid.github.io/pull/5618/files
adapters/adhese/adhese.go
Outdated
if responseData.StatusCode == http.StatusNoContent { | ||
return nil, nil | ||
} | ||
|
||
if responseData.StatusCode == http.StatusBadRequest { | ||
err := &errortypes.BadInput{ | ||
Message: "Unexpected status code: 400. Bad request from publisher. Run with request.debug = 1 for more info.", | ||
} | ||
return nil, []error{err} | ||
} | ||
return bidderResponse, errs | ||
} | ||
|
||
func convertAdheseBid(adheseBid AdheseBid, adheseExt AdheseExt, adheseOriginData AdheseOriginData) openrtb2.BidResponse { | ||
adheseExtJson, err := json.Marshal(adheseOriginData) | ||
if err != nil { | ||
adheseExtJson = make([]byte, 0) | ||
if responseData.StatusCode != http.StatusOK { | ||
err := &errortypes.BadServerResponse{ | ||
Message: fmt.Sprintf("Unexpected status code: %d. Run with request.debug = 1 for more info.", responseData.StatusCode), | ||
} | ||
return nil, []error{err} | ||
} |
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.
could make use of response util here
prebid-server/adapters/response.go
Lines 1 to 28 in ec729e6
package adapters | |
import ( | |
"fmt" | |
"net/http" | |
"github.com/prebid/prebid-server/v2/errortypes" | |
) | |
func CheckResponseStatusCodeForErrors(response *ResponseData) error { | |
if response.StatusCode == http.StatusBadRequest { | |
return &errortypes.BadInput{ | |
Message: fmt.Sprintf("Unexpected status code: %d. Run with request.debug = 1 for more info", response.StatusCode), | |
} | |
} | |
if response.StatusCode != http.StatusOK { | |
return &errortypes.BadServerResponse{ | |
Message: fmt.Sprintf("Unexpected status code: %d. Run with request.debug = 1 for more info", response.StatusCode), | |
} | |
} | |
return nil | |
} | |
func IsResponseStatusCodeNoContent(response *ResponseData) bool { | |
return response.StatusCode == http.StatusNoContent | |
} |
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.
done!
adapters/adhese/adhese.go
Outdated
} | ||
// create a new bidResponse with a capacity of 1 because we only expect 1 bid | ||
bidResponse := adapters.NewBidderResponseWithBidsCapacity(1) | ||
bidResponse.Currency = response.Cur |
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.
adapters.NewBidderResponseWithBidsCapacity
initialises bidResponse.Currency
with USD
value
consider scenario where bidResponse.Currency
is empty, assigning response.Cur
directly may set bidResponse.Currency
to empty value
consider implementing following changes:
bidResponse := adapters.NewBidderResponseWithBidsCapacity(len(response.SeatBid[0].Bid))
if response.Cur != "" {
bidResponse.Currency = response.Cur
}
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.
done!
@@ -1,4 +1,4 @@ | |||
endpoint: "https://ads-{{.AccountID}}.adhese.com/json" | |||
endpoint: "https://ads-{{.AccountID}}.adhese.com/openrtb2" |
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.
consider that usage of partial dynamic urls is being discouraged and prebid team in future will be working to avoid usage of dynamic subdomain in endpoint url.
Major concerns with such usage are,
- security concerns
The security aspect is alleviated by using a fixed top level domain. Due to the potential harm to hosts, we are strict in this requirement. We are working towards fixing the few adapters currently in violation. - connection performance
The connection performance advice is for your benefit. Client specific subdomains prevent Prebid Server from reusing connections across your clients which results in more connections needed for your adapter. The issue gets worse the more successful you are in attracting new clients. Hosts may choose to disable your adapter due to this behavior.
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.
Can we address this concern separately from this PR?
Historically we've been always assigning individual domains to each tenant in out system, eg. ads-demo.adhese.com
for demo
account.
To change this approach we'd need to introduce a few architectural changes to our services. I'm happy to plan them in, but we'd rather move forward with the new adapter before the new endpoint approach is implemented.
Code coverage summaryNote:
adheseRefer here for heat map coverage report
|
Code coverage summaryNote:
adheseRefer here for heat map coverage report
|
* change endpoint * initial setup rewrite to POST endpoint * modify MakeBids and MakeRequests * change the name from Keywords to Targets so it matches json * tests --------- Co-authored-by: skonky <[email protected]> Co-authored-by: frank <[email protected]>
0e97d54
to
8f5225f
Compare
Code coverage summaryNote:
adheseRefer here for heat map coverage report
|
Hi @onkarvhanumante I think we've addressed (or replied to) all the comments in the PR. Could we have another round of review please? |
Code coverage summaryNote:
adheseRefer here for heat map coverage report
|
Code coverage summaryNote:
adheseRefer here for heat map coverage report
|
Code coverage summaryNote:
adheseRefer here for heat map coverage report
|
@SyntaxNode @bsardo Thanks alot for reviewing, I have implemented your suggestions and feedback. Is there anything else you want to be changed or are we OK to merge now :)? |
Not for this PR, but so that you know. |
@hhhjort |
bump 👀 |
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.
LGTM
What do you think @SyntaxNode @bsardo @onkarvhanumante @Sonali-More-Xandr |
Think we can merge this before the EOY ? |
I think so. @VeronikaSolovei9 is reviewing it. |
adapters/adhese/adhese.go
Outdated
if err := jsonutil.Unmarshal(request.User.Ext, &extUser); err == nil { | ||
return "/xt" + extUser.Consent | ||
} | ||
if params.Account == "" || params.Location == "" { |
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.
Can you please double check? This if still present in the adapter code.
Also it doesn't match the properties setup in in adhere.json: "required": ["account", "location", "format"]
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.
i have removed the check, it is indeed already handled by the adhese.json
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.
@VeronikaSolovei9 Could you check again?
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.
@VeronikaSolovei9 Could you check again?
Code coverage summaryNote:
adheseRefer here for heat map coverage report
|
if len(request.Imp) == 0 { | ||
return nil, []error{&errortypes.BadServerResponse{ | ||
Message: "No impression in the bid 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.
This is handled by PBS core, you don't need to check this. Request will never reach the adapter if there are no impressions.
Seat: "", | ||
}}, | ||
// create a new bidResponse with a capacity of 1 because we only expect 1 bid | ||
bidResponse := adapters.NewBidderResponseWithBidsCapacity(len(response.SeatBid[0].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.
When I run the request to Adhese I see a NPE in logs when response is empty and doesn't have any bids back. Please take a look at this expression: response.SeatBid[0].Bid
.
It should follow after the check if bid response has any bids, you have it already starting line 158:
if len(bidResponse.SeatBid) == 0 {
return nil, []error{&errortypes.BadServerResponse{
Message: "Empty SeatBid array",
}}
}
if len(imp.Ext) == 0 { | ||
return nil, []error{&errortypes.BadInput{Message: "imp.Ext is empty"}} |
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.
This check is not needed, it is handled by PBS core
if len(bidderExt.Bidder) == 0 { | ||
return nil, []error{&errortypes.BadInput{Message: "bidderExt.Bidder is empty"}} |
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.
This check is not needed as well, it is handled by PBS core
Method: "POST", | ||
Uri: endpoint, | ||
Body: requestJSON, | ||
ImpIDs: openrtb_ext.GetImpIDs(request.Imp), |
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.
If you only take first imp you probably don't need all ids.
var extArray []AdheseExt | ||
var originDataArray []AdheseOriginData |
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.
adapters/adhese/ directory still has utils.go
file where this data structs are defined. I didn't see any usages for these structs in code. May you please delete utils.go
file?
@@ -31,8 +31,8 @@ | |||
"expectedBidResponses": [], | |||
"expectedMakeRequestsErrors": [ | |||
{ | |||
"value": "Request could not be parsed as ExtImpAdhese due to: expect { or n, but found", |
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 don't need to test this. Please delete the check(if len(imp.Ext) == 0, I left the comment) , and this test case.
{ | ||
"value": "Request could not be parsed as ExtImpBidder due to: expect { or n, but found", |
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.
Please delete this test case as well
Switch the Adhese adapter from
/json
to/openrtb2
endpoint. This is to keep our go adapter in line with with prebid-server-java Adhese adapter.