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

When prebid server issues a no-bid response, call addBidResponse for every adUnit requested #1204

Merged
merged 7 commits into from
May 19, 2017

Conversation

dmitriyshashkin
Copy link
Contributor

Type of change

  • Bugfix

Description of change

Currently prebid-server issues single no-bid response for all the add units and the response doesn't have the ad unit code field (the only exception is indexExchange, server side has different logic just for this bidder). Yet addBidResponse is being called and this non-existing field is passed as it's first argument. addBidResponse rejects such call, so prebid keeps waiting for response until timeout happens. I've decided that if there is no ad unit specified in response I should call addBidResponse for all ad units requested by the bidder. Not sure if that's the right approach but it solves the issue.

Other information

Fixes #1196

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.

Thanks for the PR! Looks good, just one slight update required.

bidmanager.addBidResponse(bidObject.adUnitCode, bidObject);
if (!bidder.ad_unit) {
utils.getBidderRequestAllAdUnits(bidder.bidder).bids.forEach(bid => {
let bidObject = bidfactory.createBid(STATUS.NO_BID);
Copy link
Member

Choose a reason for hiding this comment

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

Pass through the bid object as the 2nd param to createBid so we can get the ID (see line 78 below as an example)

@dmitriyshashkin
Copy link
Contributor Author

dmitriyshashkin commented May 17, 2017

@mkendall07 fixed! I'm passing bid request object to createBid, also added new test into spec file to make sure that bid request id is used as bid's ad id. Also merged changes from master and changed code formating in accordance with the new liting rules

@dmitriyshashkin
Copy link
Contributor Author

@mkendall07 upon further testing I found that I missed few cases.

  1. If there is a bid for one unit and no bid for the other, there is no no_bid response, instead there is response with num_bids less then bids requested. And again in this case auction ends only after timeout bacause bidsBackAll always returns false.
  2. Another case is when istead of no_bid server responds with no_cookie. As I understood from my analysis of server-side code, if server generates no_cookie response, it doesn't even query the exchange for bids. So again there should be a status 2 response for every ad unit.
  3. Specifically for indexExchange in case of no_bid response I should call addBidResponse as many times as there are sizes in the bid request for the current ad unit

I'm not quite sure if I should adress these in the same PR or I can create a new one for that purpose.

@mkendall07
Copy link
Member

Thanks for the analysis. Sounds like #1 might be a prebid server bug - I'll take a look. 2 should be straightforward and I think should be included in this PR. #3 I don't generally like the approach so I'd skip it for no.

@dmitriyshashkin
Copy link
Contributor Author

Example of the response for the first case below. Request included two different ad units. Can post full request payload too, if needed.

{
        "tid": "0b54cd48-934f-40ef-bd36-f1e6a9a7ba4b",
        "status": "OK",
        "bidder_status": [{
            "bidder": "rubicon",
            "response_time_ms": 150,
            "num_bids": 1
        }],
        "bids": [{
            "bid_id": "2c4a7ec8d3c126",
            "code": "ad_cs_9493688_728_90",
            "creative_id": "4227344",
            "bidder": "rubicon",
            "price": 0.226596,
            "adm": "%ad code here%",
            "width": 728,
            "height": 90
        }]
}

Server side has the following logic (pbs_light.go):

} else if bid_list != nil {
	bidder.NumBids = len(bid_list)
	am.BidsReceivedMeter.Mark(int64(bidder.NumBids))
	for _, bid := range bid_list {
		ametrics.PriceHistogram.Update(int64(bid.Price * 1000))
		am.PriceHistogram.Update(int64(bid.Price * 1000))
	}
} else {
	bidder.NoBid = true
	ametrics.NoBidMeter.Mark(1)
}

So if there's at least one bid, NoBid flag is not set.

@dmitriyshashkin
Copy link
Contributor Author

@mkendall07 added no bid response generation for the # 2 (when user sync is requested). Waiting for your review. And I'll add an issue for # 3

@mkendall07
Copy link
Member

LGTM. Can you also open a ticket for #1 ? after review, I think this should be fixed in the adapter, but it can be separate PR. Thanks!

@mkendall07 mkendall07 merged commit baff871 into prebid:master May 19, 2017
outoftime pushed a commit to Genius/Prebid.js that referenced this pull request May 24, 2017
…built

* 'master' of https://github.com/prebid/Prebid.js: (23 commits)
  Increment pre version
  Probed 0.24.0 Release
  Beachfront adapter - add ad unit size (prebid#1183)
  Thoughtleadr adapter - fix postMessage (prebid#1207)
  When prebid server issues a no-bid response, call addBidResponse for every adUnit requested (prebid#1204)
  Improvement/timeout xhr (prebid#1172)
  Add native support (prebid#1072)
  Improvement/alias queue (prebid#1156)
  Updated documentaion (prebid#1160)
  Improvement/prebid iframes amp pages (prebid#1119)
  Fixes prebid#1114 possible xss issue (prebid#1186)
  Allowed setTargetingForGPTAsync() to target specific ad unit codes. (prebid#1158)
  updated tag (prebid#1212)
  Common user-sync (prebid#1144)
  Rename secureCreatives file and lint (prebid#1203)
  HIRO Media: Remove batching mechanism and use AJAX instead of JSONP (prebid#1133)
  Add Support for DigiTrust in Rubicon Adapter (prebid#1201)
  Upgrade linters to ESLint with stricter code style (prebid#1111)
  Add dynamic bidfloor parameter to Smart Adserver Adapter (prebid#1194)
  Bug fix: bids served by secure creatives does not get pushed into _winningBids (prebid#1192)
  ...
vzhukovsky added a commit to aol/Prebid.js that referenced this pull request Jul 17, 2017
….23.0 to aolgithub-master

* commit '136fc37637749a764070c35c03e7e87a5c157947': (33 commits)
  Added changelog entry.
  Implemented passing key values feature.
  Update code to ESlint rules.
  Prebid 0.24.1 Release
  tests: drop ie9 browserstack test
  Audience Network: separate size from format (prebid#1218)
  Bugfix/target filtering api fix (prebid#1220)
  Map sponsor request param to endpoint param (prebid#1219)
  Increment pre version
  Probed 0.24.0 Release
  Beachfront adapter - add ad unit size (prebid#1183)
  Thoughtleadr adapter - fix postMessage (prebid#1207)
  When prebid server issues a no-bid response, call addBidResponse for every adUnit requested (prebid#1204)
  Improvement/timeout xhr (prebid#1172)
  Add native support (prebid#1072)
  Improvement/alias queue (prebid#1156)
  Updated documentaion (prebid#1160)
  Improvement/prebid iframes amp pages (prebid#1119)
  Fixes prebid#1114 possible xss issue (prebid#1186)
  Allowed setTargetingForGPTAsync() to target specific ad unit codes. (prebid#1158)
  ...
dluxemburg pushed a commit to Genius/Prebid.js that referenced this pull request Jul 17, 2018
…every adUnit requested (prebid#1204)

* No bid response has no ad unit, create bid response for every ad unit requested

To fix prebid#1196

* Test to cover cases when response has no bids

* Pass through the bid object to link request and response

* Some fixes to support new linting

* Populated wrong field

* Generate status NO_BID response if server requested usersync
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

NoBid response from prebid server has no adUnitCode, addBitResponse discards it
4 participants