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.

Sign up for GitHub

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

Floors Module update to include floorMin #5805

Merged
merged 8 commits into from
Oct 21, 2020
Merged

Floors Module update to include floorMin #5805

Show file tree
Hide file tree
Changes from 7 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
b8ac8d5
Update to floors module to allow floorMin definition using setConfig(…
mmoschovas Sep 30, 2020
372f3b0
Remove extra spaces
mmoschovas Sep 30, 2020
fd96dc1
Package Lock revert
mmoschovas Oct 1, 2020
2bbe936
Updates to commit
mmoschovas Oct 2, 2020
d9e37e4
Merge branch 'floorMin-update' of https://github.com/prebid/Prebid.js…
mmoschovas Oct 2, 2020
e61582e
Remove comment
mmoschovas Oct 2, 2020
ec1d286
Remove excess spaces
mmoschovas Oct 2, 2020
7be959e
Update to priceFloor and rubiconAnalytics adapters
mmoschovas Oct 13, 2020
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
14 changes: 10 additions & 4 deletions modules/priceFloors.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,7 +55,7 @@ export let _floorDataForAuction = {};
* @summary Simple function to round up to a certain decimal degree
*/
function roundUp(number, precision) {
return Math.ceil(parseFloat(number) * Math.pow(10, precision)) / Math.pow(10, precision);
return Math.ceil((parseFloat(number) * Math.pow(10, precision)).toFixed(1)) / Math.pow(10, precision);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can you explain how this solves the rounding issue?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Nevermind, took a look and it works. Good job!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The rounding issue was due to the JS evaluation of the number after multiplying the two values. So 0.6271 multiplied by 10000 would be set as 6271.0000000000000001. Running the ceiling on this will cause it to round up to 6272. By fixing to only one decimal allowed it to round this only to the first decimal place prior to running the ceiling, thus avoiding rounding up unnecessarily.

Copy link
Collaborator

Choose a reason for hiding this comment

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

why round instead of down? is this a convention of sorts?

Copy link
Collaborator

Choose a reason for hiding this comment

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

@patmmccann

Say the actual floor is:

0.611

if we round to

0.61

and send that to the bidder, and they bid at 0.61, they will be floored.

So rounding up, makes sure we never run into this scenario.

Copy link
Collaborator

Choose a reason for hiding this comment

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

interesting, in the case of not enforcing, seems like it should be flipped. However, probably quite the edge case, fyi @punkiller

}

let referrerHostname;
Expand Down Expand Up @@ -98,7 +98,7 @@ export function getFirstMatchingFloor(floorData, bidObject, responseObject = {})
let fieldValues = enumeratePossibleFieldValues(utils.deepAccess(floorData, 'schema.fields') || [], bidObject, responseObject);
if (!fieldValues.length) return { matchingFloor: floorData.default };

// look to see iof a request for this context was made already
// look to see if a request for this context was made already
let matchingInput = fieldValues.map(field => field[0]).join('-');
// if we already have gotten the matching rule from this matching input then use it! No need to look again
let previousMatch = utils.deepAccess(floorData, `matchingInputs.${matchingInput}`);
Expand All @@ -109,10 +109,12 @@ export function getFirstMatchingFloor(floorData, bidObject, responseObject = {})
let matchingRule = find(allPossibleMatches, hashValue => floorData.values.hasOwnProperty(hashValue));

let matchingData = {
matchingFloor: floorData.values[matchingRule] || floorData.default,
floorMin: floorData.floorMin || 0,
floorRuleValue: floorData.values[matchingRule] || floorData.default,
matchingData: allPossibleMatches[0], // the first possible match is an "exact" so contains all data relevant for anlaytics adapters
matchingRule
};
matchingData.matchingFloor = Math.max(matchingData.floorMin, matchingData.floorRuleValue);
// save for later lookup if needed
utils.deepSetValue(floorData, `matchingInputs.${matchingInput}`, {...matchingData});
return matchingData;
Expand Down Expand Up @@ -190,6 +192,7 @@ export function getFloor(requestParams = {currency: 'USD', mediaType: '*', size:
let bidRequest = this;
let floorData = _floorDataForAuction[bidRequest.auctionId];
if (!floorData || floorData.skipped) return {};
if (floorData.hasOwnProperty('floorMin')) floorData.data.floorMin = floorData.floorMin;
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is going to run each time getFloor is called. Which is probably not ideal.

Better to just set floorData.data.floorMin once.

I would recommend doing this in createFloorsDataForAuction which will initialize the entire data object, then you will just be able to access it where needed.


requestParams = updateRequestParamsFromContext(bidRequest, requestParams);
let floorInfo = getFirstMatchingFloor(floorData.data, {...bidRequest}, {mediaType: requestParams.mediaType, size: requestParams.size});
Expand Down Expand Up @@ -287,11 +290,12 @@ export function updateAdUnitsForAuction(adUnits, floorData, auctionId) {
bid.floorData = {
skipped: floorData.skipped,
skipRate: floorData.skipRate,
floorMin: floorData.floorMin,
Copy link
Collaborator

Choose a reason for hiding this comment

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

@bszekely1

Do we want to expose floorMin to bid adapters?

Copy link

Choose a reason for hiding this comment

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

There is not much a bid adapter can do with this information. So long as analytics adapters have access to it, we should be covered. Bid adapters would still receive the floor, just the information feeding that floor should be available to analytics.

Copy link
Collaborator

Choose a reason for hiding this comment

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

ok so if we leave this as is then a bid adapter will know at bidRequest time what the floorMin is.

As long as that is okay, we can and should leave it 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.

@robertrmartinez is anything else needed for this or is this good to go?

modelVersion: utils.deepAccess(floorData, 'data.modelVersion'),
location: utils.deepAccess(floorData, 'data.location', 'noData'),
floorProvider: floorData.floorProvider,
fetchStatus: _floorsConfig.fetchStatus
}
};
});
});
}
Expand Down Expand Up @@ -568,6 +572,7 @@ function addFieldOverrides(overrides) {
*/
export function handleSetFloorsConfig(config) {
_floorsConfig = utils.pick(config, [
'floorMin', floorMin => floorMin,
Copy link
Collaborator

Choose a reason for hiding this comment

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

With utils.pick you can just leave it directly as floorMin instead of with the function floorMin => floorMin

It will pick the floorMin on the config level, and since we do not need to default it, the extra use of function call is unneeded.

'enabled', enabled => enabled !== false, // defaults to true
'auctionDelay', auctionDelay => auctionDelay || 0,
'floorProvider', floorProvider => utils.deepAccess(config, 'data.floorProvider', floorProvider),
Expand Down Expand Up @@ -623,6 +628,7 @@ function addFloorDataToBid(floorData, floorInfo, bid, adjustedCpm) {
bid.floorData = {
floorValue: floorInfo.matchingFloor,
floorRule: floorInfo.matchingRule,
floorRuleValue: floorInfo.floorRuleValue,
floorCurrency: floorData.data.currency,
cpmAfterAdjustments: adjustedCpm,
enforcements: {...floorData.enforcement},
Expand Down
4 changes: 4 additions & 0 deletions modules/rubiconAnalyticsAdapter.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -117,6 +117,7 @@ function sendMessage(auctionId, bidWonId) {
'dimensions',
'mediaType',
'floorValue',
'floorRuleValue',
'floorRule'
]) : undefined
]);
Expand Down Expand Up @@ -224,6 +225,8 @@ function sendMessage(auctionId, bidWonId) {
'dealsEnforced', () => utils.deepAccess(auctionCache.floorData, 'enforcements.floorDeals'),
'skipRate',
'fetchStatus',
'floorMin',
'floorRuleValue',
Copy link
Collaborator

Choose a reason for hiding this comment

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

floorRuleValue does not make sense to be in the top level floorData object of the message. It is a per bidResponse

I think this was a typo in the requirements haha.

'floorProvider as provider'
]);
}
Expand Down Expand Up @@ -335,6 +338,7 @@ export function parseBidResponse(bid, previousBidResponse, auctionFloorData) {
},
'seatBidId',
'floorValue', () => utils.deepAccess(bid, 'floorData.floorValue'),
'floorRuleValue', () => utils.deepAccess(bid, 'floorData.floorRuleValue'),
'floorRule', () => utils.debugTurnedOn() ? utils.deepAccess(bid, 'floorData.floorRule') : undefined
]);
}
Expand Down
Loading