-
Notifications
You must be signed in to change notification settings - Fork 762
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
Bizzclick: rename bizzclick to blasto #3688
Conversation
Code coverage summaryNote:
blastoRefer here for heat map coverage report
|
Code coverage summaryNote:
blastoRefer here for heat map coverage report
|
Consider adding prebid-server/exchange/adapter_util.go Lines 106 to 123 in ec729e6
|
static/bidder-info/blasto.yaml
Outdated
endpoint: 'http://{{.Host}}.bizzclick.com/bid?rtb_seat_id={{.SourceId}}&secret_key={{.AccountID}}' | ||
endpoint: 'http://{{.Host}}.blasto.ai/bid?rtb_seat_id={{.SourceId}}&secret_key={{.AccountID}}' |
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.
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.
I understand, do you have an alternative solution for such cases?
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.
pass host
as query string if you would like 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.
@gargcreation1992 Do you mean like this http://blasto.ai/bid?rtb_seat_id={{.SourceId}}&secret_key={{.AccountID}}&host={{.Host}}
?
I don't think we can do this, we have three regions with this subdomains:
us-e-node1 - us east, n1 - eu, n3 - apac. So this is the easiest way to do it, just to make subdomain dynamic
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 understand, do you have an alternative solution for such cases?
In the case of region specific routing, we recommend the solution of documenting in the docs PR 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 don't like 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.
@SyntaxNode Got it! Thus by default in blasto.yaml file I can define such endpoint with us east region
# We have the following regional endpoint sub-domains:
# US East: us-e-node1
# EU: n1
# APAC: n3
# Please deploy this config in each of your datacenters with the appropriate regional subdomain
endpoint: 'http://us-e-node1.blasto.ai/bid?rtb_seat_id={{.SourceId}}&secret_key={{.AccountID}}'
And then pbs.yaml can be modified accordingly to the region? For example:
adapters:
blasto:
endpoint: http://n1.blasto.ai/bid?rtb_seat_id={{.SourceId}}&secret_key={{.AccountID}}
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.
Yes, exactly. You're also welcomed to disable the adapter by default with a placeholder endpoint like in the Rubicon adapter:
# Contact [email protected] to ask about enabling a connection to the Magnite (formerly Rubicon) exchange.
# We have the following regional endpoint domains: exapi-us-east, exapi-us-west, exapi-apac, exapi-eu
# Please deploy this config in each of your datacenters with the appropriate regional subdomain
endpoint: "https://REGION.rubiconproject.com/a/api/exchange"
disabled: true
Either are good approaches IMHO.
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.
fixed
Code coverage summaryNote:
blastoRefer here for heat map coverage report
|
Code coverage summaryNote:
blastoRefer here for heat map coverage report
|
Code coverage summaryNote:
blastoRefer here for heat map coverage report
|
@BizzClick There was a comment on a similar PR recently where @bretg strongly felt the bizzclick name should rename active for a transition period unless there is literally zero current traffic. Is this the case for you? Does bizzclick currently receive no traffic? |
@SyntaxNode @bretg yes, bizzclick now has no traffic from pbs. Should i use |
@BizzClick - yes, that is the guideline. Adding an alias differs in PBS-Go and PBS-Java. See
If you are 110% sure that not a single Prebid Server host company is using your adapter, then ok. But if anyone is using bizzclick, changing the name is going to break them when they upgrade and then people are going to get mad at Prebid for you changing your name. There are probably at least 50 companies using Prebid Server... how can you be sure that no one has enabled your adapter? |
@bretg i'm 110% sure about that and it was considered before renaming. |
Very well. |
LGTM except for the YAML file update with respect to regional endpoints. We would also need to see another docs PR opened to capture the regional endpoint details. |
Code coverage summaryNote:
blastoRefer here for heat map coverage report
|
@bsardo sure, here's related PR |
Bizzclick has a big update, we are officially changing our name to blasto.
Related PR to prebid.github.io