add step to evaluate s2sconfig timeout #3291
Conversation
jaiminpanchal27
added
needs 2nd review
|
leaving open until someone else can review. |
| @@ -1143,3 +1143,12 @@ export function convertTypes(types, params) { | |||
| }); | |||
| return params; | |||
| } | |||
|
|
|||
| export function evaluateTimeout(auctionTimeout, adapterTimeout, adapterName = 's2sConfig') { | |||
| if (auctionTimeout <= adapterTimeout) { | |||
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
thanks. Jason is out this week so will wait for him to get back and update this.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
@bretg Any thoughts on the above?
There was a problem hiding this comment.
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:
- The
user-specified timeoutvalue given to PBJS (bidderTimeout). - The
effective framework timeout-- PBJS adds 400ms (or timeoutBuffer) to this value and then calls bidsBackHandler when the timer pops. - The
user-specified server adapter timeout(s2sConfig.timeout) - what we currently tell PBS it's timeout is - 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. 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.
mkendall07
added
needs update
and removed
needs 2nd review
|
Due to the points raised here about the current nature of timeout settings in Prebid, I'm just closing this PR. I created #3368 to discuss the topic further. |
Type of change
Description of change
Added some logic to check the s2sconfig timeout value and set a new value if it's found to be larger than the auction's provided timeout. The new timeout is set at 75% of the auction timeout (this can be changed).
Note - we set the value in the requestBids method as it's the last place where the timeout gets set (ie in the
timeoutfield in the object config you pass in thepbjs.requestBidsfunction).