-
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
LemmaDigital: add host parameter #3750
Conversation
Code coverage summaryNote:
lemmadigitalRefer here for heat map coverage report
|
"host": { | ||
"type": "string", | ||
"description": "Network Host to request from", | ||
"enum": ["uses", "usws", "usct", "emea", "sg", "doohsg", "doohus"] |
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.
In the case of region specific routing, we recommend the solution of documenting the different endpoints supported by your adapter and hosts can configure their data centers to match. PBS already allows endpoints to be configured via application settings (environment variable or pbs.yaml).
We prefer not asking publishers to enter the region in each one of their placement configs. This is really a server level configuration.
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.
Good catch @SyntaxNode . @lm-ved , please see https://docs.prebid.org/prebid-server/developers/add-new-bidder-go.html#consider-your-geography and the example . Basically, pubs can't reasonably determine the geography.
# We have the following regional endpoint domains: "uses" (us-east), "usws" (us-west), "usct" (us-central), "emea" (europe middle east africa), "sg" (where's this?)
# Please deploy this config in each of your datacenters with the appropriate regional subdomain
endpoint: "http://REGION.example.com/openrtb2"
geoscope:
- USA
- CAN
As for separate endpoints for DOOH vs display, I think that's a problem. @SyntaxNode - can we allow them to create a bidder-specific "regioncode" YAML entry that's filled in by the host company and is available to their runtime code? If so, then the adapter can look at regioncode along with site/app/dooh and determine the endpoint.
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.
@SyntaxNode / @bretg ,
Would I be correct in assuming the following change would suffice in this adapter's case?
# We have the following regional endpoint domains: "uses" (us-east), "usws" (us-west), "usct" (us-central), "emea" (europe middle east africa), "sg" (singapore)
# Please deploy this config in each of your datacenters with the appropriate regional subdomain
endpoint: "https://REGION.ads.lemmatechnologies.com/lemma/servad?src=prebid&pid={{.PublisherID}}&aid={{.AdUnit}}"
geoscope:
- global
instead of the current:
endpoint: "https://{{.Host}}.ads.lemmatechnologies.com/lemma/servad?src=prebid&pid={{.PublisherID}}&aid={{.AdUnit}}"
It also would depend on the decision regarding how to handle the different endpoints for DOOH.
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.
@SyntaxNode - can we allow them to create a bidder-specific "regioncode" YAML entry that's filled in by the host company and is available to their runtime code?
Yup. This is a good use for the extra_info
field. Hosts can override this value in the appropriate data centers just as they would the endpoint. For details, please visit https://docs.prebid.org/prebid-server/developers/add-new-bidder-go.html and search for "extra_info" and "ExtraAdapterInfo".
Would I be correct in assuming the following change would suffice in this adapter's case?
Yes, but please disable this adapter by default (hosts can enable it) since the endpoint is not valid by default. This is the same approach used by the Rubicon/Magnite adapter.
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.
@SyntaxNode , thank you for the details.
I've added the host option as an extra_info
field. I have also kept a default value and removed host from the required
array of bidder-params. This would allow for both - a server-level config and a publisher to provide it if needed, with a default(set to the current adapter's host ) in case it is neither.
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 would allow for both - a server-level config and a publisher to provide it if needed, with a default(set to the current adapter's host ) in case it is neither.
@bretg Are you good with giving publishers an option to set it per-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.
I suppose. Don't expect anyone to get it right.
Code coverage summaryNote:
lemmadigitalRefer here for heat map coverage report
|
"host": { | ||
"type": "string", | ||
"description": "Network Host to request from", | ||
"enum": ["uses", "usws", "usct", "emea", "sg", "doohsg", "doohus"] |
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 would allow for both - a server-level config and a publisher to provide it if needed, with a default(set to the current adapter's host ) in case it is neither.
@bretg Are you good with giving publishers an option to set it per-request?
Code coverage summaryNote:
lemmadigitalRefer here for heat map coverage report
|
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.
My review is for approach only. Did not look at the code.
Hi @SyntaxNode / @bretg / @bsardo , |
Code coverage summaryNote:
lemmadigitalRefer here for heat map coverage report
|
Hi @SyntaxNode / @bretg / @bsardo , |
@lm-ved, I will give this a look shortly. |
@bsardo Could you please review this? It is a very important and business-impacting change for us. Let us know if there are any outstanding action items for approval. Thank you. |
Hi @bsardo / @SyntaxNode / @bretg, |
@lm-ved your other PR has been merged. Let me know when this is ready for review again. |
@lm-ved The prerequisite PR has been merged. You are clear to proceed with this one. If you're unable to revisit this PR at this time, we will close it next week due to inactivity. You are welcome to reopen it anytime after when you're ready. |
@SyntaxNode , thanks for informing. I would like to revisit this PR in the near future so I will re-open it later. Closing it out until then. |
📋 Checklist