Filter bid arrays using adUnitsFilter function #750

protonate · 2016-10-27T22:43:44Z

Type of change

Bugfix

Description of change

The requested and received bid arrays must retain bid objects from multiple calls to pbjs .requestBids. However this causes unexpected behavior in the reporting APIs as bids from
previous auctions were returned. This PR addresses this by creating a pbjs._adUnitCodes primary
data structure to be used as a filter when Prebid needs only the current "auction". While not
ideal this allows us to retain bid data for in-flight calls to an ad server while also allowing
additional bid requests to be made without the introduction of Prebid Auction instances.

Closes #590

reset externalCallbacks.all.called
use adUnitsFilter to filter bid arrays to current auction

protonate · 2016-10-27T22:59:44Z

@dugwood you will notice here that I've reversed myself on an adUnitCodes data structure. It will be useful to define the filter for now until we've restructured how multiple bid requests are handled. Thanks for the idea.

dugwood · 2016-10-28T07:07:57Z

@protonate yes, that's great I think :-) I've reviewed a bit your code, and there's a lot of changes! Wouldn't it be useful to set a big global object that would group all current global variables?

  $$PREBID_GLOBAL$$._bidsRequested = [];
  $$PREBID_GLOBAL$$._bidsReceived = [];
  $$PREBID_GLOBAL$$._adUnitCodes = [];
  $$PREBID_GLOBAL$$._winningBids = [];
  $$PREBID_GLOBAL$$._adsReceived = [];
  $$PREBID_GLOBAL$$._sendAllBids = false;

I'll try this code today, as I have issues with Rubicon and multiple sizes. Maybe that will fix it too. Rubicon team is working on the issue anyway.

mkendall07 · 2016-10-31T15:05:34Z

@protonate Looks like there is conflicts now. Please resolve thanks

protonate · 2016-11-01T07:48:42Z

@mkendall07 conflicts fixed

mkendall07 · 2016-11-01T16:21:16Z

src/prebid.js

@@ -38,6 +38,7 @@ var eventValidators = {

 $$PREBID_GLOBAL$$._bidsRequested = [];
 $$PREBID_GLOBAL$$._bidsReceived = [];
+$$PREBID_GLOBAL$$._adUnitCodes = [];


this name isn't super intuitive. Maybe just add comment here on what this is.

mkendall07 · 2016-11-01T16:23:23Z

src/prebid.js

  utils.logInfo('Invoking $$PREBID_GLOBAL$$.requestBids', arguments);

  if (!adUnits || adUnits.length === 0) {
    utils.logMessage('No adUnits configured. No bids requested.');
    if (typeof bidsBackHandler === objectType_function) {
      bidmanager.addOneTimeCallback(bidsBackHandler, false);
    }
+


extra line breaks.

mkendall07 · 2016-11-01T16:26:45Z

src/prebid.js

  }

+  // we will use adUnitCodes for filtering the current auction
+  $$PREBID_GLOBAL$$._adUnitCodes = adUnitCodes;


doesn't this clubber a current auction running codes since it's before if (auctionRunning) check?

protonate · 2016-11-01T17:00:02Z

@mkendall07 note addressed

protonate · 2016-11-06T03:53:24Z

@mkendall07 ready for review

protonate · 2016-11-07T03:15:35Z

This PR has been updated and is ready to test. The following changes have been made.

Applies the current adUnitCodes filter when calling reporting APIs getBidResponses, getAdserverTargeting.
Queues a bid request if a previous request is still running, closing over instance data.
Clears previous bids from placements included in current bid request (remove stale bids).

Note that if a placement is included in one request and a second request as well, and the second request is made before the first request has rendered an ad, the bids for the first request will be cleared and the renderAd function will throw an error as the bids cannot be found. However, the placement should be immediately filled with the results of the second request. The error is not caught so as to warn that a placement was overwritten, and care should be taken not to request bids for a placement with a request already in flight.

fixes #772
fixes #590

mkendall07

Just a few comments.

mkendall07 · 2016-11-07T21:11:27Z

src/bidmanager.js

@@ -43,6 +43,7 @@ function getBidders(bid) {
 function bidsBackAdUnit(adUnitCode) {
  const requested = $$PREBID_GLOBAL$$._bidsRequested
    .map(request => request.bids
+      .filter(adUnitsFilter.bind(this, $$PREBID_GLOBAL$$._adUnitCodes))


What's the reason for using bind here?

It's isn't needed here, only in places where the filter may be passed in rather than the current pbjs._adUnitCodes array. I'll remove from here and elsewhere where we can and have utils.adUnitsFilter check for an array passed in and if not found use ad unit codes defined on global.

Actually if we use .bind for processCallbacks to support a single ad unit code or all ad unit codes (for bidsBackAdUnit and bidsBackAll respectively) then we should use the same partial application pattern in all of these cases. This is probably preferable as the ad unit codes to use as a filter can be set as instance data on the requestBids function and we can remove the _adUnitCodes global property.

mkendall07 · 2016-11-07T21:16:32Z

src/bidmanager.js

@@ -273,19 +278,26 @@ exports.executeCallback = function (timedOut) {
  }
 };

+exports.externalCallbackReset = function () {
+  externalCallbacks.all.called = false;


externalCallbacks has evolved into complex object. Should we formalize this into int's own file / class constructor?

I like that idea.

protonate · 2016-11-07T23:02:21Z

@mkendall07 any changes needed to merge?

mkendall07 · 2016-11-09T15:35:12Z

Sorry @protonate I didn't get a chance to test this PR yet. I'll do that today and sans any issues we can merge it.

mkendall07 · 2016-11-09T20:38:06Z

Tested functionality, LGTM. Can merge after resolving conflicts.

The requested and received bid arrays must retain bid objects from multiple calls to `pbjs .requestBids`. However this causes unexpected behavior in the reporting APIs as bids from previous auctions were returned. This PR addresses this by creating a `pbjs._adUnitCodes` primary data structure to be used as a filter when Prebid needs only the current "auction". While not ideal this allows us to retain bid data for in-flight calls to an ad server while also allowing additional bid requests to be made without the introduction of Prebid Auction instances. Closes #590 * reset externalCallbacks.all.called * use adUnitsFilter to filter bid arrays to current auction

Applies the filter when calling reporting APIs `getBidResponses`, `getAdserverTargeting`. Queues a bid request if a previous request is still running, closing over instance data. Clears previous bids from placements included in current bid request (remove stale bids). Note that if a placement is included in one request and a second request as well, and the second request is made before the first request has rendered an ad, the bids for the first request will be cleared and the `renderAd` function will throw an error as the bids cannot be found. However, the placement should be immediately filled with the results of the second request. The error is not caught so as to warn that a placement was overwritten, and care should be taken not to request bids for a placement with a request already in flight. fixes #772 fixes #590

protonate · 2016-11-10T17:52:30Z

@mkendall07 conflicts are resolved

protonate mentioned this pull request Oct 27, 2016

Prebid 12 - pbjs.requestBids - returns all ads, not just ads in paths array. #590

Closed

matthewlane mentioned this pull request Oct 28, 2016

Add pbjs.getHighestCpmBids for getting winning bids #755

Merged

1 task

mkendall07 self-assigned this Oct 31, 2016

protonate force-pushed the bugfix/callback-uses-placement-filter branch from f73f234 to 4e03174 Compare November 1, 2016 06:42

mkendall07 requested changes Nov 1, 2016

View reviewed changes

protonate force-pushed the bugfix/callback-uses-placement-filter branch from 4e03174 to fa0feb4 Compare November 1, 2016 16:57

mkendall07 added this to the Prebid 0.15.0 milestone Nov 1, 2016

mkendall07 added the needs review label Nov 2, 2016

protonate force-pushed the bugfix/callback-uses-placement-filter branch 4 times, most recently from 417d87a to 0a37f14 Compare November 6, 2016 03:47

protonate mentioned this pull request Nov 6, 2016

Prebid re-uses old timed-out bids when refreshing ad units also time out #772

Closed

protonate force-pushed the bugfix/callback-uses-placement-filter branch from 0a37f14 to 1edc943 Compare November 7, 2016 03:09

mkendall07 reviewed Nov 7, 2016

View reviewed changes

mkendall07 assigned protonate Nov 10, 2016

protonate added 4 commits November 10, 2016 09:47

targeting function use adUnitsFilter, rollback style fixes

98c92ae

fix clearPlacements bug

7b237a7

filter getBidLandscapeTargeting for adUnitCodes

1d27e81

protonate added 11 commits November 10, 2016 09:47

updates to address review notes

70d9f78

rebase onto master

8665190

rebase cleanup

5b2c35a

debug shuffle bidders unit test

fa4da5d

restore shuffle bidders test and resetAuction

6806cc6

use adUnitCodes filter in getWinningBids

03c5fa9

add tests

712872c

revert style changes

df0f6d0

restore webpack settings

4a71fc5

remove line break

6d97e91

protonate force-pushed the bugfix/callback-uses-placement-filter branch from e53f07a to 6d97e91 Compare November 10, 2016 17:52

mkendall07 approved these changes Nov 10, 2016

View reviewed changes

mkendall07 merged commit 16ea57c into master Nov 11, 2016

mkendall07 deleted the bugfix/callback-uses-placement-filter branch November 11, 2016 14:39

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Filter bid arrays using adUnitsFilter function #750

Filter bid arrays using adUnitsFilter function #750

protonate commented Oct 27, 2016

protonate commented Oct 27, 2016

dugwood commented Oct 28, 2016

mkendall07 commented Oct 31, 2016

protonate commented Nov 1, 2016

mkendall07 Nov 1, 2016

mkendall07 Nov 1, 2016

mkendall07 Nov 1, 2016

protonate commented Nov 1, 2016

protonate commented Nov 6, 2016

protonate commented Nov 7, 2016

mkendall07 left a comment

mkendall07 Nov 7, 2016

protonate Nov 7, 2016

protonate Nov 7, 2016

mkendall07 Nov 7, 2016

protonate Nov 7, 2016

protonate commented Nov 7, 2016

mkendall07 commented Nov 9, 2016

mkendall07 commented Nov 9, 2016

protonate commented Nov 10, 2016

Filter bid arrays using adUnitsFilter function #750

Filter bid arrays using adUnitsFilter function #750

Conversation

protonate commented Oct 27, 2016

Type of change

Description of change

protonate commented Oct 27, 2016

dugwood commented Oct 28, 2016

mkendall07 commented Oct 31, 2016

protonate commented Nov 1, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

protonate commented Nov 1, 2016

protonate commented Nov 6, 2016

protonate commented Nov 7, 2016

mkendall07 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

protonate commented Nov 7, 2016

mkendall07 commented Nov 9, 2016

mkendall07 commented Nov 9, 2016

protonate commented Nov 10, 2016