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.

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

Timeout coefficients #4070

Closed
wants to merge 1 commit into from
Closed

Timeout coefficients #4070

wants to merge 1 commit into from

Conversation

jaiminpanchal27
Copy link
Collaborator

Do not merge

Type of change

  • Feature

Description of change

Another attempt to solve #3368

Publishers will only need to worry about 2 timeout values.

  1. The user-specified timeout value given to PBJS (bidderTimeout).
  2. 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

We will calculate other 2 timeout values timeoutBuffer and s2sConfig.timeout using our default coefficient. Defaults will be only available in 3.0
3. timeoutBufferCoefficient - 0.20
4. s2sConfig.timeoutCoefficient - 0.70

Logic is simple, evaluate timeoutBuffer and s2sConfig timeout on call to requestBids or on demand and make sure that new timeout does not exceed auction timeout. If it is higher use default value.

How can publisher set their coefficient ?

pbjs.setConfig({
  timeoutBufferCoefficient: 0.20,
  s2sConfig: {
    timeoutCoefficient: 0.70
  } 
})

I have added some placeholders for github issues and pr. If this is approved I will create an issue and update

@harpere harpere added needs 2nd review Core module updates require two approvals from the core team needs review labels Aug 9, 2019
@harpere harpere requested review from robertrmartinez and removed request for robertrmartinez August 9, 2019 22:33
@harpere harpere requested a review from robertrmartinez August 9, 2019 22:34
Copy link
Collaborator

@jsnellbaker jsnellbaker left a comment

Choose a reason for hiding this comment

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

Hi @jaiminpanchal27

To confirm on the logic at bit given these changes, publishers would be expected to set the page-level timeout and the bidderTimeout, which would be treated as the new overall internal prebid timer.

The new coefficient values are to modify the internal timeouts and (currently) need to be set by the publisher in order to be used. If they are set, they would replace the (currently still in place) s2sconfig timeout and the timeoutBuffer value used by adapters.

If all the above is correct - could you clarify how much of this is temporary (in 2.x) and how much would change in 3.x; for example, would certain fields be dropped entirely from the API to control the internal timeouts through coefficients, would some of those internal timeouts stay distinct or would they be consolidated under this new logic?

if (buffer) {
buffer = newBuffer
}
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Would it be possible to somehow cache/store these derived timeout values inside the auction instance - so that this logic only performed once (per auction) and then we just reference the derived value each time we execute this function?

We call this doCallbackIfTimedout function each time we add a bid response to the auction instance, so it's going to be potentially ran many times. As most of the values we're using here aren't going to change (once the auction is running), this type of change may save some time.

@stale
Copy link

stale bot commented Aug 29, 2019

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the stale label Aug 29, 2019
@jsnellbaker jsnellbaker removed the stale label Aug 29, 2019
@bretg
Copy link
Collaborator

bretg commented Sep 4, 2019

So let me see if I have this right. This is the diagram I use to make sense of the various timeouts:

Screen Shot 2019-09-04 at 7 04 01 PM

What you're doing here is removing timeouts #3 and #4, but replacing them with a ratio relative to #2. At first glance, this doesn't save any config at all -- there are still options we have to document on the API page that make the system feel complicated.

But I suppose the idea is that the ratios are something that we should be able to set as a default and only change in bizarre edge cases. Does anyone have any data about the ratios?

Honestly my take it to leave things as they are -- this feels nearly as complicated and harder to think about.

@bretg
Copy link
Collaborator

bretg commented Sep 16, 2019

Chatted with @mkendall07. He says "the idea was to not make the coefficients configurable, thus making less decision making for the pub." He also notes that this was just a backlog item, not necessarily driven by a recent request.

Before we make this change which I think makes timeouts even harder to understand, I would like pub feedback to see how much of a problem this is.

@stale
Copy link

stale bot commented Sep 30, 2019

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the stale label Sep 30, 2019
@jsnellbaker jsnellbaker added pinned won't be closed by stalebot and removed stale labels Oct 7, 2019
@jaiminpanchal27
Copy link
Collaborator Author

Closing this as we feel that this approach does not solve the problem

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs review needs 2nd review Core module updates require two approvals from the core team pinned won't be closed by stalebot
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants