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

Prebid Core: Support for multiple bidder codes from a single adapter #8216

Merged

Conversation

pm-azhar-mulla
Copy link
Contributor

@pm-azhar-mulla pm-azhar-mulla commented Mar 25, 2022

Type of change

  • Feature

Description of change

With respect to draft 8129. We have made changes to the bidderFactory.js.
After receiving the bids from adapters, we look for eligible bids depending upon values of allowAlternateBidderCodes and allowedAlternateBidderCodes values in bidderSettings.

  • If allowAlternateBidderCodes is false for given bidder, we log warning on console and ignore the bid.

  • If allowAlternateBidderCodes is true and allowedAlternateBidderCodes is specified in terms of an array, we look for respective bidderCode in that array. If we find the bidderCode we continue otherwise again we log the warning on console and ignore the bid

  • If allowAlternateBidderCodes is true and allowedAlternateBidderCodes is not specified or value for allowedAlternateBidderCodes is ['*'] then we don't apply any restrictions.

After bid is considered as eligible, we add adapterCode property to the bid which represents name of the adapter through which the bid was initiated.

pbjs.bidderSettings = {
    standard: {
         allowAlternateBidderCodes: false,    // the default if not specified for a specific adapter
         [...]
    },
    bidderA: {
        allowAlternateBidderCodes: true,
        allowedAlternateBidderCodes: ["bidderX", "bidderY"], // bidderA may add X or Y if it wishes
        [...]
    },
    bidderB: {
        allowAlternateBidderCodes: true,
        allowedAlternateBidderCodes: ["*"],        // the default allows this bidder to respond with any biddercodes
        [...]
    }
}

@bretg @patmmccann This is an initial draft, we are in process of testing and adding unit test cases. Please share your views on the same.

Checklist for the above feature

  • Make changes in bidderFactory.js
  • Make changes in bidderSetting.js
  • Make related changes in core files

@pm-azhar-mulla pm-azhar-mulla changed the title Support for unknown bidders Prebid Core : Support for unknown bidders Mar 25, 2022
@pm-azhar-mulla pm-azhar-mulla changed the title Prebid Core : Support for unknown bidders WIP Prebid Core : Support for unknown bidders Mar 25, 2022
@lgtm-com
Copy link

lgtm-com bot commented Mar 25, 2022

This pull request introduces 1 alert when merging eaa0603 into 3fbea52 - view on LGTM.com

new alerts:

  • 1 for Useless conditional

@patmmccann patmmccann linked an issue Mar 25, 2022 that may be closed by this pull request
@patmmccann patmmccann added needs 2nd review Core module updates require two approvals from the core team CORE HIGH IMPACT feature labels Mar 25, 2022
@pm-harshad-mane pm-harshad-mane changed the title WIP Prebid Core : Support for unknown bidders Work In Progress: Prebid Core : Support for unknown bidders Mar 25, 2022
@@ -235,6 +236,12 @@ export function newBidder(spec) {
onBid: (bid) => {
const bidRequest = bidRequestMap[bid.requestId];
if (bidRequest) {
if (isUnknownBidder(bid.bidderCode, bidRequest.bidder)) {
logWarn(bid.bidderCode + ' is not a registered partner or known bidder. Continuing without bids.');
Copy link
Contributor

Choose a reason for hiding this comment

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

In the warning, please mention the original bidder name, inform that the current bid will not be accepted, and also mention that allowUnknownBidderCodes should be true in bidder settings to allow passing such bids.

@pm-harshad-mane
Copy link
Contributor

An Org can be seen in Prebid as,

  • an original registered bidder, example pubmatic-bidder, pubmatic
  • a registered alias, example alias pubmatic-2
  • alternate bidder, example GroupM, groupm

I would like to keep things simple.
For a publisher, it will be difficult to define the bidder settings. The pub will need to know which role an Org has chosen.
Irrespective of the role an org has chosen from the above-mentioned roles, if the specific bidder setting is present for the org name then that should be applied, otherwise, the standard bidder setting should be used.
If a pub wants to apply partial or full bidder settings of pubmatic to groupm then it can be done by defining the config in JS variables and using the variables appropriately.

@dgirardi
Copy link
Collaborator

@pm-harshad-mane, I'll see if we can re-discuss the requirements again. If we agree on reducing the scope, I'll come back to ask to revert the changes to bidderSettings, since they would also no longer be necessary then.

@mike-chowla
Copy link
Contributor

My preference is keep things as simple as possible for the user to understand. It feels complicated to me for the user to understand that there's a relationship between bidder codes that can cause settings such as bidCpmAdjustment to be applied from bidder code to another, especially when it doesn't happen other circumstances where there's a relationship.

If I create an alias of bidderA called bidderB, I have to create bidCpmAdjustment for bidderB is if I want to adjust the bid CPM for bidderB.

Seems to me the simplest way conceptually to handle bids with a different bidder code is to only apply bid settings for the bidder code attached to the bid. Targeting key settings should work the same way. If you want a custom targeting key for bidderB, you need to set it on bidderB

@dgirardi
Copy link
Collaborator

@pm-azhar-mulla, if you revert the changes to bidderSettings (to remove the parentScope logic that we are not using) I'll approve this PR; hopefully by the time the second review completes we'll all have agreed that we don't need that.

The missing s2sConfig piece I'm not sure about, but it looks to me like something that could be added later.

@pm-azhar-mulla
Copy link
Contributor Author

Hi @dgirardi,
I have reverted the changes for bidderSetting's parentScope

Copy link
Collaborator

@dgirardi dgirardi left a comment

Choose a reason for hiding this comment

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

LGTM, with the caveats above - we are reducing the scope to just enforcement of allowAlternate bidderSettings config, leaving out proposed changes to adapterCode fallbacks and the PBS adapter.

@patmmccann
Copy link
Collaborator

@pm-harshad-mane we need your second review and we need docs

@patmmccann patmmccann changed the title Work In Progress: Prebid Core : Support for unknown bidders Prebid Core : Support for unknown bidders Apr 21, 2022
@patmmccann patmmccann changed the title Prebid Core : Support for unknown bidders Prebid Core: Support for multiple bidder codes from a single adapter Apr 21, 2022
@pm-harshad-mane
Copy link
Contributor

@pm-azhar-mulla , Please link the documentation PR to this PR.

Let's say pubmatic is configured on the server-side in PBS, will the pubmatic bidder will get to know the config values for allowedAlternateBidderCodes? I think we should pass the newly added config params to PBS bidders as well. Create a separate PR if many changes are needed.

@dgirardi
Copy link
Collaborator

@pm-harshad-mane, Prebid Server is not involved at all here - I don't understand what you mean. This affects exclusively client-side adapters whose backends want to bid on behalf of other bidder codes.

For PBS, there's a separate s2sConfig.allowUnknownBidderCodes flag that allows it to bid with any bidder. But the way you'd use that (as an SSP) would be entirely different - you'd ask clients to use a PBS instance that bids that way (I am not sure how that'd be implemented server-side). There would be no bidderSettings and no bidder code list.

@patmmccann
Copy link
Collaborator

if (!s2sConfig.allowUnknownBidderCodes) {
for reference

Copy link
Contributor

@pm-harshad-mane pm-harshad-mane left a comment

Choose a reason for hiding this comment

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

LGTM

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CORE HIGH IMPACT feature needs docs needs 2nd review Core module updates require two approvals from the core team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Proposal - expand "extra" bid support
6 participants