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
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
130 changes: 130 additions & 0 deletions libraries/previousAuctionInfo/previousAuctionInfo.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,130 @@
import {on as onEvent} from '../../src/events.js';
import { EVENTS } from '../../src/constants.js';

export let previousAuctionInfoEnabled = false;
let enabledBidders = [];
export let winningBidsMap = {};

export const resetPreviousAuctionInfo = () => {
previousAuctionInfoEnabled = false;
enabledBidders = [];
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.

const { bidderCode, isBidRequestValid } = config;
const enabledBidder = enabledBidders.find(bidder => bidder.bidderCode === bidderCode);
if (!enabledBidder) enabledBidders.push({ bidderCode, isBidRequestValid, maxQueueLength: config.maxQueueLength || 10 });
if (previousAuctionInfoEnabled) return;
previousAuctionInfoEnabled = true;
cb();
}

export const initHandlers = () => {
onEvent(EVENTS.AUCTION_END, onAuctionEndHandler);
onEvent(EVENTS.BID_WON, onBidWonHandler);
onEvent(EVENTS.BID_REQUESTED, onBidRequestedHandler);
};

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?


try {
let highestCpmBid = 0;
const receivedBidsMap = {};
const rejectedBidsMap = {};

if (auctionDetails.bidsReceived && auctionDetails.bidsReceived.length) {
highestCpmBid = auctionDetails.bidsReceived.reduce((highestBid, currentBid) => {
return currentBid.cpm > highestBid.cpm ? currentBid : highestBid;
}, 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 (auctionDetails.bidsRejected && auctionDetails.bidsRejected.length) {
auctionDetails.bidsRejected.forEach(bidRejected => {
rejectedBidsMap[bidRejected.requestId] = bidRejected;
});
}

if (auctionDetails.bidderRequests && auctionDetails.bidderRequests.length) {
auctionDetails.bidderRequests.forEach(bidderRequest => {
const enabledBidder = enabledBidders.find(bidder => bidder.bidderCode === bidderRequest.bidderCode);

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.


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?

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?

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.

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.

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.

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

highestcpm: highestCpmBid?.cpm || 0,
cur: bid.ortb2.cur,
bidderCpm: receivedBidsMap[bid.bidId] ? receivedBidsMap[bid.bidId].cpm : 'nobid',
biddererrorcode: rejectedBidsMap[bid.bidId] ? rejectedBidsMap[bid.bidId].rejectionReason : -1,
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.

if (!window.pbpai[bidderRequest.bidderCode]) {
window.pbpai[bidderRequest.bidderCode] = [];
}

if (window.pbpai[bidderRequest.bidderCode].length > enabledBidder.maxQueueLength) {
window.pbpai[bidderRequest.bidderCode].shift();
}

window.pbpai[bidderRequest.bidderCode].push(previousAuctionInfoPayload);
});
}
});
}
} catch (error) {
// eslint-disable-next-line no-console
console.error(error);
}
}

const onBidWonHandler = (winningBid) => {
// // eslint-disable-next-line no-console
// console.log('onBidWonHandler', winningBid);
winningBidsMap[winningBid.transactionId] = winningBid;
}

export const onBidRequestedHandler = (bidRequest) => {
try {
const enabledBidder = enabledBidders.find(bidder => bidder.bidderCode === bidRequest.bidderCode);
window.pbpai = window.pbpai || {};

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;
}
});
Comment on lines +112 to +118
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


bidRequest.ortb2 ??= {};
bidRequest.ortb2.ext ??= {};
bidRequest.ortb2.ext.prebid ??= {};
bidRequest.ortb2.ext.prebid.previousauctioninfo = window.pbpai[bidRequest.bidderCode];
delete window.pbpai[bidRequest.bidderCode];
}
} catch (error) {
// eslint-disable-next-line no-console
console.error(error);
}
}
81 changes: 81 additions & 0 deletions test/spec/libraries/previousAuctionInfo_spec.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,81 @@
import * as previousAuctionInfo from 'libraries/previousAuctionInfo/previousAuctionInfo.js';
import sinon from 'sinon';

describe('previous auction info', () => {
let sandbox;
let initHandlersStub;

const auctionDetails = {
auctionId: 'auction123',
bidsReceived: [{ requestId: 'bid1', bidderCode: 'testBidder2', cpm: 2 }],
bidsRejected: [{ requestId: 'bid2', rejectionReason: 1 }],
bidderRequests: [
{
bidderCode: 'testBidder2',
bidderRequestId: 'req1',
bids: [{ bidId: 'bid1', ortb2: { cur: ['US'] }, ortb2Imp: { ext: { tid: 'trans123' } }, adUnitCode: 'adUnit1' }],
},
],
timestamp: Date.now(),
};

beforeEach(() => {
previousAuctionInfo.resetPreviousAuctionInfo();
if (window.pbpai) delete window.pbpai;
sandbox = sinon.createSandbox();
initHandlersStub = sandbox.stub();
});

afterEach(() => {
sandbox.restore();
if (window.pbpai) delete window.pbpai;
});

describe('config', () => {
it('should only be initialized once', () => {
const config = { bidderCode: 'testBidder', isBidRequestValid: () => true };
previousAuctionInfo.enablePreviousAuctionInfo(config, initHandlersStub);
sandbox.assert.calledOnce(initHandlersStub);
previousAuctionInfo.enablePreviousAuctionInfo(config, initHandlersStub);
sandbox.assert.calledOnce(initHandlersStub);
});
});

describe('on auction end', () => {
it('should only capture data for enabled bids who submitted a valid bid', () => {
const config = { bidderCode: 'testBidder2', isBidRequestValid: (bid) => bid.bidId === 'bid1' };
previousAuctionInfo.enablePreviousAuctionInfo(config, initHandlersStub);
previousAuctionInfo.onAuctionEndHandler(auctionDetails);

expect(window.pbpai.testBidder2).to.be.an('array').with.lengthOf(1);
expect(window.pbpai.testBidder2[0]).to.include({
auctionId: 'auction123',
minBidToWin: 2,
transactionId: 'trans123',
rendered: 0,
});
});
});

describe('on bid requested', () => {
it('should update the minBidToWin and rendered fields if a pbjs bid wins', () => {
const config = { bidderCode: 'testBidder3', isBidRequestValid: () => true };
previousAuctionInfo.enablePreviousAuctionInfo(config, initHandlersStub);
const bidRequest = {
bidderCode: 'testBidder3',
ortb2: { ext: { prebid: {} } },
};

previousAuctionInfo.winningBidsMap['trans123'] = { cpm: 5, transactionId: 'trans123' };

window.pbpai = {
testBidder3: [{ transactionId: 'trans123', minBidToWin: 0, rendered: 0 }],
};

previousAuctionInfo.onBidRequestedHandler(bidRequest);
const updatedInfo = bidRequest.ortb2.ext.prebid.previousauctioninfo;
expect(updatedInfo).to.be.an('array').with.lengthOf(1);
expect(updatedInfo[0]).to.include({ minBidToWin: 5, rendered: 1 });
});
});
});