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

Support adapters to set seat for a bid #2257

Conversation

pm-nilesh-chate
Copy link
Contributor

@pm-nilesh-chate pm-nilesh-chate commented May 26, 2022

Implements #2174

  • Populate new field seatbid.bid.ext.prebid.meta.adaptercode with the adapter code.
  • Allow adapters to set seat of bid(s).
  • PubMatic sets seat for bids with value from bid.ext.marketplace.
  • update existing tests
    • assert bid seat for all adapters.
    • update requestBid() mock to include seat and adaptercode.
  • add new tests
    • add exchange test to validate extra bids under new seat.
    • add exchange test to validate extra bids and their adaptercode under new seat of an alias bidder.
    • add test to assert requestBid() changes.
    • Add pubmatic adapter test to assert extra-bid and a new response ext field marketplace.
  • Validate extra-bidders as per phase-2 description.
  • Do bid adjustment for extra-bid as per phase-2 description.

@pm-nilesh-chate pm-nilesh-chate force-pushed the feature-2174-add-extra-bid-1 branch from 8bf979b to 8cd19b3 Compare May 26, 2022 09:10
@pm-nilesh-chate pm-nilesh-chate changed the title Support adapters to set bid seat #2174 Support adapters to set bid seat May 26, 2022
@pm-nilesh-chate pm-nilesh-chate changed the title Support adapters to set bid seat Support adapters to set seat for a bid May 26, 2022
@@ -667,12 +675,12 @@ func (e *exchange) buildBidResponse(ctx context.Context, liveAdapters []openrtb_
// Create the SeatBids. We use a zero sized slice so that we can append non-zero seat bids, and not include seatBid
// objects for seatBids without any bids. Preallocate the max possible size to avoid reallocating the array as we go.
seatBids := make([]openrtb2.SeatBid, 0, len(liveAdapters))
for _, a := range liveAdapters {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Cannot loop over liveAdapters as now, getAllBids() could bring extra bidders

// Do we need to address duplicate names here?. What if different BidderNames have exta bids with same seat value.
// Ex. 'pubmatic' adapter has extra bids under new 'groupm' seat and 'appnexus' also comes with additional bids under seat 'groupm'
// Above example might be more relatable with 'allowUnknownBidderCodes'
adapterBids[openrtb_ext.BidderName(seatBid.seat)] = seatBid
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do we need to address duplicate names here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do we clubs the bids in case multiple bidders returns the same alternate bidder code?. Ex. appnexus and Pubmatic returning groupm as a new bidder

@bretg thoughts?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

updated code to append the bids

@pm-nilesh-chate pm-nilesh-chate marked this pull request as ready for review May 27, 2022 12:25
@mansinahar mansinahar requested review from bsardo and hhhjort May 31, 2022 17:33
@pm-nilesh-chate
Copy link
Contributor Author

Implemented validation in a separate PR PubMatic-OpenWrap#309 against this branch

@hhhjort
Copy link
Collaborator

hhhjort commented Jun 2, 2022

I think we don't want to merge and release this without phase 2, as that will give PBS hosts some control over what adapters are allowed to do. Or perhaps include an off switch will prevent adapters from being able to rewrite the seat in seat bids. I can see a potential issue that downstream consumers could be caught off guard on assuming a one to one relation between seat and bidder, and this breaking some logic that depends on that.

As an example, Pub A is using Pubmatic and thus has a relation to them. It is not guaranteed that Pub A has a separate relationship with GroupM. Pub A may have some client logic to generate billing reports based on the seat names of the seat bid. This would result in billing reports for "GroupM" that Pub A does not know what to do with, since they don't have a direct relation there. Pub A can get the information from the adaptercode meta, but that will require them retooling their scripts to account for this new definition of seat. We can't require all publishers to retool their script in lock step with a new PBS release.

@pm-nilesh-chate
Copy link
Contributor Author

I think we don't want to merge and release this without phase 2, as that will give PBS hosts some control over what adapters are allowed to do. Or perhaps include an off switch will prevent adapters from being able to rewrite the seat in seat bids. I can see a potential issue that downstream consumers could be caught off guard on assuming a one to one relation between seat and bidder, and this breaking some logic that depends on that.

As an example, Pub A is using Pubmatic and thus has a relation to them. It is not guaranteed that Pub A has a separate relationship with GroupM. Pub A may have some client logic to generate billing reports based on the seat names of the seat bid. This would result in billing reports for "GroupM" that Pub A does not know what to do with, since they don't have a direct relation there. Pub A can get the information from the adaptercode meta, but that will require them retooling their scripts to account for this new definition of seat. We can't require all publishers to retool their script in lock step with a new PBS release.

Merged phase-2 implementation (PubMatic-OpenWrap#309) in this branch. Will address all the comments here :)

config/accounts.go Outdated Show resolved Hide resolved
Copy link
Collaborator

@hhhjort hhhjort left a comment

Choose a reason for hiding this comment

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

LGTM

@pm-nilesh-chate
Copy link
Contributor Author

Raised a new PR as this one is from wrong account. New PR: #2266

Apologies for the inconvenience.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants