Skip to content
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

MgidX Bid Adapter: add optional region param #4932

Merged
merged 24 commits into from
Mar 18, 2024
Merged

MgidX Bid Adapter: add optional region param #4932

merged 24 commits into from
Mar 18, 2024

Conversation

xmgiddev
Copy link
Contributor

🏷 Type of documentation

  • new bid adapter
  • update bid adapter
  • new feature
  • text edit only (wording, typos)
  • bugfix (code examples)
  • new examples

📋 Checklist

  • Related pull requests in prebid.js or server are linked -> Paste link in this list or reference it on the PR itself
  • For new adapters check submitting your adapter docs

@netlify
Copy link

netlify bot commented Oct 16, 2023

Deploy Preview for prebid-docs-preview ready!

Name Link
🔨 Latest commit 7a40fea
🔍 Latest deploy log https://app.netlify.com/sites/prebid-docs-preview/deploys/65f813f5046ca7000894e33c
😎 Deploy Preview https://deploy-preview-4932--prebid-docs-preview.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

Copy link
Contributor

@bretg bretg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copying the comment I made over in the PBS PR


Sorry, but requiring users to enter a region is not acceptable.

I'll write up the solution in a more formal way but here's a summary:

  • document for host companies that you request regional endpoints
  • when they deploy your adapter's YAML file, they are responsible for using an appropriate regional endpoint

It is unrealistic to expect that multinational publishers can sort of which endpoint a given host should use. This places a heavy (or impossible) burden on the implementation of websites.

It's ok if host is an optional parameter for host companies that cannot support deploying region-specific endpoints.

@xmgiddev xmgiddev changed the title MgidX Bid Adapter: add new required param MgidX Bid Adapter: add optional region param Jan 3, 2024
@xmgiddev
Copy link
Contributor Author

xmgiddev commented Jan 3, 2024

@bretg Sorry for not answering for a long time. We have changed the required HOST param to the optional REGION param

@xmgiddev xmgiddev requested a review from bretg March 5, 2024 16:01
Copy link
Contributor

@bretg bretg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, but there's still a problem here. Publishers cannot determine the region on a per-request basis. So it's good that you made region an optional parameter, but the text underneath says "you must enable ... and set the endpoint... "

And this publisher-facing doc now conflicts with the Prebid Server host company documentation at https://github.com/prebid/prebid-server/blob/master/static/bidder-info/mgidX.yaml -- THIS is the right way to do this: Prebid Server host companies can set the correct endpoint based on their datacenters.

I suggest dropping the publisher-facing region parameter entirely.

@xmgiddev
Copy link
Contributor Author

Sorry, but there's still a problem here. Publishers cannot determine the region on a per-request basis. So it's good that you made region an optional parameter, but the text underneath says "you must enable ... and set the endpoint... "

And this publisher-facing doc now conflicts with the Prebid Server host company documentation at https://github.com/prebid/prebid-server/blob/master/static/bidder-info/mgidX.yaml -- THIS is the right way to do this: Prebid Server host companies can set the correct endpoint based on their datacenters.

I suggest dropping the publisher-facing region parameter entirely.

"you must enable ... and set the endpoint... " - This is a setting for the prebid-server host. And this is a normal approach to solving the problem of multi-regionality, based on your documentation.

The "region" parameter is made for Prebid.JS. And it is optional.

In addition, both adapters are already merged and are already present in releases. Tell me if I'm wrong.

@xmgiddev xmgiddev requested a review from bretg March 14, 2024 08:56
@bretg
Copy link
Contributor

bretg commented Mar 14, 2024

@xmgiddev - here's the issue... this text does not belong in this publisher facing document... they do not read YAML files. It's not the publisher that enables the adapter.

For the prebid server, you must enable the adapter and set the endpoint based on your geographical region. Please refer to the file static/bidder-info/mgidX.yaml for details.

Drop this line and the doc is good to merge.

If you want to leave instructions for PBS host companies, you can open a PR against https://github.com/prebid/prebid.github.io/blob/master/prebid-server/hosting/bidder-specific-guidelines.md

@xmgiddev
Copy link
Contributor Author

@xmgiddev - here's the issue... this text does not belong in this publisher facing document... they do not read YAML files. It's not the publisher that enables the adapter.

For the prebid server, you must enable the adapter and set the endpoint based on your geographical region. Please refer to the file static/bidder-info/mgidX.yaml for details.

Drop this line and the doc is good to merge.

If you want to leave instructions for PBS host companies, you can open a PR against https://github.com/prebid/prebid.github.io/blob/master/prebid-server/hosting/bidder-specific-guidelines.md

thx, dropped =)

@bretg bretg merged commit 0c4b9cd into prebid:master Mar 18, 2024
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants