-
Notifications
You must be signed in to change notification settings - Fork 750
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
Configurable sync URLs #763
Merged
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
dbemiller
added
the
breaking change - hosts
Has breaking changes for companies that host Prebid Server
label
Dec 7, 2018
TravisCI build is passing... not sure why the Github UI is showing them as in-progress. |
Closed
hhhjort
previously approved these changes
Dec 14, 2018
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
hhhjort
approved these changes
Jan 7, 2019
katsuo5
pushed a commit
to flux-dev-team/prebid-server-1
that referenced
this pull request
Dec 1, 2020
* Added a config property for base usersync url. Updated the syncers to use it. * Added tests for the new config. * Made sync URLs fully configurable. * Updated the gumgum syncer, and fixed some broken tests. * Handled the errors. * Cleaned up a test. * Polished the grid syncer test a bit. * Fixed a bug related to bidder ID string cases.
katsuo5
pushed a commit
to flux-dev-team/prebid-server-1
that referenced
this pull request
Dec 2, 2020
* Added a config property for base usersync url. Updated the syncers to use it. * Added tests for the new config. * Made sync URLs fully configurable. * Updated the gumgum syncer, and fixed some broken tests. * Handled the errors. * Cleaned up a test. * Polished the grid syncer test a bit. * Fixed a bug related to bidder ID string cases.
katsuo5
pushed a commit
to flux-dev-team/prebid-server-1
that referenced
this pull request
Dec 4, 2020
* Added a config property for base usersync url. Updated the syncers to use it. * Added tests for the new config. * Made sync URLs fully configurable. * Updated the gumgum syncer, and fixed some broken tests. * Handled the errors. * Cleaned up a test. * Polished the grid syncer test a bit. * Fixed a bug related to bidder ID string cases.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This would be a breaking change for Prebid Server host companies. It's an alternative to #753 that I think puts the project in a better place moving forward.
How may this break things?
If merged, Prebid Server host companies will need to update any
config.adapters.{bidder}.usersync_url
config parameters that they define. If you haven't defined any of these, you have nothing to update. If you do define them, keep reading.If you deploy with any
config.adapters.{bidder}.usersync_url
values defined, you must:{{gdpr}}
with{{.GDPR}}
.{{gdpr_consent}}
with{{.GDPRConsent}}
./cookie_sync
is called. Before these changes, some bidders only interpreted these changes as config values as "URL fragments." After these changes, all Bidders will expect the full URL.What does this improve?
A few things.
First: It makes Prebid Server config more flexible so that it's easier to run in test environments (e.g. load & integration tests). The config values can be pointed to mock servers without those servers having to be updated whenever a Bidder's actual sync URL changes.
Second: We've been having some bugs with AMP due to invalid usersync URLs. Publishers are seeing these in the terminal and get worried. Most recently, these are coming from some code in the 33across usersyncer. We're looking for ways to minimize these errors. To catch this most recent bug today, we would have to write custom validation logic for this Bidder (
config.adapters.33across.partner_id
is a required config value, butconfig.adapters.{everyone-else}.partner_id
is not). After these changes, it'll be easy to implement some simpleisValidURL()
validation logic that works across all the bidders.Third: As noted above, that
partner_id
is specific to33across
. This was a totally legitimate addition to the project, given how their sync URL works. However, it also means they had to make changes to the PBS config in order to contribute their Bidder to the project. With the whole URL configurable, it would have been easier for them.Fourth: It takes a major step towards an architecture that doesn't require Usersyncers for Bidders at all. After these changes, every Usersyncer in the project looks like:
In a future change, we can move
vendorConsentID
andsyncType
into the bidder-info files and remove Usersyncers from the code completely. This would farther simplify things and make it easier to contribute Bidders to the project.Fifth: It lets us use Golang Templates to populate GDPR info rather than having us implement our own.
This also fixes #509 by making all the defaults use
https
.