Concurrent auctions for Prebid 1.0

[jaiminpanchal27]

Type of change

• [x ] Feature

Description of change

Current status: Not ready to merge (Unit test in progress)
The purpose of creating this PR is to get everyone's thoughts on the work done till now.

Implemented concurrent auction for Prebid 1.0 following tech spec over here : Kendall's gist, Nate's gist

This feature gives us the facility to run auction in parallel without changing public API. Publishers will be using the same requestBids function but at the core we are replacing bidManager with auctionManager. A new auction instance will be created by auctionManager each time when requestBids is called. Also we are removing pbjs._bidsRequested and 'pbjs._bidsReceived'. Now auction instance will store both and will not be available publicly.

Removed bidManager usage from all the places in the core but not removed bidManager file because than build-bundle-dev task fails completely as all adapters are using bidManager

For now to start server, remove dependent task watch and test from serve task,
i.e. gulp.task('serve', ['lint', 'build-bundle-dev']);

I have updated appnexus, appnexusAst and rubicon adapter with the new changes for this PR and tested with hello_world test page.

Other information

#1087
@prebid/core
Taken some of the stuff from @protonate's earlier Prebid auctions attempt #386 #609

[snapwich]

Just thought of an issue in regards to the currency module #1374

Currently it decorates the bidmanager.addBidResponse so it can modify the cpm values of bids as they come in to handle currency conversion rates. How would the currency module hook into this process given this new flow of an auctionManager with separate instances of addBidResponse?

[dbemiller]

Some very high-level, initial feedback.

There's almost certainly more to say... but I don't want to block you either.

[dbemiller]

How sure are you that this is safe? Linter errors usually exist for good reason, and know better than the average programmer. This one looks like it exists because it's not legal ES5.

Does our babel/webpack transform it into code which is ES5 compliant? If you've done the research and are certain it's safe, disable it globally in the .eslintrc and leave links in comments so that we know why it's disabled. That way if (for example) we switch to rollup or upgrade babel, we'll know whether we have to rethink it.

If (like me) you're too lazy for that, you could also just fix the code by declaring it like const handleResponse = function handleResponse(response) { ... }. Or just move the declarations out of the if block.

[dbemiller]

Let's split the constructor from the singleton, to make this testable.

export function newAuctionManager() {
  var _auctions = [];
  ...
}

export const auctionManager = newAuctionManager();

If #1453 gets merged first, it would become:

import { events as globalEvents } from 'src/events';

export function newAuctionManager(events) {
  var _auctions = [];
  ...
}

export const auctionManager = newAuctionManager(globalEvents);

You haven't written unit tests yet... but when you do, they can use that newAuctionManager function to avoid side effects in the global state.

[jaiminpanchal27]

Done

[dbemiller]

@mkendall07 mentioned here that there's no real value in these constants anymore... and I'm inclined to agree.

Using export const AUCTION_STARTED = 'started' (or similar) somewhere near the relevant part of the code has a few advantages.

[jaiminpanchal27]

Done

[dbemiller]

I see that you're binding this function in adapterManager, but... does this function have any value if it's not bound? It seems like it'd immediately crash, and a very easy thing for calling code to forget to do

On a related note: I saw a Douglas Crockford talk a few years back where he basically said "I stopped using this, and found that I like the language much better this way". I took his advice and experimented with it for a few projects, and never looked back.

I do think it's stylistic, so I won't insist on it for this PR (although I will insist that you bind this function before returning it, unless there's really, really good reason not to)... but I thought I'd throw the idea out there in case you hadn't heard it elsewhere. In my experience, it's faster to write code without this than to try to debug all the this-related errors.

[dbemiller]

You've got a circular dependency here. The auctionManager imports the adapterManager, but the adapterManager has a callBids(auction, cbTimeout) method. Circular dependencies cause a bunch of problems (we can talk if you're interested)... so one of these two should not exist.

IMO auctionManager should depend on adapterManager, because you can't do an Auction without Adapters to call. An Adapter could be used outside of the context of an Auction, though.

[jaiminpanchal27]

Done

[dbemiller]

As long as you're rewriting these... you might want to check out the format proposed by @snapwich

Other adapter maintainers are going to look to these for inspiration... so setting a good pattern will pay dividends in maintenance.

[jaiminpanchal27]

@snapwich In regards to currency module, i thought of two options.
Since addBidResponse is now tied to auction instance we can do

Option 1
import addBidResponseDecorator into auctionManager and when instance of auction is created here i decorate the method auction.addBidResponse

Option 2
import addBidResponseDecorator into adapterManager and decorate auction.addBidResponse in callBids function.

[pm-harshad-mane]

How soon can we expect this change to be available ?

[dbemiller]

@pm-harshad-mane It will likely be a few months before it's the official release.

This PR is pretty close to done... but since it changes the adapter-facing API, it'll be merging into a 1.0 branch. We'll then have to notify the adapter maintainers, and wait for them to upgrade it.

If you convince the bidders you're using to upgrade quickly, though, then you could probably build straight from the 1.0 branch. Right now the big blocker is #1494, which gives adapters an API to help ease the transition.