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

Added the timeout to the response #756

Merged
merged 2 commits into from
Dec 5, 2018
Merged

Added the timeout to the response #756

merged 2 commits into from
Dec 5, 2018

Conversation

dbemiller
Copy link
Contributor

@dbemiller dbemiller commented Nov 29, 2018

This was a request from @anwzhang over on Prebid Mobile.

The issue they need to solve is: "how long should the client wait for a response before it cancels the request?"

Ideally they want this to be tmax + {buffer for latency}... but if tmax is defined on the server in a Stored Request, then the Mobile client has no way of accessing it today.

Their request was that we return this so that they can make a single auction request with a "long" timeout, and then use the value we return to tune future calls.

@dbemiller dbemiller added the External API impact Tag for issues and PRs which affect the external API label Nov 29, 2018
@dbemiller
Copy link
Contributor Author

@bretg you'll probably care about this one too.

Copy link
Collaborator

@hhhjort hhhjort left a comment

Choose a reason for hiding this comment

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

LGTM

@bretg
Copy link
Contributor

bretg commented Nov 30, 2018

The way this is handled in PBJS is that we recommend a "failsafe" timeout in the page. The page picks some crazy big number that should never happen unless there's a problem. e.g. 3 seconds. The ad server is called either when PBJS returns bids or when the failsafe timer pops, whichever comes first.

So this is a different model, or maybe just a more refined model. Seems to me that the SDK needs to have a failsafe for the scenario where the first request doesn't respond anyhow. So returning the timeout is a refinement to that, allowing the SDK to trim down the "failsafe".

Personally I think it's unnecessarily complicated on the SDK side -- a static failsafe seems enough. But if the mobile team feels strongly they want to be more sophisticated here, I'm not against exposing the timeout in the PBS response.

@hhhjort
Copy link
Collaborator

hhhjort commented Nov 30, 2018

My understanding is the prebid mobile folk like to store the configs server side. This is because apps have a very slow update cycle, so storing configs in the app will make them very difficult to update. (I might suggest having the app regularly download configs from a server, but I suppose this would require maintaining a config server someplace.) Given this, the SDK cannot be sure what the timeout should be, so they want to get the current timeout in the return to do their stuff. I agree it seems a good chunk on their work for little benefit, but perhaps it is common to make the app functionality wait on ads before displaying anything. It is a small lift on our side, so no argument against doing it.

@anwzhang
Copy link
Contributor

anwzhang commented Dec 3, 2018

Hi @bretg,

The SDK has a default static value, which is 10 seconds for the first call, and in case server doesn't return any value.

@bretg
Copy link
Contributor

bretg commented Dec 4, 2018

Agreed with @anwzhang that this attribute should be called appTimeout. Would like to make sure the stored_request docs are pretty clear here. Something like:

appTimeout is meant to be set on stored_requests and will be used by Prebid Mobile SDK as the 'failsafe' timeout on subsequent requests. This failsafe timeout is larger in value than the normal auction timeout, utilized by the caller in scenario where the client cannot reach Prebid Server due to a network or other hardware issue. e.g. if the regular auction timeout (tmax) is 750ms, we recommend that the appTimeout should be 2000ms or higher.

@anwzhang
Copy link
Contributor

anwzhang commented Dec 4, 2018

So had an offline discussion with Bret.

There are two values, one is the max time PBS waiting for the auctions, one is how long SDK should wait until the server comes back.
I looked up openrtb spec, ‘tmax’ indicates the first value.

So question is can the prebid server allow publisher to specify a SDK timeout value and send it back instead of having the sdk to calculate one based on the 'tmax' value?

@dbemiller
Copy link
Contributor Author

dbemiller commented Dec 4, 2018

um.... I certainly could add that property, but... why? It sounds like a worse design across the board.

This "overhead" varies on a user-by-user basis. One user might have a fast phone on a 5g network. Another might have a slow phone on a 3g network. If we make publishers responsible for setting a single timeout, how could they possibly make a "good" choice?

They can't. It's impossible, because the correct value isn't constant across all the publisher's traffic.

By contrast, this PR adds what PBM needs to derive the correct value. PBS already returns its own response time in response.ext.responsetimemillis. If PBM measures the time of that first call, it can use measurement and the response values to calculate the actual overhead--taking phone/network speed into account.

By adding that overhead to this tmaxrequest, you'll get much more flexible & accurate timeouts.

And since publishers don't have to see or set it, there's no risk of repeating prebid/Prebid.js#2648 in Prebid Mobile.

@bretg
Copy link
Contributor

bretg commented Dec 4, 2018

Thanks Dave. You prompted me to actually look at the code and gain a better understanding of the issue here.

how could they possibly make a "good" choice?
They can't. It's impossible, because the correct value isn't constant across all the publisher's traffic.

This part I agree with.

By contrast, this PR adds what PBM needs to derive the correct value.

I disagree with the word "correct" here, but am sympathetic to replacing that word with "better". If I was just interested in PBS, I'd have less to say about this. But as someone who's likely to get questions about the SDK as well, it being over-clever concerns me.

Let's say that the SDK takes a simplistic approach to calculating the failsafe timeout: it times the first req/resp interchange, looks at the tmax, makes some assumptions, and comes up with a number. There's no guaranteed that number is going to be constant as the user moves between cell towers. So that means the SDK should be adjusting this number over time. How will we know that any given algorithm is going to perform better than a static failsafe over the long term? But I suspect we're not the first mobile app in the history of the world to face this problem. @anwzhang - have we done research into best practices in this area?

In any case, from a Prebid Server perspective, I'm fine reflecting back tmax in response.bids.ext.tmaxrequest.

We'll cover the SDK's use of this value (clever or no) in the Prebid Mobile PMC.

@dbemiller
Copy link
Contributor Author

You can improve a bad implementation in the next version. You can't change a badly defined API.

I don't have much else to add here. Both of you have a much higher stake in this project's long-term than I do. Let me know what you eventually decide.

@bretg
Copy link
Contributor

bretg commented Dec 4, 2018

The API is fine - go ahead and reflect tmax back in response.bids.ext.tmaxrequest.

@dbemiller dbemiller merged commit 9546039 into master Dec 5, 2018
@dbemiller dbemiller deleted the return-timeout branch December 5, 2018 20:05
katsuo5 pushed a commit to flux-dev-team/prebid-server-1 that referenced this pull request Dec 1, 2020
* Added the timeout to the response.

* Shorter property name... I think a bit better.
katsuo5 pushed a commit to flux-dev-team/prebid-server-1 that referenced this pull request Dec 2, 2020
* Added the timeout to the response.

* Shorter property name... I think a bit better.
katsuo5 pushed a commit to flux-dev-team/prebid-server-1 that referenced this pull request Dec 4, 2020
* Added the timeout to the response.

* Shorter property name... I think a bit better.
mobfxoHB pushed a commit to mobfxoHB/prebid-server that referenced this pull request Aug 22, 2023
* and postMessage listener and handler

* edit to sandbox domain

* update postMessage domain

* update postMessage domain

* work on render second ad bug

* rollback changes to match master

* add AMP example

* updates to address review notes

* updates to address review notes
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
External API impact Tag for issues and PRs which affect the external API
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants