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

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

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

jlquaccia
Copy link
Collaborator

Type of change

  • 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

#10788

@jlquaccia
Copy link
Collaborator Author

#10788 (comment)

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


if (bidder.beaconUrl) {
// use js beacon api to fire payload immediately to ssp provided endpoint
navigator.sendBeacon(bidder.beaconUrl, JSON.stringify(lossNotificationPayload));
Copy link
Collaborator

@patmmccann patmmccann Jun 5, 2024

Choose a reason for hiding this comment

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

please use our methods; fetch keepalive has replaced sendBeacon and is now supported by importing the ajax library

@bretg
Copy link
Collaborator

bretg commented Jun 5, 2024

Would suggest naming this "auction info" instead of trying to promise true "loss notification". prebid/prebid-server#3333

@patmmccann
Copy link
Collaborator

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

@patmmccann
Copy link
Collaborator

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

Looking forward to this feature!

src/lossNotifications.js Outdated Show resolved Hide resolved
@mlapeyre3
Copy link

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!

@patmmccann
Copy link
Collaborator

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

@bretg
Copy link
Collaborator

bretg commented Jul 12, 2024

Turns out the Prebid Server committee has been discussing this general topic as well. See prebid/prebid-server#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
Copy link
Collaborator

bretg commented Jul 26, 2024

@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. :-)

@jlquaccia
Copy link
Collaborator Author

@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?

@patmmccann
Copy link
Collaborator

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 patmmccann removed the on hold label Oct 7, 2024
@patmmccann
Copy link
Collaborator

@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 patmmccann changed the title New Module: Loss Notifications: Created a new module to support sending loss notifications to bidders New Module: MinBidToWin Notifications: Created a new module to support sending loss notifications to bidders Oct 9, 2024
@patmmccann patmmccann changed the title New Module: MinBidToWin Notifications: Created a new module to support sending loss notifications to bidders New Module: MinBidToWin Notifications: Created a new module to support sending minbidtowin notifications to bidders Oct 9, 2024
@patmmccann
Copy link
Collaborator

noting the file is in /src/ not /modules

} else {
pbln[bidder.bidderCode] = [lossNotificationPayload];
}
storage.setDataInLocalStorage('pbln', JSON.stringify(pbln));
Copy link
Collaborator

Choose a reason for hiding this comment

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

why use storage instead of just an object in window?

@jlquaccia
Copy link
Collaborator Author

@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.

Copy link

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

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

@jlquaccia
Copy link
Collaborator Author

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

@jlquaccia
Copy link
Collaborator Author

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


export const onAuctionEndHandler = (auctionDetails) => {
// eslint-disable-next-line no-console
console.log('onAuctionEndHandler', auctionDetails);
Copy link
Collaborator

Choose a reason for hiding this comment

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

should this be here?

}, auctionDetails.bidsReceived[0]);

auctionDetails.bidsReceived.forEach(bidReceived => {
receivedBidsMap[bidReceived.requestId] = bidReceived;
Copy link
Collaborator

Choose a reason for hiding this comment

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

this could be done in the reduce above to spare a loop


if (enabledBidder) {
bidderRequest.bids.forEach(bid => {
const isValidBid = enabledBidder.isBidRequestValid(bid);
Copy link
Collaborator

Choose a reason for hiding this comment

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

(After writing what's below, I realized isBidRequestValid is not necessarily coming from the adapter, so I'm not sure if my point is valid - what's the idea for this validation?)

I'm worried that some adapters may not be expecting this. They could for example generate duplicate error messages, or if they keep state, break them.

Is it necessary? Would it be acceptable to have an "empty" prevAuctionInfo object for invalid requests, as if no-one bid?

If not, I think it'd be preferable to mark invalid bids where we already call isBidRequestValid, so that you can recognize them here without calling it again.

Comment on lines +112 to +118
if (enabledBidder && window.pbpai[bidRequest.bidderCode]) {
window.pbpai[bidRequest.bidderCode].forEach(prevAuctPayload => {
if (winningBidsMap[prevAuctPayload.transactionId]) {
prevAuctPayload.minBidToWin = winningBidsMap[prevAuctPayload.transactionId].cpm;
prevAuctPayload.rendered = 1;
}
});
Copy link
Collaborator

Choose a reason for hiding this comment

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

The purpose of this is to update the prevAuctionInfo object with the winning bid's cpm, right? If so, I think it'd be more intuitive to put it in the BID_WON handler. But see also below on transactionId

bidderRequestId: bidderRequest.bidderRequestId,
minBidToWin: highestCpmBid?.cpm || 0,
rendered: 0,
transactionId: bid.ortb2Imp.ext.tid || bid.transactionId,
Copy link
Collaborator

Choose a reason for hiding this comment

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

this leaks transactionId to the bidder, which is hidden by default. You could still use this to keep track of winners without exposing it, for example by using a map from transactionId -> prevAuctionInfo, which would make it easier to update the winning cpm on BID_WON, and pass only the values to the bidder on the next request.

transactionId: bid.ortb2Imp.ext.tid || bid.transactionId,
source: 'pbjs',
auctionId: auctionDetails.auctionId,
impId: bid.adUnitCode,
Copy link
Collaborator

Choose a reason for hiding this comment

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

IMO adUnitCode is a better name. impId makes me think of ORTB imp.id, which most of the time is different from the ad unit code.

timestamp: auctionDetails.timestamp,
}

window.pbpai = window.pbpai || {};
Copy link
Collaborator

Choose a reason for hiding this comment

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

why put this in window? it looks like internal state to me.

winningBidsMap = {};
};

export const enablePreviousAuctionInfo = (config, cb = initHandlers) => {
Copy link
Collaborator

Choose a reason for hiding this comment

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

It's unclear to me how this is intended to be used.

If it's an optional module chosen by the publisher, this should be in the modules folder and listen for changes to setConfig.

Alternatively we could let individual bidders opt in, in which case the only parameter needed should be the bidder code, and this should allow for being called multiple times with different bidders.

rendered: 0,
transactionId: bid.ortb2Imp.ext.tid || bid.transactionId,
source: 'pbjs',
auctionId: auctionDetails.auctionId,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Same issue as transactionId - this is hidden by default. This should either be omitted, or included only when the transmitTid activity is allowed

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think it should be debated whether any of this data is useful to bidders if there's no IDs. If I understand the purpose correctly, the receiver will join these logs with their bid logs so they can do stuff like "oh, when the browser is X and the country is Y, my bid has to be higher". In order to do that, I suspect that there's a minimal set of IDs or the join will be impossible.

Copy link
Collaborator

Choose a reason for hiding this comment

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

we can leave in requestId and bidderRequestId. They have the same domain (allow the same join) as transactionId / auctionId, the difference is they're not shared across bidders.

Or we can do what I think the first join would have to be - each request's prevAuctionInfo only pertains bids on the same ad unit.

if (!isValidBid) return;

const previousAuctionInfoPayload = {
bidderRequestId: bidderRequest.bidderRequestId,
Copy link
Collaborator

Choose a reason for hiding this comment

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

What is bidderRequest.bidderRequestId?


const previousAuctionInfoPayload = {
bidderRequestId: bidderRequest.bidderRequestId,
minBidToWin: highestCpmBid?.cpm || 0,
Copy link
Collaborator

Choose a reason for hiding this comment

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

How does 'minBidToWin' differ from the 'highestcpm' field? I can live with this term because it's sort of an industry standard, but it's actually not correct. Not guaranteed at all that they would have won with this bid. It just happens to be the highest bid that Prebid knows about. Which is why I prefer the term 'highestcpm' -- there's less of a promise involved. But anyhow, I get that 'minBidToWin' is a cow that's left the barn, so I would suggest removing 'highestcpm' then. Please make sure that it's positioned right next to the cur field though. Another pet peeve of mine is when it's not clear what currency a price is stated in.

Which I suppose opens a can of worms. The obvious minBidToWin currency is the ortb.cur, but there are going to bidders that will only accept minBidToWin in USD, EUR, or whatever. Do we need this module to convert to different currencies, so an array of minBidToWin entries?

auctionId: auctionDetails.auctionId,
impId: bid.adUnitCode,
// bidResponseId: FLOAT, // don't think this is available client side?
// targetedbidcpm: FLOAT, // don't think this is available client side?
Copy link
Collaborator

Choose a reason for hiding this comment

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

Yes, targetedbidcpm is available in PBJS. If deals are prioritized over open market, the highestcpm might not be the one that Prebid chose to send in hb_pb.

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.

Prebid.js client side loss notifications, adding info to future events or own event?
8 participants