Concurrent auctions #1593

jaiminpanchal27 · 2017-09-16T00:18:09Z

Continuing from here #1421

…auctions # Conflicts: # modules/appnexusAstBidAdapter.js # src/targeting.js

…auctions # Conflicts: # modules/appnexusAstBidAdapter.js

…auctions

This reverts commit 616409e.

… responses are back

…auctions # Conflicts: # modules/rubiconBidAdapter.js # src/adaptermanager.js # src/bidmanager.js # src/prebid.js

Add getBidsRequested function

Added exclude to karma temporarily to support unit tests

…auctions # Conflicts: # modules/appnexusBidAdapter.js # src/prebid.js # test/spec/unit/pbjs_api_spec.js

…auctions # Conflicts: # src/bidmanager.js

…auctions

…auctions # Conflicts: # modules/appnexusAstBidAdapter.js # src/ajax.js

…auctions # Conflicts: # src/adaptermanager.js # src/bidmanager.js # test/spec/unit/pbjs_api_spec.js

…auctions # Conflicts: # src/adapters/bidderFactory.js # src/native.js # src/prebid.js # test/spec/unit/core/bidderFactory_spec.js

…auctions

dbemiller · 2017-09-21T13:37:08Z

src/adapters/bidderFactory.js

@@ -152,16 +150,17 @@ export function newBidder(spec) {
      const adUnitCodesHandled = {};
      function addBidWithCode(adUnitCode, bid) {
        adUnitCodesHandled[adUnitCode] = true;
-        bidmanager.addBidResponse(adUnitCode, bid);
+        addBidResponse(adUnitCode, bid);
      }
      function fillNoBids() {


Calls to this function can be replaced by done(). Prebid 0.x used empty bids to "infer" when the bidder was done. In Prebid 1.x it's explicit... so the rest of this logic should be deletable.

Updated code

dbemiller · 2017-09-21T13:42:50Z

src/prebid.js

@@ -272,7 +236,8 @@ $$PREBID_GLOBAL$$.renderAd = function (doc, id) {
  if (doc && id) {
    try {
      // lookup ad by ad Id
-      const bid = $$PREBID_GLOBAL$$._bidsReceived.find(bid => bid.adId === id);
+      const bid = auctionManager.findBidByAdId(id);
+      // const bid = $$PREBID_GLOBAL$$._bidsReceived.find(bid => bid.adId === id);


can be deleted

dbemiller · 2017-09-21T14:03:17Z

src/targeting.js

 var pbTargetingKeys = [];

-targeting.resetPresetTargeting = function(adUnitCode) {
-  if (isGptPubadsDefined()) {
+export function newTargeting(auctionManager) {


If you're going to update targeting to use the newTargeting pattern, I would strongly recommend doing it in master.

It looks like your changes here are confined to the implementations of the methods. So if you do it master, then the only merge conflicts will be if someone updates those implementations in master, too. If do it in 1.0 only, then there will also be conflicts whenever someone in master uses these methods.

If these two branches diverge too much, then prebid 1.0 will fall apart... so try to minimize the chances for conflict whenever you can.

Done. #1606

dbemiller · 2017-09-21T14:12:00Z

src/auction.js

+
+export function createAuction({adUnits, adUnitCodes}) {
+  return newAuction({adUnits, adUnitCodes});
+}


Why not just export newAuction?

Done here #1644

dbemiller · 2017-09-21T14:14:19Z

src/auction.js

+  var code = bid.bidderCode;
+  var bidPriceAdjusted = bid.cpm;
+  if (code && $$PREBID_GLOBAL$$.bidderSettings && $$PREBID_GLOBAL$$.bidderSettings[code]) {
+    if (typeof $$PREBID_GLOBAL$$.bidderSettings[code].bidCpmAdjustment === 'function') {


Should these bidderSettings be moved to setConfig?

Not in this PR, of course. Another question for @matthewlane probably.

dbemiller · 2017-09-21T14:59:31Z

src/auction.js

+    getBidRequests: () => _bidderRequests,
+    getBidsReceived: () => _bidsReceived,
+    startAuctionTimer,
+    callBids


I'm a little concerned by this API. How would you write code to do something with the bidsReceived once the auction completes?

The best I can come up with is something like this:

auction = newAuction(...); ... setInterval(function() { if (auction.getAuctionStatus === AUCTION_COMPLETED) { bids = auction.getBidsReceived(); // do stuff with bids } }, 50)

...but this would be pretty crappy code. If the auction happened to finish in 201 ms, I'd be wasting 49ms until your code could actually do something with the bids.

Another (smaller) issue is that this API leaves a lot of room for caller errors. Remember one of the defining features of a good API is "makes mistakes impossible." Here's a small list of possible mistakes:

Calling startAuctionTimer significantly later (or earlier) than callBids

Calling startAuctionTimer multiple times

Forgetting to callBids after loading someone's callback into startAuctionTimer

All that in mind... I'd suggest something like the following:

function newAuction({ adUnits, adUnitCodes, done, timeout }

An Auction would start immediately upon construction. It would promise to finish before the timeout, and execute done exactly once, using the bidsReceived (and possibly other data) as an argument as soon as it completes. That way there'd be no dead time (as in the setInterval example), and no chance of misusing the auction timer.

1st part related to bidsReceived discussed offline.
Auction api improvement #1644

dbemiller · 2017-09-21T15:02:04Z

src/auction.js

+import { getCacheUrl, store } from './videoCache';
+import { Renderer } from 'src/Renderer';
+import { config } from 'src/config';
+import { userSync } from 'src/userSync.js';


should be src/userSync

dbemiller · 2017-09-21T15:09:52Z

src/auction.js

+    }
+
+    function doCallbacksIfNeeded() {
+      if (bid.timeToRespond > $$PREBID_GLOBAL$$.cbTimeout + $$PREBID_GLOBAL$$.timeoutBuffer) {


Not specifically for this PR, but... should these be on config @matthewlane?

If they make sense as global configs, then they probably should. If not, then you should accept them as params here. I don't think it makes sense to read from $$PREBID_GLOBAL$$ in either case, though.

If they are moving to config, that change should go in master.

…auctions

dbemiller · 2017-09-22T20:42:34Z

test/spec/modules/appnexusAstBidAdapter_spec.js

-  describe('request function', () => {
-    let xhr;
-    let requests;
+  describe('inherited functions', () => {


This is a good test... but it's actually testing the newBidder implementation. It should move to bidderFactory_spec if it exists.

That said... nearly every test in bidderFactory_spec calls callBids... so they'd all fail if it weren't there.

Yes it can be added their as well.

In bidderFactory_spec we are using sampleBidder. And here it tests the actual implementation of bidder. So this would test that implementation is correct and it has inherited needed functions

dbemiller · 2017-09-22T20:44:10Z

test/spec/modules/appnexusAstBidAdapter_spec.js

+    });
+
+    it('should attach valid video params to the tag', () => {
+      bidRequests[0].params.video = {


This is a side-effect. If the test fails, the delete line at the end will never be executed, which causes cascading failures.

You've got to either copy the object before mutating, or move this to a beforeEach and afterEach block. Either one is fine... but for what it's worth, I tend to find the first one easier. JSON.parse(JSON.stringify(obj)) is an inefficient, but concise way to make a deep copy.

The same goes for some of these other tests as well.

dbemiller · 2017-09-22T20:47:26Z

test/spec/modules/appnexusAstBidAdapter_spec.js

    });

    it('handles non-banner media responses', () => {
-      server.respondWith(JSON.stringify({
+      // debugger; // eslint-disable-line


Stray comment

Damn, i tend to forget these debugger's a lot. Removing it.

dbemiller · 2017-09-25T13:22:39Z

test/spec/modules/appnexusAstBidAdapter_spec.js


 const ENDPOINT = '//ib.adnxs.com/ut/v3/prebid';

-const REQUEST = {


Also... shouldn't these tests be updated in master? There's nothing 1.0 or concurrent-auctions about them.

Did we give up on making those centralized tests so that adapter-specific ones only need to define JSON files?

Yes, tests can be updated for master as well. I will create a PR for that.

Plan for JSON file based tests is still on. I will be adding that pretty soon.

* Concurrent auctions (#1593) * Adding timestamp function to utils * Auction manager (WIP) * WIP * Updated targeting with auction instances and added auction status * Updated appnexus and appnexusAst adapter * Added done callback * Removing bidmanager functions * Removing deprecated public api functions * remove bidmanager file * Revert "remove bidmanager file" This reverts commit 616409e. * Updated appnexus and rubicon adapters to call done callback after all responses are back * Added todo * Fixing circular dependency * filter s2s requests * Emit auction end Add getBidsRequested function * remove unused stuff from adapters * cleanup and fixes * Unit test fixes Added exclude to karma temporarily to support unit tests * added setTimeout for each auction instance * added constants closer to code * binded auction object inside done method * Adding auctionmanager_spec tests and fixes * Remove breakpoint * Moving auction to separate file and unit tests * Merged manually * Merged config api changes * Passing adunits, adunitscodes via constructor and fixing unit tests * added ajax with timeout * convert targeting module to factory pattern * Auction code refactored and unit tests * Updated bidderfactory for 1.0 changes and added unit tests * Renamed placementCode to adUnitCode * Refactor auction and update unit tests * JSDoc for auction and auctionManager * Unit test fixes after rebase * Manual merge new updates to bidmanager and its spec file into auction * Update appnexusAst spec file * Refactor * Prebid 1.0 does not require no_bid to be added * removed side effect by cloning objects * bug fix to set status on already used bid * Removing all !1.0 complaiant adapters * Adding aliases to appnexusAst adapter * Removed bidmanager from currency * Add ignore-loader to handle .md files (#1646) * Auction module refactor (#1644) * Auction moduel refactor * remove comment and global cbtimeout * Config api updates (#1633) * Add timeoutBuffer, s2sconfig, bidderSequence to config * Removing deprecated function and its unit test * fixed linting errors * Adapter/rubicon bid adapter markup (#1674) * initial take on rubiconBidAdapter markup * formatting * formatting, cont. * formatting, cont. * Prebid 1.0 Fix issue with video bid validation (#1680) * Fix issue with video bid validation * Modified tests to stub `auctionManager.getBidsRequested` instead of `getBidRequest` * Move stub to beforeEach hook * Fix lint errors * Add bidRequests param to bid validation * Auction module updated to support currency/hook (#1733) * added hook module to prebid core that allows extension of arbitrary functions * remove unused dependency tiny-queue * change PluginFunction to HookedFunction * more hook documentation fixes * Auction module updated to support currency/hook * remove unused dependency tiny-queue * change PluginFunction to HookedFunction * more hook documentation fixes * WIP * allow context for hooked functions * added tests for context * remove withContext, just use bind * fix in hooks so asyncSeries keeps proper bound context * Unit test fixes * Updated bid validation function * Fixed video unit test * updated sizeMapping to use sizeConfig and support labels (#1772) * updated sizeMapping to use sizeConfig and support labels * added new tests for labels and sizes w/ sizeConfig when making auction * made some names clearer and added type to labels for sizeMapping * make error message more descriptive in adaptermanager * remove extra line in adpatermanager * update package.json with correct version. (#1813) * Unit test fixes for IE 10 and other old browsers (#1810) * Added fix for location.origin * Fixed test case failing in IE and Safari browsers * Added utils.getOrigin method * Updated renderer to use hooks * Rename appnexusAst adapter to appnexus adapter (#1848) * Renamed appnexusAst adapter to appnexus * Updated unit test to use appnexus bidderCode * Remove completed todo comment * fixed safeframe for 1.0 (#1834) * Emit array of objects from BID_TIMEOUT event (#1824) * Emit array of objects from BID_TIMEOUT event * requestId is now auctionId * Use v4 UUID to match previous requestId format * Move function * Move public winningBids to auction (#1828) * Move public winningBids to auction * removed _winningBids * removed _winningBids * bugfix: return only new bids * Updates for PubWise Prebid 1.0 Support (#1847) * Updates for Prebid 1.0 Support * Updates for Bug Fixes and a small refactor * update JSDoc comment. Remove trailing space (#1872) tests were failing due to no trailing space eslint rule. * Prebid 1.0 adxcg analytics adapter fix for bidtimeout event (#1871) * adxcg analytics adapter for 1.0 bidtimeout event * update tests for adxcg analytics 1.0 * Prebid 1.0 prebid server (#1846) * fix adaptermanager s2sTest unit tests * fix s2s log message * remove errant comment * fixed log statement * removed seemingly unnecessary call to transformHeightWidth(adUnit); * removed legacy sizeMapping code block * initial refactor of prebidServerBidAdapter working w/o tests (cherry picked from commit 2b843d0) * add transformSizes back for prebidServer adUnits to fix request * fixed adapterManager_spec tests * added prebidServerBidAdapter tests for 1.0 * fixed lint errors * make sure addBidResponse and doneCb are stubbed for s2s calls * s2s requests now firing BID_REQUESTED event * fixed commented tests and other minor fixes * update defaults in prebidServerBidAdapter and fix doBidderSync bug * add new API for setting defaults in config for modules * Targeting updates (#1689) * Cherry pick alias bidder * Cherry pick alias bidder * Cherry pick alias bidder * Updated test case to not import adapter * targeting updates * targeting functions refactoring * Refactored functions * more refactor of function * added jsdoc and some more refactor * check bid expiry and filtering used bids * make sure we have the right version * Update version to fix invalid semver * video bug fix (#1906) * video bug fix added required params to prebidServerAdapter * Emitted auction_end and updated ttl for prebidServer * fix logging of server adapters * Manually merge bidManager code to auction module (#1905) * Unit test fix in IE for adxcgAnalytics Adapter (#1929) * IE bug fix * get only unique bidders * added log message for xhr timeout (#1928) * remove polyfill.js and remove global polyfills (#1918) * remove polyfill.js and remove global polyfills * make sure find, findIndex, and includes use core-js in tests * switch from virtual function bind core-js to explicit usage. * remove transform-function-bind babel plugin * Replace usePrebidCache with cache:url and remove default (#1904) * replace usePrebidCache with video:cacheUrl and remove default * remove newConfig in dfpAdServerVideo_spec that isn't doing anything * change video.cacheUrl to cache.url * update to cache.url in auction and remove from adapter * pulsepointLiteBidAdapter renamed to pulsepointBidAdapter (#1931) * ET-1691: Pulsepoint Analytics adapter for Prebid. (#1) * ET-1691: Adding pulsepoint analytics and tests for pulsepoint adapter * ET-1691: Adding pulsepoint analytics and tests for pulsepoint adapter * ET-1691: cleanup * ET-1691: minor * ET-1691: revert package.json change * Adding bidRequest to bidFactory.createBid method as per #509 * ET-1765: Adding support for additional params in PulsePoint adapter (#2) * ET-1850: Fixing #866 * Minor fix * Adding mandatory parameters to Bid * Pulsepoint adapter changes * readme updates * add 'x_source.tid' to rubicon requests (#1933) * Bring in pre-1.0 native-image asset adapter change (#1934) * 33Across Adapter: Removed the usage of utils library (#1917) * Removed the usage of utils library to get bidder requests during userSync since this info will now be passed into the method * Fixed extra space which cause lint to fail * Implemented user sync per code review feedback in #1917 * Minor feedback changes * Re-instated lint check for all files which was accidentally commited after testing * Prebid 1.0 server cache (#1935) * add cache stuff to video for prebid server * add description for cacheMarkup

jaiminpanchal27 added 30 commits July 17, 2017 17:22

Adding timestamp function to utils

6ee842b

Auction manager (WIP)

e7c0580

WIP

b3b1407

Updated targeting with auction instances and added auction status

cdf168c

Updated appnexus and appnexusAst adapter

644517c

Merge branch 'master' of github.com:prebid/Prebid.js into concurrent-…

c3b7efe

…auctions # Conflicts: # modules/appnexusAstBidAdapter.js # src/targeting.js

Added done callback

36288a1

Merge branch 'master' of github.com:prebid/Prebid.js into concurrent-…

3f99402

…auctions # Conflicts: # modules/appnexusAstBidAdapter.js

Removing bidmanager functions

3b4e80b

Removing deprecated public api functions

094a052

Merge branch 'master' of github.com:prebid/Prebid.js into concurrent-…

3192320

…auctions

remove bidmanager file

616409e

Revert "remove bidmanager file"

51a393a

This reverts commit 616409e.

Updated appnexus and rubicon adapters to call done callback after all…

73bd242

… responses are back

Added todo

51a3b94

Merge branch 'master' of github.com:prebid/Prebid.js into concurrent-…

ef4e490

…auctions # Conflicts: # modules/rubiconBidAdapter.js # src/adaptermanager.js # src/bidmanager.js # src/prebid.js

Fixing circular dependency

87168aa

filter s2s requests

6300a8d

Emit auction end

aa6c9f9

Add getBidsRequested function

remove unused stuff from adapters

c59bf05

cleanup and fixes

fb485b7

Unit test fixes

52e8ddc

Added exclude to karma temporarily to support unit tests

Merge branch 'master' of github.com:prebid/Prebid.js into concurrent-…

4b3b907

…auctions # Conflicts: # modules/appnexusBidAdapter.js # src/prebid.js # test/spec/unit/pbjs_api_spec.js

added setTimeout for each auction instance

407562a

Merge branch 'master' of github.com:prebid/Prebid.js into concurrent-…

54e43e3

…auctions # Conflicts: # src/bidmanager.js

added constants closer to code

73d93d4

binded auction object inside done method

d0ba5c7

Merge branch 'master' of github.com:prebid/Prebid.js into concurrent-…

fe380ee

…auctions

Adding auctionmanager_spec tests and fixes

aec756b

Merge branch 'master' of github.com:prebid/Prebid.js into concurrent-…

ed5a815

…auctions

jaiminpanchal27 added 10 commits September 14, 2017 10:25

Merge branch 'master' of github.com:prebid/Prebid.js into concurrent-…

d43bff1

…auctions # Conflicts: # modules/appnexusAstBidAdapter.js # src/ajax.js

Updated bidderfactory for 1.0 changes and added unit tests

a84f3e8

Renamed placementCode to adUnitCode

dab3373

Refactor auction and update unit tests

b5946fb

JSDoc for auction and auctionManager

2e734ff

Merge branch 'master' of github.com:prebid/Prebid.js into concurrent-…

c4553f8

…auctions # Conflicts: # src/adaptermanager.js # src/bidmanager.js # test/spec/unit/pbjs_api_spec.js

Merge branch 'master' of github.com:prebid/Prebid.js into concurrent-…

a1ddb66

…auctions # Conflicts: # src/adapters/bidderFactory.js # src/native.js # src/prebid.js # test/spec/unit/core/bidderFactory_spec.js

Unit test fixes after rebase

ec9d1bf

Merge branch 'master' of github.com:prebid/Prebid.js into concurrent-…

564e985

…auctions

Manual merge new updates to bidmanager and its spec file into auction

bf1aaa2

dbemiller suggested changes Sep 21, 2017

View reviewed changes

jaiminpanchal27 added 4 commits September 21, 2017 17:20

Update appnexusAst spec file

ad393f5

Merge branch 'master' of github.com:prebid/Prebid.js into concurrent-…

4e5b808

…auctions

Refactor

b118982

Prebid 1.0 does not require no_bid to be added

91d1e9e

dbemiller reviewed Sep 22, 2017

View reviewed changes

dbemiller reviewed Sep 25, 2017

View reviewed changes

removed side effect by cloning objects

d3d5d24

mkendall07 self-assigned this Sep 25, 2017

jaiminpanchal27 added 4 commits September 25, 2017 15:08

bug fix to set status on already used bid

72ecab4

Removing all !1.0 complaiant adapters

909863d

Adding aliases to appnexusAst adapter

89c6b02

Removed bidmanager from currency

05697d0

dbemiller approved these changes Sep 26, 2017

View reviewed changes

mkendall07 approved these changes Sep 26, 2017

View reviewed changes

jaiminpanchal27 merged commit 1bdedd6 into prebid-1.0 Sep 26, 2017

jaiminpanchal27 mentioned this pull request Sep 29, 2017

Auction module refactor #1644

Merged

1 task

matthewlane deleted the concurrent-auctions branch October 6, 2017 23:54

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Concurrent auctions #1593

Concurrent auctions #1593

jaiminpanchal27 commented Sep 16, 2017

dbemiller Sep 21, 2017

jaiminpanchal27 Sep 22, 2017

dbemiller Sep 21, 2017

dbemiller Sep 21, 2017

jaiminpanchal27 Sep 22, 2017

dbemiller Sep 21, 2017

jaiminpanchal27 Sep 29, 2017

dbemiller Sep 21, 2017

dbemiller Sep 21, 2017

jaiminpanchal27 Sep 29, 2017

dbemiller Sep 21, 2017

dbemiller Sep 21, 2017

dbemiller Sep 22, 2017

jaiminpanchal27 Sep 25, 2017

dbemiller Sep 22, 2017

jaiminpanchal27 Sep 25, 2017

dbemiller Sep 22, 2017

jaiminpanchal27 Sep 22, 2017

dbemiller Sep 25, 2017

jaiminpanchal27 Sep 25, 2017


		const ENDPOINT = '//ib.adnxs.com/ut/v3/prebid';

		const REQUEST = {

Concurrent auctions #1593

Concurrent auctions #1593

Conversation

jaiminpanchal27 commented Sep 16, 2017

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

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment