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

add workaround to prevent IX adapter from ending auction early #763

Merged
merged 1 commit into from
Nov 1, 2016

Conversation

protonate
Copy link
Collaborator

Type of change

  • Bugfix

Description of change

add workaround to prevent IX adapter from ending auction early

@protonate
Copy link
Collaborator Author

@mkendall07 @matthewlane for review

@mkendall07
Copy link
Member

LGTM.

@mkendall07 mkendall07 merged commit 01da63b into master Nov 1, 2016
@dugwood
Copy link
Contributor

dugwood commented Nov 3, 2016

@protonate why is it only for this bidder? I'm having the same issue with Rubicon: #496 (comment)

With the current code:

 1505 Request bid for rubicon
 1899 rubicon:    Time: 392
 1911 All bids are back (or timeout was hit): 406ms
 1912 bid #0: rubicon     CPM: 2.255  Time: 392
 1935 Winning bid: rubicon (price bucket: 2.20)
 2128 rubicon:    Time: 622

With a fix such as bid.bidder === 'indexExchange' || bid.bidder === 'rubicon':

 1506 Request bid for rubicon
 1916 rubicon:    Time: 410
 1971 rubicon:    Time: 466
 1976 All bids are back (or timeout was hit): 471ms
 1977 bid #0: rubicon     CPM: 2.255  Time: 410
 1977 bid #1: rubicon     CPM: 4.679  Time: 466
 2001 Winning bid: rubicon (price bucket: 4.60)

I already hit this when I was struggling in my #700 pull request.

I'll send a PR for this, at least for Rubicon and RubiconLite (I'll test this one).

Okay, I know why you don't do it for all bidders: Criteo (that I'm testing in a beta preview) sends only one request for 3 zones with 2 sizes each, where Rubicon would launch 6 requests. So it depends on the bidder... And even more, it depends on the setup of Rubicon for each account.

This should be a parameter in the adUnits sent to addAdUnits, so that it doesn't break everything for currently valid setups.

@mkendall07
Copy link
Member

mkendall07 commented Nov 3, 2016

I don't think it should be a parameter in the adUnits. We need standardized behavior across the bidders and in exception cases the bidder should tell us automatically how many bids they expect. Previously we had an expectedBids param that could be set. I think we can bring this back.

@dugwood
Copy link
Contributor

dugwood commented Nov 3, 2016

Thanks @mkendall07, but as you can see in my latest comment (#496 (comment), sorry for the cross posting...), there can't be a standard as it depends on the good will of bidders to setup correctly. And I must add that the Rubicon setup with fallback to another size is very bad, as you may have a better bid on the fallback than on the current bid, but you'll never know. And if you count the bids, you'll break current setups. So the end user must have a parameter for this (that could be set as default, for instance with IX).

EDIT: Rubicon has a different setup for each user account, so mine is different from another one. There's no way to detect this behaviour within Prebid, that's why we need a parameter for this!

As you said there was expectedBids, can you point me to this code?

@dugwood
Copy link
Contributor

dugwood commented Nov 3, 2016

I can see it here:

//bidmanager.setExpectedBidsCount(ADAPTER_CODE, expectedBids);

But it's outside the user's scope. Meaning I can't set it for my Rubicon setup if I don't change the Rubicon adapter. So it must be set outside of the adapter.

Can I add a parameter for this? Would be set here:

{
    bidder: 'rubicon',
    params: { ... },
    oneRequestPerSize: true
}

And I'll remove all the expectedBids that are no longer used.

@protonate protonate deleted the bugfix/ix-adapter-bid-counts branch November 3, 2016 20:39
@protonate
Copy link
Collaborator Author

@dugwood have a look at this PR #773. Would this approach resolve your issues with Rubicon adapters?

@dugwood
Copy link
Contributor

dugwood commented Nov 7, 2016

@protonate it's a great PR, but I don't think it will help, as Rubicon setup changes from one account to another. So for two different accounts, you can have one that uses the «fallback» and only returns one bid and one only, and the other without fallback that will fire multiple bids. Maybe we can find a solution within the Rubicon adapter, to detect the different setups. Maybe :-)

@dugwood
Copy link
Contributor

dugwood commented Nov 7, 2016

I've narrowed it down to this piece of code, by comparing my setup and the one from another website with fallback (I've removed all useless variables):

window.rubicontag.loadConfiguration({
    "configuration": {"useMultiSize": true},
});
window.rubicontag.init();

So, the good news is that there's a variable in the /header/accountID.js with Rubicon.

I'm trying to create a patch for Rubicon adapter based on your PR (too bad it's not already merged!).

@dugwood
Copy link
Contributor

dugwood commented Nov 7, 2016

The bad news: I just can't access the variable. It's hidden in the main object Rubicon create, and there's no public function getConfigValue. So there's no way to find out about this value, unless Rubicon returns it in the setIntegration function.

Anyway, the simple use of useMultiSize would be wrong, as Rubicon also tests for other cases using useSingleRequest (XHR and tests like that).

So, again, we're stuck with Rubicon bad implementation (the adapter can't do anything), same as in #496 (comment)

I'll ask my Rubicon contact to send this to their engineers to fix this, but I've no real hope here...

@dugwood
Copy link
Contributor

dugwood commented Nov 9, 2016

@protonate I did some more tests today. It's even worse than before (the ad size is set by the first bid returned, so sometimes I have a 728x90 in a frame of 1000x300!).

Anyway, I've just read the following info:
#783
#782 (comment)

Hence I'm getting rid of rubicon, and restarted on rubiconLite. My main issue is that rubiconLite always answers only with one size, even if the url clearly states that there are multiple alt sizes.

So I guess that's another Rubicon bad setup on my account. I'm asking them right now to add the useMultiSize so that I can test it correctly.

That would remove the need for expectedBids if rubicon adapter is fully removed as it should.

I'll keep you posted. This is a big issue, that'll make anyone thinks it's a Prebid issue whereas it's a Rubicon issue in their account setup.

@mkendall07
Copy link
Member

Hey @dugwood
You said:

My main issue is that rubiconLite always answers only with one size

I think this ideal isn't it? Rubicon is answer with the highest CPM ad that matches the given size(s). Seems correct to me unless I'm missing something?

@dugwood
Copy link
Contributor

dugwood commented Nov 9, 2016

@mkendall07 yes, that would be great if it was working correctly, but, as I've just stated here: #496 (comment), if you don't have the MAS enabled, you'll have only one size requested and not all the sizes.

But you're right, it only sends one request, so no need for countBids() here. As the rubicon old adapter is removed, that makes things easier, but there are a few bugs to address.

Anyway, for this very PR, we're good!

@mkendall07
Copy link
Member

Right, I wrote that before I read your other comment. Makes sense now. I think Rubicon is on the right track.

mp-12301 pushed a commit to aol/Prebid.js that referenced this pull request Apr 10, 2017
…ebid-0.14.0 to release/1.7.0

* commit '3eefcf043466f5457c81bfec18b032b53490b78b': (52 commits)
  New adapters ids.
  Prebid 0.14.0 Release
  add workaround to prevent IX adapter from ending auction earlier due to one request becoming many responses (prebid#763)
  Add pbjs.getHighestCpmBids for getting winning bids (prebid#755)
  Fix build
  Drop Sekindo adapter.
  Bugfix/suppress prebid request if sizeMapping undefined for device width (prebid#758)
  amp integration (prebid#756)
  Add contribution guidelines (prebid#761)
  Add api method to shuffle the order bidders are called in (prebid#760)
  build adapter from custom source path (prebid#753)
  reduce duplication and ignore ga.js in coverage (prebid#754)
  Log /ut response errors in dev console (prebid#747)
  Fix indentation (code style error when building) (prebid#751)
  Add package keywords (prebid#746)
  added new rubiconLite adapter (prebid#740)
  Show warning if bidCpmAdjustment is set for AOL bidder (closes prebid#725) (prebid#728)
  IX adapter code refactoring (prebid#711)
  Update memeglobal.js (prebid#737)
  Pulsepoint Analytics adapter for Prebid (prebid#706)
  ...
mp-12301 pushed a commit to aol/Prebid.js that referenced this pull request Apr 10, 2017
…7.0 to master

* commit '262f371a5c855419c3ef357fb1f0cf87a107749f': (52 commits)
  New adapters ids.
  Prebid 0.14.0 Release
  add workaround to prevent IX adapter from ending auction earlier due to one request becoming many responses (prebid#763)
  Add pbjs.getHighestCpmBids for getting winning bids (prebid#755)
  Fix build
  Drop Sekindo adapter.
  Bugfix/suppress prebid request if sizeMapping undefined for device width (prebid#758)
  amp integration (prebid#756)
  Add contribution guidelines (prebid#761)
  Add api method to shuffle the order bidders are called in (prebid#760)
  build adapter from custom source path (prebid#753)
  reduce duplication and ignore ga.js in coverage (prebid#754)
  Log /ut response errors in dev console (prebid#747)
  Fix indentation (code style error when building) (prebid#751)
  Add package keywords (prebid#746)
  added new rubiconLite adapter (prebid#740)
  Show warning if bidCpmAdjustment is set for AOL bidder (closes prebid#725) (prebid#728)
  IX adapter code refactoring (prebid#711)
  Update memeglobal.js (prebid#737)
  Pulsepoint Analytics adapter for Prebid (prebid#706)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants