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

Fix multi bidder prebid server #3618

Closed
wants to merge 4 commits into from
Closed

Conversation

AntoineJac
Copy link
Contributor

@AntoineJac AntoineJac commented Mar 7, 2019

Some bidders such as Pubmatic and Index are using a bid for each ad size as they don’t support multi-size. It is fine on client side except the fact that it will trigger 2 requests.
However, it is NOT working for Prebid server as only the last bid will be triggered server side.

As you can in the following test page, only the last Pubmatic bid (1410742@320x50) is sent to Prebid server and the first one is not present in the request (1410741@320x100):
https://jsfiddle.net/cbqzrjhx/

You can create an alias as a workaround and I have added this logic in the prebidBidServerBidAdapter to add an alias if the adUnit has multiple time the same bidder.

Thanks

Type of change

  • Bugfix
  • Feature
  • New bidder adapter
  • Code style update (formatting, local variables)
  • Refactoring (no functional changes, no api changes)
  • Build related changes
  • CI related changes
  • Does this change affect user-facing APIs or examples documented on http://prebid.org?
  • Other

Description of change

  • test parameters for validating bids
{
  bidder: '<bidder name>',
  params: {
    // ...
  }
}

Be sure to test the integration with your adserver using the Hello World sample page.

  • contact email of the adapter’s maintainer
  • official adapter submission

For any changes that affect user-facing APIs or example code documented on http://prebid.org, please provide:

Other information

@bretg
Copy link
Collaborator

bretg commented Mar 7, 2019

My take - this should really be fixed in Prebid Server, not PBJS. Since the issue has been outstanding for a year, I'd like to understand why the issue can't persist for another couple of weeks while the Go and Java versions of the servers are patched.

Once we do patch this in PBS, this alias workaround would need to be removed.

@AntoineJac
Copy link
Contributor Author

@bretg , agree but the issue is PBJS will also have to pass the bid.bidder property several time.
Current code is "acc[bid.bidder] =" so if same bidder is present multiple time it will override the property value. Some change will have to be done on the PrebidServeBidAdapter.

@bretg bretg requested a review from mkendall07 March 7, 2019 20:14
@bretg
Copy link
Collaborator

bretg commented Mar 7, 2019

ok, but my vote would be to not create aliases. Adding @mkendall07 for his take. In otherwords, I propose the alias part of this PR gets removed.

@AntoineJac
Copy link
Contributor Author

Hi Bret, the logic then will have to be different. Here is the bidder is present several time I am passing it as a new property name (pubmatic, pubmaticAlias_1,...).
I am force to do this as a Json object can't have twice the same property name.

The 3 Solutions I am seeing are:

  • If we keep the same logic we can pass same bidder string (pubmatic, pubmatic1, ...) and you will need in prebid server to do a regex matching the bidder string. Which globally is similar than using an alias...

  • change the acc fields from object to array

  • use the alias workaround

@mike-chowla
Copy link
Contributor

Speaking for PubMatic, we should really fix our ad slot validation in our Prebid Server adapter. The client-side adapter now supports multi-sized ad units but our server-side adapter is failing to validate them. I'll get this taken care of in the PubMatic Prebid Server adapter.

@AntoineJac
Copy link
Contributor Author

@mike-chowla Thanks for the update. Will be indeed better to wait for the PubMatic Prebid Server adapter thus.
@bretg , should we ask other bidder to have a Prebid JS and Prebid Server adapter handling multisize?
If yes we can then close this PR. Thanks,

@mkendall07
Copy link
Member

+1 to Bret's comment. Let's address in PBS.

Copy link
Member

@mkendall07 mkendall07 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rather be fixed in PBS

@AntoineJac AntoineJac closed this Mar 12, 2019
@AntoineJac
Copy link
Contributor Author

I have closed this PR as it will be fixed in PBS! Thanks,

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.

None yet

5 participants