Support for Prebid Server Bid Adapter to support 'extra' bids

jsnellbaker commented 2 years ago

Type of issue

Request/Issue

Description

The appnexus PSP endpoint used through the Prebid Server Bid Adapter file has a use-case that currently doesn't fit with the existing logic/workflow.

Some Context

The PSP endpoint normally communicates with different configured bidders through an appnexus placement to determine the ideal fill for a given request - picking a single bid to return to prebid.js. It also has the ability to return multiple bids from different bidders as its response, instead of just the single 'winning' bid; it's similar to what our enableSendAllBids setting does.

For example, 1 request to an appnexus placement could return 2 bid responses, 1 appnexus bid + 1 pubmatic bid (or rubicon, or whoever).

Issue

However in PBS bid adapter there is logic in place to map the bid response to its original bidRequest, based on the adUnitCode+bidderCode. Reference to the code in question here: https://github.com/prebid/Prebid.js/blob/master/modules/prebidServerBidAdapter/index.js#L867

Because the original request was made by the appnexus bidder alone, these incoming extra bids (that look like they came from other bidders) can't be properly mapped to the appnexus request. We don't know all the time which bidders will respond for a request, so it could potentially change in real-world scnearios.

Given this logic is ran through the PBS bid adapter (which has it has to support its use-cases for the main PBS endpoints), I had some potential thoughts on different ways this use-case may be supported:

alter the logic within the request payload to potentially pass in the bidId value (that's created for each bidder in each adUnit) > have the system keep a reference to this value and return it in the bid response - to signify which 'request' this bid is meant to associated to. This approach would in theory replace the internal mapping logic done within the PBS Bid adapter file. Perhaps we could use the ext fields somehow to pass along this data? I'm honestly not sure if that would work in the expectations of the ORTB spec itself.
create a retroactive bidRequest object when we can't find the original. I did some initial tests with this approach and I can put up a draft PR to show-case what it may look like. We may want to consider creating a proper retroactive bidderRequest object as well to have it work fully with the other features of prebid that look up bidderRequest/bidRequest to store/read values.
create a one-off block of code (activated by reading the endpoint) that utilizes the appnexus bid request (by hard-coding the bidder-part of the key) and cause the bid to interpreted as an appnexus bid (while the targeting information provided by the PSP endpoint - with its bidder specific keys - would remain intact). I did an initial test on this approach as well; it seemed to work but it created duplicate appnexus keys in the resulting targeting data that was passed to the adserver. Ideally we'd need to edit this logic somehow to not add the duplicate appnexus keys.

These were some potential options; most which need to be more fully explored to ensure it would work without adversely effecting other use-cases.

We strong interest to make this overall use-case (of multiple bids from a single request point) work if possible. If this type of scenario shouldn't be allowed, we'd like to understand/discuss that as well.

Any thoughts/feedback would be appreciated.

dgirardi commented 2 years ago

I don't like the idea of retroactively creating request objects, it's not clear to me how it would work with stuff like this: https://github.com/prebid/Prebid.js/blob/782b32b673a29b7016f7e5791b481edd1053922b/modules/prebidServerBidAdapter/index.js#L920-L922

More in general it just seems like a big abstraction violation - it can potentially create all sorts of weird problems where one makes the assumption that a request happens before the response (which is a reasonable assumption). For example a module author may expect to see a BID_REQUESTED event for all the responses it sees in BID_RESPONSE. Or it may use the bid request as a handle to keep track of work it needs to do only once per request. This is the type of issue that would be hard to track down or even detect in a review.

The first option, recruiting PBS to help mapping back to the right request, seems a better one to me because the assumption it invalidates is not as reasonable: that every response should map 1-to-1 with a single request.

mike-chowla commented 2 years ago

I'm in support of overall use-case (of multiple bids from a single request point). PubMatic would utilize this functionality as well.

fbastello commented 2 years ago

Could the multi-bid module be modified to support that use case? I.e. have the ability to map multi-bids to specific bidder names (rather than have a prefix with incremental number) ?

dgirardi commented 2 years ago

These are the places I found where we need the original request during response handling:

https://github.com/prebid/Prebid.js/blob/822ff85bf56ae4b8f416acd8d45bf58482e5e5db/modules/prebidServerBidAdapter/index.js#L922-L926

(odd that we always pick sizes[0] - is that right?)

https://github.com/prebid/Prebid.js/blob/822ff85bf56ae4b8f416acd8d45bf58482e5e5db/src/auction.js#L455-L462

https://github.com/prebid/Prebid.js/blob/822ff85bf56ae4b8f416acd8d45bf58482e5e5db/src/videoCache.js#L62-L64

https://github.com/prebid/Prebid.js/blob/822ff85bf56ae4b8f416acd8d45bf58482e5e5db/modules/adpod.js#L228-L229

https://github.com/prebid/Prebid.js/blob/822ff85bf56ae4b8f416acd8d45bf58482e5e5db/modules/instreamTracking.js#L43-L55

jsnellbaker commented 2 years ago

Thanks @dgirardi . There's another section in the auction.js that uses it: https://github.com/prebid/Prebid.js/blob/822ff85bf56ae4b8f416acd8d45bf58482e5e5db/src/auction.js#L444-L447

it cascades down later to several targeting key functions, that ultimately to providing the request object for (what I believe) is to allow a publisher to define their own override function for the targeting keys; here: https://github.com/prebid/Prebid.js/blob/822ff85bf56ae4b8f416acd8d45bf58482e5e5db/src/auction.js#L738-L752

dgirardi commented 2 years ago

@jsnellbaker that seems hard to work around, but it does not appear to be documented - so maybe we can drop support for it? https://docs.prebid.org/dev-docs/publisher-api-reference/bidderSettings.html - I see no mention of the bid request being passed in as the second parameter.

dgirardi commented 2 years ago

I am also confused by the use of bidRequest during video caching. This is where it ultimately is used ~~(bid is the request)~~ correction: the bid request is passed as this:

https://github.com/prebid/Prebid.js/blob/822ff85bf56ae4b8f416acd8d45bf58482e5e5db/src/videoCache.js#L75-L78

jsnellbaker commented 2 years ago

We may be able to drop the bid request object from the targeting key function as it's not documented, not sure if that would have to be treated as a breaking in the case that someone actually realized it's there and relied on it. @bretg to share thoughts?

For the video caching - I'm not familiar with this particular config around vasttrack. I think that's a relatively new thing. If they need a timestamp to be present in the cached object, then maybe there's a way to pull in the auction object itself into the file instead of the bidderRequest. If the value doesn't need to be uniform between all the bids/auction, maybe we could see if there's a timestamp like value for when bids are created and use that instead?

It was added through this PR https://github.com/prebid/Prebid.js/pull/5288 and its related issue https://github.com/prebid/Prebid.js/issues/5180.

jsnellbaker commented 2 years ago

@dgirardi for a late reference, below is a test page that someone on my end created that demonstrates the issue: https://codepen.io/adam_xandr/pen/ExvvdeK?editors=1000&pbjs_debug=true

If you open the console and see the undefined error, that stems from the bidRequest not being there in PBS adapter's interpretResponse code.

bretg commented 2 years ago

I'd like to note that the multibid feature is intended to control bids from a single biddercode. i.e. how many times the appnexus or pubmatic bidders can supply bidResponses. The use case covered in this issue is when a bidder configured in PBJS can bid on behalf itself and other bidders.

Each biddercode should be subject to the multibid limit. i.e. unless multibid is turned on, each biddercode gets only one bidResponse. If multibid is configured to 2, each gets 2, etc.

As for utilizing bidRequest in bidderSettings functions, that sounds like an unfortunate breaking change, even if currently undocumented. Publishers could be making use of it and we wouldn't know. Which means we ought to configure for it. e.g.

pbjs.setConfig({
    s2sConfig:{
       ...
       allowUnknownBidderCodes: true,   // defaults to false for now
       ...
    }
});

So the proposal is that if s2sConfig.allowUnknownBidderCodes is false, pbsBidAdapter will use the current behavior. If true, the bidRequest object will not be passed around the ecosystem. Is this feasible or does it double the work, making a big mess?

For the video caching

The vasttrack timestamp is used to help associate events to their auction.

In the scenario where vasttrack is needed, the client-side analytics adapter logs the auction timestamp with the auction event, and the imp/midPoint/etc events would get the same timestamp. So when those events happen (maybe many seconds later), and are logged by the server-side analytics adapter, the offline system can more easily verify that the event matches the auction when sewing the logs together.

It actually doesn't matter which timestamp is used -- just want a timestamp that links the auction to the event. However, setting a breakpoint, I don't see an attribute on bidRequest called 'auctionStart'. My vasttrack test page doesn't even send a timestamp in the post body. On one hand: sigh. On the other hand, problem solved.

It seems to me that a decent data model would tag each bidresponse with an auctionId. Then code that wanted some auction info could lookup by auctionId and just get it.

dgirardi commented 2 years ago

So the proposal is that if s2sConfig.allowUnknownBidderCodes is false, pbsBidAdapter will use the current behavior. If true, the bidRequest object will not be passed around the ecosystem. Is this feasible or does it double the work, making a big mess?

It should be OK, given that it's only one use case so far, we would still "clean up" bid requests from most of the response side.

dgirardi commented 2 years ago

Bid response validation is another case where we currently use the request:

https://github.com/prebid/Prebid.js/blob/8ea96a582572c98b963c7f35e50f922814a67e8b/src/adapters/bidderFactory.js#L479

All of these so far only use the request to access its mediaTypes property, which is almost the same as the adUnit's mediaTypes (which is what is available for "unknown" bidders). "Almost" because size mapping can change it bidder-by-bidder:

https://github.com/prebid/Prebid.js/blob/8ea96a582572c98b963c7f35e50f922814a67e8b/src/adapterManager.js#L79-L85

However there are what look like two separate bugs that act as a workaround:

the PBS adapter does not look at bidder-specific size mapping configuration when building out the request - for the most part bid requests are ignored and everything is built on top of the ad unit. As far as I can tell bidder-level size labels do not work through PBS now. https://github.com/prebid/Prebid.js/blob/8ea96a582572c98b963c7f35e50f922814a67e8b/modules/prebidServerBidAdapter/index.js#L495-L858
size mapping only acts on the banner mediatype, and its validation also does not attempt to respect bidder-level labels, just picks the first one it finds: https://github.com/prebid/Prebid.js/blob/8ea96a582572c98b963c7f35e50f922814a67e8b/src/adapters/bidderFactory.js#L461-L463

So we can keep basically the same behavior by mapping bid responses back to their ad unit (instead of bid request). I am not sure if doing so will make it harder to fix the bugs above: if two bidders with different labels (and thus requesting different sets of banner sizes) will get translated as two different imp objects in the request then it should pose no problem. But if size labels will get encoded somewhere in imp[].ext.prebid.[bidder] then we will have the problem of guessing which bidder was used as the intermediary.

dgirardi commented 2 years ago

More instances of bid request currently being used during response processing. At this point I believe I'll try to address some of these in separate PRs so keeping track of them here:

priceFloors module - should be able to reproduce same behavior with just the adUnit https://github.com/prebid/Prebid.js/blob/4e3173e2f66d820259b946a00a2052732309d44e/modules/priceFloors.js#L667
mass module - exposes request to custom publisher code and needs to be guarded by allowUnknownBidderCodes config flag https://github.com/prebid/Prebid.js/blob/4e3173e2f66d820259b946a00a2052732309d44e/modules/mass.js#L95-L102

bretg commented 2 years ago

All good finds @dgirardi . My proposal is that the pbsBidAdapter continues to be simple when it comes to labels and sizemappings.

I've already got it on my list to detail the requirements for a "Conditional Bidder" module in Prebid Server that would be able to handle the underlying requirements: skipping bidders or bidder/mediatype combinations based on request params. There are some details to work out, e.g. pbsBidAdapter doesn't currently pass viewport size by default. But there's another open issue where a number of additional fields are likely to be added that should cover this.

dgirardi commented 2 years ago

@bretg an additional requirement should then be that the client-side knows which mediatypes where considered (or, equivalently, skipped) for each seatbid in the response. To avoid the same issue of client asking "bidderA only if condition X, bidderY if Y" but getting back a bidderC and not knowing which condition it should satisfy.

prebid / Prebid.js