prebid / Prebid.js

Setup and manage header bidding advertising partners without writing code or confusing line items. Prebid.js is open source and free.
https://docs.prebid.org
Apache License 2.0
1.33k stars 2.08k forks source link

Rubicon Adapter allow multi bids configuration per adUnit #496

Closed cwbeck closed 7 years ago

cwbeck commented 8 years ago

This is Rubicon only, all other adapters appear to be working as expect for us. Sending out a request with two sizes (728x90 and 970x250) will result in the following:

I wonder if a deep copy is not being made somewhere it should. Both requests go out for sizeId 2 and 57, both come back in, but 970x250 is always ignored and 728x90 response is cloned.

screen shot 2016-08-02 at 15 39 53

All other adapters work correctly..

...

bretg commented 8 years ago

We're looking into this.

protonate commented 8 years ago

This issue may be related to this fix #498

bretg commented 8 years ago

We reproduced the problem in the enableSendAllBids() scenario. The Rubicon adapter is only registering the highest bid, which causes the platform to duplicate that bid. The 728x90 normally has the higher bid, explaining why that size is the one seen most often.

A fix is underway.

bretg commented 8 years ago

@cwbeck - how are you getting the log entries "Bid [728x90] rubicon @ $0.42 | RT: 438ms" ? We have other evidence, but we'd like to make sure the issue is fixed for you as well.

cwbeck commented 8 years ago

@bretg thanks for your quick response on this. We have a custom abstraction over prebid.js. We have a layer in our own debug setup that gives us this view so our adops can see what is going on.

We also colour-code the output, so we can see what is going on for each ad-unit. It's actually proved to be very useful.

screen shot 2016-08-04 at 11 58 42

Console in Chrome: -

screen shot 2016-08-04 at 11 58 54

mkendall07 commented 8 years ago

@cwbeck That is pretty slick! Is that something that you'd be willing to commit to prebid.js?

snapwich commented 8 years ago

@cwbeck I'm working with @bretg on this issue and I'm actually having a bit of a hard time recreating it. However, I think I've fixed what appears to be an issue in the registering of multiple bids from rubicon per adUnit.

Can you give me more of an example of how your adUnits are configured so I can properly duplicate the issue and confirm the fix?

edit - or if you would like, pull the commit linked below and re-test.

cwbeck commented 8 years ago

@mkendall07 This is currently outside of prebid.js library, but it should be easy enough to move into the render part of prebid.js - we do augment the data with a few extra bits however.

Public gist of our render.js - https://gist.github.com/cwbeck/bdea9f22ceed8af489fb047f357f77cc

I'll see what I can do!

cwbeck commented 8 years ago

@snapwich @bretg I have applied the latest Rubicon adapter from https://github.com/rubicon-project/Prebid.js/blob/967f2da600d2406c7eb708f5296c0742bf4cd0be/src/adapters/rubicon.js

I'm still seeing the same issue as before: - (on all sites)

screen shot 2016-08-05 at 10 32 31

Where [Object, Object] is passed directly to pbjs.addAdUnits(adUnits);

Result: -

screen shot 2016-08-05 at 10 33 10

We form the bid requests are per prebid.js documentation - we don't do anything special here.

Hope that helps!

-- edit...

This is how it looks before processed by pbjs.addAdUnits(adUnits); function: -

screen shot 2016-08-05 at 10 41 00

mercuryyy commented 8 years ago

why not just use something like

function effectiveDeviceWidth() { var deviceWidth = window.orientation == 0 ? window.screen.width : window.screen.height; if (navigator.userAgent.indexOf('Android') >= 0 && window.devicePixelRatio) { deviceWidth = deviceWidth / window.devicePixelRatio; } return deviceWidth; } if (effectiveDeviceWidth() > 1400) { var tsize = [970, 90]; var tsizeid = 43; } else { var tsize = [728, 90]; var tsizeid = 2; } var PREBID_TIMEOUT=750, adUnits=[{ code:"div-gpt-ad-xxx-1", sizes:[tsize], bids:[{ bidder:"rubicon", params: { accountId: "xxx", siteId: "xxx", zoneId: "xxx", sizes: [tsizeid] } // rest of your config.

this works well for me, then you also save on calling 2 ads you can set the min width to w/e you want or simply have it random, of course it will never out preform calling 2 ads and picking the highest bidder but for now im using this.

cwbeck commented 8 years ago

@mercuryyy , thanks, but I don't think you fully understand the thread! This has nothing at all todo with screen size. This is a promo tag issue.

snapwich commented 8 years ago

Thanks @cwbeck, I've been able to recreate the issue with the adUnit config you provided. I was originally working on the assumption that you were doing one placement with two possible sizes, which apparently has an issue as well (so I'll leave my earlier fix in there). I'll see if I can discover what's going on here and provide more info / an additional fix.

snapwich commented 8 years ago

@cwbeck in the first screenshot you provided it appears that you are reusing the same code across two different adUnits, which should be unique: http://prebid.org/dev-docs/publisher-api-reference.html#module_pbjs.addAdUnits

That code maps to a slot id in the rubicon adapter which will return the existing slot object if there is an attempt to define a new slot with the same id. If you are indeed using non-unique codes I'm not sure why, or if it even should work in the other adapters.

If you are attempting to run an auction with a single adUnit code that requires multiple sizes, you can list the sizes in the array of the original adUnit.

Let me know if I'm misunderstanding since I'm only assuming you are reusing the same code value because of the placementCode in the second adUnit, and I'm not sure if those keys map directly.

@mkendall07 can probably correct me if I'm making any mistakes; I'm new to the Prebid.js world myself.

cwbeck commented 8 years ago

@snapwich, thanks for looking into this. You are correct, we could provide two sizes, but that would only a be a single auction. We have deliberately forced two auctions for a promo tag as the results have proved to be better (only with certain partners).

The object we build is correct as per the docs. It should allow multiple bids to come back in this way. We designed everything around this on our side.

screen shot 2016-08-05 at 10 41 00

The first screenshot the data has been augmented by the prebid.js library. We don't set most of those values.

-- edit - also worth pointing out, the pre-fastlane adapter used to work like this.

snapwich commented 8 years ago

You are correct, we could provide two sizes, but that would only a be a single auction.

That's not true in the case of the Rubicon adapter, currently there are two auctions performed, one for each size, and the bid with the higher CPM is registered with Prebid. When the pull-request I've attached to this issue is merged, both bids will be registered.

Are you sure the other adapters don't behave similarly?

As to whether the adUnits should accept a duplicate code value to run multiple auctions, I'll have to defer to @mkendall07's expertise. Currently I interpret the documentation as saying that the code value should not be duplicated across adUnits. If I'm mistaken, I don't think we'd have any problem making adjustments to behave accordingly.

protonate commented 8 years ago

We recently merged #498 and now adapters can create bid responses with an ID that matches the bid request ID. See the Sovrn adapter in that PR for an example of passing the bid request object into bidfactory.createBid. This allows support for multiple bids per placement and multiple ad units per placement in the same auction, as the renderAd function will be able to locate the source of the bid. @snapwich if you can update #503 to use this approach it should solve these issues.

cwbeck commented 8 years ago

@protonate thanks for the update. @snapwich more than happy to test your fork when ready.

mkendall07 commented 8 years ago

Hey all - currently reviewing this and will reply shortly.

snapwich commented 8 years ago

@protonate @cwbeck, I've updated my pull-request to allow the bidRequests to be attached to the createBid call. However, I don't think this will fix the root issue of using duplicate adUnit.code which breaks the rubicon slot definitions.

On a side note, using duplicate adUnit.codes doesn't just break the Rubicon adapter. To verify, I did a quick audit of some other Prebid.js code and found other places that will break inside Prebid.js if adUnit.code values are allowed to be duplicated.

https://github.com/prebid/Prebid.js/blob/master/src/bidmanager.js#L46 Since requested will only contain bids for the first adUnit for a given adUnit.code, and received will contain all the bids across all adUnits for a given adUnit.code, if there are adUnits with duplicate codes, the trigger at https://github.com/prebid/Prebid.js/blob/master/src/bidmanager.js#L120 will fire early

https://github.com/prebid/Prebid.js/blob/master/src/bidmanager.js#L74 With duplicate adUnit.codes there will be multiple bidRequests with the same placementCode. Since .find will always only return the first bidRequest to match, it's possible this will create mismatches

https://github.com/prebid/Prebid.js/blob/master/src/prebid.js#L459 This code loops forward through the adUnits array while mutating it to delete entries. That's actually an anti-pattern that could have the potential of skipping entries as well as accessing out-of-bounds, although is unlikely to cause bugs unless you have duplicate adUnit.codes. This probably needs to be fixed regardless, I'll submit a pull-request.

I didn't check if this breaks code in any other adapters.

mkendall07 commented 8 years ago

adUnit.code should always be unique, what we had allowed previously was for a specific adUnit to wire up multiple requests to the same bidder (1-to-many). Example:

    var adUnits = [{
                code: 'code',
                sizes: [[300, 250], [300, 600]],
                bids: [{
                        bidder: "rubicon",
                        params: {
                            accountId: "123",
                            siteId: "123",
                            zoneId: "1234",
                            sizes: [15]
                        }
                    },{
                        bidder: "rubicon",
                        params: {
                            accountId: "123",
                            siteId: "123",
                            zoneId: "1234",
                            sizes: [10]
                        }
                    }
                    ]
            }];

I'm not clear on exactly the case you would want to do this (Does it have to do with multiple sizes not being supported in a single request?) At any rate, it would be specific to bidder. The above may not be working 100% now with the recent refactors we have done to the prebid.js core, although it was never a use case we were working hard to support.

cwbeck commented 8 years ago

@mkendall07 that is exact setup we use (your Rubicon example). Is this now considered deprecated? If so we can refactor, although not all adapters full support multiple size arrays last time I checked.

mkendall07 commented 8 years ago

@snapwich @cwbeck I merged #503 as it addressed the issue of multiple sizes for bid requests in a single bidder configuration. Note this doesn't solve for a 1-to-many adunit to bidder configuration scenario.

@snapwich Would you be able to update Rubicon Project adapter to support the configuration example I posted above? Note that this is not the same thing as allowing duplicate adUnit codes, rather it is allowing multiple duplicate bidder configurations per adUnit.

bretg commented 8 years ago

@mkendall07 -- isn't having multiple bids in an AdUnit is functionally the same as having the same AdUnit repeated with different bids? Both scenarios require the system to keep track of disjoint bid parameters and sew them back together into a bidResponse. The examples given above have the same site ID -- just size differs, so that special case is pretty easy to handle. But if the site or zone differ, they're different auction requests - basically prompting an expansion of the definition of "ad slot" to be an array of params objects.

We'll need to consider the impact of this request on the single-request feature we're in the midst of building.

mkendall07 commented 8 years ago

@bretg I'm not convinced it's required, would there be a use case that you would need to request a different zone or site for the same adUnit?

bretg commented 8 years ago

@mkendall07 - there is actually a case of different sites that we're aware of -- publisher cooperatives. We support that scenario in our native header bidding platform, but so far we haven't had that type of request for Prebid scenarios, unless this is one.

How about I put it on our queue -- we should be able to get to it in a couple of weeks, after the current round of development drains.

dugwood commented 8 years ago

FYI I've spent a lot of time implementing Rubicon with no luck. I've just switched to rubiconLite, and it's working great with 2 sizes (one fallbacks to the other). Not ideal, but it doesn't work with the rubicon adapter for now (second bid is ignored, or both bids are done on the same size).

cwbeck commented 8 years ago

@dugwood - cheers for update. one day Rubicon will get there.

dugwood commented 8 years ago

@cwbeck I really doubt that :-) But I think I'll add a fix one day... rubiconLite or rubicon is the same in the end, as there's a fallback from one size to another, which doesn't work with custom bidCpmAdjustment function (say you set a floor higher on a size and not on the other one, you may refuse an ad without knowing the other one). I'll wait for my CEO choice before looking for a fix :-P

dugwood commented 8 years ago

I'm looking into this. Here's what I already know, on either rubicon or rubiconLite:

So I'm working on the first case, which is the simplest to me (the second is very similar but the mapper of sizes is working, so why bother).

dugwood commented 8 years ago

Narrowed down to this test: https://github.com/prebid/Prebid.js/blob/master/src/bidmanager.js#L67

As I tried to fix it a while ago (https://github.com/prebid/Prebid.js/pull/700/files#diff-96cd0debf7b766d72d2bcf987385e960R51), there's still an issue here.

I'm looking at all the changes and will provide a PR for this. Currently setting bid.bidder === 'indexExchange' || bid.bidder === 'rubicon' works great.

dugwood commented 8 years ago

As I said here: https://github.com/prebid/Prebid.js/pull/763#issuecomment-258154642

For example: http://www.carmagazine.co.uk/car-reviews/bmw/bmw-5-series-2017-prototype-review/ => only one call to Rubicon, but there's a dedicated setup I don't have on my website: http://optimized-by.rubiconproject.com/a/api/fastlane.json?account_id=15356&site_id=98436&zone_id=461586&size_id=15&p_pos=btf&rp_floor=0.01&rf=http%3A%2F%2Fwww.carmagazine.co.uk%2Fcar-reviews%2Fbmw%2Fbmw-5-series-2017-prototype-review%2F&p_screen_res=1680x1050&tg_fl.eid=mpu-1&alt_size_ids=10&kw=rp.fastlane&tk_flint=custom&tg_i.site=carmagazine.co.uk&tg_i.pagetype=&rand=0.2573140383327752 Answers: { "status": "ok", "account_id": 15356, "site_id": 98436, "zone_id": 461586, "size_id": 15, "alt_size_ids": [ 10 ], "tracking": "", ...

As you can see, there's an alt_size_ids which ensures that there will only be one request to Rubicon (Rubicon will fallback to 10 if 15 doesn't return something).

So, for Rubicon, it depends on the setup of your account! So there's no way to detect this, on the Prebid side, as the alt_size_ids isn't available in the addBidResponse().

My suggestion: add a oneRequest: true to the adUnits along with the bidder and params properties.

caseybryan commented 8 years ago

@dugwood the request is going out for all the sizes but is only returning the highest value size. So as long as your sizes are listed in the array all requests will go out. This is a recent change on how the requests fire within the last couple months.

dugwood commented 8 years ago

@caseybryan this isn't a good approach either: I've setup different floor prices, so I may ignore the bid in 300x600 but I would have accepted a 300x250 with a lower CPM. And this is a setup on the Rubicon account, that's not a default. RubiconLite runs automatically on the alt_size_ids setup, but not the Rubicon adapter. To me that's really difficult to handle, as there are many different setups and no easy way to get all the parameters I want.

dugwood commented 8 years ago

@caseybryan I do think you're wrong. For instance, with rubiconLite: http://fastlane.rubiconproject.com/a/api/fastlane.json?account_id=9314&site_id=18370&zone_id=54808&size_id=57&alt_size_ids=2&p_pos=atf&rp_floor=0.01 => always returns the size 57 http://fastlane.rubiconproject.com/a/api/fastlane.json?account_id=9314&site_id=18370&zone_id=54808&size_id=2&alt_size_ids=57&p_pos=atf&rp_floor=0.01 => always returns the size 2 Even if size 2's CPM is often over the 57's CPM.

So either your server doesn't answer correctly, or you're mistaken on your fallback system. To me, the fallback occurs only when the floor CPM is not met on the first size.

Hence, I have to run 2 separate calls, and then compare these in Prebid, outside of Rubicon servers.

dugwood commented 8 years ago

@cwbeck did you found a solution for this issue? I thought my issue was that I didn't have the useMultiSize in my setup, but I see you have it. Can you try it against the rubiconLite?

The rubicon adapter is clearly no fit to deal with their /header/accountId.js file, which changes without changing the adapter.

dugwood commented 8 years ago

I'm currently dealing all those bugs with Rubicon engineers.

So:

Updating your account to support MAS should fix this. But there's still the issue of counting expected bids, for those who don't have MAS enabled. The rubiconLite adapter can't guess the number of bids because it doesn't know the setup of MAS.

dugwood commented 8 years ago

Rubicon team updated my setup for multiple sizes, and it's now working as expected. Note that I don't see the useMultiSize parameter in my header/XXXX.js because there's some cache and the like.

But requesting size 2 with alt to 57&113 answers both 3 sizes with multiple refreshes. So that's good. And there's still a bug, they working on it.

We should update the documentation stating that rubiconLite needs MAS enabled.

mkendall07 commented 7 years ago

@bretg @snapwich There was several issues in this thread. Are they all resolved with the latest Rubicon adapter? If there are any outstanding issues, can you please open a new ticket to track? I would like to close this if possible.

snapwich commented 7 years ago

I think @dugwood's last issue was #802 which was resolved, and as far as I know there is no other outstanding known issues; so should be fine to close. @dugwood is also correct in that MAS must be enabled to do the multi-size bidding.

dugwood commented 7 years ago

@snapwich is right. To me the main issue reported by @cwbeck is fixed: MAS enabled, multiple fixes and the rubiconLite adapter (Prebid v0.15.0+) did the trick. So @mkendall07 you can close this. Thanks for all the time spent on this :-)

mkendall07 commented 7 years ago

thanks all. @prebid/rubicon when you get a chance can you put in a PR for updating the Rubicon bidder settings page on prebid.org with the info about multiple sizes/MAS. http://prebid.org/dev-docs/bidders.html#rubicon