Axonix: enabled by default #1936
Conversation
static/bidder-params/axonix.json
Outdated
| "type": "string", | ||
| "minLength": 1, | ||
| "description": "Datacenter zone" | ||
| }, |
There was a problem hiding this comment.
I'm curios why you are adding the region to the bidder parameters. The common solution on the server side is to instruct Prebid Server hosts via your bidder docs to set different endpoints in their server regions to ensure the shortest routing path. This is different than the approach used in Prebid.js.
There was a problem hiding this comment.
Yeah... I am kind of lost with this tbh.
Looking at the EndpointTemplateParams struct and how some other adapters use it I figured out the usual way is to set a template endpoint and fill the macros with whatever variable parts the endpoint might need. Kind of like algorix.
But the values of these macros come from the bidder parameters, right? Should I use a totally different approach?
There was a problem hiding this comment.
I've simplified how the endpoint is resolved using Host instead of region as it is more flexible.
There was a problem hiding this comment.
Sorry the delayed response. I've been busy with the large amount of PRs open and with summer holidays.
You have the correct approach for using macros, however macros should not be used in the host for geo regions and the entire host may not be supplied as a bidder parameter. That's an anti-pattern which we as a Prebid Server committee strongly oppose. It's odd to ask the publisher to specify the server address of the bidder, it greatly harms Prebid Server's performance which is based on keep alive connections to bidders, and allowing the publisher to pass the host through opens up PBS to a variety of attacks.
I am aware of some bidders doing this today. We're in talks with them to correct their behavior and we're focused on preventing this design from propagating further.
I recommend using a default endpoint pointing to your company's global load balancer. If you don't have a global load balancer, consider marking the adapter as disabled by default and add instructions in your bidder docs to let hosts know which endpoints to use for their geo regions, or ask them to contact you directly to setup if you don't want to publicly post those endpoints. This is the preferred approach for geo region configurations.
There was a problem hiding this comment.
Thank you for the explanation, I've changed it so that only the path uses macros.
adapters/axonix/axonix.go
Outdated
| } | ||
| return bidder, nil | ||
| } | ||
|
|
||
| func (a *adapter) getEndPoint(ext *openrtb_ext.ExtImpAxonix) (string, error) { |
There was a problem hiding this comment.
Nitpick: P in getEndPoint doesn't need to be capital. Please consider changing it to a lower-case p
| thisURI = "https://openrtb-us-east-1.axonix.com/supply/prebid-server/" + axonixExt.SupplyId | ||
| endpoint, err := a.getEndpoint(&axonixExt) | ||
| if err != nil { | ||
| return nil, errors |
There was a problem hiding this comment.
Please add the err from this call in the errors slice before returning it:
endpoint, err := a.getEndpoint(&axonixExt)
if err != nil {
errors = append(errors, err)
return nil, errors
}
* 'master' of https://github.com/wwwyyy/prebid-server: Axonix: enabled by default (prebid#1936) IX: update required site id field to be more flexible (prebid#1934) New adapter: Adagio (prebid#1907) Smaato: Split multiple media types (prebid#1930) Fix CVE-2020-35381 (prebid#1942) Tappx: new bidder params (prebid#1931) InMobi: adding native support (prebid#1928) Admixer: Fix for bid floor issue#1787 (prebid#1872)
Axonix adapter is now live.