Prebid.js: [Bug] getHighestCpmBids returns rendered bids

[bradchoate]

Type of issue

Bug

Description

This is similar to #2539. With the PrebidJS 1.x updates, the manner in which returned bids are saved into an internal 'pool' complicates the way a winning bid is retrieved. It appears that bids from prior auctions-- even ones that are rendered-- are not removed from the pool.

Steps to reproduce

Running additional auctions for a given ad unit, you will sometimes receive a winning bid that was previously rendered for the ad unit when calling getHighestCpmBids.

Test page

Test page exhibiting the problem: https://jsfiddle.net/bradchoate/gvkpz6wy/

Open the console and click the button to refresh the ad a few times. Any bid returned that was already returned via a prior call to getHighestCpmBids will echo in the console.

Expected results

getHighestCpmBids should never return a bid that has been rendered.

Actual results

getHighestCpmBids can return a bid that was already rendered.

Platform details

Observed on macOS, but not specific to that platform.

Other information

Related to #2539

[jsnellbaker]

@jaiminpanchal27 Could you take a look?

[bradchoate]

If my expectations are wrong about this function, please let me know what a good alternative is to easily access the winning (highest-CPM) bid for the auction that just ran for a specific ad unit code. We're calling getHighestCpmBids within the callback and I know the callback receives auction results that could be used, but would require additional filtering, since it returns all received bids, not just the winners. This function (getHighestCpmBids) seems to do what we want for PrebidJS 0.x, but not for version 1.

I also have concerns about how the auction manager's use of the bid pool is considering bids from prior auctions (based on my interpretations, this appears to be the case). We don't want to include any; each bid winner should be derived on a per-auction basis. Considering bid wins from prior auctions seems improper.

[mkendall07]

this does sound like a bug

[bradchoate]

Thanks for looking at this... in the meantime, I was considering using this:

function selectHighestBid(bids) {
    // exclude bids that have no cpm at all
    bids = bids.filter(bid => bid.cpm > 0);
    // sort by (adjusted) cpm, from highest to lowest
    bids.sort((a, b) => b.cpm - a.cpm);
    // return highest cpm from resulting list, or return null
    // if no bid is available
    return bids.length > 0 ? bids[0] : null;
}

It would be called from the 'bidsBackHandler' callback, which receives the auction results by unit code. The callback would call this for each unit code's array of bid results to determine the winner for that placement.

[jaiminpanchal27]

@bradchoate yes i think above will work for you.

[andyblackwell]

I think the main problem is that the code to pick the high bids is done in several places, and done differently. Might be better for the winning bid logic to be consolidated in the auction object on auction end, then those results can be merely read from everywhere else (pbjs.getHighestCpmBids, targeting.getWinningBids, etc). That would mean though, that there would need to be better access to state on some events, like in pbjs.getHighestCpmBids "what auction is this coming from?", which isn't super easy to do currently.

And ad refreshing/bid caching add a whole different layer on top of all that, when a bid from an older auction is promoted to being used in the current auction.

I'm doing some interesting things with bid caching, and have had to hack in flags here and there in order to get things working properly. My getHighestCpmBids currently just filters on those flags, and doesn't re-run the "winning bid" calculation.

[bradchoate]

Is bid caching (where bids from prior auctions are held and included for consideration for later auctions) something that is just on as a default? We'd like the choice to disable that, or if a given ad vendor is okay with it, enable/disable on a per vendor basis.

[jaiminpanchal27]

@bradchoate yes it is on by default. If bid pool has two unused and unexpired bids for same adunit by same bidder than it will do an auction for both and will select the highest cpm one. In case of tie it will select the older one. Why would you need to disable it ?

[andyblackwell]

the vendors have all added their bid TTL settings in their adapters, just for bid caching, so we know when they expire. Maybe if a vendor doesn't have the TTL setting, then their bids aren't cached? I haven't looked through all the adapters to check, but all the ones we use have TTL configured.

[jaiminpanchal27]

ttl is a required response param. http://prebid.org/dev-docs/bidder-adaptor.html

[dbemiller]

That would mean though, that there would need to be better access to state on some events, like in pbjs.getHighestCpmBids "what auction is this coming from?", which isn't super easy to do currently.

Long ago, I remember having some conversations about replacing these global API functions with auction-specific ones. So rather than pbjs.getHighestCpmBids(), make pbjs.requestBids() return the auction object, and then define an auction.getHighestCpmBids() which gets the highest bids in the auction. You'd probably want an async signature for it... but it would fit this use case. Might be worth re-visiting here?

[bradchoate]

Our understanding from talking with a few exchanges is that they expect a bid response to be used only one time, and only one bid response to be used per ad slot. If you receive a bid, hold an auction, and that bid does not win, it should not be kept to be re-used in future auctions.

When you refresh an ad slot and receive new bids, only those bids are intended to compete. Prior bids were given at a different time, for a different ad slot (even though it might be the same placement; a refresh would amount to it being a different in terms of the auction... first impression for that slot would have different value than the second impression, etc.). Evaluating multiple bids from a single exchange seems conceptually similar to making multiple requests and picking the highest, which seems like that would not be allowed.

It’s possible that our understanding may not be correct. We aren’t sure if the behavior of getHighestCpmBids for refreshing ad slots is intentional, or inadvertent.

There are two potential cases to consider:

• Bid re-use, when an exchange does not return a response (or times out), but a previously un-used response is available.
• Multiple bids, when a exchange does return a response, but a previously un-used response has a higher CPM.

To your knowledge, do exchanges allow bid re-use and multiple bids to be evaluated for a single ad slot?

[bradchoate]

This seems apropos with respect to the bid caching mechanism being on as a default for PrebidJS 1.x - https://adexchanger.com/platforms/index-exchange-called-out-for-tweaking-its-auction/

[bretg]

Re-opening as #2990 is being reverted as it was a breaking change. Back the drawing board here.

[bduclaux]

The same type of issue applies to getBidResponsesForAdUnitCode . This method returns bids for all previous auctions, not only for the last one.

This is inconsistent with the behavior of getBidResponses, which filters for the latest auctionId.
I think the Prebid API should allow users to specify whether users want to get bids for all previous auctions, of the latest one.

[bjorn-lw]

I'm seeing this issue as well. I can use the workaround described above, but it would be great if the provided functions work as expected.

[brianxieseattle]

Please fix this issue. getBidResponsesForAdUnitCode returns the "rendered" bid. This should be considered a bug to be fixed.

[lutzklai]

I like idea of auction specific functions. It seems like a cleaner declarative approach.

However, the current way I'm handling this is to tag the bid if it has gone through my bidsBackHandler

// top of my bidsBackHandler callback
var bids = pbjs.getBidResponsesForAdUnitCode(code).bids
    .filter(function(bid) {
        return !bid.processed;
    })
    .map(function(bid) {
        bid.processed = true;
        return bid;
    });

I'm not a huge fan of it playing off the bid reference, but it works for my use case.

[mgriego]

I've run into this as well. One other thing I've noticed that feels somewhat related is that there doesn't seem to be any garbage collection of old auctions. The auctionManager keeps every auction indefinitely. On long-running pages where there might be slot refreshes, infinite scroll, single page apps, etc, this can lead to some serious memory leaks.

[arjitsachdeva]

Is this expected to be fixed in any upcoming prebid updates? Or the docs for how it should be used currently can be improved maybe.

[jsnellbaker]

@arjitsachdeva This fix is going to be part of prebid 3.0. This is the PR for the fix that was put together:
https://github.com/prebid/Prebid.js/pull/4247

[jaiminpanchal27]

This is done in 3.0 and released. Closing this.