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

add floor module support to prebid server #5721

Closed
wants to merge 37 commits into from
Closed

add floor module support to prebid server #5721

wants to merge 37 commits into from

Conversation

patmmccann
Copy link
Collaborator

@patmmccann patmmccann commented Sep 9, 2020

Type of change

  • Bugfix
  • Feature
  • New bidder adapter
  • Code style update (formatting, local variables)
  • Refactoring (no functional changes, no api changes)
  • Build related changes
  • CI related changes
  • Does this change affect user-facing APIs or examples documented on http://prebid.org?
  • Other

Description of change

this adds floor and floor curreny to prebid server requests in field imp[].bidfloor and imp[].bidfloorcur

@@ -569,6 +593,8 @@ const OPEN_RTB_PROTOCOL = {
utils.deepSetValue(imp, 'ext.prebid.storedauctionresponse.id', storedAuctionResponseBid.storedAuctionResponse.toString());
}

_appendFloor(firstBidRequest.bids[0], imp);
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I can't figure out what should go here in the first argument

Copy link
Collaborator

@robertrmartinez robertrmartinez Sep 10, 2020

Choose a reason for hiding this comment

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

Yeah, we'll need some logic to find the right bid to use.

Need's the context of the adUnit, so we'll want to use adUnit.code, and find a bid in firstBidRequest.bids which matches it.

Something like:

let matchedBid = find(firstBidRequest.bids, bid => bid.adUnitCode === adUnit.code);
if (matchedBid) {
   _appendFloor(matchedBid, imp);
}

An issue even this may have:

  • firstBidRequest may be from a bidder who is not included in all adUnits, so we may miss out on setting floor for some imps.

Which means we may need / want to do a join on ALL bids not just the ones in firstBidRequest.

I would have to take a deeper look, but could come up with something.

@bszekely1 any input on using PBJS floors module to set imp.bidFloor?

I know PBS will eventually have some floor module support, but may be some time from now.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

thanks so much for taking a look! @bszekely1 and I were talking about this in the prebid server meeting recently and it seems this is quite a valid use case. @bretg recently filed #5720, which this partially solves, partly based on that discussion. Also all prebid server adapters are being asked to honor this parameter when it is available (imp[].bidfloor and imp[].bidlfoorcur) instead of reading from adapterOptions, but there is no way to currently write to it from js.

@stale
Copy link

stale bot commented Oct 4, 2020

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the stale label Oct 4, 2020
@bretg
Copy link
Collaborator

bretg commented Oct 7, 2020

@patmmccann - what's the status of this one? It's marked as "draft"

@patmmccann
Copy link
Collaborator Author

patmmccann commented Oct 7, 2020 via email

@stale stale bot removed the stale label Oct 7, 2020
@bretg
Copy link
Collaborator

bretg commented Oct 7, 2020

Ok - I'll work towards getting this prioritized for next internal Sprint

@robertrmartinez robertrmartinez removed their assignment Oct 13, 2020
@patmmccann
Copy link
Collaborator Author

@bretg any luck putting this on the dev calendar? With the prebid 5.0 issue announced, I think it would be great to have the pbs adapter in compliance

@bszekely1 bszekely1 linked an issue Nov 30, 2020 that may be closed by this pull request
@stale
Copy link

stale bot commented Dec 19, 2020

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the stale label Dec 19, 2020
@robertrmartinez
Copy link
Collaborator

Closing this as I have opened #6273

@stale stale bot removed the stale label Feb 4, 2021
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.

Support arbitrary OpenRTB in s2sConfig
3 participants