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.

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

Rubicon Adapter allow multi bids configuration per adUnit #496

Closed
cwbeck opened this issue Aug 2, 2016 · 41 comments
Closed

Rubicon Adapter allow multi bids configuration per adUnit #496

cwbeck opened this issue Aug 2, 2016 · 41 comments
Assignees
Labels

Comments

@cwbeck
Copy link

cwbeck commented Aug 2, 2016

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:

  • Bid [728x90] rubicon @ $0.42 | RT: 438ms
  • Bid [728x90] rubicon @ $0.42 | RT: 438ms

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..

...

  • Bid [728x90] pulsepoint @ $0.18 | RT: 312ms
  • Bid [970x250] pulsepoint @ $0.03 | RT: 318ms
  • Bid [728x90] indexExchange @ $0.44 | RT: 408ms
  • Bid [970x250] indexExchange @ $0.02 | RT: 409ms
    ...
@bretg
Copy link
Collaborator

bretg commented Aug 2, 2016

We're looking into this.

@protonate
Copy link
Collaborator

This issue may be related to this fix #498

@bretg
Copy link
Collaborator

bretg commented Aug 3, 2016

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
Copy link
Collaborator

bretg commented Aug 3, 2016

@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
Copy link
Author

cwbeck commented Aug 4, 2016

@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
Copy link
Member

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

@snapwich
Copy link
Collaborator

snapwich commented Aug 4, 2016

@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.

snapwich added a commit to rubicon-project/Prebid.js that referenced this issue Aug 4, 2016
@cwbeck
Copy link
Author

cwbeck commented Aug 5, 2016

@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
Copy link
Author

cwbeck commented Aug 5, 2016

@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
Copy link
Contributor

mercuryyy commented Aug 5, 2016

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
Copy link
Author

cwbeck commented Aug 5, 2016

@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
Copy link
Collaborator

snapwich commented Aug 5, 2016

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
Copy link
Collaborator

snapwich commented Aug 5, 2016

@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.

snapwich added a commit to rubicon-project/Prebid.js that referenced this issue Aug 5, 2016
@cwbeck
Copy link
Author

cwbeck commented Aug 6, 2016

@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
Copy link
Collaborator

snapwich commented Aug 6, 2016

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
Copy link
Collaborator

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
Copy link
Author

cwbeck commented Aug 10, 2016

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

@mkendall07
Copy link
Member

Hey all - currently reviewing this and will reply shortly.

snapwich added a commit to rubicon-project/Prebid.js that referenced this issue Aug 11, 2016
@snapwich
Copy link
Collaborator

@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
Copy link
Member

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
Copy link
Author

cwbeck commented Aug 12, 2016

@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 pushed a commit that referenced this issue Aug 17, 2016
* allows multiple bids to be registered per a slot, related to #496

* keep track of bid request in rubicon adapter to use when calling createBid
@mkendall07
Copy link
Member

@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
Copy link
Collaborator

bretg commented Aug 18, 2016

@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
Copy link
Member

@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
Copy link
Collaborator

bretg commented Aug 18, 2016

@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.

protonate pushed a commit that referenced this issue Aug 20, 2016
* allows multiple bids to be registered per a slot, related to #496

* keep track of bid request in rubicon adapter to use when calling createBid
@mkendall07 mkendall07 changed the title Rubicon Adapter Issue Rubicon Adapter allow multi bids configuration per adUnit Sep 7, 2016
marian-r added a commit to aol/Prebid.js that referenced this issue Sep 23, 2016
…2.0 to master

* commit '40fdbed10c218a993df9e6665797b36f402ded18': (52 commits)
  Added new adapter for AOL analytics
  Fixed ES6 features which require a polyfill
  Updated CHANGELOG
  Fixed per-adapter timeouts
  Fixed sorting of per-adapter timeouts
  Added horizontal rule to delimit the original README part
  Link to the AOL documentation made more visible
  Fixed unit tests for AOL analytics
  Made changes required due to pull from Prebid official
  Updated README
  Updated LICENSE
  Reverted AOL branding
  restore url.js and modifcations to ajax.js (prebid#551)
  Log unsupported ad type only for good bids (prebid#547)
  use var ad instead of incorrect ads in rubicon adapter (prebid#546)
  Karma opens debug.html by default (prebid#540)
  Prebid 0.12.0 release
  Add tests for triplelift adapter. (prebid#531)
  allows multiple bids to be registered per a slot, related to prebid#496 (prebid#503)
  Add tests for aardvark adapter. (prebid#529)
  ...
@dugwood dugwood mentioned this issue Oct 13, 2016
8 tasks
@dugwood
Copy link
Contributor

dugwood commented Oct 28, 2016

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
Copy link
Author

cwbeck commented Oct 28, 2016

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

@dugwood
Copy link
Contributor

dugwood commented Nov 3, 2016

@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
Copy link
Contributor

dugwood commented Nov 3, 2016

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

  • one bid, without sizes parameter: double hit on Rubicon fastlane, but the second is ignored because of timeout
  • one bid, sizes set to Rubicon's specific sizes (see https://github.com/prebid/Prebid.js/blob/master/src/adapters/rubicon.js#L20): same effect
  • two bids, with specific sizes (only one): Rubicon will call twice the fastlane, the timeout won't interfer, but both calls are on the same size (overriden by their code, outside of Prebid's scope).

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
Copy link
Contributor

dugwood commented Nov 3, 2016

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
Copy link
Contributor

dugwood commented Nov 3, 2016

As I said here: #763 (comment)

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
Copy link

@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
Copy link
Contributor

dugwood commented Nov 3, 2016

@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
Copy link
Contributor

dugwood commented Nov 3, 2016

@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
Copy link
Contributor

dugwood commented Nov 9, 2016

@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
Copy link
Contributor

dugwood commented Nov 9, 2016

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

So:

  • there's a bug with multiples bids of one size (as I already sent them): all bids get the same size_id
  • there's a bug if your account doesn't have MAS (multiple ad sizes): only the first size is requested (even if alt_size_ids is present in the url)

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
Copy link
Contributor

dugwood commented Nov 10, 2016

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
Copy link
Member

@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
Copy link
Collaborator

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
Copy link
Contributor

dugwood commented Nov 29, 2016

@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
Copy link
Member

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

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

No branches or pull requests

8 participants