Prebid.js icon indicating copy to clipboard operation
Prebid.js copied to clipboard

New Module: MinBidToWin Notifications: Created a new module to support sending minbidtowin notifications to bidders

Open jlquaccia opened this issue 1 year ago • 13 comments

Type of change

  • [x] Feature

Description of change

  • Created a new "lossNotifications" module in core that can optionally be used internally within bid adapters
  • When enabled, the module supports 2 methods for issuing out loss notifications to bidders:
  1. By default, loss notifications will be sent out to respective bidders in subsequent auctions within a bidders outgoing bid request (for example, bidder A and bidder B are participating in auction 1. bidder A wins. during auction 2 (or next auction that bidder B is involved in), bidder B will receive the loss notification within their bid request.
  2. If a bidder provides an endpoint of their own, they can send loss notifications about their bids to their own endpoint via the JS Beacon API: https://developer.mozilla.org/en-US/docs/Web/API/Beacon_API

Other information

https://github.com/prebid/Prebid.js/issues/10788

jlquaccia avatar Feb 09 '24 21:02 jlquaccia

https://github.com/prebid/Prebid.js/issues/10788#issuecomment-1936653860

will write tests for this PR once the questions within the comment above are addressed

jlquaccia avatar Feb 09 '24 21:02 jlquaccia

Would suggest naming this "auction info" instead of trying to promise true "loss notification". https://github.com/prebid/prebid-server/issues/3333

bretg avatar Jun 05 '24 16:06 bretg

One thought in committee is keeping direct calls for the unload event; we would attempt to send all notifications on the next payload.

patmmccann avatar Jun 05 '24 16:06 patmmccann

Some notes in committee discussion https://docs.google.com/document/d/1AojPDLnJzXaxwmFE5U7Qsm2GAV1fVwjcx4YyI6kX9ew/edit

Looking forward to this feature!

patmmccann avatar Jun 05 '24 16:06 patmmccann

Hey, chiming in to check what is required for this PR to be approved? As a Prebid Server bidder, we are very interested with this feature as it was proven very efficient with other partners in terms of optimizing our bidding behavior and winrate. Thanks!

mlapeyre3 avatar Jul 12 '24 13:07 mlapeyre3

I think it is important to try and censure bidder B listening to scenarios they have submitted valid bids. We dont want to incentivize invalid bid submissions to get these events

patmmccann avatar Jul 12 '24 14:07 patmmccann

Turns out the Prebid Server committee has been discussing this general topic as well. See https://github.com/prebid/prebid-server/issues/3333 . The scenarios are different, but it might make sense to standardize on the ORTB extension here.

Haven't done a full analysis, but did note some key points of difference:

  • "fire payload immediately to ssp provided endpoint" - Prebid Server can't do this. And I think doing this in Prebid.js could be a concern to the Sustainability committee since it could nearly double the amount of traffic generated by the browser. Another solution would be to piggyback previous auction data on the next auction. Heads up @GLStephen .
  • Assuming the piggyback solution is what's implemented I'd suggest not calling this thing "loss notifications" or even "minBidToWin". It's great that Prebid.js can utilize the render event. Prebid Server cannot. However, a bid might lose the first auction but later be pulled out of the bid cache and therefore be a future winner, making the term "loss" murkier. On the server-side, we've been referring to this whole thing as "previous auction info" which sidesteps any preconceived notions about "win" or "lose".
  • The proposed JSON object here is missing currency. That's a must-have.

PBJS proposed payload:

        bidderRequestId: bidder.bidderRequestId,
        auctionId: winningBidData.transactionId,
        minBidToWin: winningBidData.cpm,
        rendered: winningBidData.status === CONSTANTS.BID_STATUS.RENDERED ? 1 : 0,

PBJS proposed ORTB extension at ext.prebid.previousauctioninfo

source: "pbjs",
auctionId: STRING, // $.id of the previous auction
impid: STRING,       // $.imp[].id of the previous auction
bidresponseid: STRING, // seatbid[].bid[].id of the previous auction
targetedbidcpm: FLOAT,          // the bid targeted as the 'winner' within PBS targeting. Not specified if includewinners flag not present
highestcpm: FLOAT,        // the highest bid seen by Prebid in the publisher's requested currency
cur: STRING,
biddercpm: FLOAT,    // the price submitted by this bidder
biddererrorcode: INTEGER,  // if the bidder's bid was rejected, let them know the seatnonbid code
timestamp: INTEGER

Perhaps these approaches should be discussed in next week's committee meetings?

bretg avatar Jul 12 '24 18:07 bretg

@jlquaccia, @patmmccann, @mlapeyre3 - any response to my concerns?

Most importantly, I believe currency is a must-have. But also I think a "piggybacking-on-the-next auction" approach is superior to the current "fire payload immediately to ssp provided endpoint" implementation. Am ready to debate this. :-)

bretg avatar Jul 26 '24 13:07 bretg

@bretg I'm good with the ext.prebid.previousauctioninfo approach mentioned above with the additional keys you proposed, makes sense to me. Aligning with PBS would definitely be ideal. I can also see why immediately firing a response would have drawbacks (as also mentioned above).

What if during subsequent auctions, for ssp's that have opted in to receive previousauctioninfo, in order for them to actually receive this info, their subsequent bid request has to pass their own bid adapter isBidRequestValid check? (to help try filter out invalid submissions)

@patmmccann @mlapeyre3 thoughts on your end?

jlquaccia avatar Aug 05 '24 22:08 jlquaccia

What if during subsequent auctions, for ssp's that have opted in to receive previousauctioninfo, in order for them to actually receive this info, their subsequent bid request has to pass their own bid adapter isBidRequestValid check? (to help try filter out invalid submissions)

@patmmccann @mlapeyre3 thoughts on your end?

Works for me

patmmccann avatar Sep 04 '24 13:09 patmmccann

@jlquaccia we'd love to make progress on this. Do you think we're in a position to start writing tests? Can we pitch in?

patmmccann avatar Oct 07 '24 16:10 patmmccann

noting the file is in /src/ not /modules

patmmccann avatar Oct 09 '24 16:10 patmmccann

@jlquaccia we'd love to make progress on this. Do you think we're in a position to start writing tests? Can we pitch in?

@patmmccann thanks for the recent updates above. yes, i can pick this up again (will aim for next week). currently wrapping up with some internal project work over here.

jlquaccia avatar Oct 09 '24 17:10 jlquaccia

Tread carefully! This PR adds 3 linter errors (possibly disabled through directives):

  • libraries/previousAuctionInfo/previousAuctionInfo.js (+3 errors)

github-actions[bot] avatar Nov 12 '24 00:11 github-actions[bot]

@patmmccann addressed the items above and wrote some new tests. let me know if you have any other feedback

jlquaccia avatar Nov 12 '24 00:11 jlquaccia

hey @patmmccann, just wanted to follow up with this one for a code review?

jlquaccia avatar Dec 04 '24 00:12 jlquaccia

@dgirardi @bretg, addressed your feedback with the latest round of commits

jlquaccia avatar Feb 11 '25 22:02 jlquaccia

Hi @dgirardi, I pushed a few other commits after some additional feedback from the PMC call earlier today. Can you review again when you get a chance? Thanks!

I also linked a docs PR: https://github.com/prebid/prebid.github.io/pull/5881

jlquaccia avatar Feb 12 '25 23:02 jlquaccia

thanks @dgirardi @patmmccann! are we good to merge now?

jlquaccia avatar Feb 14 '25 18:02 jlquaccia

@jlquaccia, I have issues to use the module so I have opened the issue https://github.com/prebid/Prebid.js/issues/12825.

krzysztofequativ avatar Mar 03 '25 11:03 krzysztofequativ