Skip to content

Provide deterministic behavior for sort function. #372

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

Closed
wants to merge 3 commits into from
Closed

Provide deterministic behavior for sort function. #372

wants to merge 3 commits into from

Conversation

CarsonBanov
Copy link
Contributor

@mkendall07

The current behavior for sorting the bids (not actually sorting, just picking the highest) does not handle the case in which bids are equal/tied. This leads to non-deterministic behavior in picking the highest bid.

The addition provided in this PR has two goals. 1) Make the behavior of the sort function deterministic so an identical algorithm implemented elsewhere using the raw bid data will return the same results. 2) Use the existing timeToRespond as a secondary sorting key which has the added benefit of slightly incentivizing faster bids while still being simple to implement.

For our project we have been noticing a sizable amount of equal bids, so this issue actually occurs quite frequently. This would help publishers doing their own logging to better match Prebid (since the algorithm will now be deterministic) and it will enable bidders to get a better read on why a bid won/lost at a certain price.

.cc @mmilleruva

@mkendall07
Copy link
Member

@CarsonBanov
Thanks! Can you provide a unit test for this change?

@@ -141,6 +141,9 @@ function getWinningBidTargeting() {
return winners;

function getHighestCpm(previous, current) {
if (previous.cpm == current.cpm) {
Copy link
Member

Choose a reason for hiding this comment

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

please use ===

@CarsonBanov
Copy link
Contributor Author

@mkendall07 Updated.

@mkendall07
Copy link
Member

@CarsonBanov
Thanks for updating. I moved the getHighestCpm function into utils. Closed this PR in favor of #381

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants