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.

Sign up for GitHub

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

add step to evaluate s2sconfig timeout #3291

Closed
wants to merge 1 commit into from
Closed

add step to evaluate s2sconfig timeout #3291

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 10 additions & 2 deletions src/prebid.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -315,8 +315,17 @@ $$PREBID_GLOBAL$$.removeAdUnit = function (adUnitCode) {
*/
$$PREBID_GLOBAL$$.requestBids = createHook('asyncSeries', function ({ bidsBackHandler, timeout, adUnits, adUnitCodes, labels } = {}) {
events.emit(REQUEST_BIDS);
const cbTimeout = timeout || config.getConfig('bidderTimeout');
adUnits = adUnits || $$PREBID_GLOBAL$$.adUnits;
const cbTimeout = timeout || config.getConfig('bidderTimeout');
const s2sConfig = config.getConfig('s2sConfig');

if (s2sConfig) {
let newTimeout = utils.evaluateTimeout(cbTimeout, s2sConfig.timeout);
if (newTimeout !== s2sConfig.timeout) {
s2sConfig.timeout = newTimeout;
config.setConfig({s2sConfig: s2sConfig});
}
}

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

Expand All @@ -342,7 +351,6 @@ $$PREBID_GLOBAL$$.requestBids = createHook('asyncSeries', function ({ bidsBackHa
const allBidders = adUnit.bids.map(bid => bid.bidder);
const bidderRegistry = adaptermanager.bidderRegistry;

const s2sConfig = config.getConfig('s2sConfig');
const s2sBidders = s2sConfig && s2sConfig.bidders;
const bidders = (s2sBidders) ? allBidders.filter(bidder => {
return !includes(s2sBidders, bidder);
Expand Down
9 changes: 9 additions & 0 deletions src/utils.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1143,3 +1143,12 @@ export function convertTypes(types, params) {
});
return params;
}

export function evaluateTimeout(auctionTimeout, adapterTimeout, adapterName = 's2sConfig') {
if (auctionTimeout <= adapterTimeout) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

In general I'm ok with shaving the timeout, but unclear 75% is the right number for every case. How about s2sConfig.timeoutBufferPercent?

Shouldn't this be auctionTimeout * PERCENT <= adapterTimeout?

Consider these scenarios:

auctionTimeout original adapterTimeout new adapterTimeout Notes
1000 600 600 original value under PERCENT
1000 800 750 should pull adapterTimeout a little lower. Current code wouldn't notice this.
1100 1000 825 current alg. also wouldn't notice this scenario

Copy link
Collaborator

Choose a reason for hiding this comment

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

agree with Bret's suggestion of auctionTimeout * PERCENT <= adapterTimeout, except that we don't need the or equal part. < is good enough. Don't need to reset the value if they are equal.

Copy link
Member

Choose a reason for hiding this comment

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

thanks. Jason is out this week so will wait for him to get back and update this.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@bretg

I was thinking we could one day extend the use of the evaluteTimeout function to also work with the client-side adapters as well. So instead of setting the coefficient value in the s2sConfig, what if we created another variable in the general setConfig to house the coefficient (ie similar to bidderTimeout)? That way it would generally available and consistent throughout the auction.

As a follow-up either way - if we were to set the value through a config, should we have a default/backup value setup and if so, what value should it be if not 75%? I set it to 75% as I thought it was a reasonable chunk of the auction's time to allow adapters to respond in most cases.

I see your point about adjusting the if logic, I'll make those changes.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@bretg Any thoughts on the above?

Copy link
Collaborator

@bretg bretg Dec 6, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

So you propose putting the adapterTimeoutBufferPercent config at the top level of setConfig. Honestly I think this could get confused with the similarly named timeoutBuffer (http://prebid.org/dev-docs/publisher-api-reference.html#setConfig-timeoutBuffer), which is a rather different thing.

Stepping back, I feel that an option to lower the timeout on the client side to account for network delay works directly against the existing timeoutBuffer, which raises the timeout. I think users may be challenged to determine which of these they need and what values to set to them to.

My position is that the Prebid.js PMC should establish a timeout rationale:

  • what flexibilities in timeout are needed? client-vs-server? different timeouts for different adapters?
  • what do we call Prebid.js' view of the timeout as opposed any adjusted versions we provide to adapters?

Here's a cut at a rationale that contains 5 flavors of timeout values:

  1. The user-specified timeout value given to PBJS (bidderTimeout).
  2. The effective framework timeout -- PBJS adds 400ms (or timeoutBuffer) to this value and then calls bidsBackHandler when the timer pops.
  3. The user-specified server adapter timeout (s2sConfig.timeout) - what we currently tell PBS it's timeout is
  4. This effective adapter timeout (the thing that's new to this PR, perhaps calculated as bidderTimeout * adapterTimeoutBufferPercent) - this is what's sent to adapters so they make an effort to hurry back with their responses.
  5. failsafe timeout - a timer the page puts around PBJS as a whole. http://prebid.org/dev-docs/faq.html#when-starting-out-what-should-my-timeouts-be

@mkendall07 to weigh in.

let newTimeout = Math.floor(0.75 * auctionTimeout);
exports.logInfo(`Detected auction timeout was smaller than the timeout set for ${adapterName}. Adjusting ${adapterName} timeout to new value to compensate: ${newTimeout}`);
return newTimeout;
}
return adapterTimeout;
}
24 changes: 24 additions & 0 deletions test/spec/utils_spec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -915,4 +915,28 @@ describe('Utils', function () {
}]);
});
});

describe('evaluateTimeout', function() {
let logInfoStub;
before(function() {
logInfoStub = sinon.stub(utils, 'logInfo');
});

after(function() {
logInfoStub.restore();
})

it('returns original timeout when auction timeout is higher', function() {
let auctionTimeout = 3500;
let adapterTimeout = 1000;
expect(utils.evaluateTimeout(auctionTimeout, adapterTimeout)).to.equal(adapterTimeout);
});

it('returns modified timeout when auction timeout is lower', function() {
let auctionTimeout = 1000;
let adapterTimeout = 1000;
expect(utils.evaluateTimeout(auctionTimeout, adapterTimeout)).to.equal(750);
expect(logInfoStub.calledOnce).to.be.true;
});
});
});